diff mbox series

[v2] PCI: Align pci memory space base address with page size

Message ID 20230605034423.1528206-1-maobibo@loongson.cn
State New
Headers
Series [v2] PCI: Align pci memory space base address with page size |

Commit Message

maobibo June 5, 2023, 3:44 a.m. UTC 
  Some PCI devices has only 4K memory space size, it is normal in general
machines and aligned with page size. However some architectures which
support different page size, default page size on LoongArch is 16K, and
ARM64 supports page size varying from 4K to 64K. On machines where larger
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 requirement,
on 64 bit system it should not be a problem. And UEFI bios set pci memory
base address with 4K fixed-size aligned, the safer solution is to align
with larger size on UEFI BIOS stage on these architectures, linux kernel
can reuse resource from UEFI bios. For new devices such hotplug pci
devices and sriov devices, pci resource is assigned in Linux kernel side.

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(+)

Comments

Huacai Chen June 5, 2023, 3:58 a.m. UTC | #1 
Hi, Bibo,

Three suggestions:
1, split to two patches.
2, the "(align < PAGE_SIZE)" condition can be removed.
3, you can unify the comments between ARM64 and LoongArch.

Huacai

On Mon, Jun 5, 2023 at 11:44 AM Bibo Mao <maobibo@loongson.cn> wrote:
>
> Some PCI devices has only 4K memory space size, it is normal in general
> machines and aligned with page size. However some architectures which
> support different page size, default page size on LoongArch is 16K, and
> ARM64 supports page size varying from 4K to 64K. On machines where larger
> 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 requirement,
> on 64 bit system it should not be a problem. And UEFI bios set pci memory
> base address with 4K fixed-size aligned, the safer solution is to align
> with larger size on UEFI BIOS stage on these architectures, linux kernel
> can reuse resource from UEFI bios. For new devices such hotplug pci
> devices and sriov devices, pci resource is assigned in Linux kernel side.
>
> 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 resource *res,
> +                               resource_size_t size, resource_size_t align)
> +{
> +       resource_size_t start = res->start;
> +
> +       /*
> +        * align base address of pci memory resource with page size
> +        */
> +       if ((res->flags & IORESOURCE_MEM) && (align < PAGE_SIZE))
> +               start = 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 system
> + * is 16K, memory space of different pci cards may share the same page
> + * on LoongArch, it is not safe here.
> + */
> +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) && (align < PAGE_SIZE))
> +               start = ALIGN(start, PAGE_SIZE);
> +
> +       return start;
> +}
> +
>  static void pci_fixup_vgadev(struct pci_dev *pdev)
>  {
>         struct pci_dev *devp = NULL;
> --
> 2.27.0
>
maobibo June 5, 2023, 6:31 a.m. UTC | #2 
在 2023/6/5 11:58, Huacai Chen 写道:
> 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 compared
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;
}

or do you mean something this?
       if (res->flags & IORESOURCE_MEM) {
		align = max_t(resource_size_t, PAGE_SIZE, align);
		start = ALIGN(start, align);
	}
   

> 3, you can unify the comments between ARM64 and LoongArch.
will do.

Regards
Bibo, Mao

