On Fri, Oct 11, 2019 at 06:25:18PM -0700, Ram Pai wrote:> From: Thiago Jung Bauermann <bauerman at linux.ibm.com> > > In order to safely use the DMA API, virtio needs to know whether DMA > addresses are in fact physical addresses and for that purpose, > dma_addr_is_phys_addr() is introduced. > > cc: Benjamin Herrenschmidt <benh at kernel.crashing.org> > cc: David Gibson <david at gibson.dropbear.id.au> > cc: Michael Ellerman <mpe at ellerman.id.au> > cc: Paul Mackerras <paulus at ozlabs.org> > cc: Michael Roth <mdroth at linux.vnet.ibm.com> > cc: Alexey Kardashevskiy <aik at linux.ibm.com> > cc: Paul Burton <paul.burton at mips.com> > cc: Robin Murphy <robin.murphy at arm.com> > cc: Bartlomiej Zolnierkiewicz <b.zolnierkie at samsung.com> > cc: Marek Szyprowski <m.szyprowski at samsung.com> > cc: Christoph Hellwig <hch at lst.de> > Suggested-by: Michael S. Tsirkin <mst at redhat.com> > Signed-off-by: Ram Pai <linuxram at us.ibm.com> > Signed-off-by: Thiago Jung Bauermann <bauerman at linux.ibm.com>The change itself looks ok, so Reviewed-by: David Gibson <david at gibson.dropbear.id.au> However, I would like to see the commit message (and maybe the inline comments) expanded a bit on what the distinction here is about. Some of the text from the next patch would be suitable, about DMA addresses usually being in a different address space but not in the case of bounce buffering.> --- > arch/powerpc/include/asm/dma-mapping.h | 21 +++++++++++++++++++++ > arch/powerpc/platforms/pseries/Kconfig | 1 + > include/linux/dma-mapping.h | 20 ++++++++++++++++++++ > kernel/dma/Kconfig | 3 +++ > 4 files changed, 45 insertions(+) > > diff --git a/arch/powerpc/include/asm/dma-mapping.h b/arch/powerpc/include/asm/dma-mapping.h > index 565d6f7..f92c0a4b 100644 > --- a/arch/powerpc/include/asm/dma-mapping.h > +++ b/arch/powerpc/include/asm/dma-mapping.h > @@ -5,6 +5,8 @@ > #ifndef _ASM_DMA_MAPPING_H > #define _ASM_DMA_MAPPING_H > > +#include <asm/svm.h> > + > static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus) > { > /* We don't handle the NULL dev case for ISA for now. We could > @@ -15,4 +17,23 @@ static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus) > return NULL; > } > > +#ifdef CONFIG_ARCH_HAS_DMA_ADDR_IS_PHYS_ADDR > +/** > + * dma_addr_is_phys_addr - check whether a device DMA address is a physical > + * address > + * @dev: device to check > + * > + * Returns %true if any DMA address for this device happens to also be a valid > + * physical address (not necessarily of the same page). > + */ > +static inline bool dma_addr_is_phys_addr(struct device *dev) > +{ > + /* > + * Secure guests always use the SWIOTLB, therefore DMA addresses are > + * actually the physical address of the bounce buffer. > + */ > + return is_secure_guest(); > +} > +#endif > + > #endif /* _ASM_DMA_MAPPING_H */ > diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig > index 9e35cdd..0108150 100644 > --- a/arch/powerpc/platforms/pseries/Kconfig > +++ b/arch/powerpc/platforms/pseries/Kconfig > @@ -152,6 +152,7 @@ config PPC_SVM > select SWIOTLB > select ARCH_HAS_MEM_ENCRYPT > select ARCH_HAS_FORCE_DMA_UNENCRYPTED > + select ARCH_HAS_DMA_ADDR_IS_PHYS_ADDR > help > There are certain POWER platforms which support secure guests using > the Protected Execution Facility, with the help of an Ultravisor > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > index f7d1eea..6df5664 100644 > --- a/include/linux/dma-mapping.h > +++ b/include/linux/dma-mapping.h > @@ -693,6 +693,26 @@ static inline bool dma_addressing_limited(struct device *dev) > dma_get_required_mask(dev); > } > > +#ifndef CONFIG_ARCH_HAS_DMA_ADDR_IS_PHYS_ADDR > +/** > + * dma_addr_is_phys_addr - check whether a device DMA address is a physical > + * address > + * @dev: device to check > + * > + * Returns %true if any DMA address for this device happens to also be a valid > + * physical address (not necessarily of the same page). > + */ > +static inline bool dma_addr_is_phys_addr(struct device *dev) > +{ > + /* > + * Except in very specific setups, DMA addresses exist in a different > + * address space from CPU physical addresses and cannot be directly used > + * to reference system memory. > + */ > + return false; > +} > +#endif > + > #ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS > void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, > const struct iommu_ops *iommu, bool coherent); > diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig > index 9decbba..6209b46 100644 > --- a/kernel/dma/Kconfig > +++ b/kernel/dma/Kconfig > @@ -51,6 +51,9 @@ config ARCH_HAS_DMA_MMAP_PGPROT > config ARCH_HAS_FORCE_DMA_UNENCRYPTED > bool > > +config ARCH_HAS_DMA_ADDR_IS_PHYS_ADDR > + bool > + > config DMA_NONCOHERENT_CACHE_SYNC > bool >-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 833 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20191014/7cb36119/attachment.sig>
On 14/10/2019 05:51, David Gibson wrote:> On Fri, Oct 11, 2019 at 06:25:18PM -0700, Ram Pai wrote: >> From: Thiago Jung Bauermann <bauerman at linux.ibm.com> >> >> In order to safely use the DMA API, virtio needs to know whether DMA >> addresses are in fact physical addresses and for that purpose, >> dma_addr_is_phys_addr() is introduced. >> >> cc: Benjamin Herrenschmidt <benh at kernel.crashing.org> >> cc: David Gibson <david at gibson.dropbear.id.au> >> cc: Michael Ellerman <mpe at ellerman.id.au> >> cc: Paul Mackerras <paulus at ozlabs.org> >> cc: Michael Roth <mdroth at linux.vnet.ibm.com> >> cc: Alexey Kardashevskiy <aik at linux.ibm.com> >> cc: Paul Burton <paul.burton at mips.com> >> cc: Robin Murphy <robin.murphy at arm.com> >> cc: Bartlomiej Zolnierkiewicz <b.zolnierkie at samsung.com> >> cc: Marek Szyprowski <m.szyprowski at samsung.com> >> cc: Christoph Hellwig <hch at lst.de> >> Suggested-by: Michael S. Tsirkin <mst at redhat.com> >> Signed-off-by: Ram Pai <linuxram at us.ibm.com> >> Signed-off-by: Thiago Jung Bauermann <bauerman at linux.ibm.com> > > The change itself looks ok, so > > Reviewed-by: David Gibson <david at gibson.dropbear.id.au> > > However, I would like to see the commit message (and maybe the inline > comments) expanded a bit on what the distinction here is about. Some > of the text from the next patch would be suitable, about DMA addresses > usually being in a different address space but not in the case of > bounce buffering.Right, this needs a much tighter definition. "DMA address happens to be a valid physical address" is true of various IOMMU setups too, but I can't believe it's meaningful in such cases. If what you actually want is "DMA is direct or SWIOTLB" - i.e. "DMA address is physical address of DMA data (not necessarily the original buffer)" - wouldn't dma_is_direct() suffice? Robin.>> --- >> arch/powerpc/include/asm/dma-mapping.h | 21 +++++++++++++++++++++ >> arch/powerpc/platforms/pseries/Kconfig | 1 + >> include/linux/dma-mapping.h | 20 ++++++++++++++++++++ >> kernel/dma/Kconfig | 3 +++ >> 4 files changed, 45 insertions(+) >> >> diff --git a/arch/powerpc/include/asm/dma-mapping.h b/arch/powerpc/include/asm/dma-mapping.h >> index 565d6f7..f92c0a4b 100644 >> --- a/arch/powerpc/include/asm/dma-mapping.h >> +++ b/arch/powerpc/include/asm/dma-mapping.h >> @@ -5,6 +5,8 @@ >> #ifndef _ASM_DMA_MAPPING_H >> #define _ASM_DMA_MAPPING_H >> >> +#include <asm/svm.h> >> + >> static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus) >> { >> /* We don't handle the NULL dev case for ISA for now. We could >> @@ -15,4 +17,23 @@ static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus) >> return NULL; >> } >> >> +#ifdef CONFIG_ARCH_HAS_DMA_ADDR_IS_PHYS_ADDR >> +/** >> + * dma_addr_is_phys_addr - check whether a device DMA address is a physical >> + * address >> + * @dev: device to check >> + * >> + * Returns %true if any DMA address for this device happens to also be a valid >> + * physical address (not necessarily of the same page). >> + */ >> +static inline bool dma_addr_is_phys_addr(struct device *dev) >> +{ >> + /* >> + * Secure guests always use the SWIOTLB, therefore DMA addresses are >> + * actually the physical address of the bounce buffer. >> + */ >> + return is_secure_guest(); >> +} >> +#endif >> + >> #endif /* _ASM_DMA_MAPPING_H */ >> diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig >> index 9e35cdd..0108150 100644 >> --- a/arch/powerpc/platforms/pseries/Kconfig >> +++ b/arch/powerpc/platforms/pseries/Kconfig >> @@ -152,6 +152,7 @@ config PPC_SVM >> select SWIOTLB >> select ARCH_HAS_MEM_ENCRYPT >> select ARCH_HAS_FORCE_DMA_UNENCRYPTED >> + select ARCH_HAS_DMA_ADDR_IS_PHYS_ADDR >> help >> There are certain POWER platforms which support secure guests using >> the Protected Execution Facility, with the help of an Ultravisor >> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h >> index f7d1eea..6df5664 100644 >> --- a/include/linux/dma-mapping.h >> +++ b/include/linux/dma-mapping.h >> @@ -693,6 +693,26 @@ static inline bool dma_addressing_limited(struct device *dev) >> dma_get_required_mask(dev); >> } >> >> +#ifndef CONFIG_ARCH_HAS_DMA_ADDR_IS_PHYS_ADDR >> +/** >> + * dma_addr_is_phys_addr - check whether a device DMA address is a physical >> + * address >> + * @dev: device to check >> + * >> + * Returns %true if any DMA address for this device happens to also be a valid >> + * physical address (not necessarily of the same page). >> + */ >> +static inline bool dma_addr_is_phys_addr(struct device *dev) >> +{ >> + /* >> + * Except in very specific setups, DMA addresses exist in a different >> + * address space from CPU physical addresses and cannot be directly used >> + * to reference system memory. >> + */ >> + return false; >> +} >> +#endif >> + >> #ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS >> void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, >> const struct iommu_ops *iommu, bool coherent); >> diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig >> index 9decbba..6209b46 100644 >> --- a/kernel/dma/Kconfig >> +++ b/kernel/dma/Kconfig >> @@ -51,6 +51,9 @@ config ARCH_HAS_DMA_MMAP_PGPROT >> config ARCH_HAS_FORCE_DMA_UNENCRYPTED >> bool >> >> +config ARCH_HAS_DMA_ADDR_IS_PHYS_ADDR >> + bool >> + >> config DMA_NONCOHERENT_CACHE_SYNC >> bool >> >
On Mon, Oct 14, 2019 at 11:29:24AM +0100, Robin Murphy wrote:> On 14/10/2019 05:51, David Gibson wrote: > >On Fri, Oct 11, 2019 at 06:25:18PM -0700, Ram Pai wrote: > >>From: Thiago Jung Bauermann <bauerman at linux.ibm.com> > >> > >>In order to safely use the DMA API, virtio needs to know whether DMA > >>addresses are in fact physical addresses and for that purpose, > >>dma_addr_is_phys_addr() is introduced. > >> > >>cc: Benjamin Herrenschmidt <benh at kernel.crashing.org> > >>cc: David Gibson <david at gibson.dropbear.id.au> > >>cc: Michael Ellerman <mpe at ellerman.id.au> > >>cc: Paul Mackerras <paulus at ozlabs.org> > >>cc: Michael Roth <mdroth at linux.vnet.ibm.com> > >>cc: Alexey Kardashevskiy <aik at linux.ibm.com> > >>cc: Paul Burton <paul.burton at mips.com> > >>cc: Robin Murphy <robin.murphy at arm.com> > >>cc: Bartlomiej Zolnierkiewicz <b.zolnierkie at samsung.com> > >>cc: Marek Szyprowski <m.szyprowski at samsung.com> > >>cc: Christoph Hellwig <hch at lst.de> > >>Suggested-by: Michael S. Tsirkin <mst at redhat.com> > >>Signed-off-by: Ram Pai <linuxram at us.ibm.com> > >>Signed-off-by: Thiago Jung Bauermann <bauerman at linux.ibm.com> > > > >The change itself looks ok, so > > > >Reviewed-by: David Gibson <david at gibson.dropbear.id.au> > > > >However, I would like to see the commit message (and maybe the inline > >comments) expanded a bit on what the distinction here is about. Some > >of the text from the next patch would be suitable, about DMA addresses > >usually being in a different address space but not in the case of > >bounce buffering. > > Right, this needs a much tighter definition. "DMA address happens to > be a valid physical address" is true of various IOMMU setups too, > but I can't believe it's meaningful in such cases.The definition by itself is meaningful AFAICT. At its core its just indicating whether DMA addresses are physical addresses or not. However its up to the caller to use it meaningfully. For non-virtio caller, dma_addr_is_phys_addr() by itself may or may not be meaningful. But for a virtio caller, this information when paired with mem_encrypt_active() is meaningful. For IOMMU setups DMA API will get used regardless of "DMA address happens to be a valid physical address"> > If what you actually want is "DMA is direct or SWIOTLB" - i.e. "DMA > address is physical address of DMA data (not necessarily the > original buffer)" - wouldn't dma_is_direct() suffice?This may also work, I think. MST: thoughts? RP
Christoph Hellwig
2019-Oct-15 07:31 UTC
[PATCH 1/2] dma-mapping: Add dma_addr_is_phys_addr()
On Mon, Oct 14, 2019 at 11:29:24AM +0100, Robin Murphy wrote:>> However, I would like to see the commit message (and maybe the inline >> comments) expanded a bit on what the distinction here is about. Some >> of the text from the next patch would be suitable, about DMA addresses >> usually being in a different address space but not in the case of >> bounce buffering. > > Right, this needs a much tighter definition. "DMA address happens to be a > valid physical address" is true of various IOMMU setups too, but I can't > believe it's meaningful in such cases. > > If what you actually want is "DMA is direct or SWIOTLB" - i.e. "DMA address > is physical address of DMA data (not necessarily the original buffer)" - > wouldn't dma_is_direct() suffice?It would. But drivers have absolutely no business knowing any of this.
Seemingly Similar Threads
- [PATCH 1/2] dma-mapping: Add dma_addr_is_phys_addr()
- [PATCH 1/2] dma-mapping: Add dma_addr_is_phys_addr()
- [PATCH 1/2] dma-mapping: Add dma_addr_is_phys_addr()
- [PATCH 0/2] virtio: Support encrypted memory on powerpc secure guests
- [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted