| Message ID | 1570843519-8696-2-git-send-email-linuxram@us.ibm.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | virtio: Support encrypted memory on powerpc secure guests | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (600802af9049be799465b24d14162918545634bf) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 73 lines checked |
On Fri, Oct 11, 2019 at 06:25:18PM -0700, Ram Pai wrote: > From: Thiago Jung Bauermann <bauerman@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@kernel.crashing.org> > cc: David Gibson <david@gibson.dropbear.id.au> > cc: Michael Ellerman <mpe@ellerman.id.au> > cc: Paul Mackerras <paulus@ozlabs.org> > cc: Michael Roth <mdroth@linux.vnet.ibm.com> > cc: Alexey Kardashevskiy <aik@linux.ibm.com> > cc: Paul Burton <paul.burton@mips.com> > cc: Robin Murphy <robin.murphy@arm.com> > cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > cc: Marek Szyprowski <m.szyprowski@samsung.com> > cc: Christoph Hellwig <hch@lst.de> > Suggested-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Ram Pai <linuxram@us.ibm.com> > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> The change itself looks ok, so Reviewed-by: David Gibson <david@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 >
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@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@kernel.crashing.org> >> cc: David Gibson <david@gibson.dropbear.id.au> >> cc: Michael Ellerman <mpe@ellerman.id.au> >> cc: Paul Mackerras <paulus@ozlabs.org> >> cc: Michael Roth <mdroth@linux.vnet.ibm.com> >> cc: Alexey Kardashevskiy <aik@linux.ibm.com> >> cc: Paul Burton <paul.burton@mips.com> >> cc: Robin Murphy <robin.murphy@arm.com> >> cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >> cc: Marek Szyprowski <m.szyprowski@samsung.com> >> cc: Christoph Hellwig <hch@lst.de> >> Suggested-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Ram Pai <linuxram@us.ibm.com> >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > The change itself looks ok, so > > Reviewed-by: David Gibson <david@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@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@kernel.crashing.org> > >>cc: David Gibson <david@gibson.dropbear.id.au> > >>cc: Michael Ellerman <mpe@ellerman.id.au> > >>cc: Paul Mackerras <paulus@ozlabs.org> > >>cc: Michael Roth <mdroth@linux.vnet.ibm.com> > >>cc: Alexey Kardashevskiy <aik@linux.ibm.com> > >>cc: Paul Burton <paul.burton@mips.com> > >>cc: Robin Murphy <robin.murphy@arm.com> > >>cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > >>cc: Marek Szyprowski <m.szyprowski@samsung.com> > >>cc: Christoph Hellwig <hch@lst.de> > >>Suggested-by: Michael S. Tsirkin <mst@redhat.com> > >>Signed-off-by: Ram Pai <linuxram@us.ibm.com> > >>Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > > >The change itself looks ok, so > > > >Reviewed-by: David Gibson <david@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
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.
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