> 
> Huacai
> 
> On Mon, Jun 5, 2023 at 11:44 AM Bibo Mao <maobibo@loongson.cn> wrote:
>>
>> Some PCI devices has only 4K memory space size, it is normal in general
>> machines and aligned with page size. However some architectures which
>> support different page size, default page size on LoongArch is 16K, and
>> ARM64 supports page size varying from 4K to 64K. On machines where larger
>> 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 requirement,
>> on 64 bit system it should not be a problem. And UEFI bios set pci memory
>> base address with 4K fixed-size aligned, the safer solution is to align
>> with larger size on UEFI BIOS stage on these architectures, linux kernel
>> can reuse resource from UEFI bios. For new devices such hotplug pci
>> devices and sriov devices, pci resource is assigned in Linux kernel side.
>>
>> 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 resource *res,
>> +                               resource_size_t size, resource_size_t align)
>> +{
>> +       resource_size_t start = res->start;
>> +
>> +       /*
>> +        * align base address of pci memory resource with page size
>> +        */
>> +       if ((res->flags & IORESOURCE_MEM) && (align < PAGE_SIZE))
>> +               start = 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 system
>> + * is 16K, memory space of different pci cards may share the same page
>> + * on LoongArch, it is not safe here.
>> + */
>> +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) && (align < PAGE_SIZE))
>> +               start = ALIGN(start, PAGE_SIZE);
>> +
>> +       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@lists.loongnix.cn
> To unsubscribe send an email to loongson-kernel-leave@lists.loongnix.cn
Will Deacon June 5, 2023, 2:51 p.m. UTC | #3 
On Mon, Jun 05, 2023 at 11:44:23AM +0800, Bibo Mao wrote:
> Some PCI devices has only 4K memory space size, it is normal in general
> machines and aligned with page size. However some architectures which
> support different page size, default page size on LoongArch is 16K, and
> ARM64 supports page size varying from 4K to 64K. On machines where larger
> 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 requirement,
> on 64 bit system it should not be a problem. And UEFI bios set pci memory
> base address with 4K fixed-size aligned, the safer solution is to align
> with larger size on UEFI BIOS stage on these architectures, linux kernel
> can reuse resource from UEFI bios. For new devices such hotplug pci
> devices and sriov devices, pci resource is assigned in Linux kernel side.
> 
> 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 resource *res,
> +				resource_size_t size, resource_size_t align)
> +{
> +	resource_size_t start = res->start;
> +
> +	/*
> +	 * align base address of pci memory resource with page size
> +	 */
> +	if ((res->flags & IORESOURCE_MEM) && (align < PAGE_SIZE))
> +		start = ALIGN(start, PAGE_SIZE);
> +
> +	return start;
> +}
>  #endif

If this is needed by all architectures with !4k page size, why don't we
instead handle the alignment in the core code, given that it is aware
of the page size?

Will
Huacai Chen June 6, 2023, 2:17 a.m. UTC | #4 
On Mon, Jun 5, 2023 at 2:31 PM bibo, mao <maobibo@loongson.cn> wrote:
>
>
>
> 在 2023/6/5 11:58, Huacai Chen 写道:
> > 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 compared
> 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 = max_t(resource_size_t, PAGE_SIZE, align);
>                 start = ALIGN(start, align);
>         }
>
No, I mean use "start = 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 AM Bibo Mao <maobibo@loongson.cn> wrote:
> >>
> >> Some PCI devices has only 4K memory space size, it is normal in general
> >> machines and aligned with page size. However some architectures which
> >> support different page size, default page size on LoongArch is 16K, and
> >> ARM64 supports page size varying from 4K to 64K. On machines where larger
> >> 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 requirement,
> >> on 64 bit system it should not be a problem. And UEFI bios set pci memory
> >> base address with 4K fixed-size aligned, the safer solution is to align
> >> with larger size on UEFI BIOS stage on these architectures, linux kernel
> >> can reuse resource from UEFI bios. For new devices such hotplug pci
> >> devices and sriov devices, pci resource is assigned in Linux kernel side.
> >>
> >> 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 resource *res,
> >> +                               resource_size_t size, resource_size_t align)
> >> +{
> >> +       resource_size_t start = res->start;
> >> +
> >> +       /*
> >> +        * align base address of pci memory resource with page size
> >> +        */
> >> +       if ((res->flags & IORESOURCE_MEM) && (align < PAGE_SIZE))
> >> +               start = 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 system
> >> + * is 16K, memory space of different pci cards may share the same page
> >> + * on LoongArch, it is not safe here.
> >> + */
> >> +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) && (align < PAGE_SIZE))
> >> +               start = ALIGN(start, PAGE_SIZE);
> >> +
> >> +       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@lists.loongnix.cn
> > To unsubscribe send an email to loongson-kernel-leave@lists.loongnix.cn
>
Huacai Chen June 6, 2023, 2:27 a.m. UTC | #5 
On Mon, Jun 5, 2023 at 10:51 PM Will Deacon <will@kernel.org> wrote:
>
> On Mon, Jun 05, 2023 at 11:44:23AM +0800, Bibo Mao wrote:
> > Some PCI devices has only 4K memory space size, it is normal in general
> > machines and aligned with page size. However some architectures which
> > support different page size, default page size on LoongArch is 16K, and
> > ARM64 supports page size varying from 4K to 64K. On machines where larger
> > 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 requirement,
> > on 64 bit system it should not be a problem. And UEFI bios set pci memory
> > base address with 4K fixed-size aligned, the safer solution is to align
> > with larger size on UEFI BIOS stage on these architectures, linux kernel
> > can reuse resource from UEFI bios. For new devices such hotplug pci
> > devices and sriov devices, pci resource is assigned in Linux kernel side.
> >
> > 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 resource *res,
> > +                             resource_size_t size, resource_size_t align)
> > +{
> > +     resource_size_t start = res->start;
> > +
> > +     /*
> > +      * align base address of pci memory resource with page size
> > +      */
> > +     if ((res->flags & IORESOURCE_MEM) && (align < PAGE_SIZE))
> > +             start = ALIGN(start, PAGE_SIZE);
> > +
> > +     return start;
> > +}
> >  #endif
>
> If this is needed by all architectures with !4k page size, why don't we
> instead handle the alignment in the core code, given that it is aware
> of the page size?
Agree, modifying the common weak version of pcibios_align_resource()
seems better. Maybe we can use CONFIG_VFIO_PCI to guard the align
operation.

