* [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests. @ 2019-12-09 23:13 Ashish Kalra 2019-12-20 1:52 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 14+ messages in thread From: Ashish Kalra @ 2019-12-09 23:13 UTC (permalink / raw) To: hch, tglx, mingo, bp, hpa, x86, luto, peterz, dave.hansen, konrad.wilk, iommu, linux-kernel, brijesh.singh, Thomas.Lendacky From: Ashish Kalra <ashish.kalra@amd.com> For SEV, all DMA to and from guest has to use shared (un-encrypted) pages. SEV uses SWIOTLB to make this happen without requiring changes to device drivers. However, depending on workload being run, the default 64MB of SWIOTLB might not be enough and SWIOTLB may run out of buffers to use for DMA, resulting in I/O errors. Increase the default size of SWIOTLB for SEV guests using a minimum value of 128MB and a maximum value of 512MB, determining on amount of provisioned guest memory. The SWIOTLB default size adjustment is added as an architecture specific interface/callback to allow architectures such as those supporting memory encryption to adjust/expand SWIOTLB size for their use. Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> --- Changes in v2: - Fix compile errors as Reported-by: kbuild test robot <lkp@intel.com> arch/x86/Kconfig | 1 + arch/x86/mm/mem_encrypt.c | 36 ++++++++++++++++++++++++++++++++++++ include/linux/dma-direct.h | 10 ++++++++++ kernel/dma/Kconfig | 3 +++ kernel/dma/swiotlb.c | 14 ++++++++++++-- 5 files changed, 62 insertions(+), 2 deletions(-) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 5e8949953660..e75622e58d34 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -1522,6 +1522,7 @@ config AMD_MEM_ENCRYPT select DYNAMIC_PHYSICAL_MASK select ARCH_USE_MEMREMAP_PROT select ARCH_HAS_FORCE_DMA_UNENCRYPTED + select ARCH_HAS_ADJUST_SWIOTLB_DEFAULT ---help--- Say yes to enable support for the encryption of system memory. This requires an AMD processor that supports Secure Memory diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c index a03614bd3e1a..f4bd4b431ba1 100644 --- a/arch/x86/mm/mem_encrypt.c +++ b/arch/x86/mm/mem_encrypt.c @@ -376,6 +376,42 @@ bool force_dma_unencrypted(struct device *dev) return false; } +#define TOTAL_MEM_1G 0x40000000U +#define TOTAL_MEM_4G 0x100000000U + +/* + * Override for SWIOTLB default size adjustment - + * ARCH_HAS_ADJUST_SWIOTLB_DEFAULT + */ +unsigned long adjust_swiotlb_default_size(unsigned long default_size) +{ + /* + * For SEV, all DMA has to occur via shared/unencrypted pages. + * SEV uses SWOTLB to make this happen without changing device + * drivers. However, depending on the workload being run, the + * default 64MB of SWIOTLB may not be enough & SWIOTLB may + * run out of buffers for using DMA, resulting in I/O errors. + * Increase the default size of SWIOTLB for SEV guests using + * a minimum value of 128MB and a maximum value of 512GB, + * depending on amount of provisioned guest memory. + */ + if (sev_active()) { + unsigned long total_mem = get_num_physpages() << PAGE_SHIFT; + + if (total_mem <= TOTAL_MEM_1G) + default_size = default_size * 2; + else if (total_mem <= TOTAL_MEM_4G) + default_size = default_size * 4; + else + default_size = default_size * 8; + + pr_info_once("SEV is active, SWIOTLB default size set to %luMB\n", + default_size >> 20); + } + + return default_size; +} + /* Architecture __weak replacement functions */ void __init mem_encrypt_free_decrypted_mem(void) { diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h index 24b8684aa21d..85507d21493f 100644 --- a/include/linux/dma-direct.h +++ b/include/linux/dma-direct.h @@ -35,6 +35,16 @@ static inline bool force_dma_unencrypted(struct device *dev) } #endif /* CONFIG_ARCH_HAS_FORCE_DMA_UNENCRYPTED */ +#ifdef CONFIG_ARCH_HAS_ADJUST_SWIOTLB_DEFAULT +unsigned long adjust_swiotlb_default_size(unsigned long default_size); +#else +static inline unsigned long adjust_swiotlb_default_size + (unsigned long default_size) +{ + return default_size; +} +#endif /* CONFIG_ARCH_HAS_ADJUST_SWIOTLB_DEFAULT */ + /* * If memory encryption is supported, phys_to_dma will set the memory encryption * bit in the DMA address, and dma_to_phys will clear it. The raw __phys_to_dma diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig index 4c103a24e380..851c4500ff88 100644 --- a/kernel/dma/Kconfig +++ b/kernel/dma/Kconfig @@ -54,6 +54,9 @@ config ARCH_HAS_DMA_PREP_COHERENT config ARCH_HAS_FORCE_DMA_UNENCRYPTED bool +config ARCH_HAS_ADJUST_SWIOTLB_DEFAULT + bool + config DMA_NONCOHERENT_CACHE_SYNC bool diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c index 9280d6f8271e..7dd72bd88f1c 100644 --- a/kernel/dma/swiotlb.c +++ b/kernel/dma/swiotlb.c @@ -155,11 +155,21 @@ void swiotlb_set_max_segment(unsigned int val) #define IO_TLB_DEFAULT_SIZE (64UL<<20) unsigned long swiotlb_size_or_default(void) { + unsigned long default_size = IO_TLB_DEFAULT_SIZE; unsigned long size; + /* + * If swiotlb size/amount of slabs are not defined on kernel command + * line, then give a chance to architectures to adjust swiotlb + * size, this may be required by some architectures such as those + * supporting memory encryption. + */ + if (!io_tlb_nslabs) + default_size = adjust_swiotlb_default_size(default_size); + size = io_tlb_nslabs << IO_TLB_SHIFT; - return size ? size : (IO_TLB_DEFAULT_SIZE); + return size ? size : default_size; } void swiotlb_print_info(void) @@ -245,7 +255,7 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose) void __init swiotlb_init(int verbose) { - size_t default_size = IO_TLB_DEFAULT_SIZE; + unsigned long default_size = swiotlb_size_or_default(); unsigned char *vstart; unsigned long bytes; -- 2.17.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests. 2019-12-09 23:13 [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests Ashish Kalra @ 2019-12-20 1:52 ` Konrad Rzeszutek Wilk 2020-01-21 20:09 ` Ashish Kalra 0 siblings, 1 reply; 14+ messages in thread From: Konrad Rzeszutek Wilk @ 2019-12-20 1:52 UTC (permalink / raw) To: Ashish Kalra Cc: Thomas.Lendacky, brijesh.singh, dave.hansen, konrad.wilk, peterz, x86, linux-kernel, iommu, mingo, bp, luto, hpa, tglx, hch On Mon, Dec 09, 2019 at 11:13:46PM +0000, Ashish Kalra wrote: > From: Ashish Kalra <ashish.kalra@amd.com> > > For SEV, all DMA to and from guest has to use shared > (un-encrypted) pages. SEV uses SWIOTLB to make this happen > without requiring changes to device drivers. However, > depending on workload being run, the default 64MB of SWIOTLB > might not be enough and SWIOTLB may run out of buffers to > use for DMA, resulting in I/O errors. > > Increase the default size of SWIOTLB for SEV guests using > a minimum value of 128MB and a maximum value of 512MB, > determining on amount of provisioned guest memory. > > The SWIOTLB default size adjustment is added as an > architecture specific interface/callback to allow > architectures such as those supporting memory encryption > to adjust/expand SWIOTLB size for their use. What if this was made dynamic? That is if there is a memory pressure you end up expanding the SWIOTLB dynamically? Also is it worth doing this calculation based on memory or more on the # of PCI devices + their MMIO ranges size? > > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > --- > Changes in v2: > - Fix compile errors as > Reported-by: kbuild test robot <lkp@intel.com> > > arch/x86/Kconfig | 1 + > arch/x86/mm/mem_encrypt.c | 36 ++++++++++++++++++++++++++++++++++++ > include/linux/dma-direct.h | 10 ++++++++++ > kernel/dma/Kconfig | 3 +++ > kernel/dma/swiotlb.c | 14 ++++++++++++-- > 5 files changed, 62 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index 5e8949953660..e75622e58d34 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -1522,6 +1522,7 @@ config AMD_MEM_ENCRYPT > select DYNAMIC_PHYSICAL_MASK > select ARCH_USE_MEMREMAP_PROT > select ARCH_HAS_FORCE_DMA_UNENCRYPTED > + select ARCH_HAS_ADJUST_SWIOTLB_DEFAULT > ---help--- > Say yes to enable support for the encryption of system memory. > This requires an AMD processor that supports Secure Memory > diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c > index a03614bd3e1a..f4bd4b431ba1 100644 > --- a/arch/x86/mm/mem_encrypt.c > +++ b/arch/x86/mm/mem_encrypt.c > @@ -376,6 +376,42 @@ bool force_dma_unencrypted(struct device *dev) > return false; > } > > +#define TOTAL_MEM_1G 0x40000000U > +#define TOTAL_MEM_4G 0x100000000U > + > +/* > + * Override for SWIOTLB default size adjustment - > + * ARCH_HAS_ADJUST_SWIOTLB_DEFAULT > + */ > +unsigned long adjust_swiotlb_default_size(unsigned long default_size) > +{ > + /* > + * For SEV, all DMA has to occur via shared/unencrypted pages. > + * SEV uses SWOTLB to make this happen without changing device > + * drivers. However, depending on the workload being run, the > + * default 64MB of SWIOTLB may not be enough & SWIOTLB may > + * run out of buffers for using DMA, resulting in I/O errors. > + * Increase the default size of SWIOTLB for SEV guests using > + * a minimum value of 128MB and a maximum value of 512GB, > + * depending on amount of provisioned guest memory. > + */ > + if (sev_active()) { > + unsigned long total_mem = get_num_physpages() << PAGE_SHIFT; > + > + if (total_mem <= TOTAL_MEM_1G) > + default_size = default_size * 2; > + else if (total_mem <= TOTAL_MEM_4G) > + default_size = default_size * 4; > + else > + default_size = default_size * 8; > + > + pr_info_once("SEV is active, SWIOTLB default size set to %luMB\n", > + default_size >> 20); > + } > + > + return default_size; > +} > + > /* Architecture __weak replacement functions */ > void __init mem_encrypt_free_decrypted_mem(void) > { > diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h > index 24b8684aa21d..85507d21493f 100644 > --- a/include/linux/dma-direct.h > +++ b/include/linux/dma-direct.h > @@ -35,6 +35,16 @@ static inline bool force_dma_unencrypted(struct device *dev) > } > #endif /* CONFIG_ARCH_HAS_FORCE_DMA_UNENCRYPTED */ > > +#ifdef CONFIG_ARCH_HAS_ADJUST_SWIOTLB_DEFAULT > +unsigned long adjust_swiotlb_default_size(unsigned long default_size); > +#else > +static inline unsigned long adjust_swiotlb_default_size > + (unsigned long default_size) > +{ > + return default_size; > +} > +#endif /* CONFIG_ARCH_HAS_ADJUST_SWIOTLB_DEFAULT */ > + > /* > * If memory encryption is supported, phys_to_dma will set the memory encryption > * bit in the DMA address, and dma_to_phys will clear it. The raw __phys_to_dma > diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig > index 4c103a24e380..851c4500ff88 100644 > --- a/kernel/dma/Kconfig > +++ b/kernel/dma/Kconfig > @@ -54,6 +54,9 @@ config ARCH_HAS_DMA_PREP_COHERENT > config ARCH_HAS_FORCE_DMA_UNENCRYPTED > bool > > +config ARCH_HAS_ADJUST_SWIOTLB_DEFAULT > + bool > + > config DMA_NONCOHERENT_CACHE_SYNC > bool > > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c > index 9280d6f8271e..7dd72bd88f1c 100644 > --- a/kernel/dma/swiotlb.c > +++ b/kernel/dma/swiotlb.c > @@ -155,11 +155,21 @@ void swiotlb_set_max_segment(unsigned int val) > #define IO_TLB_DEFAULT_SIZE (64UL<<20) > unsigned long swiotlb_size_or_default(void) > { > + unsigned long default_size = IO_TLB_DEFAULT_SIZE; > unsigned long size; > > + /* > + * If swiotlb size/amount of slabs are not defined on kernel command > + * line, then give a chance to architectures to adjust swiotlb > + * size, this may be required by some architectures such as those > + * supporting memory encryption. > + */ > + if (!io_tlb_nslabs) > + default_size = adjust_swiotlb_default_size(default_size); > + > size = io_tlb_nslabs << IO_TLB_SHIFT; > > - return size ? size : (IO_TLB_DEFAULT_SIZE); > + return size ? size : default_size; > } > > void swiotlb_print_info(void) > @@ -245,7 +255,7 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose) > void __init > swiotlb_init(int verbose) > { > - size_t default_size = IO_TLB_DEFAULT_SIZE; > + unsigned long default_size = swiotlb_size_or_default(); > unsigned char *vstart; > unsigned long bytes; > > -- > 2.17.1 > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests. 2019-12-20 1:52 ` Konrad Rzeszutek Wilk @ 2020-01-21 20:09 ` Ashish Kalra 2020-01-21 20:54 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 14+ messages in thread From: Ashish Kalra @ 2020-01-21 20:09 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Thomas.Lendacky, brijesh.singh, dave.hansen, konrad.wilk, peterz, x86, linux-kernel, iommu, mingo, bp, luto, hpa, tglx, hch On Thu, Dec 19, 2019 at 08:52:45PM -0500, Konrad Rzeszutek Wilk wrote: > On Mon, Dec 09, 2019 at 11:13:46PM +0000, Ashish Kalra wrote: > > From: Ashish Kalra <ashish.kalra@amd.com> > > > > For SEV, all DMA to and from guest has to use shared > > (un-encrypted) pages. SEV uses SWIOTLB to make this happen > > without requiring changes to device drivers. However, > > depending on workload being run, the default 64MB of SWIOTLB > > might not be enough and SWIOTLB may run out of buffers to > > use for DMA, resulting in I/O errors. > > > > Increase the default size of SWIOTLB for SEV guests using > > a minimum value of 128MB and a maximum value of 512MB, > > determining on amount of provisioned guest memory. > > > > The SWIOTLB default size adjustment is added as an > > architecture specific interface/callback to allow > > architectures such as those supporting memory encryption > > to adjust/expand SWIOTLB size for their use. > > What if this was made dynamic? That is if there is a memory > pressure you end up expanding the SWIOTLB dynamically? As of now we want to keep it as simple as possible and more like a stop-gap arrangement till something more elegant is available. > >> Also is it worth doing this calculation based on memory or >> more on the # of PCI devices + their MMIO ranges size? Additional memory calculations based on # of PCI devices and their memory ranges will make it more complicated with so many other permutations and combinations to explore, it is essential to keep this patch as simple as possible by adjusting the bounce buffer size simply by determining it from the amount of provisioned guest memory. Thanks, Ashish _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests. 2020-01-21 20:09 ` Ashish Kalra @ 2020-01-21 20:54 ` Konrad Rzeszutek Wilk 2020-01-24 23:00 ` Ashish Kalra 0 siblings, 1 reply; 14+ messages in thread From: Konrad Rzeszutek Wilk @ 2020-01-21 20:54 UTC (permalink / raw) To: Ashish Kalra Cc: Thomas.Lendacky, brijesh.singh, dave.hansen, peterz, x86, linux-kernel, iommu, mingo, bp, luto, hpa, Konrad Rzeszutek Wilk, tglx, hch On Tue, Jan 21, 2020 at 08:09:47PM +0000, Ashish Kalra wrote: > On Thu, Dec 19, 2019 at 08:52:45PM -0500, Konrad Rzeszutek Wilk wrote: > > On Mon, Dec 09, 2019 at 11:13:46PM +0000, Ashish Kalra wrote: > > > From: Ashish Kalra <ashish.kalra@amd.com> > > > > > > For SEV, all DMA to and from guest has to use shared > > > (un-encrypted) pages. SEV uses SWIOTLB to make this happen > > > without requiring changes to device drivers. However, > > > depending on workload being run, the default 64MB of SWIOTLB > > > might not be enough and SWIOTLB may run out of buffers to > > > use for DMA, resulting in I/O errors. > > > > > > Increase the default size of SWIOTLB for SEV guests using > > > a minimum value of 128MB and a maximum value of 512MB, > > > determining on amount of provisioned guest memory. > > > > > > The SWIOTLB default size adjustment is added as an > > > architecture specific interface/callback to allow > > > architectures such as those supporting memory encryption > > > to adjust/expand SWIOTLB size for their use. > > > > What if this was made dynamic? That is if there is a memory > > pressure you end up expanding the SWIOTLB dynamically? > > As of now we want to keep it as simple as possible and more > like a stop-gap arrangement till something more elegant is > available. That is nice. But past experience has shown that stop-gap arrangments end up being the defacto solution. > > > > >> Also is it worth doing this calculation based on memory or > >> more on the # of PCI devices + their MMIO ranges size? > > Additional memory calculations based on # of PCI devices and > their memory ranges will make it more complicated with so > many other permutations and combinations to explore, it is > essential to keep this patch as simple as possible by > adjusting the bounce buffer size simply by determining it > from the amount of provisioned guest memory. Please rework the patch to: - Use a log solution instead of the multiplication. Feel free to cap it at a sensible value. - Also the code depends on SWIOTLB calling in to the adjust_swiotlb_default_size which looks wrong. You should not adjust io_tlb_nslabs from swiotlb_size_or_default. That function's purpose is to report a value. - Make io_tlb_nslabs be visible outside of the SWIOTLB code. - Can you utilize the IOMMU_INIT APIs and have your own detect which would modify the io_tlb_nslabs (and set swiotbl=1?). Actually you seem to be piggybacking on pci_swiotlb_detect_4gb - so perhaps add in this code ? Albeit it really should be in it's own file, not in arch/x86/kernel/pci-swiotlb.c - Tweak the code in the swiotlb code to make sure it can deal with io_tlb_nslabs being modified outside of the code at the start. It should have no trouble, but only testing will tell for sure. > > Thanks, > Ashish _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests. 2020-01-21 20:54 ` Konrad Rzeszutek Wilk @ 2020-01-24 23:00 ` Ashish Kalra 2020-02-04 19:35 ` Ashish Kalra 0 siblings, 1 reply; 14+ messages in thread From: Ashish Kalra @ 2020-01-24 23:00 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Thomas.Lendacky, brijesh.singh, dave.hansen, peterz, x86, linux-kernel, iommu, mingo, bp, luto, hpa, Konrad Rzeszutek Wilk, tglx, hch On Tue, Jan 21, 2020 at 03:54:03PM -0500, Konrad Rzeszutek Wilk wrote: > > > > Additional memory calculations based on # of PCI devices and > > their memory ranges will make it more complicated with so > > many other permutations and combinations to explore, it is > > essential to keep this patch as simple as possible by > > adjusting the bounce buffer size simply by determining it > > from the amount of provisioned guest memory. >> >> Please rework the patch to: >> >> - Use a log solution instead of the multiplication. >> Feel free to cap it at a sensible value. Ok. >> >> - Also the code depends on SWIOTLB calling in to the >> adjust_swiotlb_default_size which looks wrong. >> >> You should not adjust io_tlb_nslabs from swiotlb_size_or_default. >> That function's purpose is to report a value. >> >> - Make io_tlb_nslabs be visible outside of the SWIOTLB code. >> >> - Can you utilize the IOMMU_INIT APIs and have your own detect which would >> modify the io_tlb_nslabs (and set swiotbl=1?). This seems to be a nice option, but then IOMMU_INIT APIs are x86-specific and this swiotlb buffer size adjustment is also needed for other memory encryption architectures like Power, S390, etc. >> >> Actually you seem to be piggybacking on pci_swiotlb_detect_4gb - so >> perhaps add in this code ? Albeit it really should be in it's own >> file, not in arch/x86/kernel/pci-swiotlb.c Actually, we piggyback on pci_swiotlb_detect_override which sets swiotlb=1 as x86_64_start_kernel() and invocation of sme_early_init() forces swiotlb on, but again this is all x86 architecture specific. Thanks, Ashish _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests. 2020-01-24 23:00 ` Ashish Kalra @ 2020-02-04 19:35 ` Ashish Kalra 2020-03-03 17:03 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 14+ messages in thread From: Ashish Kalra @ 2020-02-04 19:35 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Thomas.Lendacky, brijesh.singh, dave.hansen, peterz, x86, linux-kernel, iommu, mingo, bp, luto, hpa, Konrad Rzeszutek Wilk, tglx, hch Hello Konrad, Looking fwd. to your feedback regarding support of other memory encryption architectures such as Power, S390, etc. Thanks, Ashish On Fri, Jan 24, 2020 at 11:00:08PM +0000, Ashish Kalra wrote: > On Tue, Jan 21, 2020 at 03:54:03PM -0500, Konrad Rzeszutek Wilk wrote: > > > > > > Additional memory calculations based on # of PCI devices and > > > their memory ranges will make it more complicated with so > > > many other permutations and combinations to explore, it is > > > essential to keep this patch as simple as possible by > > > adjusting the bounce buffer size simply by determining it > > > from the amount of provisioned guest memory. > >> > >> Please rework the patch to: > >> > >> - Use a log solution instead of the multiplication. > >> Feel free to cap it at a sensible value. > > Ok. > > >> > >> - Also the code depends on SWIOTLB calling in to the > >> adjust_swiotlb_default_size which looks wrong. > >> > >> You should not adjust io_tlb_nslabs from swiotlb_size_or_default. > > >> That function's purpose is to report a value. > >> > >> - Make io_tlb_nslabs be visible outside of the SWIOTLB code. > >> > >> - Can you utilize the IOMMU_INIT APIs and have your own detect which would > >> modify the io_tlb_nslabs (and set swiotbl=1?). > > This seems to be a nice option, but then IOMMU_INIT APIs are > x86-specific and this swiotlb buffer size adjustment is also needed > for other memory encryption architectures like Power, S390, etc. > > >> > >> Actually you seem to be piggybacking on pci_swiotlb_detect_4gb - so > >> perhaps add in this code ? Albeit it really should be in it's own > >> file, not in arch/x86/kernel/pci-swiotlb.c > > Actually, we piggyback on pci_swiotlb_detect_override which sets > swiotlb=1 as x86_64_start_kernel() and invocation of sme_early_init() > forces swiotlb on, but again this is all x86 architecture specific. > > Thanks, > Ashish _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests. 2020-02-04 19:35 ` Ashish Kalra @ 2020-03-03 17:03 ` Konrad Rzeszutek Wilk 2020-03-12 0:43 ` Ashish Kalra 2020-03-30 22:25 ` Ashish Kalra 0 siblings, 2 replies; 14+ messages in thread From: Konrad Rzeszutek Wilk @ 2020-03-03 17:03 UTC (permalink / raw) To: Ashish Kalra Cc: Thomas.Lendacky, brijesh.singh, dave.hansen, peterz, x86, linux-kernel, iommu, mingo, bp, luto, hpa, Konrad Rzeszutek Wilk, tglx, hch On Tue, Feb 04, 2020 at 07:35:00PM +0000, Ashish Kalra wrote: > Hello Konrad, > > Looking fwd. to your feedback regarding support of other memory > encryption architectures such as Power, S390, etc. > > Thanks, > Ashish > > On Fri, Jan 24, 2020 at 11:00:08PM +0000, Ashish Kalra wrote: > > On Tue, Jan 21, 2020 at 03:54:03PM -0500, Konrad Rzeszutek Wilk wrote: > > > > > > > > Additional memory calculations based on # of PCI devices and > > > > their memory ranges will make it more complicated with so > > > > many other permutations and combinations to explore, it is > > > > essential to keep this patch as simple as possible by > > > > adjusting the bounce buffer size simply by determining it > > > > from the amount of provisioned guest memory. > > >> > > >> Please rework the patch to: > > >> > > >> - Use a log solution instead of the multiplication. > > >> Feel free to cap it at a sensible value. > > > > Ok. > > > > >> > > >> - Also the code depends on SWIOTLB calling in to the > > >> adjust_swiotlb_default_size which looks wrong. > > >> > > >> You should not adjust io_tlb_nslabs from swiotlb_size_or_default. > > > > >> That function's purpose is to report a value. > > >> > > >> - Make io_tlb_nslabs be visible outside of the SWIOTLB code. > > >> > > >> - Can you utilize the IOMMU_INIT APIs and have your own detect which would > > >> modify the io_tlb_nslabs (and set swiotbl=1?). > > > > This seems to be a nice option, but then IOMMU_INIT APIs are > > x86-specific and this swiotlb buffer size adjustment is also needed > > for other memory encryption architectures like Power, S390, etc. Oh dear. That I hadn't considered. > > > > >> > > >> Actually you seem to be piggybacking on pci_swiotlb_detect_4gb - so > > >> perhaps add in this code ? Albeit it really should be in it's own > > >> file, not in arch/x86/kernel/pci-swiotlb.c > > > > Actually, we piggyback on pci_swiotlb_detect_override which sets > > swiotlb=1 as x86_64_start_kernel() and invocation of sme_early_init() > > forces swiotlb on, but again this is all x86 architecture specific. Then it looks like the best bet is to do it from within swiotlb_init? We really can't do it from swiotlb_size_or_default - that function should just return a value and nothing else. > > > > Thanks, > > Ashish _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests. 2020-03-03 17:03 ` Konrad Rzeszutek Wilk @ 2020-03-12 0:43 ` Ashish Kalra 2020-03-30 22:25 ` Ashish Kalra 1 sibling, 0 replies; 14+ messages in thread From: Ashish Kalra @ 2020-03-12 0:43 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Thomas.Lendacky, brijesh.singh, dave.hansen, peterz, x86, linux-kernel, iommu, mingo, bp, luto, hpa, Konrad Rzeszutek Wilk, tglx, hch On Tue, Mar 03, 2020 at 12:03:53PM -0500, Konrad Rzeszutek Wilk wrote: > > > > > > >> > > > >> Actually you seem to be piggybacking on pci_swiotlb_detect_4gb - so > > > >> perhaps add in this code ? Albeit it really should be in it's own > > > >> file, not in arch/x86/kernel/pci-swiotlb.c > > > > > > Actually, we piggyback on pci_swiotlb_detect_override which sets > > > swiotlb=1 as x86_64_start_kernel() and invocation of sme_early_init() > > > forces swiotlb on, but again this is all x86 architecture specific. > > Then it looks like the best bet is to do it from within swiotlb_init? > We really can't do it from swiotlb_size_or_default - that function > should just return a value and nothing else. Ok, i will fix this patch accordingly and resubmit it. Thanks, Ashish _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests. 2020-03-03 17:03 ` Konrad Rzeszutek Wilk 2020-03-12 0:43 ` Ashish Kalra @ 2020-03-30 22:25 ` Ashish Kalra 2020-04-27 18:53 ` Ashish Kalra 1 sibling, 1 reply; 14+ messages in thread From: Ashish Kalra @ 2020-03-30 22:25 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Thomas.Lendacky, brijesh.singh, dave.hansen, peterz, x86, linux-kernel, iommu, mingo, bp, luto, hpa, Konrad Rzeszutek Wilk, tglx, hch Hello Konrad, On Tue, Mar 03, 2020 at 12:03:53PM -0500, Konrad Rzeszutek Wilk wrote: > On Tue, Feb 04, 2020 at 07:35:00PM +0000, Ashish Kalra wrote: > > Hello Konrad, > > > > Looking fwd. to your feedback regarding support of other memory > > encryption architectures such as Power, S390, etc. > > > > Thanks, > > Ashish > > > > On Fri, Jan 24, 2020 at 11:00:08PM +0000, Ashish Kalra wrote: > > > On Tue, Jan 21, 2020 at 03:54:03PM -0500, Konrad Rzeszutek Wilk wrote: > > > > > > > > > > Additional memory calculations based on # of PCI devices and > > > > > their memory ranges will make it more complicated with so > > > > > many other permutations and combinations to explore, it is > > > > > essential to keep this patch as simple as possible by > > > > > adjusting the bounce buffer size simply by determining it > > > > > from the amount of provisioned guest memory. > > > >> > > > >> Please rework the patch to: > > > >> > > > >> - Use a log solution instead of the multiplication. > > > >> Feel free to cap it at a sensible value. > > > > > > Ok. > > > > > > >> > > > >> - Also the code depends on SWIOTLB calling in to the > > > >> adjust_swiotlb_default_size which looks wrong. > > > >> > > > >> You should not adjust io_tlb_nslabs from swiotlb_size_or_default. > > > > > > >> That function's purpose is to report a value. > > > >> > > > >> - Make io_tlb_nslabs be visible outside of the SWIOTLB code. > > > >> > > > >> - Can you utilize the IOMMU_INIT APIs and have your own detect which would > > > >> modify the io_tlb_nslabs (and set swiotbl=1?). > > > > > > This seems to be a nice option, but then IOMMU_INIT APIs are > > > x86-specific and this swiotlb buffer size adjustment is also needed > > > for other memory encryption architectures like Power, S390, etc. > > Oh dear. That I hadn't considered. > > > > > > >> > > > >> Actually you seem to be piggybacking on pci_swiotlb_detect_4gb - so > > > >> perhaps add in this code ? Albeit it really should be in it's own > > > >> file, not in arch/x86/kernel/pci-swiotlb.c > > > > > > Actually, we piggyback on pci_swiotlb_detect_override which sets > > > swiotlb=1 as x86_64_start_kernel() and invocation of sme_early_init() > > > forces swiotlb on, but again this is all x86 architecture specific. > > Then it looks like the best bet is to do it from within swiotlb_init? > We really can't do it from swiotlb_size_or_default - that function > should just return a value and nothing else. > Actually, we need to do it in swiotlb_size_or_default() as this gets called by reserve_crashkernel_low() in arch/x86/kernel/setup.c and used to reserve low crashkernel memory. If we adjust swiotlb size later in swiotlb_init() which gets called later than reserve_crashkernel_low(), then any swiotlb size changes/expansion will conflict/overlap with the low memory reserved for crashkernel. Thanks, Ashish _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests. 2020-03-30 22:25 ` Ashish Kalra @ 2020-04-27 18:53 ` Ashish Kalra 2020-06-23 13:38 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 14+ messages in thread From: Ashish Kalra @ 2020-04-27 18:53 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Thomas.Lendacky, brijesh.singh, dave.hansen, peterz, x86, linux-kernel, iommu, mingo, bp, luto, hpa, Konrad Rzeszutek Wilk, tglx, hch Hello Konrad, On Mon, Mar 30, 2020 at 10:25:51PM +0000, Ashish Kalra wrote: > Hello Konrad, > > On Tue, Mar 03, 2020 at 12:03:53PM -0500, Konrad Rzeszutek Wilk wrote: > > On Tue, Feb 04, 2020 at 07:35:00PM +0000, Ashish Kalra wrote: > > > Hello Konrad, > > > > > > Looking fwd. to your feedback regarding support of other memory > > > encryption architectures such as Power, S390, etc. > > > > > > Thanks, > > > Ashish > > > > > > On Fri, Jan 24, 2020 at 11:00:08PM +0000, Ashish Kalra wrote: > > > > On Tue, Jan 21, 2020 at 03:54:03PM -0500, Konrad Rzeszutek Wilk wrote: > > > > > > > > > > > > Additional memory calculations based on # of PCI devices and > > > > > > their memory ranges will make it more complicated with so > > > > > > many other permutations and combinations to explore, it is > > > > > > essential to keep this patch as simple as possible by > > > > > > adjusting the bounce buffer size simply by determining it > > > > > > from the amount of provisioned guest memory. > > > > >> > > > > >> Please rework the patch to: > > > > >> > > > > >> - Use a log solution instead of the multiplication. > > > > >> Feel free to cap it at a sensible value. > > > > > > > > Ok. > > > > > > > > >> > > > > >> - Also the code depends on SWIOTLB calling in to the > > > > >> adjust_swiotlb_default_size which looks wrong. > > > > >> > > > > >> You should not adjust io_tlb_nslabs from swiotlb_size_or_default. > > > > > > > > >> That function's purpose is to report a value. > > > > >> > > > > >> - Make io_tlb_nslabs be visible outside of the SWIOTLB code. > > > > >> > > > > >> - Can you utilize the IOMMU_INIT APIs and have your own detect which would > > > > >> modify the io_tlb_nslabs (and set swiotbl=1?). > > > > > > > > This seems to be a nice option, but then IOMMU_INIT APIs are > > > > x86-specific and this swiotlb buffer size adjustment is also needed > > > > for other memory encryption architectures like Power, S390, etc. > > > > Oh dear. That I hadn't considered. > > > > > > > > >> > > > > >> Actually you seem to be piggybacking on pci_swiotlb_detect_4gb - so > > > > >> perhaps add in this code ? Albeit it really should be in it's own > > > > >> file, not in arch/x86/kernel/pci-swiotlb.c > > > > > > > > Actually, we piggyback on pci_swiotlb_detect_override which sets > > > > swiotlb=1 as x86_64_start_kernel() and invocation of sme_early_init() > > > > forces swiotlb on, but again this is all x86 architecture specific. > > > > Then it looks like the best bet is to do it from within swiotlb_init? > > We really can't do it from swiotlb_size_or_default - that function > > should just return a value and nothing else. > > > > Actually, we need to do it in swiotlb_size_or_default() as this gets called by > reserve_crashkernel_low() in arch/x86/kernel/setup.c and used to > reserve low crashkernel memory. If we adjust swiotlb size later in > swiotlb_init() which gets called later than reserve_crashkernel_low(), > then any swiotlb size changes/expansion will conflict/overlap with the > low memory reserved for crashkernel. > and will also potentially cause SWIOTLB buffer allocation failures. Do you have any feedback, comments on the above ? As such i feel, this patch is complete otherwise and can be included as it is. Thanks, Ashish _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests. 2020-04-27 18:53 ` Ashish Kalra @ 2020-06-23 13:38 ` Konrad Rzeszutek Wilk 2020-06-24 0:23 ` Ashish Kalra 0 siblings, 1 reply; 14+ messages in thread From: Konrad Rzeszutek Wilk @ 2020-06-23 13:38 UTC (permalink / raw) To: Ashish Kalra Cc: Thomas.Lendacky, brijesh.singh, dave.hansen, Konrad Rzeszutek Wilk, peterz, x86, linux-kernel, iommu, mingo, bp, luto, hpa, tglx, hch On Mon, Apr 27, 2020 at 06:53:18PM +0000, Ashish Kalra wrote: > Hello Konrad, > > On Mon, Mar 30, 2020 at 10:25:51PM +0000, Ashish Kalra wrote: > > Hello Konrad, > > > > On Tue, Mar 03, 2020 at 12:03:53PM -0500, Konrad Rzeszutek Wilk wrote: > > > On Tue, Feb 04, 2020 at 07:35:00PM +0000, Ashish Kalra wrote: > > > > Hello Konrad, > > > > > > > > Looking fwd. to your feedback regarding support of other memory > > > > encryption architectures such as Power, S390, etc. > > > > > > > > Thanks, > > > > Ashish > > > > > > > > On Fri, Jan 24, 2020 at 11:00:08PM +0000, Ashish Kalra wrote: > > > > > On Tue, Jan 21, 2020 at 03:54:03PM -0500, Konrad Rzeszutek Wilk wrote: > > > > > > > > > > > > > > Additional memory calculations based on # of PCI devices and > > > > > > > their memory ranges will make it more complicated with so > > > > > > > many other permutations and combinations to explore, it is > > > > > > > essential to keep this patch as simple as possible by > > > > > > > adjusting the bounce buffer size simply by determining it > > > > > > > from the amount of provisioned guest memory. > > > > > >> > > > > > >> Please rework the patch to: > > > > > >> > > > > > >> - Use a log solution instead of the multiplication. > > > > > >> Feel free to cap it at a sensible value. > > > > > > > > > > Ok. > > > > > > > > > > >> > > > > > >> - Also the code depends on SWIOTLB calling in to the > > > > > >> adjust_swiotlb_default_size which looks wrong. > > > > > >> > > > > > >> You should not adjust io_tlb_nslabs from swiotlb_size_or_default. > > > > > > > > > > >> That function's purpose is to report a value. > > > > > >> > > > > > >> - Make io_tlb_nslabs be visible outside of the SWIOTLB code. > > > > > >> > > > > > >> - Can you utilize the IOMMU_INIT APIs and have your own detect which would > > > > > >> modify the io_tlb_nslabs (and set swiotbl=1?). > > > > > > > > > > This seems to be a nice option, but then IOMMU_INIT APIs are > > > > > x86-specific and this swiotlb buffer size adjustment is also needed > > > > > for other memory encryption architectures like Power, S390, etc. > > > > > > Oh dear. That I hadn't considered. > > > > > > > > > > >> > > > > > >> Actually you seem to be piggybacking on pci_swiotlb_detect_4gb - so > > > > > >> perhaps add in this code ? Albeit it really should be in it's own > > > > > >> file, not in arch/x86/kernel/pci-swiotlb.c > > > > > > > > > > Actually, we piggyback on pci_swiotlb_detect_override which sets > > > > > swiotlb=1 as x86_64_start_kernel() and invocation of sme_early_init() > > > > > forces swiotlb on, but again this is all x86 architecture specific. > > > > > > Then it looks like the best bet is to do it from within swiotlb_init? > > > We really can't do it from swiotlb_size_or_default - that function > > > should just return a value and nothing else. > > > > > > > Actually, we need to do it in swiotlb_size_or_default() as this gets called by > > reserve_crashkernel_low() in arch/x86/kernel/setup.c and used to > > reserve low crashkernel memory. If we adjust swiotlb size later in > > swiotlb_init() which gets called later than reserve_crashkernel_low(), > > then any swiotlb size changes/expansion will conflict/overlap with the > > low memory reserved for crashkernel. > > > and will also potentially cause SWIOTLB buffer allocation failures. > > Do you have any feedback, comments on the above ? The init boot chain looks like this: initmem_init pci_iommu_alloc -> pci_swiotlb_detect_4gb -> swiotlb_init reserve_crashkernel reserve_crashkernel_low -> swiotlb_size_or_default .. (rootfs code): pci_iommu_init -> a bunch of the other IOMMU late_init code gets called.. -> pci_swiotlb_late_init I have to say I am lost to how your patch fixes "If we adjust swiolb size later .. then any swiotlb size .. will overlap with the low memory reserved for crashkernel"? Or are you saying that 'reserve_crashkernel_low' is the _culprit_ and it is the one changing the size? And hence it modifying the swiotlb size will fix this problem? Aka _before_ all the other IOMMU get their hand on it? If so why not create an IOMMU_INIT(crashkernel_adjust_swiotlb,pci_swiotlb_detect_override, NULL, NULL); And crashkernel_adjust_swiotlb would change the size of swiotlb buffer if conditions are found to require it. You also may want to put a #define DEBUG in arch/x86/kernel/pci-iommu_table.c to check out whether the tree structure of IOMMU entries is correct. But still I am lost - if say the AMD one does decide for unknown reason to expand the SWIOTLB you are still stuck with the 'overlap with the low memory reserved' or so. Perhaps add a late_init that gets called as the last one to validate this ? And maybe if the swiotlb gets turned off you also take proper steps? > As such i feel, this patch is complete otherwise and can be included as > it is. > > Thanks, > Ashish _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests. 2020-06-23 13:38 ` Konrad Rzeszutek Wilk @ 2020-06-24 0:23 ` Ashish Kalra 2020-06-24 7:05 ` Ashish Kalra 0 siblings, 1 reply; 14+ messages in thread From: Ashish Kalra @ 2020-06-24 0:23 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Thomas.Lendacky, brijesh.singh, dave.hansen, Konrad Rzeszutek Wilk, peterz, x86, linux-kernel, iommu, mingo, bp, luto, hpa, tglx, hch Hello Konrad, On Tue, Jun 23, 2020 at 09:38:43AM -0400, Konrad Rzeszutek Wilk wrote: > On Mon, Apr 27, 2020 at 06:53:18PM +0000, Ashish Kalra wrote: > > Hello Konrad, > > > > On Mon, Mar 30, 2020 at 10:25:51PM +0000, Ashish Kalra wrote: > > > Hello Konrad, > > > > > > On Tue, Mar 03, 2020 at 12:03:53PM -0500, Konrad Rzeszutek Wilk wrote: > > > > On Tue, Feb 04, 2020 at 07:35:00PM +0000, Ashish Kalra wrote: > > > > > Hello Konrad, > > > > > > > > > > Looking fwd. to your feedback regarding support of other memory > > > > > encryption architectures such as Power, S390, etc. > > > > > > > > > > Thanks, > > > > > Ashish > > > > > > > > > > On Fri, Jan 24, 2020 at 11:00:08PM +0000, Ashish Kalra wrote: > > > > > > On Tue, Jan 21, 2020 at 03:54:03PM -0500, Konrad Rzeszutek Wilk wrote: > > > > > > > > > > > > > > > > Additional memory calculations based on # of PCI devices and > > > > > > > > their memory ranges will make it more complicated with so > > > > > > > > many other permutations and combinations to explore, it is > > > > > > > > essential to keep this patch as simple as possible by > > > > > > > > adjusting the bounce buffer size simply by determining it > > > > > > > > from the amount of provisioned guest memory. > > > > > > >> > > > > > > >> Please rework the patch to: > > > > > > >> > > > > > > >> - Use a log solution instead of the multiplication. > > > > > > >> Feel free to cap it at a sensible value. > > > > > > > > > > > > Ok. > > > > > > > > > > > > >> > > > > > > >> - Also the code depends on SWIOTLB calling in to the > > > > > > >> adjust_swiotlb_default_size which looks wrong. > > > > > > >> > > > > > > >> You should not adjust io_tlb_nslabs from swiotlb_size_or_default. > > > > > > > > > > > > >> That function's purpose is to report a value. > > > > > > >> > > > > > > >> - Make io_tlb_nslabs be visible outside of the SWIOTLB code. > > > > > > >> > > > > > > >> - Can you utilize the IOMMU_INIT APIs and have your own detect which would > > > > > > >> modify the io_tlb_nslabs (and set swiotbl=1?). > > > > > > > > > > > > This seems to be a nice option, but then IOMMU_INIT APIs are > > > > > > x86-specific and this swiotlb buffer size adjustment is also needed > > > > > > for other memory encryption architectures like Power, S390, etc. > > > > > > > > Oh dear. That I hadn't considered. > > > > > > > > > > > > >> > > > > > > >> Actually you seem to be piggybacking on pci_swiotlb_detect_4gb - so > > > > > > >> perhaps add in this code ? Albeit it really should be in it's own > > > > > > >> file, not in arch/x86/kernel/pci-swiotlb.c > > > > > > > > > > > > Actually, we piggyback on pci_swiotlb_detect_override which sets > > > > > > swiotlb=1 as x86_64_start_kernel() and invocation of sme_early_init() > > > > > > forces swiotlb on, but again this is all x86 architecture specific. > > > > > > > > Then it looks like the best bet is to do it from within swiotlb_init? > > > > We really can't do it from swiotlb_size_or_default - that function > > > > should just return a value and nothing else. > > > > > > > > > > Actually, we need to do it in swiotlb_size_or_default() as this gets called by > > > reserve_crashkernel_low() in arch/x86/kernel/setup.c and used to > > > reserve low crashkernel memory. If we adjust swiotlb size later in > > > swiotlb_init() which gets called later than reserve_crashkernel_low(), > > > then any swiotlb size changes/expansion will conflict/overlap with the > > > low memory reserved for crashkernel. > > > > > and will also potentially cause SWIOTLB buffer allocation failures. > > > > Do you have any feedback, comments on the above ? > > > The init boot chain looks like this: > > initmem_init > pci_iommu_alloc > -> pci_swiotlb_detect_4gb > -> swiotlb_init > > reserve_crashkernel > reserve_crashkernel_low > -> swiotlb_size_or_default > .. > > > (rootfs code): > pci_iommu_init > -> a bunch of the other IOMMU late_init code gets called.. > -> pci_swiotlb_late_init > > I have to say I am lost to how your patch fixes "If we adjust swiolb > size later .. then any swiotlb size .. will overlap with the low memory > reserved for crashkernel"? > Actually as per the boot flow : setup_arch() calls reserve_crashkernel() and pci_iommu_alloc() is invoked through mm_init()/mem_init() and not via initmem_init(). start_kernel: ... setup_arch() reserve_crashkernel reserve_crashkernel_low -> swiotlb_size_or_default ... ... mm_init() mem_init() pci_iommu_alloc -> pci_swiotlb_detect_4gb -> swiotlb_init So as per the above boot flow, reserve_crashkernel() can get called before swiotlb_detect/init, and hence, if we don't fixup or adjust the SWIOTLB buffer size in swiotlb_size_or_default() then crash kernel will reserve memory which will conflict/overlap with any SWIOTLB bounce buffer allocated memory (adjusted or fixed up later). Therefore, we need to adjust/fixup SWIOTLB bounce buffer memory in swiotlb_size_or_default() function itself, before swiotlb detect/init funtions get invoked. Thanks, Ashish > Or are you saying that 'reserve_crashkernel_low' is the _culprit_ and it > is the one changing the size? And hence it modifying the swiotlb size > will fix this problem? Aka _before_ all the other IOMMU get their hand > on it? > > If so why not create an > IOMMU_INIT(crashkernel_adjust_swiotlb,pci_swiotlb_detect_override, > NULL, NULL); > > And crashkernel_adjust_swiotlb would change the size of swiotlb buffer > if conditions are found to require it. > > You also may want to put a #define DEBUG in arch/x86/kernel/pci-iommu_table.c > to check out whether the tree structure of IOMMU entries is correct. > > > > But still I am lost - if say the AMD one does decide for unknown reason > to expand the SWIOTLB you are still stuck with the 'overlap with > the low memory reserved' or so. > > Perhaps add a late_init that gets called as the last one to validate > this ? And maybe if the swiotlb gets turned off you also take proper > steps? > > > As such i feel, this patch is complete otherwise and can be included as > > it is. > > > > Thanks, > > Ashish _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests. 2020-06-24 0:23 ` Ashish Kalra @ 2020-06-24 7:05 ` Ashish Kalra 2020-06-24 21:11 ` Konrad Rzeszutek Wilk 0 siblings, 1 reply; 14+ messages in thread From: Ashish Kalra @ 2020-06-24 7:05 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Thomas.Lendacky, brijesh.singh, dave.hansen, Konrad Rzeszutek Wilk, peterz, x86, linux-kernel, iommu, mingo, bp, luto, hpa, tglx, hch On Wed, Jun 24, 2020 at 12:23:57AM +0000, Ashish Kalra wrote: > Hello Konrad, > > On Tue, Jun 23, 2020 at 09:38:43AM -0400, Konrad Rzeszutek Wilk wrote: > > On Mon, Apr 27, 2020 at 06:53:18PM +0000, Ashish Kalra wrote: > > > Hello Konrad, > > > > > > On Mon, Mar 30, 2020 at 10:25:51PM +0000, Ashish Kalra wrote: > > > > Hello Konrad, > > > > > > > > On Tue, Mar 03, 2020 at 12:03:53PM -0500, Konrad Rzeszutek Wilk wrote: > > > > > On Tue, Feb 04, 2020 at 07:35:00PM +0000, Ashish Kalra wrote: > > > > > > Hello Konrad, > > > > > > > > > > > > Looking fwd. to your feedback regarding support of other memory > > > > > > encryption architectures such as Power, S390, etc. > > > > > > > > > > > > Thanks, > > > > > > Ashish > > > > > > > > > > > > On Fri, Jan 24, 2020 at 11:00:08PM +0000, Ashish Kalra wrote: > > > > > > > On Tue, Jan 21, 2020 at 03:54:03PM -0500, Konrad Rzeszutek Wilk wrote: > > > > > > > > > > > > > > > > > > Additional memory calculations based on # of PCI devices and > > > > > > > > > their memory ranges will make it more complicated with so > > > > > > > > > many other permutations and combinations to explore, it is > > > > > > > > > essential to keep this patch as simple as possible by > > > > > > > > > adjusting the bounce buffer size simply by determining it > > > > > > > > > from the amount of provisioned guest memory. > > > > > > > >> > > > > > > > >> Please rework the patch to: > > > > > > > >> > > > > > > > >> - Use a log solution instead of the multiplication. > > > > > > > >> Feel free to cap it at a sensible value. > > > > > > > > > > > > > > Ok. > > > > > > > > > > > > > > >> > > > > > > > >> - Also the code depends on SWIOTLB calling in to the > > > > > > > >> adjust_swiotlb_default_size which looks wrong. > > > > > > > >> > > > > > > > >> You should not adjust io_tlb_nslabs from swiotlb_size_or_default. > > > > > > > > > > > > > > >> That function's purpose is to report a value. > > > > > > > >> > > > > > > > >> - Make io_tlb_nslabs be visible outside of the SWIOTLB code. > > > > > > > >> > > > > > > > >> - Can you utilize the IOMMU_INIT APIs and have your own detect which would > > > > > > > >> modify the io_tlb_nslabs (and set swiotbl=1?). > > > > > > > > > > > > > > This seems to be a nice option, but then IOMMU_INIT APIs are > > > > > > > x86-specific and this swiotlb buffer size adjustment is also needed > > > > > > > for other memory encryption architectures like Power, S390, etc. > > > > > > > > > > Oh dear. That I hadn't considered. > > > > > > > > > > > > > > >> > > > > > > > >> Actually you seem to be piggybacking on pci_swiotlb_detect_4gb - so > > > > > > > >> perhaps add in this code ? Albeit it really should be in it's own > > > > > > > >> file, not in arch/x86/kernel/pci-swiotlb.c > > > > > > > > > > > > > > Actually, we piggyback on pci_swiotlb_detect_override which sets > > > > > > > swiotlb=1 as x86_64_start_kernel() and invocation of sme_early_init() > > > > > > > forces swiotlb on, but again this is all x86 architecture specific. > > > > > > > > > > Then it looks like the best bet is to do it from within swiotlb_init? > > > > > We really can't do it from swiotlb_size_or_default - that function > > > > > should just return a value and nothing else. > > > > > > > > > > > > > Actually, we need to do it in swiotlb_size_or_default() as this gets called by > > > > reserve_crashkernel_low() in arch/x86/kernel/setup.c and used to > > > > reserve low crashkernel memory. If we adjust swiotlb size later in > > > > swiotlb_init() which gets called later than reserve_crashkernel_low(), > > > > then any swiotlb size changes/expansion will conflict/overlap with the > > > > low memory reserved for crashkernel. > > > > > > > and will also potentially cause SWIOTLB buffer allocation failures. > > > > > > Do you have any feedback, comments on the above ? > > > > > > The init boot chain looks like this: > > > > initmem_init > > pci_iommu_alloc > > -> pci_swiotlb_detect_4gb > > -> swiotlb_init > > > > reserve_crashkernel > > reserve_crashkernel_low > > -> swiotlb_size_or_default > > .. > > > > > > (rootfs code): > > pci_iommu_init > > -> a bunch of the other IOMMU late_init code gets called.. > > -> pci_swiotlb_late_init > > > > I have to say I am lost to how your patch fixes "If we adjust swiolb > > size later .. then any swiotlb size .. will overlap with the low memory > > reserved for crashkernel"? > > > > Actually as per the boot flow : > > setup_arch() calls reserve_crashkernel() and pci_iommu_alloc() is > invoked through mm_init()/mem_init() and not via initmem_init(). > > start_kernel: > ... > setup_arch() > reserve_crashkernel > reserve_crashkernel_low > -> swiotlb_size_or_default > > ... > ... > mm_init() > mem_init() > pci_iommu_alloc > -> pci_swiotlb_detect_4gb > -> swiotlb_init > > So as per the above boot flow, reserve_crashkernel() can get called > before swiotlb_detect/init, and hence, if we don't fixup or adjust > the SWIOTLB buffer size in swiotlb_size_or_default() then crash kernel > will reserve memory which will conflict/overlap with any SWIOTLB bounce > buffer allocated memory (adjusted or fixed up later). > > Therefore, we need to adjust/fixup SWIOTLB bounce buffer memory in > swiotlb_size_or_default() function itself, before swiotlb detect/init > funtions get invoked. > Also to add here, it looks like swiotlb_size_or_default() is an interface function to get the SWIOTLB bounce buffer size for components which are initialized before swiotlb_detect/init, so that these components can reserve or allocate their memory requirements with the knowledge of how much SWIOTLB bounce buffers are going to use, so therefore, any fixups or adjustments to SWIOTLB buffer size will need to be made as part of swiotlb_size_or_default(). Thanks, Ashish > > Or are you saying that 'reserve_crashkernel_low' is the _culprit_ and it > > is the one changing the size? And hence it modifying the swiotlb size > > will fix this problem? Aka _before_ all the other IOMMU get their hand > > on it? > > > > If so why not create an > > IOMMU_INIT(crashkernel_adjust_swiotlb,pci_swiotlb_detect_override, > > NULL, NULL); > > > > And crashkernel_adjust_swiotlb would change the size of swiotlb buffer > > if conditions are found to require it. > > > > You also may want to put a #define DEBUG in arch/x86/kernel/pci-iommu_table.c > > to check out whether the tree structure of IOMMU entries is correct. > > > > > > > > But still I am lost - if say the AMD one does decide for unknown reason > > to expand the SWIOTLB you are still stuck with the 'overlap with > > the low memory reserved' or so. > > > > Perhaps add a late_init that gets called as the last one to validate > > this ? And maybe if the swiotlb gets turned off you also take proper > > steps? > > > > > As such i feel, this patch is complete otherwise and can be included as > > > it is. > > > > > > Thanks, > > > Ashish _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests. 2020-06-24 7:05 ` Ashish Kalra @ 2020-06-24 21:11 ` Konrad Rzeszutek Wilk 0 siblings, 0 replies; 14+ messages in thread From: Konrad Rzeszutek Wilk @ 2020-06-24 21:11 UTC (permalink / raw) To: Ashish Kalra Cc: Thomas.Lendacky, brijesh.singh, dave.hansen, peterz, x86, linux-kernel, iommu, mingo, bp, luto, hpa, Konrad Rzeszutek Wilk, tglx, hch .snip.. > > Actually as per the boot flow : > > > > setup_arch() calls reserve_crashkernel() and pci_iommu_alloc() is > > invoked through mm_init()/mem_init() and not via initmem_init(). > > > > start_kernel: > > ... > > setup_arch() > > reserve_crashkernel > > reserve_crashkernel_low > > -> swiotlb_size_or_default > > > > ... > > ... > > mm_init() > > mem_init() > > pci_iommu_alloc > > -> pci_swiotlb_detect_4gb > > -> swiotlb_init > > > > So as per the above boot flow, reserve_crashkernel() can get called > > before swiotlb_detect/init, and hence, if we don't fixup or adjust > > the SWIOTLB buffer size in swiotlb_size_or_default() then crash kernel > > will reserve memory which will conflict/overlap with any SWIOTLB bounce > > buffer allocated memory (adjusted or fixed up later). > > > > Therefore, we need to adjust/fixup SWIOTLB bounce buffer memory in > > swiotlb_size_or_default() function itself, before swiotlb detect/init > > funtions get invoked. > > > > Also to add here, it looks like swiotlb_size_or_default() is an > interface function to get the SWIOTLB bounce buffer size for components > which are initialized before swiotlb_detect/init, so that these > components can reserve or allocate their memory requirements with the > knowledge of how much SWIOTLB bounce buffers are going to use, so > therefore, any fixups or adjustments to SWIOTLB buffer size will need > to be made as part of swiotlb_size_or_default(). That was never its purpose. It was meant as way to figure out the segment size for DMA requests and to be used for runtime components. In fact to be idempotent. This is why I am disliking this usage and leaning towards something else. But you pointed out something interesting - you are in fact needing to adjust the size of the swiotlb based on your needs at bootup. Not any different from say 'swiotlb' paramter. Why not have an 'swiotlb_adjust' that is an __init that modifies the internal swiotlb buffer sizes? Obviously we have to account for 'swiotlb' parsing as well. The swiotlb_adjust would pick the max from those. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-06-24 21:13 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-09 23:13 [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests Ashish Kalra 2019-12-20 1:52 ` Konrad Rzeszutek Wilk 2020-01-21 20:09 ` Ashish Kalra 2020-01-21 20:54 ` Konrad Rzeszutek Wilk 2020-01-24 23:00 ` Ashish Kalra 2020-02-04 19:35 ` Ashish Kalra 2020-03-03 17:03 ` Konrad Rzeszutek Wilk 2020-03-12 0:43 ` Ashish Kalra 2020-03-30 22:25 ` Ashish Kalra 2020-04-27 18:53 ` Ashish Kalra 2020-06-23 13:38 ` Konrad Rzeszutek Wilk 2020-06-24 0:23 ` Ashish Kalra 2020-06-24 7:05 ` Ashish Kalra 2020-06-24 21:11 ` Konrad Rzeszutek Wilk
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).