| Message ID | 20170216154619.19244.76653.stgit@tlendack-t1.amdoffice.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 16, 2017 at 09:46:19AM -0600, Tom Lendacky wrote: > Add warnings to let the user know when bounce buffers are being used for > DMA when SME is active. Since the bounce buffers are not in encrypted > memory, these notifications are to allow the user to determine some > appropriate action - if necessary. > > Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > arch/x86/include/asm/mem_encrypt.h | 11 +++++++++++ > include/linux/dma-mapping.h | 11 +++++++++++ > include/linux/mem_encrypt.h | 6 ++++++ > lib/swiotlb.c | 3 +++ > 4 files changed, 31 insertions(+) > > diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h > index 87e816f..5a17f1b 100644 > --- a/arch/x86/include/asm/mem_encrypt.h > +++ b/arch/x86/include/asm/mem_encrypt.h > @@ -26,6 +26,11 @@ static inline bool sme_active(void) > return (sme_me_mask) ? true : false; > } > > +static inline u64 sme_dma_mask(void) > +{ > + return ((u64)sme_me_mask << 1) - 1; > +} > + > void __init sme_early_encrypt(resource_size_t paddr, > unsigned long size); > void __init sme_early_decrypt(resource_size_t paddr, > @@ -53,6 +58,12 @@ static inline bool sme_active(void) > { > return false; > } > + > +static inline u64 sme_dma_mask(void) > +{ > + return 0ULL; > +} > + > #endif > > static inline void __init sme_early_encrypt(resource_size_t paddr, > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > index 10c5a17..130bef7 100644 > --- a/include/linux/dma-mapping.h > +++ b/include/linux/dma-mapping.h > @@ -10,6 +10,7 @@ > #include <linux/scatterlist.h> > #include <linux/kmemcheck.h> > #include <linux/bug.h> > +#include <linux/mem_encrypt.h> > > /** > * List of possible attributes associated with a DMA mapping. The semantics > @@ -557,6 +558,11 @@ static inline int dma_set_mask(struct device *dev, u64 mask) > > if (!dev->dma_mask || !dma_supported(dev, mask)) > return -EIO; > + > + if (sme_active() && (mask < sme_dma_mask())) > + dev_warn(dev, > + "SME is active, device will require DMA bounce buffers\n"); You can make it one line. But I am wondering if you should use printk_ratelimit as this may fill the console up. > + > *dev->dma_mask = mask; > return 0; > } > @@ -576,6 +582,11 @@ static inline int dma_set_coherent_mask(struct device *dev, u64 mask) > { > if (!dma_supported(dev, mask)) > return -EIO; > + > + if (sme_active() && (mask < sme_dma_mask())) > + dev_warn(dev, > + "SME is active, device will require DMA bounce buffers\n"); Ditto. > + > dev->coherent_dma_mask = mask; > return 0; > } > diff --git a/include/linux/mem_encrypt.h b/include/linux/mem_encrypt.h > index 14a7b9f..6829ff1 100644 > --- a/include/linux/mem_encrypt.h > +++ b/include/linux/mem_encrypt.h > @@ -28,6 +28,12 @@ static inline bool sme_active(void) > { > return false; > } > + > +static inline u64 sme_dma_mask(void) > +{ > + return 0ULL; > +} > + > #endif > > #endif /* CONFIG_AMD_MEM_ENCRYPT */ > diff --git a/lib/swiotlb.c b/lib/swiotlb.c > index c463067..aff9353 100644 > --- a/lib/swiotlb.c > +++ b/lib/swiotlb.c > @@ -509,6 +509,9 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, > if (no_iotlb_memory) > panic("Can not allocate SWIOTLB buffer earlier and can't now provide you with the DMA bounce buffer"); > > + WARN_ONCE(sme_active(), > + "SME is active and system is using DMA bounce buffers\n"); How does that help? As in what can the user do with this? > + > mask = dma_get_seg_boundary(hwdev); > > tbl_dma_addr &= mask; >
On 2/17/2017 9:59 AM, Konrad Rzeszutek Wilk wrote: > On Thu, Feb 16, 2017 at 09:46:19AM -0600, Tom Lendacky wrote: >> Add warnings to let the user know when bounce buffers are being used for >> DMA when SME is active. Since the bounce buffers are not in encrypted >> memory, these notifications are to allow the user to determine some >> appropriate action - if necessary. >> >> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> >> --- >> arch/x86/include/asm/mem_encrypt.h | 11 +++++++++++ >> include/linux/dma-mapping.h | 11 +++++++++++ >> include/linux/mem_encrypt.h | 6 ++++++ >> lib/swiotlb.c | 3 +++ >> 4 files changed, 31 insertions(+) >> >> diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h >> index 87e816f..5a17f1b 100644 >> --- a/arch/x86/include/asm/mem_encrypt.h >> +++ b/arch/x86/include/asm/mem_encrypt.h >> @@ -26,6 +26,11 @@ static inline bool sme_active(void) >> return (sme_me_mask) ? true : false; >> } >> >> +static inline u64 sme_dma_mask(void) >> +{ >> + return ((u64)sme_me_mask << 1) - 1; >> +} >> + >> void __init sme_early_encrypt(resource_size_t paddr, >> unsigned long size); >> void __init sme_early_decrypt(resource_size_t paddr, >> @@ -53,6 +58,12 @@ static inline bool sme_active(void) >> { >> return false; >> } >> + >> +static inline u64 sme_dma_mask(void) >> +{ >> + return 0ULL; >> +} >> + >> #endif >> >> static inline void __init sme_early_encrypt(resource_size_t paddr, >> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h >> index 10c5a17..130bef7 100644 >> --- a/include/linux/dma-mapping.h >> +++ b/include/linux/dma-mapping.h >> @@ -10,6 +10,7 @@ >> #include <linux/scatterlist.h> >> #include <linux/kmemcheck.h> >> #include <linux/bug.h> >> +#include <linux/mem_encrypt.h> >> >> /** >> * List of possible attributes associated with a DMA mapping. The semantics >> @@ -557,6 +558,11 @@ static inline int dma_set_mask(struct device *dev, u64 mask) >> >> if (!dev->dma_mask || !dma_supported(dev, mask)) >> return -EIO; >> + >> + if (sme_active() && (mask < sme_dma_mask())) >> + dev_warn(dev, >> + "SME is active, device will require DMA bounce buffers\n"); > > You can make it one line. But I am wondering if you should use > printk_ratelimit as this may fill the console up. I thought the use of dma_set_mask() was mostly a one time probe/setup thing so I didn't think we would get that many of these messages. If dma_set_mask() is called much more often that that I can change this to a printk_ratelimit(). I'll look into it further. > >> + >> *dev->dma_mask = mask; >> return 0; >> } >> @@ -576,6 +582,11 @@ static inline int dma_set_coherent_mask(struct device *dev, u64 mask) >> { >> if (!dma_supported(dev, mask)) >> return -EIO; >> + >> + if (sme_active() && (mask < sme_dma_mask())) >> + dev_warn(dev, >> + "SME is active, device will require DMA bounce buffers\n"); > > Ditto. >> + >> dev->coherent_dma_mask = mask; >> return 0; >> } >> diff --git a/include/linux/mem_encrypt.h b/include/linux/mem_encrypt.h >> index 14a7b9f..6829ff1 100644 >> --- a/include/linux/mem_encrypt.h >> +++ b/include/linux/mem_encrypt.h >> @@ -28,6 +28,12 @@ static inline bool sme_active(void) >> { >> return false; >> } >> + >> +static inline u64 sme_dma_mask(void) >> +{ >> + return 0ULL; >> +} >> + >> #endif >> >> #endif /* CONFIG_AMD_MEM_ENCRYPT */ >> diff --git a/lib/swiotlb.c b/lib/swiotlb.c >> index c463067..aff9353 100644 >> --- a/lib/swiotlb.c >> +++ b/lib/swiotlb.c >> @@ -509,6 +509,9 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, >> if (no_iotlb_memory) >> panic("Can not allocate SWIOTLB buffer earlier and can't now provide you with the DMA bounce buffer"); >> >> + WARN_ONCE(sme_active(), >> + "SME is active and system is using DMA bounce buffers\n"); > > How does that help? > > As in what can the user do with this? It's meant just to notify the user about the condition. The user could then decide to use an alternative device that supports a greater DMA range (I can probably change it to a dev_warn_once() so that a device is identified). I would be nice if I could issue this message once per device that experienced this. I didn't see anything that would do that, though. Thanks, Tom >> + >> mask = dma_get_seg_boundary(hwdev); >> >> tbl_dma_addr &= mask; >>
On Thu, Feb 16, 2017 at 09:46:19AM -0600, Tom Lendacky wrote: > Add warnings to let the user know when bounce buffers are being used for > DMA when SME is active. Since the bounce buffers are not in encrypted > memory, these notifications are to allow the user to determine some > appropriate action - if necessary. > > Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > arch/x86/include/asm/mem_encrypt.h | 11 +++++++++++ > include/linux/dma-mapping.h | 11 +++++++++++ > include/linux/mem_encrypt.h | 6 ++++++ > lib/swiotlb.c | 3 +++ > 4 files changed, 31 insertions(+) > > diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h > index 87e816f..5a17f1b 100644 > --- a/arch/x86/include/asm/mem_encrypt.h > +++ b/arch/x86/include/asm/mem_encrypt.h > @@ -26,6 +26,11 @@ static inline bool sme_active(void) > return (sme_me_mask) ? true : false; > } > > +static inline u64 sme_dma_mask(void) > +{ > + return ((u64)sme_me_mask << 1) - 1; > +} > + > void __init sme_early_encrypt(resource_size_t paddr, > unsigned long size); > void __init sme_early_decrypt(resource_size_t paddr, > @@ -53,6 +58,12 @@ static inline bool sme_active(void) > { > return false; > } > + > +static inline u64 sme_dma_mask(void) > +{ > + return 0ULL; > +} > + > #endif > > static inline void __init sme_early_encrypt(resource_size_t paddr, > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > index 10c5a17..130bef7 100644 > --- a/include/linux/dma-mapping.h > +++ b/include/linux/dma-mapping.h > @@ -10,6 +10,7 @@ > #include <linux/scatterlist.h> > #include <linux/kmemcheck.h> > #include <linux/bug.h> > +#include <linux/mem_encrypt.h> > > /** > * List of possible attributes associated with a DMA mapping. The semantics > @@ -557,6 +558,11 @@ static inline int dma_set_mask(struct device *dev, u64 mask) > > if (!dev->dma_mask || !dma_supported(dev, mask)) > return -EIO; > + > + if (sme_active() && (mask < sme_dma_mask())) > + dev_warn(dev, > + "SME is active, device will require DMA bounce buffers\n"); > + Yes, definitely _once() here. It could be extended later to be per-device if the need arises. Also, a bit above in this function, we test if (ops->set_dma_mask) so device drivers which supply even an empty ->set_dma_mask will circumvent this check. It probably doesn't matter all that much right now because the only driver I see right now defining this method, though, is ethernet/intel/fm10k/fm10k_pf.c and some other arches' functionality which is unrelated here. But still...
On 2/27/2017 11:52 AM, Borislav Petkov wrote: > On Thu, Feb 16, 2017 at 09:46:19AM -0600, Tom Lendacky wrote: >> Add warnings to let the user know when bounce buffers are being used for >> DMA when SME is active. Since the bounce buffers are not in encrypted >> memory, these notifications are to allow the user to determine some >> appropriate action - if necessary. >> >> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> >> --- >> arch/x86/include/asm/mem_encrypt.h | 11 +++++++++++ >> include/linux/dma-mapping.h | 11 +++++++++++ >> include/linux/mem_encrypt.h | 6 ++++++ >> lib/swiotlb.c | 3 +++ >> 4 files changed, 31 insertions(+) >> >> diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h >> index 87e816f..5a17f1b 100644 >> --- a/arch/x86/include/asm/mem_encrypt.h >> +++ b/arch/x86/include/asm/mem_encrypt.h >> @@ -26,6 +26,11 @@ static inline bool sme_active(void) >> return (sme_me_mask) ? true : false; >> } >> >> +static inline u64 sme_dma_mask(void) >> +{ >> + return ((u64)sme_me_mask << 1) - 1; >> +} >> + >> void __init sme_early_encrypt(resource_size_t paddr, >> unsigned long size); >> void __init sme_early_decrypt(resource_size_t paddr, >> @@ -53,6 +58,12 @@ static inline bool sme_active(void) >> { >> return false; >> } >> + >> +static inline u64 sme_dma_mask(void) >> +{ >> + return 0ULL; >> +} >> + >> #endif >> >> static inline void __init sme_early_encrypt(resource_size_t paddr, >> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h >> index 10c5a17..130bef7 100644 >> --- a/include/linux/dma-mapping.h >> +++ b/include/linux/dma-mapping.h >> @@ -10,6 +10,7 @@ >> #include <linux/scatterlist.h> >> #include <linux/kmemcheck.h> >> #include <linux/bug.h> >> +#include <linux/mem_encrypt.h> >> >> /** >> * List of possible attributes associated with a DMA mapping. The semantics >> @@ -557,6 +558,11 @@ static inline int dma_set_mask(struct device *dev, u64 mask) >> >> if (!dev->dma_mask || !dma_supported(dev, mask)) >> return -EIO; >> + >> + if (sme_active() && (mask < sme_dma_mask())) >> + dev_warn(dev, >> + "SME is active, device will require DMA bounce buffers\n"); >> + > > Yes, definitely _once() here. Setting the mask is a probe/init type event, so I think not having the _once() would be better so that all devices that set a mask to something less than the SME encryption mask would be identified. This isn't done for every DMA, etc. > > It could be extended later to be per-device if the need arises. > > Also, a bit above in this function, we test if (ops->set_dma_mask) so > device drivers which supply even an empty ->set_dma_mask will circumvent > this check. > > It probably doesn't matter all that much right now because the > only driver I see right now defining this method, though, is > ethernet/intel/fm10k/fm10k_pf.c and some other arches' functionality > which is unrelated here. Device drivers don't supply set_dma_mask() since that is part of the dma_map_ops structure. The fm10k_pf.c file function is unrelated to this (it's part of an internal driver structure). The dma_map_ops structure is setup by the arch or an iommu. Thanks, Tom > > But still... > >
On Tue, Feb 28, 2017 at 05:19:51PM -0600, Tom Lendacky wrote: > Device drivers don't supply set_dma_mask() since that is part of the > dma_map_ops structure. The fm10k_pf.c file function is unrelated to this > (it's part of an internal driver structure). The dma_map_ops structure > is setup by the arch or an iommu. That was certainly a brainfart, sorry. Joerg explained to me on IRC how the whole dma_map_ops handling is supposed to be happening. Thanks.
On 17/02/2017 17:51, Tom Lendacky wrote: > > It's meant just to notify the user about the condition. The user could > then decide to use an alternative device that supports a greater DMA > range (I can probably change it to a dev_warn_once() so that a device > is identified). I would be nice if I could issue this message once per > device that experienced this. I didn't see anything that would do > that, though. dev_warn_once would print once only, not once per device. But if you leave the dev_warn elsewhere, this can be just pr_warn_once. Paolo
diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h index 87e816f..5a17f1b 100644 --- a/arch/x86/include/asm/mem_encrypt.h +++ b/arch/x86/include/asm/mem_encrypt.h @@ -26,6 +26,11 @@ static inline bool sme_active(void) return (sme_me_mask) ? true : false; } +static inline u64 sme_dma_mask(void) +{ + return ((u64)sme_me_mask << 1) - 1; +} + void __init sme_early_encrypt(resource_size_t paddr, unsigned long size); void __init sme_early_decrypt(resource_size_t paddr, @@ -53,6 +58,12 @@ static inline bool sme_active(void) { return false; } + +static inline u64 sme_dma_mask(void) +{ + return 0ULL; +} + #endif static inline void __init sme_early_encrypt(resource_size_t paddr, diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index 10c5a17..130bef7 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -10,6 +10,7 @@ #include <linux/scatterlist.h> #include <linux/kmemcheck.h> #include <linux/bug.h> +#include <linux/mem_encrypt.h> /** * List of possible attributes associated with a DMA mapping. The semantics @@ -557,6 +558,11 @@ static inline int dma_set_mask(struct device *dev, u64 mask) if (!dev->dma_mask || !dma_supported(dev, mask)) return -EIO; + + if (sme_active() && (mask < sme_dma_mask())) + dev_warn(dev, + "SME is active, device will require DMA bounce buffers\n"); + *dev->dma_mask = mask; return 0; } @@ -576,6 +582,11 @@ static inline int dma_set_coherent_mask(struct device *dev, u64 mask) { if (!dma_supported(dev, mask)) return -EIO; + + if (sme_active() && (mask < sme_dma_mask())) + dev_warn(dev, + "SME is active, device will require DMA bounce buffers\n"); + dev->coherent_dma_mask = mask; return 0; } diff --git a/include/linux/mem_encrypt.h b/include/linux/mem_encrypt.h index 14a7b9f..6829ff1 100644 --- a/include/linux/mem_encrypt.h +++ b/include/linux/mem_encrypt.h @@ -28,6 +28,12 @@ static inline bool sme_active(void) { return false; } + +static inline u64 sme_dma_mask(void) +{ + return 0ULL; +} + #endif #endif /* CONFIG_AMD_MEM_ENCRYPT */ diff --git a/lib/swiotlb.c b/lib/swiotlb.c index c463067..aff9353 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -509,6 +509,9 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, if (no_iotlb_memory) panic("Can not allocate SWIOTLB buffer earlier and can't now provide you with the DMA bounce buffer"); + WARN_ONCE(sme_active(), + "SME is active and system is using DMA bounce buffers\n"); + mask = dma_get_seg_boundary(hwdev); tbl_dma_addr &= mask;
Add warnings to let the user know when bounce buffers are being used for DMA when SME is active. Since the bounce buffers are not in encrypted memory, these notifications are to allow the user to determine some appropriate action - if necessary. Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> --- arch/x86/include/asm/mem_encrypt.h | 11 +++++++++++ include/linux/dma-mapping.h | 11 +++++++++++ include/linux/mem_encrypt.h | 6 ++++++ lib/swiotlb.c | 3 +++ 4 files changed, 31 insertions(+)