[PATCH V2 0/2] x86/hyperv/Swiotlb: Add swiotlb_set_alloc_from_low_pages() switch function.

[PATCH V2 0/2] x86/hyperv/Swiotlb: Add swiotlb_set_alloc_from_low_pages() switch function.

[Tianyu Lan]

From: Tianyu Lan <Tianyu.Lan@microsoft.com>

Hyper-V Isolation VM may fail to allocate swiotlb bounce buffer due
to there is no enough contiguous memory from 0 to 4G in some cases.
Current swiotlb code allocates bounce buffer in the low end memory.
This patchset adds a new function swiotlb_set_alloc_from_low_pages()
to control swiotlb bounce buffer from low pages or no limitation.
Devices in Hyper-V Isolation VM may use memory above 4G as DMA memory
and switch swiotlb allocation in order to avoid no enough contiguous
memory in low pages.

Tianyu Lan (2):
  Swiotlb: Add swiotlb_alloc_from_low_pages switch
  x86/hyperv: Make swiotlb bounce buffer allocation not just from low
    pages

 arch/x86/kernel/cpu/mshyperv.c |  1 +
 include/linux/swiotlb.h        |  1 +
 kernel/dma/swiotlb.c           | 18 ++++++++++++++++--
 3 files changed, 18 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH V2 1/2] Swiotlb: Add swiotlb_alloc_from_low_pages switch

[Tianyu Lan]

From: Tianyu Lan <Tianyu.Lan@microsoft.com>

Hyper-V Isolation VM and AMD SEV VM uses swiotlb bounce buffer to
share memory with hypervisor. Current swiotlb bounce buffer is only
allocated from 0 to ARCH_LOW_ADDRESS_LIMIT which is default to
0xffffffffUL. Isolation VM and AMD SEV VM needs 1G bounce buffer at most.
This will fail when there is not enough memory from 0 to 4G address
space and devices also may use memory above 4G address space as DMA memory.
Expose swiotlb_alloc_from_low_pages and platform mey set it to false when
it's not necessary to limit bounce buffer from 0 to 4G memory.

Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
---
 include/linux/swiotlb.h |  1 +
 kernel/dma/swiotlb.c    | 18 ++++++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index f6c3638255d5..2b4f92668bc7 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -39,6 +39,7 @@ enum swiotlb_force {
 extern void swiotlb_init(int verbose);
 int swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose);
 unsigned long swiotlb_size_or_default(void);
+void swiotlb_set_alloc_from_low_pages(bool low);
 extern int swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs);
 extern int swiotlb_late_init_with_default_size(size_t default_size);
 extern void __init swiotlb_update_mem_attributes(void);
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index f1e7ea160b43..62bf8b5cc3e4 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -73,6 +73,8 @@ enum swiotlb_force swiotlb_force;
 
 struct io_tlb_mem io_tlb_default_mem;
 
+static bool swiotlb_alloc_from_low_pages = true;
+
 phys_addr_t swiotlb_unencrypted_base;
 
 /*
@@ -116,6 +118,11 @@ void swiotlb_set_max_segment(unsigned int val)
 		max_segment = rounddown(val, PAGE_SIZE);
 }
 
+void swiotlb_set_alloc_from_low_pages(bool low)
+{
+	swiotlb_alloc_from_low_pages = low;
+}
+
 unsigned long swiotlb_size_or_default(void)
 {
 	return default_nslabs << IO_TLB_SHIFT;
@@ -284,8 +291,15 @@ swiotlb_init(int verbose)
 	if (swiotlb_force == SWIOTLB_NO_FORCE)
 		return;
 
-	/* Get IO TLB memory from the low pages */
-	tlb = memblock_alloc_low(bytes, PAGE_SIZE);
+	/*
+	 * Get IO TLB memory from the low pages if swiotlb_alloc_from_low_pages
+	 * is set.
+	 */
+	if (swiotlb_alloc_from_low_pages)
+		tlb = memblock_alloc_low(bytes, PAGE_SIZE);
+	else
+		tlb = memblock_alloc(bytes, PAGE_SIZE);
+
 	if (!tlb)
 		goto fail;
 	if (swiotlb_init_with_tbl(tlb, default_nslabs, verbose))
-- 
2.25.1

Re: [PATCH V2 1/2] Swiotlb: Add swiotlb_alloc_from_low_pages switch

[Christoph Hellwig]

