Re: [PATCH] LoongArch: Align pci memory base address with page size

[Huacai Chen]

On Fri, Jun 2, 2023 at 2:48 PM bibo, mao <maobibo@xxxxxxxxxxx> wrote:
>
>
>
> 在 2023/6/2 12:11, Huacai Chen 写道:
> > +cc Bjorn
> >
> > Hi, Bibo,
> >
> > On Fri, Jun 2, 2023 at 11:07 AM Bibo Mao <maobibo@xxxxxxxxxxx> wrote:
> >>
> >> LoongArch linux kernel uses 16K page size by default, some pci devices have
> >> only 4K memory size, it is normal in general architectures. However memory
> >> space of different pci devices will share one physical page address space.
> >> This is not safe for mmu protection, also UIO and VFIO requires base
> >> address of pci memory space page aligned.
> >>
> >> This patch adds check with function pcibios_align_resource, and set base
> >> address of resource page aligned.
> >>
> >> Signed-off-by: Bibo Mao <maobibo@xxxxxxxxxxx>
> >> ---
> >>  arch/loongarch/pci/pci.c | 23 +++++++++++++++++++++++
> >>  1 file changed, 23 insertions(+)
> >>
> >> diff --git a/arch/loongarch/pci/pci.c b/arch/loongarch/pci/pci.c
> >> index 2726639150bc..1380f3672ba2 100644
> >> --- a/arch/loongarch/pci/pci.c
> >> +++ b/arch/loongarch/pci/pci.c
> >> @@ -83,6 +83,29 @@ 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 system
> >> + * is 16K, memory space of different pci cards may share the same page
> >> + * on LoongArch, it is not safe here.
> >> + * Also uio drivers and vfio drivers sugguests that base address of memory
> >> + * space should page aligned. This function aligns base address with page size
> >> + */
> >> +resource_size_t pcibios_align_resource(void *data, const struct resource *res,
> >> +               resource_size_t size, resource_size_t align)
> >> +{
> >> +       resource_size_t start = res->start;
> >> +
> >> +       if (res->flags & IORESOURCE_MEM) {
> >> +               if (align & (PAGE_SIZE - 1)) {
> >> +                       align = PAGE_SIZE;
> >> +                       start = ALIGN(start, align);
> > I don't know whether this patch is really needed, but the logic here
> > has some problems.
> >
> > For example, if PAGE_SIZE=16KB, align=18KB, what should we do? Align
> > to 16KB or align to 32KB? IMO it should align to 32KB, but in your
> > patch it will align to 16KB.
> In general pci device is aligned by size, and its value is a power of 2 in value.
> I do not see such devices with 18K alignment requirements.
If so, you can simply ignore "align" and use  start = ALIGN(start, PAGE_SIZE);

>
> By pci local bus spec, there are such lines:
>
> "Devices are free to consume more address space than required, but decoding down
> to a 4 KB space for memory is suggested for devices that need less than that amount. For
> instance, a device that has 64 bytes of registers to be mapped into Memory Space may
> consume up to 4 KB of address space in order to minimize the number of bits in the address
> decoder."
>
> I cannot  think whether it is necessary simply from judging whether other
> architectures have similar code. If so, LoongArch system just  always follows others.
> It is actually one problem since LoongArch uses 16K page size.
As I know, both MIPS and ARM64 can use non-4K pages, but when I grep
pcibios_align_resource in the arch directory, none of them do
PAGE_SIZE alignment.

Huacai

>
> Regards
> Bibo, Mao
> >
> > Huacai
> >> +               }
> >> +       }
> >> +       return start;
> >> +}
> >> +
> >>  static void pci_fixup_vgadev(struct pci_dev *pdev)
> >>  {
> >>         struct pci_dev *devp = NULL;
> >> --
> >> 2.27.0
> >>
> > _______________________________________________
> > Loongson-kernel mailing list -- loongson-kernel@xxxxxxxxxxxxxxxxx
> > To unsubscribe send an email to loongson-kernel-leave@xxxxxxxxxxxxxxxxx
>
>