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

[毛碧波]

> -----原始邮件-----
> 发件人: "Huacai Chen" <chenhuacai@xxxxxxxxxx>
> 发送时间:2023-06-04 20:33:20 (星期日)
> 收件人: "bibo, mao" <maobibo@xxxxxxxxxxx>
> 抄送: loongson-kernel@xxxxxxxxxxxxxxxxx, "Bjorn Helgaas" <helgaas@xxxxxxxxxx>, "WANG Xuerui" <kernel@xxxxxxxxxx>, "Jianmin Lv" <lvjianmin@xxxxxxxxxxx>, loongarch@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx
> 主题: Re: [PATCH] LoongArch: Align pci memory base address with page size
> 
> On Fri, Jun 2, 2023 at 5:40 PM bibo, mao <maobibo@xxxxxxxxxxx> wrote:
> >
> >
> >
> > 在 2023/6/2 16:00, Huacai Chen 写道:
> > > On Fri, Jun 2, 2023 at 3:35 PM bibo, mao <maobibo@xxxxxxxxxxx> wrote:
> > >>
> > >>
> > >>
> > >> 在 2023/6/2 14:55, 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.
> > >> Here is piece of  code in drivers/vfio/pci/vfio_pci_core.c
> > >>                 /*
> > >>                  * Here we don't handle the case when the BAR is not page
> > >>                  * aligned because we can't expect the BAR will be
> > >>                  * assigned into the same location in a page in guest
> > >>                  * when we passthrough the BAR. And it's hard to access
> > >>                  * this BAR in userspace because we have no way to get
> > >>                  * the BAR's location in a page.
> > >>                  */
> > >> no_mmap:
> > >>                 vdev->bar_mmap_supported[bar] = false;
> > >>
> > >> Do you think it is a issue or not?
> > > May be or may not be, if it should be aligned to PAGE_SIZE, then MIPS
> > > and ARM64 also need this.
> > >
> > >>
> > >> You can search function pnv_pci_default_alignment or pcibios_align_resource about
> > >> alpha architecture.
> > > Alpha's pcibios_align_resource() have nothing to do with PAGE_SIZE,
> > > pnv_pci_default_alignment() seems to be the case. But if alignment is
> > > really needed, I think it is better to provide a
> > > pcibios_default_alignment() as powerpc.
> > I will double check which is better. Just be curious, how do you think it is a problem
> > or not, just checking whether other arches have similar code??
> You are probably right but I'm not sure, maybe you can submit a patch
> for ARM64 to get more people involved.
Good suggestion, will do it in next version. It will be better with more people
involved.

Regards
Bibo, Mao
> 
> Huacai
> >
> >
> > >
> > > Huacai
> > >>
> > >> Regards
> > >> Bibo, mao
> > >>
> > >>>
> > >>> 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
> > >>>>
> > >>>>
> > >>> _______________________________________________
> > >>> Loongson-kernel mailing list -- loongson-kernel@xxxxxxxxxxxxxxxxx
> > >>> To unsubscribe send an email to loongson-kernel-leave@xxxxxxxxxxxxxxxxx
> > >>
> > >>
> >
> >


本邮件及其附件含有龙芯中科的商业秘密信息，仅限于发送给上面地址中列出的个人或群组。禁止任何其他人以任何形式使用（包括但不限于全部或部分地泄露、复制或散发）本邮件及其附件中的信息。如果您错收本邮件，请您立即电话或邮件通知发件人并删除本邮件。 
This email and its attachments contain confidential information from Loongson Technology , which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this email in error, please notify the sender by phone or email immediately and delete it.