| Message ID | 20210805005218.2912076-10-sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add TDX Guest Support (shared-mm support) | expand |
Is there a branch with all of this applied? I was going to apply this to help take a look at it, but it doesn't apply to v5.14-rc1. I know you listed some prereqs in the cover letter, but it's a fair amount of work to sort all that out. On Wed, Aug 04, 2021 at 05:52:12PM -0700, Kuppuswamy Sathyanarayanan wrote: > From: Andi Kleen <ak@linux.intel.com> If I were applying these, I would silently update the subject lines to match previous commits. Since these will probably be merged via a different tree, you can update if there's a v5: PCI: Consolidate pci_iomap_range(), pci_iomap_wc_range() Also applies to 11/15 and 12/15. > pci_iomap* and pci_iomap*wc are currently duplicated code, except > that the _wc variant does not support IO ports. Replace them > with a common helper and a callback for the mapping. I used > wrappers for the maps because some architectures implement ioremap > and friends with macros. Maybe spell some of this out: pci_iomap_range() and pci_iomap_wc_range() are currently duplicated code, ... Implement them using a common helper, pci_iomap_range_map(), ... Using "pci_iomap*" obscures the name and doesn't save any space. Why is it safe to make pci_iomap_wc_range() support IO ports when it didn't before? That might be desirable, but I think it *is* a functional change here. IIUC, pci_iomap_wc_range() on an IO port range previously returned NULL, and after this patch it will work the same as pci_iomap_range(), i.e., it will return the result of __pci_ioport_map(). > This will allow to add more variants without excessive code > duplications. This patch should have no behavior change. > > Signed-off-by: Andi Kleen <ak@linux.intel.com> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > --- > lib/pci_iomap.c | 81 +++++++++++++++++++++++++++---------------------- > 1 file changed, 44 insertions(+), 37 deletions(-) > > diff --git a/lib/pci_iomap.c b/lib/pci_iomap.c > index 2d3eb1cb73b8..6251c3f651c6 100644 > --- a/lib/pci_iomap.c > +++ b/lib/pci_iomap.c > @@ -10,6 +10,46 @@ > #include <linux/export.h> > > #ifdef CONFIG_PCI > + > +/* > + * Callback wrappers because some architectures define ioremap et.al. > + * as macros. > + */ > +static void __iomem *map_ioremap(phys_addr_t addr, size_t size) > +{ > + return ioremap(addr, size); > +} > + > +static void __iomem *map_ioremap_wc(phys_addr_t addr, size_t size) > +{ > + return ioremap_wc(addr, size); > +} > + > +static void __iomem *pci_iomap_range_map(struct pci_dev *dev, > + int bar, > + unsigned long offset, > + unsigned long maxlen, > + void __iomem *(*mapm)(phys_addr_t, > + size_t)) > +{ > + resource_size_t start = pci_resource_start(dev, bar); > + resource_size_t len = pci_resource_len(dev, bar); > + unsigned long flags = pci_resource_flags(dev, bar); > + > + if (len <= offset || !start) > + return NULL; > + len -= offset; > + start += offset; > + if (maxlen && len > maxlen) > + len = maxlen; > + if (flags & IORESOURCE_IO) > + return __pci_ioport_map(dev, start, len); > + if (flags & IORESOURCE_MEM) > + return mapm(start, len); > + /* What? */ > + return NULL; > +} > + > /** > * pci_iomap_range - create a virtual mapping cookie for a PCI BAR > * @dev: PCI device that owns the BAR > @@ -30,22 +70,8 @@ void __iomem *pci_iomap_range(struct pci_dev *dev, > unsigned long offset, > unsigned long maxlen) > { > - resource_size_t start = pci_resource_start(dev, bar); > - resource_size_t len = pci_resource_len(dev, bar); > - unsigned long flags = pci_resource_flags(dev, bar); > - > - if (len <= offset || !start) > - return NULL; > - len -= offset; > - start += offset; > - if (maxlen && len > maxlen) > - len = maxlen; > - if (flags & IORESOURCE_IO) > - return __pci_ioport_map(dev, start, len); > - if (flags & IORESOURCE_MEM) > - return ioremap(start, len); > - /* What? */ > - return NULL; > + return pci_iomap_range_map(dev, bar, offset, maxlen, > + map_ioremap); > } > EXPORT_SYMBOL(pci_iomap_range); > > @@ -70,27 +96,8 @@ void __iomem *pci_iomap_wc_range(struct pci_dev *dev, > unsigned long offset, > unsigned long maxlen) > { > - resource_size_t start = pci_resource_start(dev, bar); > - resource_size_t len = pci_resource_len(dev, bar); > - unsigned long flags = pci_resource_flags(dev, bar); > - > - > - if (flags & IORESOURCE_IO) > - return NULL; > - > - if (len <= offset || !start) > - return NULL; > - > - len -= offset; > - start += offset; > - if (maxlen && len > maxlen) > - len = maxlen; > - > - if (flags & IORESOURCE_MEM) > - return ioremap_wc(start, len); > - > - /* What? */ > - return NULL; > + return pci_iomap_range_map(dev, bar, offset, maxlen, > + map_ioremap_wc); > } > EXPORT_SYMBOL_GPL(pci_iomap_wc_range); > > -- > 2.25.1 >
> Why is it safe to make pci_iomap_wc_range() support IO ports when it > didn't before? That might be desirable, but I think it *is* a > functional change here. None of the callers use it to map IO ports, so it will be a no-op for them. But you're right, it's a (minor) functional change. -Andi
On 8/12/21 12:43 PM, Bjorn Helgaas wrote: > Is there a branch with all of this applied? I was going to apply this Its is maintained in following tree. https://github.com/intel/tdx/commit/93fd5b655172ba9e3350487995102a8b2c41de27 > to help take a look at it, but it doesn't apply to v5.14-rc1. I know This patch can be applied independently. I have just applied it on top of v5.14-rc5, and it seems to apply clean. Can you try -rc5? > you listed some prereqs in the cover letter, but it's a fair amount of > work to sort all that out. > > On Wed, Aug 04, 2021 at 05:52:12PM -0700, Kuppuswamy Sathyanarayanan wrote: >> From: Andi Kleen <ak@linux.intel.com> > > If I were applying these, I would silently update the subject lines to > match previous commits. Since these will probably be merged via a > different tree, you can update if there's a v5: > > PCI: Consolidate pci_iomap_range(), pci_iomap_wc_range() Yes. I will fix this in next version. > > Also applies to 11/15 and 12/15. Will do the same. > >> pci_iomap* and pci_iomap*wc are currently duplicated code, except >> that the _wc variant does not support IO ports. Replace them >> with a common helper and a callback for the mapping. I used >> wrappers for the maps because some architectures implement ioremap >> and friends with macros. > > Maybe spell some of this out: > > pci_iomap_range() and pci_iomap_wc_range() are currently duplicated > code, ... Implement them using a common helper, > pci_iomap_range_map(), ... > > Using "pci_iomap*" obscures the name and doesn't save any space. > > Why is it safe to make pci_iomap_wc_range() support IO ports when it > didn't before? That might be desirable, but I think it *is* a > functional change here. Agree. Commit log had to be updated. I will include these details in next submission. > > IIUC, pci_iomap_wc_range() on an IO port range previously returned > NULL, and after this patch it will work the same as pci_iomap_range(), > i.e., it will return the result of __pci_ioport_map(). > >> This will allow to add more variants without excessive code >> duplications. This patch should have no behavior change. >> >> Signed-off-by: Andi Kleen <ak@linux.intel.com> >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> --- >> lib/pci_iomap.c | 81 +++++++++++++++++++++++++++---------------------- >> 1 file changed, 44 insertions(+), 37 deletions(-) >>
diff --git a/lib/pci_iomap.c b/lib/pci_iomap.c index 2d3eb1cb73b8..6251c3f651c6 100644 --- a/lib/pci_iomap.c +++ b/lib/pci_iomap.c @@ -10,6 +10,46 @@ #include <linux/export.h> #ifdef CONFIG_PCI + +/* + * Callback wrappers because some architectures define ioremap et.al. + * as macros. + */ +static void __iomem *map_ioremap(phys_addr_t addr, size_t size) +{ + return ioremap(addr, size); +} + +static void __iomem *map_ioremap_wc(phys_addr_t addr, size_t size) +{ + return ioremap_wc(addr, size); +} + +static void __iomem *pci_iomap_range_map(struct pci_dev *dev, + int bar, + unsigned long offset, + unsigned long maxlen, + void __iomem *(*mapm)(phys_addr_t, + size_t)) +{ + resource_size_t start = pci_resource_start(dev, bar); + resource_size_t len = pci_resource_len(dev, bar); + unsigned long flags = pci_resource_flags(dev, bar); + + if (len <= offset || !start) + return NULL; + len -= offset; + start += offset; + if (maxlen && len > maxlen) + len = maxlen; + if (flags & IORESOURCE_IO) + return __pci_ioport_map(dev, start, len); + if (flags & IORESOURCE_MEM) + return mapm(start, len); + /* What? */ + return NULL; +} + /** * pci_iomap_range - create a virtual mapping cookie for a PCI BAR * @dev: PCI device that owns the BAR @@ -30,22 +70,8 @@ void __iomem *pci_iomap_range(struct pci_dev *dev, unsigned long offset, unsigned long maxlen) { - resource_size_t start = pci_resource_start(dev, bar); - resource_size_t len = pci_resource_len(dev, bar); - unsigned long flags = pci_resource_flags(dev, bar); - - if (len <= offset || !start) - return NULL; - len -= offset; - start += offset; - if (maxlen && len > maxlen) - len = maxlen; - if (flags & IORESOURCE_IO) - return __pci_ioport_map(dev, start, len); - if (flags & IORESOURCE_MEM) - return ioremap(start, len); - /* What? */ - return NULL; + return pci_iomap_range_map(dev, bar, offset, maxlen, + map_ioremap); } EXPORT_SYMBOL(pci_iomap_range); @@ -70,27 +96,8 @@ void __iomem *pci_iomap_wc_range(struct pci_dev *dev, unsigned long offset, unsigned long maxlen) { - resource_size_t start = pci_resource_start(dev, bar); - resource_size_t len = pci_resource_len(dev, bar); - unsigned long flags = pci_resource_flags(dev, bar); - - - if (flags & IORESOURCE_IO) - return NULL; - - if (len <= offset || !start) - return NULL; - - len -= offset; - start += offset; - if (maxlen && len > maxlen) - len = maxlen; - - if (flags & IORESOURCE_MEM) - return ioremap_wc(start, len); - - /* What? */ - return NULL; + return pci_iomap_range_map(dev, bar, offset, maxlen, + map_ioremap_wc); } EXPORT_SYMBOL_GPL(pci_iomap_wc_range);