Huacai
>
> Will
maobibo June 6, 2023, 6:50 a.m. UTC | #6 
在 2023/6/6 10:27, Huacai Chen 写道:
> On Mon, Jun 5, 2023 at 10:51 PM Will Deacon <will@kernel.org> wrote:
>>
>> On Mon, Jun 05, 2023 at 11:44:23AM +0800, Bibo Mao wrote:
>>> Some PCI devices has only 4K memory space size, it is normal in general
>>> machines and aligned with page size. However some architectures which
>>> support different page size, default page size on LoongArch is 16K, and
>>> ARM64 supports page size varying from 4K to 64K. On machines where larger
>>> 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 requirement,
>>> on 64 bit system it should not be a problem. And UEFI bios set pci memory
>>> base address with 4K fixed-size aligned, the safer solution is to align
>>> with larger size on UEFI BIOS stage on these architectures, linux kernel
>>> can reuse resource from UEFI bios. For new devices such hotplug pci
>>> devices and sriov devices, pci resource is assigned in Linux kernel side.
>>>
>>> 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 resource *res,
>>> +                             resource_size_t size, resource_size_t align)
>>> +{
>>> +     resource_size_t start = res->start;
>>> +
>>> +     /*
>>> +      * align base address of pci memory resource with page size
>>> +      */
>>> +     if ((res->flags & IORESOURCE_MEM) && (align < PAGE_SIZE))
>>> +             start = ALIGN(start, PAGE_SIZE);
>>> +
>>> +     return start;
>>> +}
>>>  #endif
>>
>> If this is needed by all architectures with !4k page size, why don't we
>> instead handle the alignment in the core code, given that it is aware
>> of the page size?
> Agree, modifying the common weak version of pcibios_align_resource()
> seems better. Maybe we can use CONFIG_VFIO_PCI to guard the align
> operation.
will do. only that I am not sure it is ok for all architectures, after all it consumes
more pci memory resource if forced to page size aligned.
 
Regards
Bibo, Mao
> 
> Huacai
>>
>> Will
diff mbox series

Patch

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 resource *res,
+				resource_size_t size, resource_size_t align)
+{
+	resource_size_t start = res->start;
+
+	/*
+	 * align base address of pci memory resource with page size
+	 */
+	if ((res->flags & IORESOURCE_MEM) && (align < PAGE_SIZE))
+		start = 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 system
+ * is 16K, memory space of different pci cards may share the same page
+ * on LoongArch, it is not safe here.
+ */
+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) && (align < PAGE_SIZE))
+		start = ALIGN(start, PAGE_SIZE);
+
+	return start;
+}
+
 static void pci_fixup_vgadev(struct pci_dev *pdev)
 {
 	struct pci_dev *devp = NULL;