[v11,4/7] powerpc/powernv: Override pcibios_default_alignment() to force PCI devices to be page aligned

Submitted by Bjorn Helgaas on April 17, 2017, 9:36 p.m.

Details

Message ID 20170417213653.21092.27784.stgit@bhelgaas-glaptop.roam.corp.google.com
State Not Applicable
Headers show

Commit Message

Bjorn Helgaas April 17, 2017, 9:36 p.m.
From: Yongji Xie <elohimes@gmail.com>

This overrides pcibios_default_alignment() to set default alignment
to PAGE_SIZE for all PCI devices on PowerNV platform. Thus sub-page
BARs would not share a page and could be mapped into guest when VFIO
passthrough them.

Signed-off-by: Yongji Xie <elohimes@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/powerpc/include/asm/machdep.h        |    2 ++
 arch/powerpc/kernel/pci-common.c          |    8 ++++++++
 arch/powerpc/platforms/powernv/pci-ioda.c |    7 +++++++
 3 files changed, 17 insertions(+)

Comments

Michael Ellerman April 18, 2017, 5:30 a.m.
Bjorn Helgaas <bhelgaas@google.com> writes:

> From: Yongji Xie <elohimes@gmail.com>
>
> This overrides pcibios_default_alignment() to set default alignment
> to PAGE_SIZE for all PCI devices on PowerNV platform. Thus sub-page
> BARs would not share a page and could be mapped into guest when VFIO
> passthrough them.
>
> Signed-off-by: Yongji Xie <elohimes@gmail.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  arch/powerpc/include/asm/machdep.h        |    2 ++
>  arch/powerpc/kernel/pci-common.c          |    8 ++++++++
>  arch/powerpc/platforms/powernv/pci-ioda.c |    7 +++++++

This looks fine to me, here's an ack, but don't feel you have to rebase
to add it:

Acked-by: Michael Ellerman <mpe@ellerman.id.au>


cheers
Bjorn Helgaas April 18, 2017, 1:53 p.m.
On Mon, Apr 17, 2017 at 4:36 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> From: Yongji Xie <elohimes@gmail.com>
>
> This overrides pcibios_default_alignment() to set default alignment
> to PAGE_SIZE for all PCI devices on PowerNV platform. Thus sub-page
> BARs would not share a page and could be mapped into guest when VFIO
> passthrough them.
>
> Signed-off-by: Yongji Xie <elohimes@gmail.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  arch/powerpc/include/asm/machdep.h        |    2 ++
>  arch/powerpc/kernel/pci-common.c          |    8 ++++++++
>  arch/powerpc/platforms/powernv/pci-ioda.c |    7 +++++++
>  3 files changed, 17 insertions(+)

> +resource_size_t pcibios_default_alignment(struct pci_dev *pdev)
> +{
> +       if (ppc_md.pcibios_default_alignment)
> +               return ppc_md.pcibios_default_alignment(pdev);
> +
> +       return 0;
> +}
> +
>  #ifdef CONFIG_PCI_IOV
>  resource_size_t pcibios_iov_resource_alignment(struct pci_dev *pdev, int resno)
>  {
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 6901a06da2f9..b724487cbd0f 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -3287,6 +3287,11 @@ static void pnv_pci_setup_bridge(struct pci_bus *bus, unsigned long type)
>         }
>  }
>
> +static resource_size_t pnv_pci_default_alignment(struct pci_dev *pdev)
> +{
> +       return PAGE_SIZE;
> +}

Is it necessary that pcibios_default_alignment() take a pci_dev
pointer?  I'd like this better if it were:

  resource_size_t pcibios_default_alignment(void) { ... }

because the last patch relies on the assumption that all resources of
*all* devices will be realigned to the same alignment.

Bjorn
Michael Ellerman April 19, 2017, 1:47 a.m.
Bjorn Helgaas <bhelgaas@google.com> writes:

> On Mon, Apr 17, 2017 at 4:36 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> From: Yongji Xie <elohimes@gmail.com>
>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>> index 6901a06da2f9..b724487cbd0f 100644
>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>> @@ -3287,6 +3287,11 @@ static void pnv_pci_setup_bridge(struct pci_bus *bus, unsigned long type)
>>         }
>>  }
>>
>> +static resource_size_t pnv_pci_default_alignment(struct pci_dev *pdev)
>> +{
>> +       return PAGE_SIZE;
>> +}
>
> Is it necessary that pcibios_default_alignment() take a pci_dev
> pointer?

It's not necessary given the current implementation, obviously.

But it did strike me as a good idea to pass it in case we ever want to
do anything device specific in future.

> I'd like this better if it were:
>
>   resource_size_t pcibios_default_alignment(void) { ... }
>
> because the last patch relies on the assumption that all resources of
> *all* devices will be realigned to the same alignment.

But I guess that precludes doing anything device specific, at least
without further changes. So in that case it would be better if the API
didn't include the pci_dev.

Hopefully Yongji can confirm that there were no plans to use the
pci_dev in future patches.