Adding a function to set the flag doesn't really change much.  As Robin
pointed out last time you should fine a way to just call
swiotlb_init_with_tbl directly with the memory allocated the way you
like it.  Or given that we have quite a few of these trusted hypervisor
schemes maybe add an argument to swiotlb_init that specifies how to
allocate the memory.

Re: [PATCH V2 1/2] Swiotlb: Add swiotlb_alloc_from_low_pages switch

[Tianyu Lan]

On 2/14/2022 4:19 PM, Christoph Hellwig wrote:
> Adding a function to set the flag doesn't really change much.  As Robin
> pointed out last time you should fine a way to just call
> swiotlb_init_with_tbl directly with the memory allocated the way you
> like it.  Or given that we have quite a few of these trusted hypervisor
> schemes maybe add an argument to swiotlb_init that specifies how to
> allocate the memory.

Thanks for your suggestion. I will try the first approach first approach.

Re: [PATCH V2 1/2] Swiotlb: Add swiotlb_alloc_from_low_pages switch

[Christoph Hellwig]

On Mon, Feb 14, 2022 at 07:28:40PM +0800, Tianyu Lan wrote:
> On 2/14/2022 4:19 PM, Christoph Hellwig wrote:
>> Adding a function to set the flag doesn't really change much.  As Robin
>> pointed out last time you should fine a way to just call
>> swiotlb_init_with_tbl directly with the memory allocated the way you
>> like it.  Or given that we have quite a few of these trusted hypervisor
>> schemes maybe add an argument to swiotlb_init that specifies how to
>> allocate the memory.
>
> Thanks for your suggestion. I will try the first approach first approach.

Take a look at the SWIOTLB_ANY flag in this WIP branch:

   http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/swiotlb-init-cleanup

That being said I'm not sure that either this flag or the existing powerpc
code iѕ actually the right thing to do.  We still need the 4G limited
buffer to support devices with addressing limitations.  So I think we need
an additional io_tlb_mem instance for the devices without addressing
limitations instead.

	

Re: [PATCH V2 1/2] Swiotlb: Add swiotlb_alloc_from_low_pages switch

[Tianyu Lan]

On 2/14/2022 9:58 PM, Christoph Hellwig wrote:
> On Mon, Feb 14, 2022 at 07:28:40PM +0800, Tianyu Lan wrote:
>> On 2/14/2022 4:19 PM, Christoph Hellwig wrote:
>>> Adding a function to set the flag doesn't really change much.  As Robin
>>> pointed out last time you should fine a way to just call
>>> swiotlb_init_with_tbl directly with the memory allocated the way you
>>> like it.  Or given that we have quite a few of these trusted hypervisor
>>> schemes maybe add an argument to swiotlb_init that specifies how to
>>> allocate the memory.
>>
>> Thanks for your suggestion. I will try the first approach first approach.
> 
> Take a look at the SWIOTLB_ANY flag in this WIP branch:
> 
>     http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/swiotlb-init-cleanup
> 
> That being said I'm not sure that either this flag or the existing powerpc
> code iѕ actually the right thing to do.  We still need the 4G limited
> buffer to support devices with addressing limitations.  So I think we need
> an additional io_tlb_mem instance for the devices without addressing
> limitations instead.
> 

Hi Christoph:
      Thanks for your patches. I tested these patches in Hyper-V trusted 
VM and system can't boot up. I am debugging and will report back.


	

Re: [PATCH V2 1/2] Swiotlb: Add swiotlb_alloc_from_low_pages switch

[Tianyu Lan]

On 2/15/2022 11:32 PM, Tianyu Lan wrote:
> On 2/14/2022 9:58 PM, Christoph Hellwig wrote:
>> On Mon, Feb 14, 2022 at 07:28:40PM +0800, Tianyu Lan wrote:
>>> On 2/14/2022 4:19 PM, Christoph Hellwig wrote:
>>>> Adding a function to set the flag doesn't really change much.  As Robin
>>>> pointed out last time you should fine a way to just call
>>>> swiotlb_init_with_tbl directly with the memory allocated the way you
>>>> like it.  Or given that we have quite a few of these trusted hypervisor
>>>> schemes maybe add an argument to swiotlb_init that specifies how to
>>>> allocate the memory.
>>>
>>> Thanks for your suggestion. I will try the first approach first 
>>> approach.
>>
>> Take a look at the SWIOTLB_ANY flag in this WIP branch:
>>
>>     
>> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/swiotlb-init-cleanup 
>>
>>
>> That being said I'm not sure that either this flag or the existing 
>> powerpc
>> code iѕ actually the right thing to do.  We still need the 4G limited
>> buffer to support devices with addressing limitations.  So I think we 
>> need
>> an additional io_tlb_mem instance for the devices without addressing
>> limitations instead.
>>
> 
> Hi Christoph:
>       Thanks for your patches. I tested these patches in Hyper-V trusted 
> VM and system can't boot up. I am debugging and will report back.

