Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230605034423.1528206-1-maobibo@loongson.cn> <CAAhV-H6O+_EnmDU6kE-19PP+8tD3wwbzzuakE_vHZGCtjZWDbg@mail.gmail.com>
 <749898e2-01c2-9f74-f939-fac36227431a@loongson.cn>
In-Reply-To: <749898e2-01c2-9f74-f939-fac36227431a@loongson.cn>
From:   Huacai Chen <chenhuacai@kernel.org>
Date:   Tue, 6 Jun 2023 10:17:35 +0800
Message-ID: <CAAhV-H6o8Dx0g+UaNjh9JfAVv=aAU=GFkUS9aO6tsBim3mYZjQ@mail.gmail.com>
Subject: Re: [PATCH v2] PCI: Align pci memory space base address with page size
To:     "bibo, mao" <maobibo@loongson.cn>
Cc:     Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>, helgaas@kernel.org,
        linux-pci@vger.kernel.org, Jianmin Lv <lvjianmin@loongson.cn>,
        WANG Xuerui <kernel@xen0n.name>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        loongarch@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jun 5, 2023 at 2:31=E2=80=AFPM bibo, mao <maobibo@loongson.cn> wrot=
e:
>
>
>
> =E5=9C=A8 2023/6/5 11:58, Huacai Chen =E5=86=99=E9=81=93:
> > Hi, Bibo,
> >
> > Three suggestions:
> > 1, split to two patches.
> will do.
> > 2, the "(align < PAGE_SIZE)" condition can be removed.
> With  "(align < PAGE_SIZE)" condition, there is little modification compa=
red
> to the weak function.
> resource_size_t __weak pcibios_align_resource(void *data,
>                                               const struct resource *res,
>                                               resource_size_t size,
>                                               resource_size_t align)
> {
>        return res->start;
> }
Why? I think "align > PAGE_SIZE" doesn't ensure res->start is aligned here.

>
> or do you mean something this?
>        if (res->flags & IORESOURCE_MEM) {
>                 align =3D max_t(resource_size_t, PAGE_SIZE, align);
>                 start =3D ALIGN(start, align);
>         }
>
No, I mean use "start =3D ALIGN(start, PAGE_SIZE)" unconditionally.

Huacai
>
> > 3, you can unify the comments between ARM64 and LoongArch.
> will do.
>
> Regards
> Bibo, Mao
>
> >
> > Huacai
> >
> > On Mon, Jun 5, 2023 at 11:44=E2=80=AFAM Bibo Mao <maobibo@loongson.cn> =
wrote:
> >>
> >> Some PCI devices has only 4K memory space size, it is normal in genera=
l
> >> machines and aligned with page size. However some architectures which
> >> support different page size, default page size on LoongArch is 16K, an=
d
> >> ARM64 supports page size varying from 4K to 64K. On machines where lar=
ger
> >> page size is use, memory space region of two different pci devices may=
 be
> >> in one page. It is not safe with mmu protection, also VFIO pci device
> >> driver requires base address of pci memory space page aligned, so that=
 it
> >> can be memory mapped to qemu user space when it is pass-through to vm.
> >>
> >> It consumes more pci memory resource with page size alignment requirem=
ent,
> >> on 64 bit system it should not be a problem. And UEFI bios set pci mem=
ory
> >> base address with 4K fixed-size aligned, the safer solution is to alig=
n
> >> with larger size on UEFI BIOS stage on these architectures, linux kern=
el
> >> can reuse resource from UEFI bios. For new devices such hotplug pci
> >> devices and sriov devices, pci resource is assigned in Linux kernel si=
de.
> >>
> >> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> >> ---
> >>  arch/arm64/kernel/pci.c  | 13 +++++++++++++
> >>  arch/loongarch/pci/pci.c | 18 ++++++++++++++++++
> >>  2 files changed, 31 insertions(+)
> >>
> >> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> >> index 2276689b5411..e2f7b176b156 100644
> >> --- a/arch/arm64/kernel/pci.c
> >> +++ b/arch/arm64/kernel/pci.c
> >> @@ -232,4 +232,17 @@ void pcibios_remove_bus(struct pci_bus *bus)
> >>         acpi_pci_remove_bus(bus);
> >>  }
> >>
> >> +resource_size_t pcibios_align_resource(void *data, const struct resou=
rce *res,
> >> +                               resource_size_t size, resource_size_t =
align)
> >> +{
> >> +       resource_size_t start =3D res->start;
> >> +
> >> +       /*
> >> +        * align base address of pci memory resource with page size
> >> +        */
> >> +       if ((res->flags & IORESOURCE_MEM) && (align < PAGE_SIZE))
> >> +               start =3D ALIGN(start, PAGE_SIZE);
> >> +
> >> +       return start;
> >> +}
> >>  #endif
> >> diff --git a/arch/loongarch/pci/pci.c b/arch/loongarch/pci/pci.c
> >> index 2726639150bc..943a48e60fb1 100644
> >> --- a/arch/loongarch/pci/pci.c
> >> +++ b/arch/loongarch/pci/pci.c
> >> @@ -83,6 +83,24 @@ int pcibios_alloc_irq(struct pci_dev *dev)
> >>         return acpi_pci_irq_enable(dev);
> >>  }
> >>
> >> +/*
> >> + * memory space size of some pci cards is 4K, it is separated with
> >> + * different pages for generic architectures, so that mmu protection =
can
> >> + * work with different pci cards. However page size for LoongArch sys=
tem
> >> + * is 16K, memory space of different pci cards may share the same pag=
e
> >> + * on LoongArch, it is not safe here.
> >> + */
> >> +resource_size_t pcibios_align_resource(void *data, const struct resou=
rce *res,
> >> +                               resource_size_t size, resource_size_t =
align)
> >> +{
> >> +       resource_size_t start =3D res->start;
> >> +
> >> +       if ((res->flags & IORESOURCE_MEM) && (align < PAGE_SIZE))
> >> +               start =3D ALIGN(start, PAGE_SIZE);
> >> +
> >> +       return start;
> >> +}
> >> +
> >>  static void pci_fixup_vgadev(struct pci_dev *pdev)
> >>  {
> >>         struct pci_dev *devp =3D NULL;
> >> --
> >> 2.27.0
> >>
> > _______________________________________________
> > Loongson-kernel mailing list -- loongson-kernel@lists.loongnix.cn
> > To unsubscribe send an email to loongson-kernel-leave@lists.loongnix.cn
>