| Message ID | 20210408170123.8788-11-logang@deltatee.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add new DMA mapping operation for P2PDMA | expand |
On 4/8/21 10:01 AM, Logan Gunthorpe wrote: > Add a flags member to the dma_map_ops structure with one flag to > indicate support for PCI P2PDMA. > > Also, add a helper to check if a device supports PCI P2PDMA. > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > --- > include/linux/dma-map-ops.h | 3 +++ > include/linux/dma-mapping.h | 5 +++++ > kernel/dma/mapping.c | 18 ++++++++++++++++++ > 3 files changed, 26 insertions(+) > > diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h > index 51872e736e7b..481892822104 100644 > --- a/include/linux/dma-map-ops.h > +++ b/include/linux/dma-map-ops.h > @@ -12,6 +12,9 @@ > struct cma; > > struct dma_map_ops { > + unsigned int flags; > +#define DMA_F_PCI_P2PDMA_SUPPORTED (1 << 0) > + Can we move this up and out of the struct member area, so that it looks more like this: /* * Values for struct dma_map_ops.flags: * * DMA_F_PCI_P2PDMA_SUPPORTED: <documentation here...this is a good place to * explain exactly what this flag is for.> */ #define DMA_F_PCI_P2PDMA_SUPPORTED (1 << 0) struct dma_map_ops { unsigned int flags; > void *(*alloc)(struct device *dev, size_t size, > dma_addr_t *dma_handle, gfp_t gfp, > unsigned long attrs); > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > index 50b8f586cf59..c31980ecca62 100644 > --- a/include/linux/dma-mapping.h > +++ b/include/linux/dma-mapping.h > @@ -146,6 +146,7 @@ int dma_mmap_attrs(struct device *dev, struct vm_area_struct *vma, > unsigned long attrs); > bool dma_can_mmap(struct device *dev); > int dma_supported(struct device *dev, u64 mask); > +bool dma_pci_p2pdma_supported(struct device *dev); > int dma_set_mask(struct device *dev, u64 mask); > int dma_set_coherent_mask(struct device *dev, u64 mask); > u64 dma_get_required_mask(struct device *dev); > @@ -247,6 +248,10 @@ static inline int dma_supported(struct device *dev, u64 mask) > { > return 0; > } > +static inline bool dma_pci_p2pdma_supported(struct device *dev) > +{ > + return 0; Should be: return false; > +} > static inline int dma_set_mask(struct device *dev, u64 mask) > { > return -EIO; > diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c > index 923089c4267b..ce44a0fcc4e5 100644 > --- a/kernel/dma/mapping.c > +++ b/kernel/dma/mapping.c > @@ -573,6 +573,24 @@ int dma_supported(struct device *dev, u64 mask) > } > EXPORT_SYMBOL(dma_supported); > > +bool dma_pci_p2pdma_supported(struct device *dev) > +{ > + const struct dma_map_ops *ops = get_dma_ops(dev); > + > + /* if ops is not set, dma direct will be used which supports P2PDMA */ > + if (!ops) > + return true; > + > + /* > + * Note: dma_ops_bypass is not checked here because P2PDMA should > + * not be used with dma mapping ops that do not have support even > + * if the specific device is bypassing them. > + */ > + > + return ops->flags & DMA_F_PCI_P2PDMA_SUPPORTED; Wow, rather unusual combination of things in order decide this. It feels a bit over-complicated to have flags and ops and a bool function all dealing with the same 1-bit answer, but there is no caller shown here, so I'll have to come back to this after reviewing subsequent patches. thanks,
On 2021-05-02 6:32 p.m., John Hubbard wrote: > On 4/8/21 10:01 AM, Logan Gunthorpe wrote: >> Add a flags member to the dma_map_ops structure with one flag to >> indicate support for PCI P2PDMA. >> >> Also, add a helper to check if a device supports PCI P2PDMA. >> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >> --- >> include/linux/dma-map-ops.h | 3 +++ >> include/linux/dma-mapping.h | 5 +++++ >> kernel/dma/mapping.c | 18 ++++++++++++++++++ >> 3 files changed, 26 insertions(+) >> >> diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h >> index 51872e736e7b..481892822104 100644 >> --- a/include/linux/dma-map-ops.h >> +++ b/include/linux/dma-map-ops.h >> @@ -12,6 +12,9 @@ >> struct cma; >> >> struct dma_map_ops { >> + unsigned int flags; >> +#define DMA_F_PCI_P2PDMA_SUPPORTED (1 << 0) >> + > > Can we move this up and out of the struct member area, so that it looks > more like this: > > /* > * Values for struct dma_map_ops.flags: > * > * DMA_F_PCI_P2PDMA_SUPPORTED: <documentation here...this is a good place to > * explain exactly what this flag is for.> > */ > #define DMA_F_PCI_P2PDMA_SUPPORTED (1 << 0) > > struct dma_map_ops { > unsigned int flags; > Sure, I don't care that much. I was just following the style in nvme.h. >> void *(*alloc)(struct device *dev, size_t size, >> dma_addr_t *dma_handle, gfp_t gfp, >> unsigned long attrs); >> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h >> index 50b8f586cf59..c31980ecca62 100644 >> --- a/include/linux/dma-mapping.h >> +++ b/include/linux/dma-mapping.h >> @@ -146,6 +146,7 @@ int dma_mmap_attrs(struct device *dev, struct vm_area_struct *vma, >> unsigned long attrs); >> bool dma_can_mmap(struct device *dev); >> int dma_supported(struct device *dev, u64 mask); >> +bool dma_pci_p2pdma_supported(struct device *dev); >> int dma_set_mask(struct device *dev, u64 mask); >> int dma_set_coherent_mask(struct device *dev, u64 mask); >> u64 dma_get_required_mask(struct device *dev); >> @@ -247,6 +248,10 @@ static inline int dma_supported(struct device *dev, u64 mask) >> { >> return 0; >> } >> +static inline bool dma_pci_p2pdma_supported(struct device *dev) >> +{ >> + return 0; > > Should be: > > return false; Yup, will fix. >> +} >> static inline int dma_set_mask(struct device *dev, u64 mask) >> { >> return -EIO; >> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c >> index 923089c4267b..ce44a0fcc4e5 100644 >> --- a/kernel/dma/mapping.c >> +++ b/kernel/dma/mapping.c >> @@ -573,6 +573,24 @@ int dma_supported(struct device *dev, u64 mask) >> } >> EXPORT_SYMBOL(dma_supported); >> >> +bool dma_pci_p2pdma_supported(struct device *dev) >> +{ >> + const struct dma_map_ops *ops = get_dma_ops(dev); >> + >> + /* if ops is not set, dma direct will be used which supports P2PDMA */ >> + if (!ops) >> + return true; >> + >> + /* >> + * Note: dma_ops_bypass is not checked here because P2PDMA should >> + * not be used with dma mapping ops that do not have support even >> + * if the specific device is bypassing them. >> + */ >> + >> + return ops->flags & DMA_F_PCI_P2PDMA_SUPPORTED; > > Wow, rather unusual combination of things in order decide this. It feels > a bit over-complicated to have flags and ops and a bool function all > dealing with the same 1-bit answer, but there is no caller shown here, > so I'll have to come back to this after reviewing subsequent patches. Yeah, I originally had it much simpler, but it confused Ira and it was clear it had to be written more explicitly and commented better. Logan
On 4/8/21 1:01 PM, Logan Gunthorpe wrote: > Add a flags member to the dma_map_ops structure with one flag to > indicate support for PCI P2PDMA. > > Also, add a helper to check if a device supports PCI P2PDMA. > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > --- > include/linux/dma-map-ops.h | 3 +++ > include/linux/dma-mapping.h | 5 +++++ > kernel/dma/mapping.c | 18 ++++++++++++++++++ > 3 files changed, 26 insertions(+) > > diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h > index 51872e736e7b..481892822104 100644 > --- a/include/linux/dma-map-ops.h > +++ b/include/linux/dma-map-ops.h > @@ -12,6 +12,9 @@ > struct cma; > > struct dma_map_ops { > + unsigned int flags; > +#define DMA_F_PCI_P2PDMA_SUPPORTED (1 << 0) > + I'm not a fan of in-line define's; if we're going to add a flags field to the dma-ops -- and logically it'd be good to have p2pdma go through the dma-ops struct -- then let's move this up in front of the dma-ops description. And now that the dma-ops struct is being 'opened' for p2pdma, should p2pdma ops be added to this struct, so all this work can be mimic'd/reflected/leveraged/refactored for CXL, GenZ, etc. p2pdma in (the near?) future? > void *(*alloc)(struct device *dev, size_t size, > dma_addr_t *dma_handle, gfp_t gfp, > unsigned long attrs); > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > index 50b8f586cf59..c31980ecca62 100644 > --- a/include/linux/dma-mapping.h > +++ b/include/linux/dma-mapping.h > @@ -146,6 +146,7 @@ int dma_mmap_attrs(struct device *dev, struct vm_area_struct *vma, > unsigned long attrs); > bool dma_can_mmap(struct device *dev); > int dma_supported(struct device *dev, u64 mask); > +bool dma_pci_p2pdma_supported(struct device *dev); > int dma_set_mask(struct device *dev, u64 mask); > int dma_set_coherent_mask(struct device *dev, u64 mask); > u64 dma_get_required_mask(struct device *dev); > @@ -247,6 +248,10 @@ static inline int dma_supported(struct device *dev, u64 mask) > { > return 0; > } > +static inline bool dma_pci_p2pdma_supported(struct device *dev) > +{ > + return 0; > +} > static inline int dma_set_mask(struct device *dev, u64 mask) > { > return -EIO; > diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c > index 923089c4267b..ce44a0fcc4e5 100644 > --- a/kernel/dma/mapping.c > +++ b/kernel/dma/mapping.c > @@ -573,6 +573,24 @@ int dma_supported(struct device *dev, u64 mask) > } > EXPORT_SYMBOL(dma_supported); > > +bool dma_pci_p2pdma_supported(struct device *dev) > +{ > + const struct dma_map_ops *ops = get_dma_ops(dev); > + > + /* if ops is not set, dma direct will be used which supports P2PDMA */ > + if (!ops) > + return true; So, this means one cannot have p2pdma with IOMMU's? ... -- or is this 'for now' and this may change? if it may change, add a note here. > + > + /* > + * Note: dma_ops_bypass is not checked here because P2PDMA should > + * not be used with dma mapping ops that do not have support even > + * if the specific device is bypassing them. > + */ > + > + return ops->flags & DMA_F_PCI_P2PDMA_SUPPORTED; that's a bool? > +} > +EXPORT_SYMBOL_GPL(dma_pci_p2pdma_supported); > + > #ifdef CONFIG_ARCH_HAS_DMA_SET_MASK > void arch_dma_set_mask(struct device *dev, u64 mask); > #else
On 2021-05-11 10:06 a.m., Don Dutile wrote: > On 4/8/21 1:01 PM, Logan Gunthorpe wrote: >> Add a flags member to the dma_map_ops structure with one flag to >> indicate support for PCI P2PDMA. >> >> Also, add a helper to check if a device supports PCI P2PDMA. >> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >> --- >> include/linux/dma-map-ops.h | 3 +++ >> include/linux/dma-mapping.h | 5 +++++ >> kernel/dma/mapping.c | 18 ++++++++++++++++++ >> 3 files changed, 26 insertions(+) >> >> diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h >> index 51872e736e7b..481892822104 100644 >> --- a/include/linux/dma-map-ops.h >> +++ b/include/linux/dma-map-ops.h >> @@ -12,6 +12,9 @@ >> struct cma; >> >> struct dma_map_ops { >> + unsigned int flags; >> +#define DMA_F_PCI_P2PDMA_SUPPORTED (1 << 0) >> + > I'm not a fan of in-line define's; if we're going to add a flags field to the dma-ops > -- and logically it'd be good to have p2pdma go through the dma-ops struct -- > then let's move this up in front of the dma-ops description. Already changed for v2. > And now that the dma-ops struct is being 'opened' for p2pdma, should p2pdma ops be added > to this struct, so all this work can be mimic'd/reflected/leveraged/refactored for CXL, GenZ, etc. p2pdma in (the near?) future? v2 no longer has a specific op for p2pdma. We are now using dma_map_sgtable() which already has the error return we need. I think any work to support CXL, GenZ, etc will need to be done when they add their own support. I can't and shouldn't guess at their needs now. Logan
diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h index 51872e736e7b..481892822104 100644 --- a/include/linux/dma-map-ops.h +++ b/include/linux/dma-map-ops.h @@ -12,6 +12,9 @@ struct cma; struct dma_map_ops { + unsigned int flags; +#define DMA_F_PCI_P2PDMA_SUPPORTED (1 << 0) + void *(*alloc)(struct device *dev, size_t size, dma_addr_t *dma_handle, gfp_t gfp, unsigned long attrs); diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index 50b8f586cf59..c31980ecca62 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -146,6 +146,7 @@ int dma_mmap_attrs(struct device *dev, struct vm_area_struct *vma, unsigned long attrs); bool dma_can_mmap(struct device *dev); int dma_supported(struct device *dev, u64 mask); +bool dma_pci_p2pdma_supported(struct device *dev); int dma_set_mask(struct device *dev, u64 mask); int dma_set_coherent_mask(struct device *dev, u64 mask); u64 dma_get_required_mask(struct device *dev); @@ -247,6 +248,10 @@ static inline int dma_supported(struct device *dev, u64 mask) { return 0; } +static inline bool dma_pci_p2pdma_supported(struct device *dev) +{ + return 0; +} static inline int dma_set_mask(struct device *dev, u64 mask) { return -EIO; diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c index 923089c4267b..ce44a0fcc4e5 100644 --- a/kernel/dma/mapping.c +++ b/kernel/dma/mapping.c @@ -573,6 +573,24 @@ int dma_supported(struct device *dev, u64 mask) } EXPORT_SYMBOL(dma_supported); +bool dma_pci_p2pdma_supported(struct device *dev) +{ + const struct dma_map_ops *ops = get_dma_ops(dev); + + /* if ops is not set, dma direct will be used which supports P2PDMA */ + if (!ops) + return true; + + /* + * Note: dma_ops_bypass is not checked here because P2PDMA should + * not be used with dma mapping ops that do not have support even + * if the specific device is bypassing them. + */ + + return ops->flags & DMA_F_PCI_P2PDMA_SUPPORTED; +} +EXPORT_SYMBOL_GPL(dma_pci_p2pdma_supported); + #ifdef CONFIG_ARCH_HAS_DMA_SET_MASK void arch_dma_set_mask(struct device *dev, u64 mask); #else
Add a flags member to the dma_map_ops structure with one flag to indicate support for PCI P2PDMA. Also, add a helper to check if a device supports PCI P2PDMA. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- include/linux/dma-map-ops.h | 3 +++ include/linux/dma-mapping.h | 5 +++++ kernel/dma/mapping.c | 18 ++++++++++++++++++ 3 files changed, 26 insertions(+)