Sorry. The boot failure is not related with these patches and the issue
has been fixed in the latest upstream code.

There is a performance bottleneck due to io tlb mem's spin lock during
performance test. All devices'io queues uses same io tlb mem entry
and the spin lock of io tlb mem introduce overheads. There is a fix 
patch from Andi Kleen in the github. Could you have a look?

https://github.com/intel/tdx/commit/4529b5784c141782c72ec9bd9a92df2b68cb7d45

Thanks.

Re: [PATCH V2 1/2] Swiotlb: Add swiotlb_alloc_from_low_pages switch

[Christoph Hellwig]

On Mon, Feb 21, 2022 at 11:14:58PM +0800, Tianyu Lan wrote:
> Sorry. The boot failure is not related with these patches and the issue
> has been fixed in the latest upstream code.
>
> There is a performance bottleneck due to io tlb mem's spin lock during
> performance test. All devices'io queues uses same io tlb mem entry
> and the spin lock of io tlb mem introduce overheads. There is a fix patch 
> from Andi Kleen in the github. Could you have a look?
>
> https://github.com/intel/tdx/commit/4529b5784c141782c72ec9bd9a92df2b68cb7d45

Please post these things to the list.

But I suspect the right answer for the "secure" hypervisor case is to
use the per-device swiotlb regions that we've recently added.

Re: [PATCH V2 1/2] Swiotlb: Add swiotlb_alloc_from_low_pages switch

[Tianyu Lan]

On 2/22/2022 4:05 PM, Christoph Hellwig wrote:
> On Mon, Feb 21, 2022 at 11:14:58PM +0800, Tianyu Lan wrote:
>> Sorry. The boot failure is not related with these patches and the issue
>> has been fixed in the latest upstream code.
>>
>> There is a performance bottleneck due to io tlb mem's spin lock during
>> performance test. All devices'io queues uses same io tlb mem entry
>> and the spin lock of io tlb mem introduce overheads. There is a fix patch
>> from Andi Kleen in the github. Could you have a look?
>>
>> https://github.com/intel/tdx/commit/4529b5784c141782c72ec9bd9a92df2b68cb7d45
> 
> Please post these things to the list.
> 
> But I suspect the right answer for the "secure" hypervisor case is to
> use the per-device swiotlb regions that we've recently added.

Thanks for your comment. That means we need to expose an 
swiotlb_device_init() interface to allocate bounce buffer and initialize
io tlb mem entry. DMA API Current  rmem_swiotlb_device_init() only works
for platform with device tree. The new API should be called in the bus
driver or new DMA API. Could you check whether this is a right way 
before we start the work.

Thanks.

Re: [PATCH V2 1/2] Swiotlb: Add swiotlb_alloc_from_low_pages switch

[Christoph Hellwig]

On Tue, Feb 22, 2022 at 11:07:19PM +0800, Tianyu Lan wrote:
> Thanks for your comment. That means we need to expose an 
> swiotlb_device_init() interface to allocate bounce buffer and initialize
> io tlb mem entry. DMA API Current  rmem_swiotlb_device_init() only works
> for platform with device tree. The new API should be called in the bus
> driver or new DMA API. Could you check whether this is a right way before 
> we start the work.

Do these VMs use ACPI?  We'd probably really want some kind of higher
level configuration and not have the drivers request it themselves.

Re: [PATCH V2 1/2] Swiotlb: Add swiotlb_alloc_from_low_pages switch

[Tianyu Lan]