cheers
Yongji Xie April 19, 2017, 2:12 a.m.
On 19 April 2017 at 09:47, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Bjorn Helgaas <bhelgaas@google.com> writes:
>
>> On Mon, Apr 17, 2017 at 4:36 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> From: Yongji Xie <elohimes@gmail.com>
>>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>>> index 6901a06da2f9..b724487cbd0f 100644
>>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
>>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>>> @@ -3287,6 +3287,11 @@ static void pnv_pci_setup_bridge(struct pci_bus *bus, unsigned long type)
>>>         }
>>>  }
>>>
>>> +static resource_size_t pnv_pci_default_alignment(struct pci_dev *pdev)
>>> +{
>>> +       return PAGE_SIZE;
>>> +}
>>
>> Is it necessary that pcibios_default_alignment() take a pci_dev
>> pointer?
>
> It's not necessary given the current implementation, obviously.
>
> But it did strike me as a good idea to pass it in case we ever want to
> do anything device specific in future.
>
>> I'd like this better if it were:
>>
>>   resource_size_t pcibios_default_alignment(void) { ... }
>>
>> because the last patch relies on the assumption that all resources of
>> *all* devices will be realigned to the same alignment.
>
> But I guess that precludes doing anything device specific, at least
> without further changes. So in that case it would be better if the API
> didn't include the pci_dev.
>
> Hopefully Yongji can confirm that there were no plans to use the
> pci_dev in future patches.
>

Yes, seems like pci_dev pointer doesn't match the assumption
that all resources will be realigned to the same alignment. It's OK
to me to remove it.

Thanks,
Yongji
Bjorn Helgaas April 19, 2017, 5:53 p.m.
On Tue, Apr 18, 2017 at 9:12 PM, Yongji Xie <elohimes@gmail.com> wrote:
> On 19 April 2017 at 09:47, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Bjorn Helgaas <bhelgaas@google.com> writes:
>>
>>> On Mon, Apr 17, 2017 at 4:36 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>>> From: Yongji Xie <elohimes@gmail.com>
>>>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>>>> index 6901a06da2f9..b724487cbd0f 100644
>>>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
>>>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>>>> @@ -3287,6 +3287,11 @@ static void pnv_pci_setup_bridge(struct pci_bus *bus, unsigned long type)
>>>>         }
>>>>  }
>>>>
>>>> +static resource_size_t pnv_pci_default_alignment(struct pci_dev *pdev)
>>>> +{
>>>> +       return PAGE_SIZE;
>>>> +}
>>>
>>> Is it necessary that pcibios_default_alignment() take a pci_dev
>>> pointer?
>>
>> It's not necessary given the current implementation, obviously.
>>
>> But it did strike me as a good idea to pass it in case we ever want to
>> do anything device specific in future.
>>
>>> I'd like this better if it were:
>>>
>>>   resource_size_t pcibios_default_alignment(void) { ... }
>>>
>>> because the last patch relies on the assumption that all resources of
>>> *all* devices will be realigned to the same alignment.
>>
>> But I guess that precludes doing anything device specific, at least
>> without further changes. So in that case it would be better if the API
>> didn't include the pci_dev.
>>
>> Hopefully Yongji can confirm that there were no plans to use the
>> pci_dev in future patches.
>>
>
> Yes, seems like pci_dev pointer doesn't match the assumption
> that all resources will be realigned to the same alignment. It's OK
> to me to remove it.

I made this change (removing the pci_dev parameter) on my pci/resource branch.

Bjorn

Patch hide | download patch | download mbox

diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
index 5011b69107a7..a82c1925ff43 100644
--- a/arch/powerpc/include/asm/machdep.h
+++ b/arch/powerpc/include/asm/machdep.h
@@ -173,6 +173,8 @@  struct machdep_calls {
 	/* Called after scan and before resource survey */
 	void (*pcibios_fixup_phb)(struct pci_controller *hose);
 
+	resource_size_t (*pcibios_default_alignment)(struct pci_dev *);
+
 #ifdef CONFIG_PCI_IOV
 	void (*pcibios_fixup_sriov)(struct pci_dev *pdev);
 	resource_size_t (*pcibios_iov_resource_alignment)(struct pci_dev *, int resno);
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index ffda24a38dda..ceda57484e8e 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -233,6 +233,14 @@  void pcibios_reset_secondary_bus(struct pci_dev *dev)
 	pci_reset_secondary_bus(dev);
 }
 
+resource_size_t pcibios_default_alignment(struct pci_dev *pdev)
+{
+	if (ppc_md.pcibios_default_alignment)
+		return ppc_md.pcibios_default_alignment(pdev);
+
+	return 0;
+}
+
 #ifdef CONFIG_PCI_IOV
 resource_size_t pcibios_iov_resource_alignment(struct pci_dev *pdev, int resno)
 {
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 6901a06da2f9..b724487cbd0f 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -3287,6 +3287,11 @@  static void pnv_pci_setup_bridge(struct pci_bus *bus, unsigned long type)
 	}
 }
 
+static resource_size_t pnv_pci_default_alignment(struct pci_dev *pdev)
+{
+	return PAGE_SIZE;
+}
+
 #ifdef CONFIG_PCI_IOV
 static resource_size_t pnv_pci_iov_resource_alignment(struct pci_dev *pdev,
 						      int resno)
@@ -3820,6 +3825,8 @@  static void __init pnv_pci_init_ioda_phb(struct device_node *np,
 		hose->controller_ops = pnv_pci_ioda_controller_ops;
 	}
 
+	ppc_md.pcibios_default_alignment = pnv_pci_default_alignment;
+
 #ifdef CONFIG_PCI_IOV
 	ppc_md.pcibios_fixup_sriov = pnv_pci_ioda_fixup_iov_resources;
 	ppc_md.pcibios_iov_resource_alignment = pnv_pci_iov_resource_alignment;