| Message ID | 20170417213653.21092.27784.stgit@bhelgaas-glaptop.roam.corp.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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
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
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
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
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
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;