On 2/23/2022 12:00 AM, Christoph Hellwig wrote:
> On Tue, Feb 22, 2022 at 11:07:19PM +0800, Tianyu Lan wrote:
>> Thanks for your comment. That means we need to expose an
>> swiotlb_device_init() interface to allocate bounce buffer and initialize
>> io tlb mem entry. DMA API Current  rmem_swiotlb_device_init() only works
>> for platform with device tree. The new API should be called in the bus
>> driver or new DMA API. Could you check whether this is a right way before
>> we start the work.
> 
> Do these VMs use ACPI?  We'd probably really want some kind of higher
> level configuration and not have the drivers request it themselves.

Yes, Hyper-V isolation VM uses ACPI. Devices are enumerated via vmbus 
host and there is no child device information in ACPI table. The host 
driver seems to be the right place to call new API.

Re: [PATCH V2 1/2] Swiotlb: Add swiotlb_alloc_from_low_pages switch

[Tianyu Lan]

On 2/23/2022 5:46 PM, Tianyu Lan wrote:
> 
> 
> On 2/23/2022 12:00 AM, Christoph Hellwig wrote:
>> On Tue, Feb 22, 2022 at 11:07:19PM +0800, Tianyu Lan wrote:
>>> Thanks for your comment. That means we need to expose an
>>> swiotlb_device_init() interface to allocate bounce buffer and initialize
>>> io tlb mem entry. DMA API Current  rmem_swiotlb_device_init() only works
>>> for platform with device tree. The new API should be called in the bus
>>> driver or new DMA API. Could you check whether this is a right way 
>>> before
>>> we start the work.
>>
>> Do these VMs use ACPI?  We'd probably really want some kind of higher
>> level configuration and not have the drivers request it themselves.
> 
> Yes, Hyper-V isolation VM uses ACPI. Devices are enumerated via vmbus 
> host and there is no child device information in ACPI table. The host 
> driver seems to be the right place to call new API.

Hi Christoph:
      One more perspective is that one device may have multiple queues 
and each queues should have independent swiotlb bounce buffer to avoid 
spin lock overhead. The number of queues is only available in the device
driver. This means new API needs to be called in the device driver 
according to queue number.

Re: [PATCH V2 1/2] Swiotlb: Add swiotlb_alloc_from_low_pages switch

[Christoph Hellwig]

On Fri, Feb 25, 2022 at 10:28:54PM +0800, Tianyu Lan wrote:
>      One more perspective is that one device may have multiple queues and 
> each queues should have independent swiotlb bounce buffer to avoid spin 
> lock overhead. The number of queues is only available in the device
> driver. This means new API needs to be called in the device driver 
> according to queue number.

Well, given how hell bent people are on bounce buffering we might
need some scalability work there anyway.

Re: [PATCH V2 1/2] Swiotlb: Add swiotlb_alloc_from_low_pages switch

[Tianyu Lan]

On 3/1/2022 7:53 PM, Christoph Hellwig wrote:
> On Fri, Feb 25, 2022 at 10:28:54PM +0800, Tianyu Lan wrote:
>>       One more perspective is that one device may have multiple queues and
>> each queues should have independent swiotlb bounce buffer to avoid spin
>> lock overhead. The number of queues is only available in the device
>> driver. This means new API needs to be called in the device driver
>> according to queue number.
> 
> Well, given how hell bent people are on bounce buffering we might
> need some scalability work there anyway.

According to my test on the local machine with two VMs, Linux guest 
without swiotlb bounce buffer or with the fix patch from Andi Kleen can
achieve about 40G/s throughput but it's just 24-25G/s with current 
swiotlb code. Otherwise, the spinlock contention also consumes more cpu 
usage.

[PATCH V2 2/2] x86/hyperv: Make swiotlb bounce buffer allocation not just from low pages

[Tianyu Lan]

From: Tianyu Lan <Tianyu.Lan@microsoft.com>

In Hyper-V Isolation VM, swiotlb bnounce buffer size maybe 1G at most
and there maybe no enough memory from 0 to 4G according to memory layout.
Devices in Isolation VM can use memory above 4G as DMA memory and call
swiotlb_alloc_from_low_pages() to allocate swiotlb bounce buffer not
limit from 0 to 4G.

Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
---
 arch/x86/kernel/cpu/mshyperv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 5a99f993e639..50ba4622c650 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -343,6 +343,7 @@ static void __init ms_hyperv_init_platform(void)
 		 * use swiotlb bounce buffer for dma transaction.
 		 */
 		swiotlb_force = SWIOTLB_FORCE;
+		swiotlb_set_alloc_from_low_pages(false);
 #endif
 	}
 
-- 
2.25.1