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

[bibo, mao]

在 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? 

You can search function pnv_pci_default_alignment or pcibios_align_resource about
alpha architecture.

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