From: Tianyu Lan <hidden> Date: 2021-08-09 17:56:30
From: Tianyu Lan <redacted>
Hyper-V provides two kinds of Isolation VMs. VBS(Virtualization-based
security) and AMD SEV-SNP unenlightened Isolation VMs. This patchset
is to add support for these Isolation VM support in Linux.
The memory of these vms are encrypted and host can't access guest
memory directly. Hyper-V provides new host visibility hvcall and
the guest needs to call new hvcall to mark memory visible to host
before sharing memory with host. For security, all network/storage
stack memory should not be shared with host and so there is bounce
buffer requests.
Vmbus channel ring buffer already plays bounce buffer role because
all data from/to host needs to copy from/to between the ring buffer
and IO stack memory. So mark vmbus channel ring buffer visible.
There are two exceptions - packets sent by vmbus_sendpacket_
pagebuffer() and vmbus_sendpacket_mpb_desc(). These packets
contains IO stack memory address and host will access these memory.
So add allocation bounce buffer support in vmbus for these packets.
For SNP isolation VM, guest needs to access the shared memory via
extra address space which is specified by Hyper-V CPUID HYPERV_CPUID_
ISOLATION_CONFIG. The access physical address of the shared memory
should be bounce buffer memory GPA plus with shared_gpa_boundary
reported by CPUID.
Change since V2:
- Drop x86_set_memory_enc static call and use platform check
in the __set_memory_enc_dec() to run platform callback of
set memory encrypted or decrypted.
Change since V1:
- Introduce x86_set_memory_enc static call and so platforms can
override __set_memory_enc_dec() with their implementation
- Introduce sev_es_ghcb_hv_call_simple() and share code
between SEV and Hyper-V code.
- Not remap monitor pages in the non-SNP isolation VM
- Make swiotlb_init_io_tlb_mem() return error code and return
error when dma_map_decrypted() fails.
Change since RFC V4:
- Introduce dma map decrypted function to remap bounce buffer
and provide dma map decrypted ops for platform to hook callback.
- Split swiotlb and dma map decrypted change into two patches
- Replace vstart with vaddr in swiotlb changes.
Change since RFC v3:
- Add interface set_memory_decrypted_map() to decrypt memory and
map bounce buffer in extra address space
- Remove swiotlb remap function and store the remap address
returned by set_memory_decrypted_map() in swiotlb mem data structure.
- Introduce hv_set_mem_enc() to make code more readable in the __set_memory_enc_dec().
Change since RFC v2:
- Remove not UIO driver in Isolation VM patch
- Use vmap_pfn() to replace ioremap_page_range function in
order to avoid exposing symbol ioremap_page_range() and
ioremap_page_range()
- Call hv set mem host visibility hvcall in set_memory_encrypted/decrypted()
- Enable swiotlb force mode instead of adding Hyper-V dma map/unmap hook
- Fix code style
Tianyu Lan (13):
x86/HV: Initialize GHCB page in Isolation VM
x86/HV: Initialize shared memory boundary in the Isolation VM.
x86/HV: Add new hvcall guest address host visibility support
HV: Mark vmbus ring buffer visible to host in Isolation VM
HV: Add Write/Read MSR registers via ghcb page
HV: Add ghcb hvcall support for SNP VM
HV/Vmbus: Add SNP support for VMbus channel initiate message
HV/Vmbus: Initialize VMbus ring buffer for Isolation VM
DMA: Add dma_map_decrypted/dma_unmap_encrypted() function
x86/Swiotlb: Add Swiotlb bounce buffer remap function for HV IVM
HV/IOMMU: Enable swiotlb bounce buffer for Isolation VM
HV/Netvsc: Add Isolation VM support for netvsc driver
HV/Storvsc: Add Isolation VM support for storvsc driver
arch/x86/hyperv/Makefile | 2 +-
arch/x86/hyperv/hv_init.c | 75 ++++++--
arch/x86/hyperv/ivm.c | 295 +++++++++++++++++++++++++++++
arch/x86/include/asm/hyperv-tlfs.h | 20 ++
arch/x86/include/asm/mshyperv.h | 87 ++++++++-
arch/x86/include/asm/sev.h | 3 +
arch/x86/kernel/cpu/mshyperv.c | 5 +
arch/x86/kernel/sev-shared.c | 63 +++---
arch/x86/mm/pat/set_memory.c | 19 +-
arch/x86/xen/pci-swiotlb-xen.c | 3 +-
drivers/hv/Kconfig | 1 +
drivers/hv/channel.c | 54 +++++-
drivers/hv/connection.c | 71 ++++++-
drivers/hv/hv.c | 129 +++++++++----
drivers/hv/hyperv_vmbus.h | 3 +
drivers/hv/ring_buffer.c | 84 ++++++--
drivers/hv/vmbus_drv.c | 3 +
drivers/iommu/hyperv-iommu.c | 65 +++++++
drivers/net/hyperv/hyperv_net.h | 6 +
drivers/net/hyperv/netvsc.c | 144 +++++++++++++-
drivers/net/hyperv/rndis_filter.c | 2 +
drivers/scsi/storvsc_drv.c | 68 ++++++-
include/asm-generic/hyperv-tlfs.h | 1 +
include/asm-generic/mshyperv.h | 54 +++++-
include/linux/dma-map-ops.h | 9 +
include/linux/hyperv.h | 17 ++
include/linux/swiotlb.h | 4 +
kernel/dma/mapping.c | 22 +++
kernel/dma/swiotlb.c | 32 +++-
29 files changed, 1212 insertions(+), 129 deletions(-)
create mode 100644 arch/x86/hyperv/ivm.c
--
2.25.1
From: Tianyu Lan <hidden> Date: 2021-08-09 17:56:35
From: Tianyu Lan <redacted>
Hyper-V exposes GHCB page via SEV ES GHCB MSR for SNP guest
to communicate with hypervisor. Map GHCB page for all
cpus to read/write MSR register and submit hvcall request
via GHCB.
Signed-off-by: Tianyu Lan <redacted>
---
arch/x86/hyperv/hv_init.c | 66 +++++++++++++++++++++++++++++++--
arch/x86/include/asm/mshyperv.h | 2 +
include/asm-generic/mshyperv.h | 2 +
3 files changed, 66 insertions(+), 4 deletions(-)
@@ -85,6 +111,8 @@ static int hv_cpu_init(unsigned int cpu)}}+hyperv_init_ghcb();+return0;}
@@ -177,6 +205,14 @@ static int hv_cpu_die(unsigned int cpu){structhv_reenlightenment_controlre_ctrl;unsignedintnew_cpu;+void**ghcb_va=NULL;++if(ms_hyperv.ghcb_base){+ghcb_va=(void**)this_cpu_ptr(ms_hyperv.ghcb_base);+if(*ghcb_va)+memunmap(*ghcb_va);+*ghcb_va=NULL;+}hv_common_cpu_die(cpu);
From: Michael Kelley <hidden> Date: 2021-08-12 19:15:08
From: Tianyu Lan <redacted> Sent: Monday, August 9, 2021 10:56 AM
Subject: [PATCH V3 01/13] x86/HV: Initialize GHCB page in Isolation VM
The subject line tag on patches under arch/x86/hyperv is generally "x86/hyperv:".
There's some variation in the spelling of "hyperv", but let's go with the all
lowercase "hyperv".
quoted hunk
Hyper-V exposes GHCB page via SEV ES GHCB MSR for SNP guest
to communicate with hypervisor. Map GHCB page for all
cpus to read/write MSR register and submit hvcall request
via GHCB.
Signed-off-by: Tianyu Lan <redacted>
---
arch/x86/hyperv/hv_init.c | 66 +++++++++++++++++++++++++++++++--
arch/x86/include/asm/mshyperv.h | 2 +
include/asm-generic/mshyperv.h | 2 +
3 files changed, 66 insertions(+), 4 deletions(-)
@@ -383,9 +419,19 @@ void __init hyperv_init(void) VMALLOC_END, GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS, NUMA_NO_NODE, __builtin_return_address(0));- if (hv_hypercall_pg == NULL) {- wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);- goto remove_cpuhp_state;+ if (hv_hypercall_pg == NULL)+ goto clean_guest_os_id;++ if (hv_isolation_type_snp()) {+ ms_hyperv.ghcb_base = alloc_percpu(void *);+ if (!ms_hyperv.ghcb_base)+ goto clean_guest_os_id;++ if (hyperv_init_ghcb()) {+ free_percpu(ms_hyperv.ghcb_base);+ ms_hyperv.ghcb_base = NULL;+ goto clean_guest_os_id;+ }
Having the GHCB setup code here splits the hypercall page setup into
two parts, which is unexpected. First the memory is allocated
for the hypercall page, then the GHCB stuff is done, then the hypercall
MSR is setup. Is there a need to do this split? Also, if the GHCB stuff
fails and you goto clean_guest_os_id, the memory allocated for the
hypercall page is never freed.
It's also unexpected to have hyperv_init_ghcb() called here and called
in hv_cpu_init(). Wouldn't it be possible to setup ghcb_base *before*
cpu_setup_state() is called, so that hv_cpu_init() would take care of
calling hyperv_init_ghcb() for the boot CPU? That's the pattern used
by the VP assist page, the percpu input page, etc.
I don't think this cleanup is necessary. The primary purpose of
hyperv_cleanup() is to ensure that things like overlay pages are
properly reset in Hyper-V before doing a kexec(), or before
panic'ing and running the kdump kernel. There's no need to do
general memory free'ing in Linux. Doing so just adds to the risk
that the panic path could itself fail.
This doesn't feel like the right place to put this pointer. The other
fields in the ms_hyperv_info structure are just fixed values obtained
from the CPUID instruction. The existing patterns similar to ghcb_base
are the VP assist page and the percpu input and output args. They are
all based on standalone global variables. It would be more consistent
to do the same with the ghcb_base.
From: Tianyu Lan <hidden> Date: 2021-08-13 15:46:40
Hi Michael:
Thanks for your review.
On 8/13/2021 3:14 AM, Michael Kelley wrote:
From: Tianyu Lan <redacted> Sent: Monday, August 9, 2021 10:56 AM
quoted
Subject: [PATCH V3 01/13] x86/HV: Initialize GHCB page in Isolation VM
The subject line tag on patches under arch/x86/hyperv is generally "x86/hyperv:".
There's some variation in the spelling of "hyperv", but let's go with the all
lowercase "hyperv".
OK. Will update.
quoted
Hyper-V exposes GHCB page via SEV ES GHCB MSR for SNP guest
to communicate with hypervisor. Map GHCB page for all
cpus to read/write MSR register and submit hvcall request
via GHCB.
Signed-off-by: Tianyu Lan <redacted>
---
arch/x86/hyperv/hv_init.c | 66 +++++++++++++++++++++++++++++++--
arch/x86/include/asm/mshyperv.h | 2 +
include/asm-generic/mshyperv.h | 2 +
3 files changed, 66 insertions(+), 4 deletions(-)
@@ -383,9 +419,19 @@ void __init hyperv_init(void) VMALLOC_END, GFP_KERNEL, PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS, NUMA_NO_NODE, __builtin_return_address(0));- if (hv_hypercall_pg == NULL) {- wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);- goto remove_cpuhp_state;+ if (hv_hypercall_pg == NULL)+ goto clean_guest_os_id;++ if (hv_isolation_type_snp()) {+ ms_hyperv.ghcb_base = alloc_percpu(void *);+ if (!ms_hyperv.ghcb_base)+ goto clean_guest_os_id;++ if (hyperv_init_ghcb()) {+ free_percpu(ms_hyperv.ghcb_base);+ ms_hyperv.ghcb_base = NULL;+ goto clean_guest_os_id;+ }
Having the GHCB setup code here splits the hypercall page setup into
two parts, which is unexpected. First the memory is allocated
for the hypercall page, then the GHCB stuff is done, then the hypercall
MSR is setup. Is there a need to do this split? Also, if the GHCB stuff
fails and you goto clean_guest_os_id, the memory allocated for the
hypercall page is never freed.
Just not enable hypercall when fails to setup ghcb. Otherwise, we need
to disable hypercall in the failure code path.
Yes,hypercall page should be freed in the clean_guest_os_id path.
It's also unexpected to have hyperv_init_ghcb() called here and called
in hv_cpu_init(). Wouldn't it be possible to setup ghcb_base *before*
cpu_setup_state() is called, so that hv_cpu_init() would take care of
calling hyperv_init_ghcb() for the boot CPU? That's the pattern used
by the VP assist page, the percpu input page, etc.
I will have a try and report back. Thanks for suggestion.
I don't think this cleanup is necessary. The primary purpose of
hyperv_cleanup() is to ensure that things like overlay pages are
properly reset in Hyper-V before doing a kexec(), or before
panic'ing and running the kdump kernel. There's no need to do
general memory free'ing in Linux. Doing so just adds to the risk
that the panic path could itself fail.
This doesn't feel like the right place to put this pointer. The other
fields in the ms_hyperv_info structure are just fixed values obtained
from the CPUID instruction. The existing patterns similar to ghcb_base
are the VP assist page and the percpu input and output args. They are
all based on standalone global variables. It would be more consistent
to do the same with the ghcb_base.
From: Tianyu Lan <hidden> Date: 2021-08-09 17:56:40
From: Tianyu Lan <redacted>
Add new hvcall guest address host visibility support to mark
memory visible to host. Call it inside set_memory_decrypted
/encrypted(). Add HYPERVISOR feature check in the
hv_is_isolation_supported() to optimize in non-virtualization
environment.
Signed-off-by: Tianyu Lan <redacted>
---
Change since v2:
* Rework __set_memory_enc_dec() and call Hyper-V and AMD function
according to platform check.
Change since v1:
* Use new staic call x86_set_memory_enc to avoid add Hyper-V
specific check in the set_memory code.
---
arch/x86/hyperv/Makefile | 2 +-
arch/x86/hyperv/hv_init.c | 6 ++
arch/x86/hyperv/ivm.c | 114 +++++++++++++++++++++++++++++
arch/x86/include/asm/hyperv-tlfs.h | 20 +++++
arch/x86/include/asm/mshyperv.h | 4 +-
arch/x86/mm/pat/set_memory.c | 19 +++--
include/asm-generic/hyperv-tlfs.h | 1 +
include/asm-generic/mshyperv.h | 1 +
8 files changed, 160 insertions(+), 7 deletions(-)
create mode 100644 arch/x86/hyperv/ivm.c
@@ -587,4 +594,17 @@ enum hv_interrupt_type {#include<asm-generic/hyperv-tlfs.h>+/* All input parameters should be in single page. */+#define HV_MAX_MODIFY_GPA_REP_COUNT \+((PAGE_SIZE/sizeof(u64))-2)++/* HvCallModifySparseGpaPageHostVisibility hypercall */+structhv_gpa_range_for_visibility{+u64partition_id;+u32host_visibility:2;+u32reserved0:30;+u32reserved1;+u64gpa_page_list[HV_MAX_MODIFY_GPA_REP_COUNT];+}__packed;+#endif
@@ -1980,15 +1982,11 @@ int set_memory_global(unsigned long addr, int numpages)__pgprot(_PAGE_GLOBAL),0);}-staticint__set_memory_enc_dec(unsignedlongaddr,intnumpages,boolenc)+staticint__set_memory_enc_pgtable(unsignedlongaddr,intnumpages,boolenc){structcpa_datacpa;intret;-/* Nothing to do if memory encryption is not active */-if(!mem_encrypt_active())-return0;-/* Should not be working on unaligned addresses */if(WARN_ONCE(addr&~PAGE_MASK,"misaligned address: %#lx\n",addr))addr&=PAGE_MASK;
@@ -2023,6 +2021,17 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)returnret;}+staticint__set_memory_enc_dec(unsignedlongaddr,intnumpages,boolenc)+{+if(hv_is_isolation_supported())+returnhv_set_mem_host_visibility(addr,numpages,!enc);++if(mem_encrypt_active())+return__set_memory_enc_pgtable(addr,numpages,enc);++return0;+}+intset_memory_encrypted(unsignedlongaddr,intnumpages){return__set_memory_enc_dec(addr,numpages,true);
From: Dave Hansen <hidden> Date: 2021-08-09 22:12:49
On 8/9/21 10:56 AM, Tianyu Lan wrote:
From: Tianyu Lan <redacted>
Add new hvcall guest address host visibility support to mark
memory visible to host. Call it inside set_memory_decrypted
/encrypted(). Add HYPERVISOR feature check in the
hv_is_isolation_supported() to optimize in non-virtualization
environment.
From an x86/mm perspective:
Acked-by: Dave Hansen <redacted>
A tiny nit:
This might be worthwhile to move to a header. That ensures that
hv_is_isolation_supported() use can avoid even a function call. But, I
see this is used in modules and its use here is also in a slow path, so
it's not a big deal
From: Tianyu Lan <hidden> Date: 2021-08-10 13:09:35
On 8/10/2021 6:12 AM, Dave Hansen wrote:
On 8/9/21 10:56 AM, Tianyu Lan wrote:
quoted
From: Tianyu Lan <redacted>
Add new hvcall guest address host visibility support to mark
memory visible to host. Call it inside set_memory_decrypted
/encrypted(). Add HYPERVISOR feature check in the
hv_is_isolation_supported() to optimize in non-virtualization
environment.
From an x86/mm perspective:
Acked-by: Dave Hansen <redacted>
This might be worthwhile to move to a header. That ensures that
hv_is_isolation_supported() use can avoid even a function call. But, I
see this is used in modules and its use here is also in a slow path, so
it's not a big deal
I will move it to header in the following version.
Thanks.
From: Wei Liu <wei.liu@kernel.org> Date: 2021-08-10 11:04:08
On Mon, Aug 09, 2021 at 01:56:07PM -0400, Tianyu Lan wrote:
quoted hunk
From: Tianyu Lan <redacted>
Add new hvcall guest address host visibility support to mark
memory visible to host. Call it inside set_memory_decrypted
/encrypted(). Add HYPERVISOR feature check in the
hv_is_isolation_supported() to optimize in non-virtualization
environment.
Signed-off-by: Tianyu Lan <redacted>
---
Change since v2:
* Rework __set_memory_enc_dec() and call Hyper-V and AMD function
according to platform check.
Change since v1:
* Use new staic call x86_set_memory_enc to avoid add Hyper-V
specific check in the set_memory code.
---
arch/x86/hyperv/Makefile | 2 +-
arch/x86/hyperv/hv_init.c | 6 ++
arch/x86/hyperv/ivm.c | 114 +++++++++++++++++++++++++++++
arch/x86/include/asm/hyperv-tlfs.h | 20 +++++
arch/x86/include/asm/mshyperv.h | 4 +-
arch/x86/mm/pat/set_memory.c | 19 +++--
include/asm-generic/hyperv-tlfs.h | 1 +
include/asm-generic/mshyperv.h | 1 +
8 files changed, 160 insertions(+), 7 deletions(-)
create mode 100644 arch/x86/hyperv/ivm.c
From: Michael Kelley <hidden> Date: 2021-08-12 19:36:17
From: Tianyu Lan <redacted> Sent: Monday, August 9, 2021 10:56 AM
Subject: [PATCH V3 03/13] x86/HV: Add new hvcall guest address host visibility support
Use "x86/hyperv:" tag in the Subject line.
quoted hunk
From: Tianyu Lan <redacted>
Add new hvcall guest address host visibility support to mark
memory visible to host. Call it inside set_memory_decrypted
/encrypted(). Add HYPERVISOR feature check in the
hv_is_isolation_supported() to optimize in non-virtualization
environment.
Signed-off-by: Tianyu Lan <redacted>
---
Change since v2:
* Rework __set_memory_enc_dec() and call Hyper-V and AMD function
according to platform check.
Change since v1:
* Use new staic call x86_set_memory_enc to avoid add Hyper-V
specific check in the set_memory code.
---
arch/x86/hyperv/Makefile | 2 +-
arch/x86/hyperv/hv_init.c | 6 ++
arch/x86/hyperv/ivm.c | 114 +++++++++++++++++++++++++++++
arch/x86/include/asm/hyperv-tlfs.h | 20 +++++
arch/x86/include/asm/mshyperv.h | 4 +-
arch/x86/mm/pat/set_memory.c | 19 +++--
include/asm-generic/hyperv-tlfs.h | 1 +
include/asm-generic/mshyperv.h | 1 +
8 files changed, 160 insertions(+), 7 deletions(-)
create mode 100644 arch/x86/hyperv/ivm.c
Could all of the tests in this function be run at initialization time, and
a single Boolean value pre-computed that this function returns? I don't
think any of tests would change during the lifetime of the Linux instance,
so running the tests every time is slower than it needs to be.
@@ -0,0 +1,114 @@+// SPDX-License-Identifier: GPL-2.0+/*+*Hyper-VIsolationVMinterfacewithparavisorandhypervisor+*+*Author:+*TianyuLan<Tianyu.Lan@microsoft.com>+*/++#include<linux/hyperv.h>+#include<linux/types.h>+#include<linux/bitfield.h>+#include<linux/slab.h>+#include<asm/io.h>+#include<asm/mshyperv.h>++/*+*hv_mark_gpa_visibility-Setpagesvisibletohostviahvcall.+*+*InIsolationVM,allguestmemoryisencriptedfromhostandguest+*needstosetmemoryvisibletohostviahvcallbeforesharingmemory+*withhost.+*/+inthv_mark_gpa_visibility(u16count,constu64pfn[],+enumhv_mem_host_visibilityvisibility)+{+structhv_gpa_range_for_visibility**input_pcpu,*input;+u16pages_processed;+u64hv_status;+unsignedlongflags;++/* no-op if partition isolation is not enabled */+if(!hv_is_isolation_supported())+return0;++if(count>HV_MAX_MODIFY_GPA_REP_COUNT){+pr_err("Hyper-V: GPA count:%d exceeds supported:%lu\n",count,+HV_MAX_MODIFY_GPA_REP_COUNT);+return-EINVAL;+}++local_irq_save(flags);+input_pcpu=(structhv_gpa_range_for_visibility**)+this_cpu_ptr(hyperv_pcpu_input_arg);+input=*input_pcpu;+if(unlikely(!input)){+local_irq_restore(flags);+return-EINVAL;+}++input->partition_id=HV_PARTITION_ID_SELF;+input->host_visibility=visibility;+input->reserved0=0;+input->reserved1=0;+memcpy((void*)input->gpa_page_list,pfn,count*sizeof(*pfn));+hv_status=hv_do_rep_hypercall(+HVCALL_MODIFY_SPARSE_GPA_PAGE_HOST_VISIBILITY,count,+0,input,&pages_processed);+local_irq_restore(flags);++if(!(hv_status&HV_HYPERCALL_RESULT_MASK))+return0;
pages_processed should also be checked to ensure that it equals count.
If not, something has gone wrong in the hypercall.
+
+ return hv_status & HV_HYPERCALL_RESULT_MASK;
+}
+EXPORT_SYMBOL(hv_mark_gpa_visibility);
+
+static int __hv_set_mem_host_visibility(void *kbuffer, int pagecount,
+ enum hv_mem_host_visibility visibility)
+{
+ u64 *pfn_array;
+ int ret = 0;
+ int i, pfn;
+
+ if (!hv_is_isolation_supported() || !ms_hyperv.ghcb_base)
+ return 0;
+
+ pfn_array = kzalloc(HV_HYP_PAGE_SIZE, GFP_KERNEL);
Does the page need to be zero'ed? All bytes that are used will
be explicitly written in the loop below.
+ if (!pfn_array)
+ return -ENOMEM;
+
+ for (i = 0, pfn = 0; i < pagecount; i++) {
+ pfn_array[pfn] = virt_to_hvpfn(kbuffer + i * HV_HYP_PAGE_SIZE);
+ pfn++;
+
+ if (pfn == HV_MAX_MODIFY_GPA_REP_COUNT || i == pagecount - 1) {
+ ret |= hv_mark_gpa_visibility(pfn, pfn_array,
+ visibility);
I don't see why value of "ret" is OR'ed. If the result of hv_mark_gpa_visibility()
is ever non-zero, we'll exit immediately. There's no need to accumulate the
results of multiple calls to hv_mark_gpa_visibility().
quoted hunk
+ pfn = 0;
+
+ if (ret)
+ goto err_free_pfn_array;
+ }
+ }
+
+ err_free_pfn_array:
+ kfree(pfn_array);
+ return ret;
+}
+
+/*
+ * hv_set_mem_host_visibility - Set specified memory visible to host.
+ *
+ * In Isolation VM, all guest memory is encrypted from host and guest
+ * needs to set memory visible to host via hvcall before sharing memory
+ * with host. This function works as wrap of hv_mark_gpa_visibility()
+ * with memory base and size.
+ */
+int hv_set_mem_host_visibility(unsigned long addr, int numpages, bool visible)
+{
+ enum hv_mem_host_visibility visibility = visible ?
+ VMBUS_PAGE_VISIBLE_READ_WRITE : VMBUS_PAGE_NOT_VISIBLE;
+
+ return __hv_set_mem_host_visibility((void *)addr, numpages, visibility);
+}
@@ -587,4 +594,17 @@ enum hv_interrupt_type {#include<asm-generic/hyperv-tlfs.h>+/* All input parameters should be in single page. */+#define HV_MAX_MODIFY_GPA_REP_COUNT \+((PAGE_SIZE/sizeof(u64))-2)++/* HvCallModifySparseGpaPageHostVisibility hypercall */+structhv_gpa_range_for_visibility{+u64partition_id;+u32host_visibility:2;+u32reserved0:30;+u32reserved1;+u64gpa_page_list[HV_MAX_MODIFY_GPA_REP_COUNT];+}__packed;+
We should avoid adding definitions *after* the #include of
<asm-generic/hyperv-tlfs.h>. That #include should be last. Any
reason these can't go earlier? And they really go together with
enum hv_mem_host_visibility.
Separately, take a look at how the structure hv_memory_hint
and HV_MEMORY_HINT_MAX_GPA_PAGE_RANGES is handled.
It's a close parallel to what you are doing above, and is a slightly
cleaner approach.
@@ -1980,15 +1982,11 @@ int set_memory_global(unsigned long addr, int numpages)__pgprot(_PAGE_GLOBAL),0);}-staticint__set_memory_enc_dec(unsignedlongaddr,intnumpages,boolenc)+staticint__set_memory_enc_pgtable(unsignedlongaddr,intnumpages,boolenc){structcpa_datacpa;intret;-/* Nothing to do if memory encryption is not active */-if(!mem_encrypt_active())-return0;-/* Should not be working on unaligned addresses */if(WARN_ONCE(addr&~PAGE_MASK,"misaligned address: %#lx\n",addr))addr&=PAGE_MASK;
@@ -2023,6 +2021,17 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)returnret;}+staticint__set_memory_enc_dec(unsignedlongaddr,intnumpages,boolenc)+{+if(hv_is_isolation_supported())+returnhv_set_mem_host_visibility(addr,numpages,!enc);++if(mem_encrypt_active())+return__set_memory_enc_pgtable(addr,numpages,enc);++return0;+}+intset_memory_encrypted(unsignedlongaddr,intnumpages){return__set_memory_enc_dec(addr,numpages,true);
@@ -1980,15 +1982,11 @@ int set_memory_global(unsigned long addr, int numpages)__pgprot(_PAGE_GLOBAL),0);}-staticint__set_memory_enc_dec(unsignedlongaddr,intnumpages,boolenc)+staticint__set_memory_enc_pgtable(unsignedlongaddr,intnumpages,boolenc){structcpa_datacpa;intret;-/* Nothing to do if memory encryption is not active */-if(!mem_encrypt_active())-return0;-/* Should not be working on unaligned addresses */if(WARN_ONCE(addr&~PAGE_MASK,"misaligned address: %#lx\n",addr))addr&=PAGE_MASK;
@@ -2023,6 +2021,17 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)returnret;}+staticint__set_memory_enc_dec(unsignedlongaddr,intnumpages,boolenc)+{+if(hv_is_isolation_supported())+returnhv_set_mem_host_visibility(addr,numpages,!enc);++if(mem_encrypt_active())+return__set_memory_enc_pgtable(addr,numpages,enc);++return0;+}+
From: Tianyu Lan <hidden> Date: 2021-08-09 17:56:45
From: Tianyu Lan <redacted>
Hyper-V exposes shared memory boundary via cpuid
HYPERV_CPUID_ISOLATION_CONFIG and store it in the
shared_gpa_boundary of ms_hyperv struct. This prepares
to share memory with host for SNP guest.
Signed-off-by: Tianyu Lan <redacted>
---
arch/x86/kernel/cpu/mshyperv.c | 2 ++
include/asm-generic/mshyperv.h | 12 +++++++++++-
2 files changed, 13 insertions(+), 1 deletion(-)
@@ -313,6 +313,8 @@ static void __init ms_hyperv_init_platform(void)if(ms_hyperv.priv_high&HV_ISOLATION){ms_hyperv.isolation_config_a=cpuid_eax(HYPERV_CPUID_ISOLATION_CONFIG);ms_hyperv.isolation_config_b=cpuid_ebx(HYPERV_CPUID_ISOLATION_CONFIG);+ms_hyperv.shared_gpa_boundary=+(u64)1<<ms_hyperv.shared_gpa_boundary_bits;pr_info("Hyper-V: Isolation Config: Group A 0x%x, Group B 0x%x\n",ms_hyperv.isolation_config_a,ms_hyperv.isolation_config_b);
From: Michael Kelley <hidden> Date: 2021-08-12 19:18:37
From: Tianyu Lan <redacted> Sent: Monday, August 9, 2021 10:56 AM
Subject: [PATCH V3 02/13] x86/HV: Initialize shared memory boundary in the Isolation VM.
As with Patch 1, use the "x86/hyperv:" tag in the Subject line.
quoted hunk
From: Tianyu Lan <redacted>
Hyper-V exposes shared memory boundary via cpuid
HYPERV_CPUID_ISOLATION_CONFIG and store it in the
shared_gpa_boundary of ms_hyperv struct. This prepares
to share memory with host for SNP guest.
Signed-off-by: Tianyu Lan <redacted>
---
arch/x86/kernel/cpu/mshyperv.c | 2 ++
include/asm-generic/mshyperv.h | 12 +++++++++++-
2 files changed, 13 insertions(+), 1 deletion(-)
From: Tianyu Lan <hidden> Date: 2021-08-14 13:33:20
On 8/13/2021 3:18 AM, Michael Kelley wrote:
From: Tianyu Lan <redacted> Sent: Monday, August 9, 2021 10:56 AM
quoted
Subject: [PATCH V3 02/13] x86/HV: Initialize shared memory boundary in the Isolation VM.
As with Patch 1, use the "x86/hyperv:" tag in the Subject line.
quoted
From: Tianyu Lan <redacted>
Hyper-V exposes shared memory boundary via cpuid
HYPERV_CPUID_ISOLATION_CONFIG and store it in the
shared_gpa_boundary of ms_hyperv struct. This prepares
to share memory with host for SNP guest.
Signed-off-by: Tianyu Lan <redacted>
---
arch/x86/kernel/cpu/mshyperv.c | 2 ++
include/asm-generic/mshyperv.h | 12 +++++++++++-
2 files changed, 13 insertions(+), 1 deletion(-)
@@ -98,7 +98,13 @@ int hv_post_message(union hv_connection_id connection_id,aligned_msg->payload_size=payload_size;memcpy((void*)aligned_msg->payload,payload,payload_size);-status=hv_do_hypercall(HVCALL_POST_MESSAGE,aligned_msg,NULL);+if(hv_isolation_type_snp())+status=hv_ghcb_hypercall(HVCALL_POST_MESSAGE,+(void*)aligned_msg,NULL,+sizeof(structhv_input_post_message));+else+status=hv_do_hypercall(HVCALL_POST_MESSAGE,+aligned_msg,NULL);/* Preemption must remain disabled until after the hypercall*sosomeotherthreadcan'tgetscheduledontothiscpuand
This function is called from architecture independent code, so it needs a
default no-op stub to enable the code to compile on ARM64. The stub should
always return failure.
quoted hunk
+
void hv_ghcb_msr_write(u64 msr, u64 value)
{
union hv_ghcb *hv_ghcb;
@@ -98,7 +98,13 @@ int hv_post_message(union hv_connection_id connection_id,aligned_msg->payload_size=payload_size;memcpy((void*)aligned_msg->payload,payload,payload_size);-status=hv_do_hypercall(HVCALL_POST_MESSAGE,aligned_msg,NULL);+if(hv_isolation_type_snp())+status=hv_ghcb_hypercall(HVCALL_POST_MESSAGE,+(void*)aligned_msg,NULL,+sizeof(structhv_input_post_message));+else+status=hv_do_hypercall(HVCALL_POST_MESSAGE,+aligned_msg,NULL);/* Preemption must remain disabled until after the hypercall*sosomeotherthreadcan'tgetscheduledontothiscpuand
Alignment should be to HV_HYP_PAGE_SIZE. And it would be a good safety
play to have a BUILD_BUG_ON() somewhere asserting that
sizeof(union hv_ghcb) == HV_HYP_PAGE_SIZE.
From: Tianyu Lan <hidden> Date: 2021-08-09 17:56:53
From: Tianyu Lan <redacted>
Hyper-V provides GHCB protocol to write Synthetic Interrupt
Controller MSR registers in Isolation VM with AMD SEV SNP
and these registers are emulated by hypervisor directly.
Hyper-V requires to write SINTx MSR registers twice. First
writes MSR via GHCB page to communicate with hypervisor
and then writes wrmsr instruction to talk with paravisor
which runs in VMPL0. Guest OS ID MSR also needs to be set
via GHCB.
Signed-off-by: Tianyu Lan <redacted>
---
Change since v1:
* Introduce sev_es_ghcb_hv_call_simple() and share code
between SEV and Hyper-V code.
---
arch/x86/hyperv/hv_init.c | 33 ++-------
arch/x86/hyperv/ivm.c | 110 +++++++++++++++++++++++++++++
arch/x86/include/asm/mshyperv.h | 78 +++++++++++++++++++-
arch/x86/include/asm/sev.h | 3 +
arch/x86/kernel/cpu/mshyperv.c | 3 +
arch/x86/kernel/sev-shared.c | 63 ++++++++++-------
drivers/hv/hv.c | 121 ++++++++++++++++++++++----------
include/asm-generic/mshyperv.h | 12 +++-
8 files changed, 329 insertions(+), 94 deletions(-)
@@ -432,6 +432,9 @@ void __init hyperv_init(void)ms_hyperv.ghcb_base=NULL;gotoclean_guest_os_id;}++/* Hyper-V requires to write guest os id via ghcb in SNP IVM. */+hv_ghcb_msr_write(HV_X64_MSR_GUEST_OS_ID,guest_id);}rdmsrl(HV_X64_MSR_HYPERCALL,hypercall_msr.as_uint64);
@@ -523,6 +526,7 @@ void hyperv_cleanup(void)/* Reset our OS id */wrmsrl(HV_X64_MSR_GUEST_OS_ID,0);+hv_ghcb_msr_write(HV_X64_MSR_GUEST_OS_ID,0);/**Resethypercallpagereferencebeforeresetthepage,
@@ -13,6 +15,114 @@#include<asm/io.h>#include<asm/mshyperv.h>+voidhv_ghcb_msr_write(u64msr,u64value)+{+unionhv_ghcb*hv_ghcb;+void**ghcb_base;+unsignedlongflags;++if(!ms_hyperv.ghcb_base)+return;++WARN_ON(in_nmi());++local_irq_save(flags);+ghcb_base=(void**)this_cpu_ptr(ms_hyperv.ghcb_base);+hv_ghcb=(unionhv_ghcb*)*ghcb_base;+if(!hv_ghcb){+local_irq_restore(flags);+return;+}++ghcb_set_rcx(&hv_ghcb->ghcb,msr);+ghcb_set_rax(&hv_ghcb->ghcb,lower_32_bits(value));+ghcb_set_rdx(&hv_ghcb->ghcb,value>>32);++if(sev_es_ghcb_hv_call_simple(&hv_ghcb->ghcb,SVM_EXIT_MSR,1,0))+pr_warn("Fail to write msr via ghcb %llx.\n",msr);++local_irq_restore(flags);+}++voidhv_ghcb_msr_read(u64msr,u64*value)+{+unionhv_ghcb*hv_ghcb;+void**ghcb_base;+unsignedlongflags;++if(!ms_hyperv.ghcb_base)+return;++WARN_ON(in_nmi());++local_irq_save(flags);+ghcb_base=(void**)this_cpu_ptr(ms_hyperv.ghcb_base);+hv_ghcb=(unionhv_ghcb*)*ghcb_base;+if(!hv_ghcb){+local_irq_restore(flags);+return;+}++ghcb_set_rcx(&hv_ghcb->ghcb,msr);+if(sev_es_ghcb_hv_call_simple(&hv_ghcb->ghcb,SVM_EXIT_MSR,0,0))+pr_warn("Fail to read msr via ghcb %llx.\n",msr);+else+*value=(u64)lower_32_bits(hv_ghcb->ghcb.save.rax)+|((u64)lower_32_bits(hv_ghcb->ghcb.save.rdx)<<32);+local_irq_restore(flags);+}++voidhv_sint_rdmsrl_ghcb(u64msr,u64*value)+{+hv_ghcb_msr_read(msr,value);+}+EXPORT_SYMBOL_GPL(hv_sint_rdmsrl_ghcb);++voidhv_sint_wrmsrl_ghcb(u64msr,u64value)+{+hv_ghcb_msr_write(msr,value);++/* Write proxy bit vua wrmsrl instruction. */+if(msr>=HV_X64_MSR_SINT0&&msr<=HV_X64_MSR_SINT15)+wrmsrl(msr,value|1<<20);+}+EXPORT_SYMBOL_GPL(hv_sint_wrmsrl_ghcb);++voidhv_signal_eom_ghcb(void)+{+hv_sint_wrmsrl_ghcb(HV_X64_MSR_EOM,0);+}+EXPORT_SYMBOL_GPL(hv_signal_eom_ghcb);++enumhv_isolation_typehv_get_isolation_type(void)+{+if(!(ms_hyperv.priv_high&HV_ISOLATION))+returnHV_ISOLATION_TYPE_NONE;+returnFIELD_GET(HV_ISOLATION_TYPE,ms_hyperv.isolation_config_b);+}+EXPORT_SYMBOL_GPL(hv_get_isolation_type);++/*+*hv_is_isolation_supported-ChecksystemrunsintheHyper-V+*isolationVM.+*/+boolhv_is_isolation_supported(void)+{+returnhv_get_isolation_type()!=HV_ISOLATION_TYPE_NONE;+}++DEFINE_STATIC_KEY_FALSE(isolation_type_snp);++/*+*hv_isolation_type_snp-ChecksystemrunsintheAMDSEV-SNPbased+*isolationVM.+*/+boolhv_isolation_type_snp(void)+{+returnstatic_branch_unlikely(&isolation_type_snp);+}+EXPORT_SYMBOL_GPL(hv_isolation_type_snp);+/**hv_mark_gpa_visibility-Setpagesvisibletohostviahvcall.*
@@ -318,6 +318,9 @@ static void __init ms_hyperv_init_platform(void)pr_info("Hyper-V: Isolation Config: Group A 0x%x, Group B 0x%x\n",ms_hyperv.isolation_config_a,ms_hyperv.isolation_config_b);++if(hv_get_isolation_type()==HV_ISOLATION_TYPE_SNP)+static_branch_enable(&isolation_type_snp);}if(hv_max_functions_eax>=HYPERV_CPUID_NESTED_FEATURES){
@@ -109,29 +108,45 @@ static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,ghcb_set_sw_exit_info_1(ghcb,exit_info_1);ghcb_set_sw_exit_info_2(ghcb,exit_info_2);-sev_es_wr_ghcb_msr(__pa(ghcb));VMGEXIT();-if((ghcb->save.sw_exit_info_1&0xffffffff)==1){-u64info=ghcb->save.sw_exit_info_2;-unsignedlongv;--info=ghcb->save.sw_exit_info_2;-v=info&SVM_EVTINJ_VEC_MASK;--/* Check if exception information from hypervisor is sane. */-if((info&SVM_EVTINJ_VALID)&&-((v==X86_TRAP_GP)||(v==X86_TRAP_UD))&&-((info&SVM_EVTINJ_TYPE_MASK)==SVM_EVTINJ_TYPE_EXEPT)){-ctxt->fi.vector=v;-if(info&SVM_EVTINJ_VALID_ERR)-ctxt->fi.error_code=info>>32;-ret=ES_EXCEPTION;-}else{-ret=ES_VMM_ERROR;-}-}else{+if((ghcb->save.sw_exit_info_1&0xffffffff)==1)+ret=ES_VMM_ERROR;+elseret=ES_OK;++returnret;+}++staticenumes_resultsev_es_ghcb_hv_call(structghcb*ghcb,+structes_em_ctxt*ctxt,+u64exit_code,u64exit_info_1,+u64exit_info_2)+{+unsignedlongv;+enumes_resultret;+u64info;++sev_es_wr_ghcb_msr(__pa(ghcb));++ret=sev_es_ghcb_hv_call_simple(ghcb,exit_code,exit_info_1,+exit_info_2);+if(ret==ES_OK)+returnret;++info=ghcb->save.sw_exit_info_2;+v=info&SVM_EVTINJ_VEC_MASK;++/* Check if exception information from hypervisor is sane. */+if((info&SVM_EVTINJ_VALID)&&+((v==X86_TRAP_GP)||(v==X86_TRAP_UD))&&+((info&SVM_EVTINJ_TYPE_MASK)==SVM_EVTINJ_TYPE_EXEPT)){+ctxt->fi.vector=v;+if(info&SVM_EVTINJ_VALID_ERR)+ctxt->fi.error_code=info>>32;+ret=ES_EXCEPTION;+}else{+ret=ES_VMM_ERROR;}returnret;
@@ -199,26 +214,43 @@ void hv_synic_enable_regs(unsigned int cpu)unionhv_synic_scontrolsctrl;/* Setup the Synic's message page */-simp.as_uint64=hv_get_register(HV_REGISTER_SIMP);+hv_get_simp(simp.as_uint64);simp.simp_enabled=1;-simp.base_simp_gpa=virt_to_phys(hv_cpu->synic_message_page)->>HV_HYP_PAGE_SHIFT;-hv_set_register(HV_REGISTER_SIMP,simp.as_uint64);+if(hv_isolation_type_snp()){+hv_cpu->synic_message_page+=memremap(simp.base_simp_gpa<<HV_HYP_PAGE_SHIFT,+HV_HYP_PAGE_SIZE,MEMREMAP_WB);+if(!hv_cpu->synic_message_page)+pr_err("Fail to map syinc message page.\n");+}else{+simp.base_simp_gpa=virt_to_phys(hv_cpu->synic_message_page)+>>HV_HYP_PAGE_SHIFT;+}++hv_set_simp(simp.as_uint64);/* Setup the Synic's event page */-siefp.as_uint64=hv_get_register(HV_REGISTER_SIEFP);+hv_get_siefp(siefp.as_uint64);siefp.siefp_enabled=1;-siefp.base_siefp_gpa=virt_to_phys(hv_cpu->synic_event_page)->>HV_HYP_PAGE_SHIFT;-hv_set_register(HV_REGISTER_SIEFP,siefp.as_uint64);+if(hv_isolation_type_snp()){+hv_cpu->synic_event_page=+memremap(siefp.base_siefp_gpa<<HV_HYP_PAGE_SHIFT,+HV_HYP_PAGE_SIZE,MEMREMAP_WB);++if(!hv_cpu->synic_event_page)+pr_err("Fail to map syinc event page.\n");+}else{+siefp.base_siefp_gpa=virt_to_phys(hv_cpu->synic_event_page)+>>HV_HYP_PAGE_SHIFT;+}+hv_set_siefp(siefp.as_uint64);/* Setup the shared SINT. */if(vmbus_irq!=-1)enable_percpu_irq(vmbus_irq,0);-shared_sint.as_uint64=hv_get_register(HV_REGISTER_SINT0+-VMBUS_MESSAGE_SINT);+hv_get_synint_state(VMBUS_MESSAGE_SINT,shared_sint.as_uint64);shared_sint.vector=vmbus_interrupt;shared_sint.masked=false;
@@ -233,14 +265,12 @@ void hv_synic_enable_regs(unsigned int cpu)#elseshared_sint.auto_eoi=0;#endif-hv_set_register(HV_REGISTER_SINT0+VMBUS_MESSAGE_SINT,-shared_sint.as_uint64);+hv_set_synint_state(VMBUS_MESSAGE_SINT,shared_sint.as_uint64);/* Enable the global synic bit */-sctrl.as_uint64=hv_get_register(HV_REGISTER_SCONTROL);+hv_get_synic_state(sctrl.as_uint64);sctrl.enable=1;--hv_set_register(HV_REGISTER_SCONTROL,sctrl.as_uint64);+hv_set_synic_state(sctrl.as_uint64);}inthv_synic_init(unsignedintcpu)
@@ -257,37 +287,50 @@ int hv_synic_init(unsigned int cpu)*/voidhv_synic_disable_regs(unsignedintcpu){+structhv_per_cpu_context*hv_cpu+=per_cpu_ptr(hv_context.cpu_context,cpu);unionhv_synic_sintshared_sint;unionhv_synic_simpsimp;unionhv_synic_siefpsiefp;unionhv_synic_scontrolsctrl;-shared_sint.as_uint64=hv_get_register(HV_REGISTER_SINT0+-VMBUS_MESSAGE_SINT);-+hv_get_synint_state(VMBUS_MESSAGE_SINT,shared_sint.as_uint64);shared_sint.masked=1;+hv_set_synint_state(VMBUS_MESSAGE_SINT,shared_sint.as_uint64);+/* Need to correctly cleanup in the case of SMP!!! *//* Disable the interrupt */-hv_set_register(HV_REGISTER_SINT0+VMBUS_MESSAGE_SINT,-shared_sint.as_uint64);+hv_get_simp(simp.as_uint64);-simp.as_uint64=hv_get_register(HV_REGISTER_SIMP);+/*+*InIsolationVM,simandsiefpagesareallocatedby+*paravisor.Thesepagesalsowillbeusedbykdump+*kernel.Sojustresetenablebithereandkeeppage+*addresses.+*/simp.simp_enabled=0;-simp.base_simp_gpa=0;+if(hv_isolation_type_snp())+memunmap(hv_cpu->synic_message_page);+else+simp.base_simp_gpa=0;-hv_set_register(HV_REGISTER_SIMP,simp.as_uint64);+hv_set_simp(simp.as_uint64);-siefp.as_uint64=hv_get_register(HV_REGISTER_SIEFP);+hv_get_siefp(siefp.as_uint64);siefp.siefp_enabled=0;-siefp.base_siefp_gpa=0;-hv_set_register(HV_REGISTER_SIEFP,siefp.as_uint64);+if(hv_isolation_type_snp())+memunmap(hv_cpu->synic_event_page);+else+siefp.base_siefp_gpa=0;++hv_set_siefp(siefp.as_uint64);/* Disable the global synic bit */-sctrl.as_uint64=hv_get_register(HV_REGISTER_SCONTROL);+hv_get_synic_state(sctrl.as_uint64);sctrl.enable=0;-hv_set_register(HV_REGISTER_SCONTROL,sctrl.as_uint64);+hv_set_synic_state(sctrl.as_uint64);if(vmbus_irq!=-1)disable_percpu_irq(vmbus_irq);
See previous comments about tag in the Subject line.
quoted hunk
Hyper-V provides GHCB protocol to write Synthetic Interrupt
Controller MSR registers in Isolation VM with AMD SEV SNP
and these registers are emulated by hypervisor directly.
Hyper-V requires to write SINTx MSR registers twice. First
writes MSR via GHCB page to communicate with hypervisor
and then writes wrmsr instruction to talk with paravisor
which runs in VMPL0. Guest OS ID MSR also needs to be set
via GHCB.
Signed-off-by: Tianyu Lan <redacted>
---
Change since v1:
* Introduce sev_es_ghcb_hv_call_simple() and share code
between SEV and Hyper-V code.
---
arch/x86/hyperv/hv_init.c | 33 ++-------
arch/x86/hyperv/ivm.c | 110 +++++++++++++++++++++++++++++
arch/x86/include/asm/mshyperv.h | 78 +++++++++++++++++++-
arch/x86/include/asm/sev.h | 3 +
arch/x86/kernel/cpu/mshyperv.c | 3 +
arch/x86/kernel/sev-shared.c | 63 ++++++++++-------
drivers/hv/hv.c | 121 ++++++++++++++++++++++----------
include/asm-generic/mshyperv.h | 12 +++-
8 files changed, 329 insertions(+), 94 deletions(-)
Is there a reason for this to be a function instead of a #define?
Seemingly parallel calls such as hv_set_synic_state_ghcb()
are #defines.
+
+enum hv_isolation_type hv_get_isolation_type(void)
+{
+ if (!(ms_hyperv.priv_high & HV_ISOLATION))
+ return HV_ISOLATION_TYPE_NONE;
+ return FIELD_GET(HV_ISOLATION_TYPE, ms_hyperv.isolation_config_b);
+}
+EXPORT_SYMBOL_GPL(hv_get_isolation_type);
+
+/*
+ * hv_is_isolation_supported - Check system runs in the Hyper-V
+ * isolation VM.
+ */
+bool hv_is_isolation_supported(void)
+{
+ return hv_get_isolation_type() != HV_ISOLATION_TYPE_NONE;
+}
+
+DEFINE_STATIC_KEY_FALSE(isolation_type_snp);
+
+/*
+ * hv_isolation_type_snp - Check system runs in the AMD SEV-SNP based
+ * isolation VM.
+ */
+bool hv_isolation_type_snp(void)
+{
+ return static_branch_unlikely(&isolation_type_snp);
+}
+EXPORT_SYMBOL_GPL(hv_isolation_type_snp);
+
hv_isolation_type_snp() is implemented here in a file under arch/x86,
but it is called from architecture independent code in drivers/hv, so it
needs to do the right thing on ARM64 as well as x86. For an example,
see the handling of hv_is_isolation_supported() in the latest linux-next
tree.
quoted hunk
/*
* hv_mark_gpa_visibility - Set pages visible to host via hvcall.
*
Many of these registers that exist on x86 and ARM64 architectures
have new names without the "X64_MSR" portion. For
example, include/asm-generic/hyperv-tlfs.h defines
HV_REGISTER_SCONTROL. The x86-specific version of
hyperv-tlfs.h currently has a #define for HV_X64_MSR_SCONTROL,
but we would like to get rid of these temporary aliases.
So prefer to use HV_REGISTER_SCONTROL.
Same comment applies several places in this code for other
similar registers.
@@ -318,6 +318,9 @@ static void __init ms_hyperv_init_platform(void)pr_info("Hyper-V: Isolation Config: Group A 0x%x, Group B 0x%x\n",ms_hyperv.isolation_config_a,ms_hyperv.isolation_config_b);++if(hv_get_isolation_type()==HV_ISOLATION_TYPE_SNP)+static_branch_enable(&isolation_type_snp);}if(hv_max_functions_eax>=HYPERV_CPUID_NESTED_FEATURES){
@@ -109,29 +108,45 @@ static enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,ghcb_set_sw_exit_info_1(ghcb,exit_info_1);ghcb_set_sw_exit_info_2(ghcb,exit_info_2);-sev_es_wr_ghcb_msr(__pa(ghcb));VMGEXIT();-if((ghcb->save.sw_exit_info_1&0xffffffff)==1){-u64info=ghcb->save.sw_exit_info_2;-unsignedlongv;--info=ghcb->save.sw_exit_info_2;-v=info&SVM_EVTINJ_VEC_MASK;--/* Check if exception information from hypervisor is sane. */-if((info&SVM_EVTINJ_VALID)&&-((v==X86_TRAP_GP)||(v==X86_TRAP_UD))&&-((info&SVM_EVTINJ_TYPE_MASK)==SVM_EVTINJ_TYPE_EXEPT)){-ctxt->fi.vector=v;-if(info&SVM_EVTINJ_VALID_ERR)-ctxt->fi.error_code=info>>32;-ret=ES_EXCEPTION;-}else{-ret=ES_VMM_ERROR;-}-}else{+if((ghcb->save.sw_exit_info_1&0xffffffff)==1)+ret=ES_VMM_ERROR;+elseret=ES_OK;++returnret;+}++staticenumes_resultsev_es_ghcb_hv_call(structghcb*ghcb,+structes_em_ctxt*ctxt,+u64exit_code,u64exit_info_1,+u64exit_info_2)+{+unsignedlongv;+enumes_resultret;+u64info;++sev_es_wr_ghcb_msr(__pa(ghcb));++ret=sev_es_ghcb_hv_call_simple(ghcb,exit_code,exit_info_1,+exit_info_2);+if(ret==ES_OK)+returnret;++info=ghcb->save.sw_exit_info_2;+v=info&SVM_EVTINJ_VEC_MASK;++/* Check if exception information from hypervisor is sane. */+if((info&SVM_EVTINJ_VALID)&&+((v==X86_TRAP_GP)||(v==X86_TRAP_UD))&&+((info&SVM_EVTINJ_TYPE_MASK)==SVM_EVTINJ_TYPE_EXEPT)){+ctxt->fi.vector=v;+if(info&SVM_EVTINJ_VALID_ERR)+ctxt->fi.error_code=info>>32;+ret=ES_EXCEPTION;+}else{+ret=ES_VMM_ERROR;}returnret;
You could skip making these changes to hv_synic_free(). If the message
and event pages aren't allocated, the pointers will be NULL and
free_page() will happily do nothing.
quoted hunk
}
kfree(hv_context.hv_numa_map);
@@ -199,26 +214,43 @@ void hv_synic_enable_regs(unsigned int cpu) union hv_synic_scontrol sctrl; /* Setup the Synic's message page */- simp.as_uint64 = hv_get_register(HV_REGISTER_SIMP);+ hv_get_simp(simp.as_uint64);
This code is intended to be architecture independent and builds for
x86 and for ARM64. Changing the use of hv_get_register() and hv_set_register()
will fail badly when built for ARM64. I haven't completely thought through
what the best solution might be, but the current set of mappings from
hv_get_simp() down to hv_ghcb_msr_read() isn't going to work on ARM64.
Is it possible to hide all the x86-side complexity in the implementation of
hv_get_register()? Certain MSRs would have to be special-cased when
SNP isolation is enabled, but that might be easier than trying to no-op out
the ghcb machinery on the ARM64 side.
@@ -233,14 +265,12 @@ void hv_synic_enable_regs(unsigned int cpu) #else shared_sint.auto_eoi = 0; #endif- hv_set_register(HV_REGISTER_SINT0 + VMBUS_MESSAGE_SINT,- shared_sint.as_uint64);+ hv_set_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64); /* Enable the global synic bit */- sctrl.as_uint64 = hv_get_register(HV_REGISTER_SCONTROL);+ hv_get_synic_state(sctrl.as_uint64); sctrl.enable = 1;-- hv_set_register(HV_REGISTER_SCONTROL, sctrl.as_uint64);+ hv_set_synic_state(sctrl.as_uint64); } int hv_synic_init(unsigned int cpu)
@@ -257,37 +287,50 @@ int hv_synic_init(unsigned int cpu) */ void hv_synic_disable_regs(unsigned int cpu) {+ struct hv_per_cpu_context *hv_cpu+ = per_cpu_ptr(hv_context.cpu_context, cpu); union hv_synic_sint shared_sint; union hv_synic_simp simp; union hv_synic_siefp siefp; union hv_synic_scontrol sctrl;- shared_sint.as_uint64 = hv_get_register(HV_REGISTER_SINT0 +- VMBUS_MESSAGE_SINT);-+ hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64); shared_sint.masked = 1;+ hv_set_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);+ /* Need to correctly cleanup in the case of SMP!!! */ /* Disable the interrupt */- hv_set_register(HV_REGISTER_SINT0 + VMBUS_MESSAGE_SINT,- shared_sint.as_uint64);+ hv_get_simp(simp.as_uint64);- simp.as_uint64 = hv_get_register(HV_REGISTER_SIMP);+ /*+ * In Isolation VM, sim and sief pages are allocated by+ * paravisor. These pages also will be used by kdump+ * kernel. So just reset enable bit here and keep page+ * addresses.+ */ simp.simp_enabled = 0;- simp.base_simp_gpa = 0;+ if (hv_isolation_type_snp())+ memunmap(hv_cpu->synic_message_page);+ else+ simp.base_simp_gpa = 0;- hv_set_register(HV_REGISTER_SIMP, simp.as_uint64);+ hv_set_simp(simp.as_uint64);- siefp.as_uint64 = hv_get_register(HV_REGISTER_SIEFP);+ hv_get_siefp(siefp.as_uint64); siefp.siefp_enabled = 0;- siefp.base_siefp_gpa = 0;- hv_set_register(HV_REGISTER_SIEFP, siefp.as_uint64);+ if (hv_isolation_type_snp())+ memunmap(hv_cpu->synic_event_page);+ else+ siefp.base_siefp_gpa = 0;++ hv_set_siefp(siefp.as_uint64); /* Disable the global synic bit */- sctrl.as_uint64 = hv_get_register(HV_REGISTER_SCONTROL);+ hv_get_synic_state(sctrl.as_uint64); sctrl.enable = 0;- hv_set_register(HV_REGISTER_SCONTROL, sctrl.as_uint64);+ hv_set_synic_state(sctrl.as_uint64); if (vmbus_irq != -1) disable_percpu_irq(vmbus_irq);
See previous comments about tag in the Subject line.
quoted
Hyper-V provides GHCB protocol to write Synthetic Interrupt
Controller MSR registers in Isolation VM with AMD SEV SNP
and these registers are emulated by hypervisor directly.
Hyper-V requires to write SINTx MSR registers twice. First
writes MSR via GHCB page to communicate with hypervisor
and then writes wrmsr instruction to talk with paravisor
which runs in VMPL0. Guest OS ID MSR also needs to be set
via GHCB.
Signed-off-by: Tianyu Lan <redacted>
---
Change since v1:
* Introduce sev_es_ghcb_hv_call_simple() and share code
between SEV and Hyper-V code.
---
arch/x86/hyperv/hv_init.c | 33 ++-------
arch/x86/hyperv/ivm.c | 110 +++++++++++++++++++++++++++++
arch/x86/include/asm/mshyperv.h | 78 +++++++++++++++++++-
arch/x86/include/asm/sev.h | 3 +
arch/x86/kernel/cpu/mshyperv.c | 3 +
arch/x86/kernel/sev-shared.c | 63 ++++++++++-------
drivers/hv/hv.c | 121 ++++++++++++++++++++++----------
include/asm-generic/mshyperv.h | 12 +++-
8 files changed, 329 insertions(+), 94 deletions(-)
From: Tianyu Lan <hidden> Date: 2021-08-09 17:56:58
From: Tianyu Lan <redacted>
The monitor pages in the CHANNELMSG_INITIATE_CONTACT msg are shared
with host in Isolation VM and so it's necessary to use hvcall to set
them visible to host. In Isolation VM with AMD SEV SNP, the access
address should be in the extra space which is above shared gpa
boundary. So remap these pages into the extra address(pa +
shared_gpa_boundary). Introduce monitor_pages_va to store
the remap address and unmap these va when disconnect vmbus.
Signed-off-by: Tianyu Lan <redacted>
---
Change since v1:
* Not remap monitor pages in the non-SNP isolation VM.
---
drivers/hv/connection.c | 65 +++++++++++++++++++++++++++++++++++++++
drivers/hv/hyperv_vmbus.h | 1 +
2 files changed, 66 insertions(+)
From: Michael Kelley <hidden> Date: 2021-08-13 21:28:56
From: Tianyu Lan <redacted> Sent: Monday, August 9, 2021 10:56 AM
quoted hunk
The monitor pages in the CHANNELMSG_INITIATE_CONTACT msg are shared
with host in Isolation VM and so it's necessary to use hvcall to set
them visible to host. In Isolation VM with AMD SEV SNP, the access
address should be in the extra space which is above shared gpa
boundary. So remap these pages into the extra address(pa +
shared_gpa_boundary). Introduce monitor_pages_va to store
the remap address and unmap these va when disconnect vmbus.
Signed-off-by: Tianyu Lan <redacted>
---
Change since v1:
* Not remap monitor pages in the non-SNP isolation VM.
---
drivers/hv/connection.c | 65 +++++++++++++++++++++++++++++++++++++++
drivers/hv/hyperv_vmbus.h | 1 +
2 files changed, 66 insertions(+)
@@ -104,6 +105,12 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)msg->monitor_page1=virt_to_phys(vmbus_connection.monitor_pages[0]);msg->monitor_page2=virt_to_phys(vmbus_connection.monitor_pages[1]);++if(hv_isolation_type_snp()){+msg->monitor_page1+=ms_hyperv.shared_gpa_boundary;+msg->monitor_page2+=ms_hyperv.shared_gpa_boundary;+}+msg->target_vcpu=hv_cpu_number_to_vp_number(VMBUS_CONNECT_CPU);/*
@@ -148,6 +155,31 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)return-ECONNREFUSED;}+if(hv_isolation_type_snp()){+vmbus_connection.monitor_pages_va[0]+=vmbus_connection.monitor_pages[0];+vmbus_connection.monitor_pages[0]+=memremap(msg->monitor_page1,HV_HYP_PAGE_SIZE,+MEMREMAP_WB);+if(!vmbus_connection.monitor_pages[0])+return-ENOMEM;
This error case causes vmbus_negotiate_version() to return with
vmbus_connection.con_state set to CONNECTED. But the caller never checks the
returned error code except for ETIMEDOUT. So the caller will think that
vmbus_negotiate_version() succeeded when it didn't. There may be some
existing bugs in that error handling code. :-(
The code in this patch feels a bit more complicated than it needs to be. Altogether,
there are two different virtual addresses and one physical address for each monitor
page. The two virtual addresses are the one obtained from the original memory
allocation, and which will be used to free the memory. The second virtual address
is the one used to actually access the data, which is the same as the first virtual
address for a non-isolated VM. The second VA is the result of memremap() call for an
isolated VM. The vmbus_connection data structure should save all three values for
each monitor page so they don't need to recomputed or moved around. Then:
1) For isolated and for non-isolated VMs, setup the virtual and physical addresses
of the monitor pages in vmbus_connect(), and store them in the vmbus_connection
data structure. The physical address should include the shared_gpa_boundary offset
in the case of an isolated VM. At this point the two virtual addresses are the same.
2) vmbus_negotiate_version() just grabs the physical address from the
vmbus_connection data structure. It doesn't make any changes to the virtual
or physical addresses, which keeps it focused just on version negotiation.
3) Once vmbus_negotiate_version() is done, vmbus_connect() can determine
the remapped virtual address, and store that. It can also change the visibility
of the two pages using the previously stored physical address.
4) vmbus_disconnect() can do the memunmaps() and change the visibility if needed,
and then free the memory using the address from the original allocation in Step 1.
From: Tianyu Lan <hidden> Date: 2021-08-09 17:57:07
From: Tianyu Lan <redacted>
VMbus ring buffer are shared with host and it's need to
be accessed via extra address space of Isolation VM with
SNP support. This patch is to map the ring buffer
address in extra address space via ioremap(). HV host
visibility hvcall smears data in the ring buffer and
so reset the ring buffer memory to zero after calling
visibility hvcall.
Signed-off-by: Tianyu Lan <redacted>
---
drivers/hv/Kconfig | 1 +
drivers/hv/channel.c | 10 +++++
drivers/hv/hyperv_vmbus.h | 2 +
drivers/hv/ring_buffer.c | 84 ++++++++++++++++++++++++++++++---------
4 files changed, 79 insertions(+), 18 deletions(-)
@@ -712,6 +712,16 @@ static int __vmbus_open(struct vmbus_channel *newchannel,if(err)gotoerror_clean_ring;+err=hv_ringbuffer_post_init(&newchannel->outbound,+page,send_pages);+if(err)+gotoerror_free_gpadl;++err=hv_ringbuffer_post_init(&newchannel->inbound,+&page[send_pages],recv_pages);+if(err)+gotoerror_free_gpadl;+/* Create and init the channel open message */open_info=kzalloc(sizeof(*open_info)+sizeof(structvmbus_channel_open_channel),
@@ -179,43 +181,89 @@ void hv_ringbuffer_pre_init(struct vmbus_channel *channel)mutex_init(&channel->outbound.ring_buffer_mutex);}-/* Initialize the ring buffer. */-inthv_ringbuffer_init(structhv_ring_buffer_info*ring_info,-structpage*pages,u32page_cnt,u32max_pkt_size)+inthv_ringbuffer_post_init(structhv_ring_buffer_info*ring_info,+structpage*pages,u32page_cnt){+u64physic_addr=page_to_pfn(pages)<<PAGE_SHIFT;+unsignedlong*pfns_wraparound;+void*vaddr;inti;-structpage**pages_wraparound;-BUILD_BUG_ON((sizeof(structhv_ring_buffer)!=PAGE_SIZE));+if(!hv_isolation_type_snp())+return0;++physic_addr+=ms_hyperv.shared_gpa_boundary;/**Firstpageholdsstructhv_ring_buffer,dowraparoundmappingfor*therest.*/-pages_wraparound=kcalloc(page_cnt*2-1,sizeof(structpage*),+pfns_wraparound=kcalloc(page_cnt*2-1,sizeof(unsignedlong),GFP_KERNEL);-if(!pages_wraparound)+if(!pfns_wraparound)return-ENOMEM;-pages_wraparound[0]=pages;+pfns_wraparound[0]=physic_addr>>PAGE_SHIFT;for(i=0;i<2*(page_cnt-1);i++)-pages_wraparound[i+1]=&pages[i%(page_cnt-1)+1];--ring_info->ring_buffer=(structhv_ring_buffer*)-vmap(pages_wraparound,page_cnt*2-1,VM_MAP,PAGE_KERNEL);--kfree(pages_wraparound);+pfns_wraparound[i+1]=(physic_addr>>PAGE_SHIFT)++i%(page_cnt-1)+1;--if(!ring_info->ring_buffer)+vaddr=vmap_pfn(pfns_wraparound,page_cnt*2-1,PAGE_KERNEL_IO);+kfree(pfns_wraparound);+if(!vaddr)return-ENOMEM;-ring_info->ring_buffer->read_index=-ring_info->ring_buffer->write_index=0;+/* Clean memory after setting host visibility. */+memset((void*)vaddr,0x00,page_cnt*PAGE_SIZE);++ring_info->ring_buffer=(structhv_ring_buffer*)vaddr;+ring_info->ring_buffer->read_index=0;+ring_info->ring_buffer->write_index=0;/* Set the feature bit for enabling flow control. */ring_info->ring_buffer->feature_bits.value=1;+return0;+}++/* Initialize the ring buffer. */+inthv_ringbuffer_init(structhv_ring_buffer_info*ring_info,+structpage*pages,u32page_cnt,u32max_pkt_size)+{+inti;+structpage**pages_wraparound;++BUILD_BUG_ON((sizeof(structhv_ring_buffer)!=PAGE_SIZE));++if(!hv_isolation_type_snp()){+/*+*Firstpageholdsstructhv_ring_buffer,dowraparoundmappingfor+*therest.+*/+pages_wraparound=kcalloc(page_cnt*2-1,sizeof(structpage*),+GFP_KERNEL);+if(!pages_wraparound)+return-ENOMEM;++pages_wraparound[0]=pages;+for(i=0;i<2*(page_cnt-1);i++)+pages_wraparound[i+1]=&pages[i%(page_cnt-1)+1];++ring_info->ring_buffer=(structhv_ring_buffer*)+vmap(pages_wraparound,page_cnt*2-1,VM_MAP,PAGE_KERNEL);++kfree(pages_wraparound);++if(!ring_info->ring_buffer)+return-ENOMEM;++ring_info->ring_buffer->read_index=+ring_info->ring_buffer->write_index=0;++/* Set the feature bit for enabling flow control. */+ring_info->ring_buffer->feature_bits.value=1;+}+ring_info->ring_size=page_cnt<<PAGE_SHIFT;ring_info->ring_size_div10_reciprocal=reciprocal_value(ring_info->ring_size/10);
From: Michael Kelley <hidden> Date: 2021-08-16 17:28:47
From: Tianyu Lan <redacted> Sent: Monday, August 9, 2021 10:56 AM
VMbus ring buffer are shared with host and it's need to
s/it's need/it needs/
be accessed via extra address space of Isolation VM with
SNP support. This patch is to map the ring buffer
address in extra address space via ioremap(). HV host
It's actually using vmap_pfn(), not ioremap().
quoted hunk
visibility hvcall smears data in the ring buffer and
so reset the ring buffer memory to zero after calling
visibility hvcall.
Signed-off-by: Tianyu Lan <redacted>
---
drivers/hv/Kconfig | 1 +
drivers/hv/channel.c | 10 +++++
drivers/hv/hyperv_vmbus.h | 2 +
drivers/hv/ring_buffer.c | 84 ++++++++++++++++++++++++++++++---------
4 files changed, 79 insertions(+), 18 deletions(-)
@@ -712,6 +712,16 @@ static int __vmbus_open(struct vmbus_channel *newchannel,if(err)gotoerror_clean_ring;+err=hv_ringbuffer_post_init(&newchannel->outbound,+page,send_pages);+if(err)+gotoerror_free_gpadl;++err=hv_ringbuffer_post_init(&newchannel->inbound,+&page[send_pages],recv_pages);+if(err)+gotoerror_free_gpadl;+/* Create and init the channel open message */open_info=kzalloc(sizeof(*open_info)+sizeof(structvmbus_channel_open_channel),
@@ -179,43 +181,89 @@ void hv_ringbuffer_pre_init(struct vmbus_channel *channel)mutex_init(&channel->outbound.ring_buffer_mutex);}-/* Initialize the ring buffer. */-inthv_ringbuffer_init(structhv_ring_buffer_info*ring_info,-structpage*pages,u32page_cnt,u32max_pkt_size)+inthv_ringbuffer_post_init(structhv_ring_buffer_info*ring_info,+structpage*pages,u32page_cnt){+u64physic_addr=page_to_pfn(pages)<<PAGE_SHIFT;+unsignedlong*pfns_wraparound;+void*vaddr;inti;-structpage**pages_wraparound;-BUILD_BUG_ON((sizeof(structhv_ring_buffer)!=PAGE_SIZE));+if(!hv_isolation_type_snp())+return0;++physic_addr+=ms_hyperv.shared_gpa_boundary;/**Firstpageholdsstructhv_ring_buffer,dowraparoundmappingfor*therest.*/-pages_wraparound=kcalloc(page_cnt*2-1,sizeof(structpage*),+pfns_wraparound=kcalloc(page_cnt*2-1,sizeof(unsignedlong),GFP_KERNEL);-if(!pages_wraparound)+if(!pfns_wraparound)return-ENOMEM;-pages_wraparound[0]=pages;+pfns_wraparound[0]=physic_addr>>PAGE_SHIFT;for(i=0;i<2*(page_cnt-1);i++)-pages_wraparound[i+1]=&pages[i%(page_cnt-1)+1];--ring_info->ring_buffer=(structhv_ring_buffer*)-vmap(pages_wraparound,page_cnt*2-1,VM_MAP,PAGE_KERNEL);--kfree(pages_wraparound);+pfns_wraparound[i+1]=(physic_addr>>PAGE_SHIFT)++i%(page_cnt-1)+1;--if(!ring_info->ring_buffer)+vaddr=vmap_pfn(pfns_wraparound,page_cnt*2-1,PAGE_KERNEL_IO);+kfree(pfns_wraparound);+if(!vaddr)return-ENOMEM;-ring_info->ring_buffer->read_index=-ring_info->ring_buffer->write_index=0;+/* Clean memory after setting host visibility. */+memset((void*)vaddr,0x00,page_cnt*PAGE_SIZE);++ring_info->ring_buffer=(structhv_ring_buffer*)vaddr;+ring_info->ring_buffer->read_index=0;+ring_info->ring_buffer->write_index=0;/* Set the feature bit for enabling flow control. */ring_info->ring_buffer->feature_bits.value=1;+return0;+}++/* Initialize the ring buffer. */+inthv_ringbuffer_init(structhv_ring_buffer_info*ring_info,+structpage*pages,u32page_cnt,u32max_pkt_size)+{+inti;+structpage**pages_wraparound;++BUILD_BUG_ON((sizeof(structhv_ring_buffer)!=PAGE_SIZE));++if(!hv_isolation_type_snp()){+/*+*Firstpageholdsstructhv_ring_buffer,dowraparoundmappingfor+*therest.+*/+pages_wraparound=kcalloc(page_cnt*2-1,sizeof(structpage*),+GFP_KERNEL);+if(!pages_wraparound)+return-ENOMEM;++pages_wraparound[0]=pages;+for(i=0;i<2*(page_cnt-1);i++)+pages_wraparound[i+1]=&pages[i%(page_cnt-1)+1];++ring_info->ring_buffer=(structhv_ring_buffer*)+vmap(pages_wraparound,page_cnt*2-1,VM_MAP,PAGE_KERNEL);++kfree(pages_wraparound);++if(!ring_info->ring_buffer)+return-ENOMEM;++ring_info->ring_buffer->read_index=+ring_info->ring_buffer->write_index=0;++/* Set the feature bit for enabling flow control. */+ring_info->ring_buffer->feature_bits.value=1;+}+ring_info->ring_size=page_cnt<<PAGE_SHIFT;ring_info->ring_size_div10_reciprocal=reciprocal_value(ring_info->ring_size/10);--
2.25.1
This patch does the following:
1) The existing ring buffer wrap-around mapping functionality is still
executed in hv_ringbuffer_init() when not doing SNP isolation.
This mapping is based on an array of struct page's that describe the
contiguous physical memory.
2) New ring buffer wrap-around mapping functionality is added in
hv_ringbuffer_post_init() for the SNP isolation case. The case is
handled in hv_ringbuffer_post_init() because it must be done after
the GPADL is established, since that's where the host visibility
is set. What's interesting is that this case is exactly the same
as #1 above, except that the mapping is based on physical
memory addresses instead of struct page's. We have to use physical
addresses because of applying the GPA boundary, and there are no
struct page's for those physical addresses.
Unfortunately, this duplicates a lot of logic in #1 and #2, except
for the struct page vs. physical address difference.
Proposal: Couldn't we always do #2, even for the normal case
where SNP isolation is not being used? The difference would
only be in whether the GPA boundary is added. And it looks like
the normal case could be done after the GPADL is established,
as setting up the GPADL doesn't have any dependencies on
having the ring buffer mapped. This approach would remove
a lot of duplication. Just move the calls to hv_ringbuffer_init()
to after the GPADL is established, and do all the work there for
both cases.
Michael
From: Tianyu Lan <hidden> Date: 2021-08-17 15:36:42
On 8/17/2021 1:28 AM, Michael Kelley wrote:
This patch does the following:
1) The existing ring buffer wrap-around mapping functionality is still
executed in hv_ringbuffer_init() when not doing SNP isolation.
This mapping is based on an array of struct page's that describe the
contiguous physical memory.
2) New ring buffer wrap-around mapping functionality is added in
hv_ringbuffer_post_init() for the SNP isolation case. The case is
handled in hv_ringbuffer_post_init() because it must be done after
the GPADL is established, since that's where the host visibility
is set. What's interesting is that this case is exactly the same
as #1 above, except that the mapping is based on physical
memory addresses instead of struct page's. We have to use physical
addresses because of applying the GPA boundary, and there are no
struct page's for those physical addresses.
Unfortunately, this duplicates a lot of logic in #1 and #2, except
for the struct page vs. physical address difference.
Proposal: Couldn't we always do #2, even for the normal case
where SNP isolation is not being used? The difference would
only be in whether the GPA boundary is added. And it looks like
the normal case could be done after the GPADL is established,
as setting up the GPADL doesn't have any dependencies on
having the ring buffer mapped. This approach would remove
a lot of duplication. Just move the calls to hv_ringbuffer_init()
to after the GPADL is established, and do all the work there for
both cases.
Hi Michael:
Thanks for suggestion. I just keep the original logic in current
code. I will try combining these two functions and report back.
Thanks.
@@ -465,7 +466,14 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,structlist_head*curr;u32next_gpadl_handle;unsignedlongflags;-intret=0;+intret=0,index;++index=atomic_inc_return(&channel->gpadl_index)-1;++if(index>VMBUS_GPADL_RANGE_COUNT-1){+pr_err("Gpadl handle position(%d) has been occupied.\n",index);+return-ENOSPC;+}next_gpadl_handle=(atomic_inc_return(&vmbus_connection.next_gpadl_handle)-1);
@@ -474,6 +482,13 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,if(ret)returnret;+ret=set_memory_decrypted((unsignedlong)kbuffer,+HVPFN_UP(size));+if(ret){+pr_warn("Failed to set host visibility.\n");+returnret;+}+init_completion(&msginfo->waitevent);msginfo->waiting_channel=channel;
@@ -539,6 +554,10 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,/* At this point, we received the gpadl created msg */*gpadl_handle=gpadlmsg->gpadl;+channel->gpadl_array[index].size=size;+channel->gpadl_array[index].buffer=kbuffer;+channel->gpadl_array[index].gpadlhandle=*gpadl_handle;+cleanup:spin_lock_irqsave(&vmbus_connection.channelmsg_lock,flags);list_del(&msginfo->msglistentry);
@@ -549,6 +568,13 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,}kfree(msginfo);++if(ret){+set_memory_encrypted((unsignedlong)kbuffer,+HVPFN_UP(size));+atomic_dec(&channel->gpadl_index);+}+returnret;}
@@ -676,6 +702,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,/* Establish the gpadl for the ring buffer */newchannel->ringbuffer_gpadlhandle=0;+atomic_set(&newchannel->gpadl_index,0);err=__vmbus_establish_gpadl(newchannel,HV_GPADL_RING,page_address(newchannel->ringbuffer_page),
@@ -811,7 +838,7 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)structvmbus_channel_gpadl_teardown*msg;structvmbus_channel_msginfo*info;unsignedlongflags;-intret;+intret,i;info=kzalloc(sizeof(*info)+sizeof(structvmbus_channel_gpadl_teardown),GFP_KERNEL);
@@ -859,6 +886,19 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock,flags);kfree(info);++/* Find gpadl buffer virtual address and size. */+for(i=0;i<VMBUS_GPADL_RANGE_COUNT;i++)+if(channel->gpadl_array[i].gpadlhandle==gpadl_handle)+break;++if(set_memory_encrypted((unsignedlong)channel->gpadl_array[i].buffer,+HVPFN_UP(channel->gpadl_array[i].size)))+pr_warn("Fail to set mem host visibility.\n");++channel->gpadl_array[i].gpadlhandle=0;+atomic_dec(&channel->gpadl_index);+returnret;}EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl);
@@ -465,7 +466,14 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,structlist_head*curr;u32next_gpadl_handle;unsignedlongflags;-intret=0;+intret=0,index;++index=atomic_inc_return(&channel->gpadl_index)-1;++if(index>VMBUS_GPADL_RANGE_COUNT-1){+pr_err("Gpadl handle position(%d) has been occupied.\n",index);+return-ENOSPC;+}next_gpadl_handle=(atomic_inc_return(&vmbus_connection.next_gpadl_handle)-1);
@@ -474,6 +482,13 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,if(ret)returnret;+ret=set_memory_decrypted((unsignedlong)kbuffer,+HVPFN_UP(size));+if(ret){+pr_warn("Failed to set host visibility.\n");
Enhance this message a bit. "Failed to set host visibility for new GPADL\n"
and also output the value of ret.
@@ -539,6 +554,10 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel, /* At this point, we received the gpadl created msg */ *gpadl_handle = gpadlmsg->gpadl;+ channel->gpadl_array[index].size = size;+ channel->gpadl_array[index].buffer = kbuffer;+ channel->gpadl_array[index].gpadlhandle = *gpadl_handle;+
I can see the merits of transparently stashing the memory address and size
that will be needed by vmbus_teardown_gpadl(), so that the callers of
__vmbus_establish_gpadl() don't have to worry about it. But doing the
stashing transparently is somewhat messy.
Given that the callers are already have memory allocated to save the
GPADL handle, a little refactoring would make for a much cleaner solution.
Instead of having memory allocated for the 32-bit GPADL handle, callers
should allocate the slightly larger struct vmbus_gpadl that you've
defined below. The calling interfaces can be updated to take a pointer
to this structure instead of a pointer to the 32-bit GPADL handle, and
you can save the memory address and size right along with the GPADL
handle. This approach touches a few more files, but I think there are
only two callers outside of the channel management code -- netvsc
and hv_uio -- so it's not a big change.
@@ -676,6 +702,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel, /* Establish the gpadl for the ring buffer */ newchannel->ringbuffer_gpadlhandle = 0;+ atomic_set(&newchannel->gpadl_index, 0); err = __vmbus_establish_gpadl(newchannel, HV_GPADL_RING, page_address(newchannel->ringbuffer_page),
@@ -811,7 +838,7 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle) struct vmbus_channel_gpadl_teardown *msg; struct vmbus_channel_msginfo *info; unsigned long flags;- int ret;+ int ret, i; info = kzalloc(sizeof(*info) + sizeof(struct vmbus_channel_gpadl_teardown), GFP_KERNEL);
@@ -859,6 +886,19 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle) spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags); kfree(info);++ /* Find gpadl buffer virtual address and size. */+ for (i = 0; i < VMBUS_GPADL_RANGE_COUNT; i++)+ if (channel->gpadl_array[i].gpadlhandle == gpadl_handle)+ break;++ if (set_memory_encrypted((unsigned long)channel->gpadl_array[i].buffer,+ HVPFN_UP(channel->gpadl_array[i].size)))+ pr_warn("Fail to set mem host visibility.\n");
Enhance this message a bit: "Failed to set host visibility in GPADL teardown\n".
Also output the returned error code to help in debugging any occurrences of
a failure.
Note that the array entry being cleared (index "i") may not
be the last used entry in the array, so decrementing the gpadl_index
might not have the right behavior. But I'm pretty sure that all
GPADL's for a channel are freed in sequence with no intervening
allocations, so nothing actually breaks. But this is one of the
messy areas with stashing the memory address and size transparently
to the caller.
From: Tianyu Lan <hidden> Date: 2021-08-15 15:21:34
On 8/13/2021 6:20 AM, Michael Kelley wrote:
quoted
@@ -474,6 +482,13 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel, if (ret) return ret;+ ret = set_memory_decrypted((unsigned long)kbuffer,+ HVPFN_UP(size));+ if (ret) {+ pr_warn("Failed to set host visibility.\n");
Enhance this message a bit. "Failed to set host visibility for new GPADL\n"
and also output the value of ret.
OK. This looks better. Thanks.
quoted
@@ -539,6 +554,10 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel, /* At this point, we received the gpadl created msg */ *gpadl_handle = gpadlmsg->gpadl;+ channel->gpadl_array[index].size = size;+ channel->gpadl_array[index].buffer = kbuffer;+ channel->gpadl_array[index].gpadlhandle = *gpadl_handle;+
I can see the merits of transparently stashing the memory address and size
that will be needed by vmbus_teardown_gpadl(), so that the callers of
__vmbus_establish_gpadl() don't have to worry about it. But doing the
stashing transparently is somewhat messy.
Given that the callers are already have memory allocated to save the
GPADL handle, a little refactoring would make for a much cleaner solution.
Instead of having memory allocated for the 32-bit GPADL handle, callers
should allocate the slightly larger struct vmbus_gpadl that you've
defined below. The calling interfaces can be updated to take a pointer
to this structure instead of a pointer to the 32-bit GPADL handle, and
you can save the memory address and size right along with the GPADL
handle. This approach touches a few more files, but I think there are
only two callers outside of the channel management code -- netvsc
and hv_uio -- so it's not a big change.
Yes, this is a good suggestion and Will update in the next version.
quoted
@@ -859,6 +886,19 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle) spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags); kfree(info);++ /* Find gpadl buffer virtual address and size. */+ for (i = 0; i < VMBUS_GPADL_RANGE_COUNT; i++)+ if (channel->gpadl_array[i].gpadlhandle == gpadl_handle)+ break;++ if (set_memory_encrypted((unsigned long)channel->gpadl_array[i].buffer,+ HVPFN_UP(channel->gpadl_array[i].size)))+ pr_warn("Fail to set mem host visibility.\n");
Enhance this message a bit: "Failed to set host visibility in GPADL teardown\n".
Also output the returned error code to help in debugging any occurrences of
a failure
From: Tianyu Lan <hidden> Date: 2021-08-09 17:57:16
From: Tianyu Lan <redacted>
In Hyper-V Isolation VM with AMD SEV, swiotlb boucne buffer
needs to be mapped into address space above vTOM and so
introduce dma_map_decrypted/dma_unmap_encrypted() to map/unmap
bounce buffer memory. The platform can populate man/unmap callback
in the dma memory decrypted ops.
---
include/linux/dma-map-ops.h | 9 +++++++++
kernel/dma/mapping.c | 22 ++++++++++++++++++++++
2 files changed, 31 insertions(+)
From: Christoph Hellwig <hch@lst.de> Date: 2021-08-12 12:27:08
On Mon, Aug 09, 2021 at 01:56:13PM -0400, Tianyu Lan wrote:
From: Tianyu Lan <redacted>
In Hyper-V Isolation VM with AMD SEV, swiotlb boucne buffer
needs to be mapped into address space above vTOM and so
introduce dma_map_decrypted/dma_unmap_encrypted() to map/unmap
bounce buffer memory. The platform can populate man/unmap callback
in the dma memory decrypted ops.
Nothing here looks actually DMA related, and the names are horribly
confusing vs the actual dma_map_* calls.
From: Tianyu Lan <hidden> Date: 2021-08-12 15:38:46
On 8/12/2021 8:26 PM, Christoph Hellwig wrote:
On Mon, Aug 09, 2021 at 01:56:13PM -0400, Tianyu Lan wrote:
quoted
From: Tianyu Lan <redacted>
In Hyper-V Isolation VM with AMD SEV, swiotlb boucne buffer
needs to be mapped into address space above vTOM and so
introduce dma_map_decrypted/dma_unmap_encrypted() to map/unmap
bounce buffer memory. The platform can populate man/unmap callback
in the dma memory decrypted ops.
Nothing here looks actually DMA related, and the names are horribly
confusing vs the actual dma_map_* calls.
OK. So this still need to keep in the set_memory.c file.
From: Tianyu Lan <hidden> Date: 2021-08-09 17:57:24
From: Tianyu Lan <redacted>
In Isolation VM with AMD SEV, bounce buffer needs to be accessed via
extra address space which is above shared_gpa_boundary
(E.G 39 bit address line) reported by Hyper-V CPUID ISOLATION_CONFIG.
The access physical address will be original physical address +
shared_gpa_boundary. The shared_gpa_boundary in the AMD SEV SNP
spec is called virtual top of memory(vTOM). Memory addresses below
vTOM are automatically treated as private while memory above
vTOM is treated as shared.
Use dma_map_decrypted() in the swiotlb code, store remap address returned
and use the remap address to copy data from/to swiotlb bounce buffer.
Signed-off-by: Tianyu Lan <redacted>
---
Change since v1:
* Make swiotlb_init_io_tlb_mem() return error code and return
error when dma_map_decrypted() fails.
---
include/linux/swiotlb.h | 4 ++++
kernel/dma/swiotlb.c | 32 ++++++++++++++++++++++++--------
2 files changed, 28 insertions(+), 8 deletions(-)
@@ -216,7 +223,11 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)panic("%s: Failed to allocate %zu bytes align=0x%lx\n",__func__,alloc_size,PAGE_SIZE);-swiotlb_init_io_tlb_mem(mem,__pa(tlb),nslabs,false);+ret=swiotlb_init_io_tlb_mem(mem,__pa(tlb),nslabs,false);+if(ret){+memblock_free(__pa(mem),alloc_size);+returnret;+}io_tlb_default_mem=mem;if(verbose)
From: Tianyu Lan <hidden> Date: 2021-08-13 17:58:50
On 8/12/2021 8:27 PM, Christoph Hellwig wrote:
This is still broken. You need to make sure the actual DMA allocations
do have struct page backing.
Hi Christoph:
swiotlb_tbl_map_single() still returns PA below vTOM/share_gpa_
boundary. These PAs has backing pages and belong to system memory.
In other word, all PAs passed to DMA API have backing pages and these is
no difference between Isolation guest and traditional guest for DMA API.
The new mapped VA for PA above vTOM here is just to access the bounce
buffer in the swiotlb code and isn't exposed to outside.
From: Tianyu Lan <hidden> Date: 2021-08-16 14:50:48
On 8/14/2021 1:58 AM, Tianyu Lan wrote:
On 8/12/2021 8:27 PM, Christoph Hellwig wrote:
quoted
This is still broken. You need to make sure the actual DMA allocations
do have struct page backing.
Hi Christoph:
swiotlb_tbl_map_single() still returns PA below vTOM/share_gpa_ > boundary. These PAs has backing pages and belong to system memory.
In other word, all PAs passed to DMA API have backing pages and these is
no difference between Isolation guest and traditional guest for DMA API.
The new mapped VA for PA above vTOM here is just to access the bounce
buffer in the swiotlb code and isn't exposed to outside.
Hi Christoph:
Sorry to bother you.Please double check with these two patches
" [PATCH V3 10/13] x86/Swiotlb: Add Swiotlb bounce buffer remap function
for HV IVM" and "[PATCH V3 09/13] DMA: Add dma_map_decrypted/dma_
unmap_encrypted() function".
The swiotlb bounce buffer in the isolation VM are allocated in the
low end memory and these memory has struct page backing. All dma address
returned by swiotlb/DMA API are low end memory and this is as same as
what happen in the traditional VM.So this means all PAs passed to DMA
API have struct page backing. The difference in Isolation VM is to
access bounce buffer via address space above vTOM/shared_guest_memory
_boundary. To access bounce buffer shared with host, the guest needs to
mark the memory visible to host via hypercall and map bounce buffer in
the extra address space(PA + shared_guest_memory_boundary). The vstart
introduced in this patch is to store va of extra address space and it's
only used to access bounce buffer in the swiotlb_bounce(). The PA in
extra space is only in the Hyper-V map function and won't be passed to
DMA API or other components.
The API dma_map_decrypted() introduced in the patch 9 is to map
the bounce buffer in the extra space and these memory in the low end
space are used as DMA memory in the driver. Do you prefer these APIs
still in the set_memory.c? I move the API to dma/mapping.c due to the
suggested name arch_dma_map_decrypted() in the previous mail
(https://lore.kernel.org/netdev/20210720135437.GA13554@lst.de/).
If there are something unclear, please let me know. Hope this
still can catch the merge window.
Thanks.
From: Christoph Hellwig <hch@lst.de> Date: 2021-08-19 08:50:08
On Mon, Aug 16, 2021 at 10:50:26PM +0800, Tianyu Lan wrote:
Hi Christoph:
Sorry to bother you.Please double check with these two patches
" [PATCH V3 10/13] x86/Swiotlb: Add Swiotlb bounce buffer remap function
for HV IVM" and "[PATCH V3 09/13] DMA: Add dma_map_decrypted/dma_
unmap_encrypted() function".
Do you have a git tree somewhere to look at the whole tree?
The swiotlb bounce buffer in the isolation VM are allocated in the
low end memory and these memory has struct page backing. All dma address
returned by swiotlb/DMA API are low end memory and this is as same as what
happen in the traditional VM.
Indeed.
The API dma_map_decrypted() introduced in the patch 9 is to map the
bounce buffer in the extra space and these memory in the low end space are
used as DMA memory in the driver. Do you prefer these APIs
still in the set_memory.c? I move the API to dma/mapping.c due to the
suggested name arch_dma_map_decrypted() in the previous mail
(https://lore.kernel.org/netdev/20210720135437.GA13554@lst.de/).
Well, what would help is a clear description of the semantics.
From: Tianyu Lan <hidden> Date: 2021-08-19 09:59:21
On 8/19/2021 4:49 PM, Christoph Hellwig wrote:
On Mon, Aug 16, 2021 at 10:50:26PM +0800, Tianyu Lan wrote:
quoted
Hi Christoph:
Sorry to bother you.Please double check with these two patches
" [PATCH V3 10/13] x86/Swiotlb: Add Swiotlb bounce buffer remap function
for HV IVM" and "[PATCH V3 09/13] DMA: Add dma_map_decrypted/dma_
unmap_encrypted() function".
Do you have a git tree somewhere to look at the whole tree?
The swiotlb bounce buffer in the isolation VM are allocated in the
low end memory and these memory has struct page backing. All dma address
returned by swiotlb/DMA API are low end memory and this is as same as what
happen in the traditional VM.
Indeed.
quoted
The API dma_map_decrypted() introduced in the patch 9 is to map the
bounce buffer in the extra space and these memory in the low end space are
used as DMA memory in the driver. Do you prefer these APIs
still in the set_memory.c? I move the API to dma/mapping.c due to the
suggested name arch_dma_map_decrypted() in the previous mail
(https://lore.kernel.org/netdev/20210720135437.GA13554@lst.de/).
Well, what would help is a clear description of the semantics.
From: Christoph Hellwig <hch@lst.de> Date: 2021-08-19 10:02:13
On Thu, Aug 19, 2021 at 05:59:02PM +0800, Tianyu Lan wrote:
On 8/19/2021 4:49 PM, Christoph Hellwig wrote:
quoted
On Mon, Aug 16, 2021 at 10:50:26PM +0800, Tianyu Lan wrote:
quoted
Hi Christoph:
Sorry to bother you.Please double check with these two patches
" [PATCH V3 10/13] x86/Swiotlb: Add Swiotlb bounce buffer remap function
for HV IVM" and "[PATCH V3 09/13] DMA: Add dma_map_decrypted/dma_
unmap_encrypted() function".
Do you have a git tree somewhere to look at the whole tree?
From: Tianyu Lan <hidden> Date: 2021-08-19 10:03:22
On 8/19/2021 6:02 PM, Christoph Hellwig wrote:
On Thu, Aug 19, 2021 at 05:59:02PM +0800, Tianyu Lan wrote:
quoted
On 8/19/2021 4:49 PM, Christoph Hellwig wrote:
quoted
On Mon, Aug 16, 2021 at 10:50:26PM +0800, Tianyu Lan wrote:
quoted
Hi Christoph:
Sorry to bother you.Please double check with these two patches
" [PATCH V3 10/13] x86/Swiotlb: Add Swiotlb bounce buffer remap function
for HV IVM" and "[PATCH V3 09/13] DMA: Add dma_map_decrypted/dma_
unmap_encrypted() function".
Do you have a git tree somewhere to look at the whole tree?
From: Tianyu Lan <hidden> Date: 2021-08-09 17:57:42
From: Tianyu Lan <redacted>
Hyper-V Isolation VM requires bounce buffer support to copy
data from/to encrypted memory and so enable swiotlb force
mode to use swiotlb bounce buffer for DMA transaction.
In Isolation VM with AMD SEV, the bounce buffer needs to be
accessed via extra address space which is above shared_gpa_boundary
(E.G 39 bit address line) reported by Hyper-V CPUID ISOLATION_CONFIG.
The access physical address will be original physical address +
shared_gpa_boundary. The shared_gpa_boundary in the AMD SEV SNP
spec is called virtual top of memory(vTOM). Memory addresses below
vTOM are automatically treated as private while memory above
vTOM is treated as shared.
Swiotlb bounce buffer code calls dma_map_decrypted()
to mark bounce buffer visible to host and map it in extra
address space. Populate dma memory decrypted ops with hv
map/unmap function.
Hyper-V initalizes swiotlb bounce buffer and default swiotlb
needs to be disabled. pci_swiotlb_detect_override() and
pci_swiotlb_detect_4gb() enable the default one. To override
the setting, hyperv_swiotlb_detect() needs to run before
these detect functions which depends on the pci_xen_swiotlb_
init(). Make pci_xen_swiotlb_init() depends on the hyperv_swiotlb
_detect() to keep the order.
The map function vmap_pfn() can't work in the early place
hyperv_iommu_swiotlb_init() and so initialize swiotlb bounce
buffer in the hyperv_iommu_swiotlb_later_init().
Signed-off-by: Tianyu Lan <redacted>
---
arch/x86/hyperv/ivm.c | 28 ++++++++++++++
arch/x86/include/asm/mshyperv.h | 2 +
arch/x86/xen/pci-swiotlb-xen.c | 3 +-
drivers/hv/vmbus_drv.c | 3 ++
drivers/iommu/hyperv-iommu.c | 65 +++++++++++++++++++++++++++++++++
include/linux/hyperv.h | 1 +
6 files changed, 101 insertions(+), 1 deletion(-)
@@ -265,3 +265,31 @@ int hv_set_mem_host_visibility(unsigned long addr, int numpages, bool visible)return__hv_set_mem_host_visibility((void*)addr,numpages,visibility);}++/*+*hv_map_memory-mapmemorytoextraspaceintheAMDSEV-SNPIsolationVM.+*/+void*hv_map_memory(void*addr,unsignedlongsize)+{+unsignedlong*pfns=kcalloc(size/HV_HYP_PAGE_SIZE,+sizeof(unsignedlong),GFP_KERNEL);+void*vaddr;+inti;++if(!pfns)+returnNULL;++for(i=0;i<size/HV_HYP_PAGE_SIZE;i++)+pfns[i]=virt_to_hvpfn(addr+i*HV_HYP_PAGE_SIZE)++(ms_hyperv.shared_gpa_boundary>>HV_HYP_PAGE_SHIFT);++vaddr=vmap_pfn(pfns,size/HV_HYP_PAGE_SIZE,PAGE_KERNEL_IO);+kfree(pfns);++returnvaddr;+}++voidhv_unmap_memory(void*addr)+{+vunmap(addr);+}
From: Michael Kelley <hidden> Date: 2021-08-19 18:11:41
From: Tianyu Lan <redacted> Sent: Monday, August 9, 2021 10:56 AM
quoted hunk
Hyper-V Isolation VM requires bounce buffer support to copy
data from/to encrypted memory and so enable swiotlb force
mode to use swiotlb bounce buffer for DMA transaction.
In Isolation VM with AMD SEV, the bounce buffer needs to be
accessed via extra address space which is above shared_gpa_boundary
(E.G 39 bit address line) reported by Hyper-V CPUID ISOLATION_CONFIG.
The access physical address will be original physical address +
shared_gpa_boundary. The shared_gpa_boundary in the AMD SEV SNP
spec is called virtual top of memory(vTOM). Memory addresses below
vTOM are automatically treated as private while memory above
vTOM is treated as shared.
Swiotlb bounce buffer code calls dma_map_decrypted()
to mark bounce buffer visible to host and map it in extra
address space. Populate dma memory decrypted ops with hv
map/unmap function.
Hyper-V initalizes swiotlb bounce buffer and default swiotlb
needs to be disabled. pci_swiotlb_detect_override() and
pci_swiotlb_detect_4gb() enable the default one. To override
the setting, hyperv_swiotlb_detect() needs to run before
these detect functions which depends on the pci_xen_swiotlb_
init(). Make pci_xen_swiotlb_init() depends on the hyperv_swiotlb
_detect() to keep the order.
The map function vmap_pfn() can't work in the early place
hyperv_iommu_swiotlb_init() and so initialize swiotlb bounce
buffer in the hyperv_iommu_swiotlb_later_init().
Signed-off-by: Tianyu Lan <redacted>
---
arch/x86/hyperv/ivm.c | 28 ++++++++++++++
arch/x86/include/asm/mshyperv.h | 2 +
arch/x86/xen/pci-swiotlb-xen.c | 3 +-
drivers/hv/vmbus_drv.c | 3 ++
drivers/iommu/hyperv-iommu.c | 65 +++++++++++++++++++++++++++++++++
include/linux/hyperv.h | 1 +
6 files changed, 101 insertions(+), 1 deletion(-)
@@ -265,3 +265,31 @@ int hv_set_mem_host_visibility(unsigned long addr, int numpages, bool visible)return__hv_set_mem_host_visibility((void*)addr,numpages,visibility);}++/*+*hv_map_memory-mapmemorytoextraspaceintheAMDSEV-SNPIsolationVM.+*/+void*hv_map_memory(void*addr,unsignedlongsize)+{+unsignedlong*pfns=kcalloc(size/HV_HYP_PAGE_SIZE,+sizeof(unsignedlong),GFP_KERNEL);+void*vaddr;+inti;++if(!pfns)+returnNULL;++for(i=0;i<size/HV_HYP_PAGE_SIZE;i++)+pfns[i]=virt_to_hvpfn(addr+i*HV_HYP_PAGE_SIZE)++(ms_hyperv.shared_gpa_boundary>>HV_HYP_PAGE_SHIFT);++vaddr=vmap_pfn(pfns,size/HV_HYP_PAGE_SIZE,PAGE_KERNEL_IO);+kfree(pfns);++returnvaddr;+}
This function is manipulating page tables in the guest VM. It is not involved
in communicating with Hyper-V, or passing PFNs to Hyper-V. The pfn array
contains guest PFNs, not Hyper-V PFNs. So it should use PAGE_SIZE
instead of HV_HYP_PAGE_SIZE, and similarly PAGE_SHIFT and virt_to_pfn().
If this code were ever to run on ARM64 in the future with PAGE_SIZE other
than 4 Kbytes, the use of PAGE_SIZE is correct choice.
A hard coded size here seems problematic. The memory size of
Isolated VMs can vary by orders of magnitude. I see that
xen_swiotlb_init() uses swiotlb_size_or_default(), which at least
pays attention to the value specified on the kernel boot line.
Another example is sev_setup_arch(), which in the native case sets
the size to 6% of main memory, with a max of 1 Gbyte. This is
the case that's closer to Isolated VMs, so doing something
similar could be a good approach.
quoted hunk
+ hyperv_io_tlb_start =
+ memblock_alloc_low(
+ PAGE_ALIGN(hyperv_io_tlb_size),
+ HV_HYP_PAGE_SIZE);
+
+ if (!hyperv_io_tlb_start) {
+ pr_warn("Fail to allocate Hyper-V swiotlb buffer.\n");
+ return;
+ }
+}
+
+int __init hyperv_swiotlb_detect(void)
+{
+ if (hypervisor_is_type(X86_HYPER_MS_HYPERV)
+ && hv_is_isolation_supported()) {
+ /*
+ * Enable swiotlb force mode in Isolation VM to
+ * use swiotlb bounce buffer for dma transaction.
+ */
+ swiotlb_force = SWIOTLB_FORCE;
+
+ dma_memory_generic_decrypted_ops.map = hv_map_memory;
+ dma_memory_generic_decrypted_ops.unmap = hv_unmap_memory;
+ return 1;
+ }
+
+ return 0;
+}
+
+void __init hyperv_iommu_swiotlb_later_init(void)
+{
+ /*
+ * Swiotlb bounce buffer needs to be mapped in extra address
+ * space. Map function doesn't work in the early place and so
+ * call swiotlb_late_init_with_tbl() here.
+ */
+ if (swiotlb_late_init_with_tbl(hyperv_io_tlb_start,
+ hyperv_io_tlb_size >> IO_TLB_SHIFT))
+ panic("Fail to initialize hyperv swiotlb.\n");
+}
+
+IOMMU_INIT_FINISH(hyperv_swiotlb_detect,
+ NULL, hyperv_iommu_swiotlb_init,
+ hyperv_iommu_swiotlb_later_init);
+
#endif
On Thu, Aug 19, 2021 at 06:11:30PM +0000, Michael Kelley wrote:
This function is manipulating page tables in the guest VM. It is not involved
in communicating with Hyper-V, or passing PFNs to Hyper-V. The pfn array
contains guest PFNs, not Hyper-V PFNs. So it should use PAGE_SIZE
instead of HV_HYP_PAGE_SIZE, and similarly PAGE_SHIFT and virt_to_pfn().
If this code were ever to run on ARM64 in the future with PAGE_SIZE other
than 4 Kbytes, the use of PAGE_SIZE is correct choice.
Yes. I just stumled over this yesterday. I think we can actually use a
nicer helper ased around vmap_range() to simplify this exactly because it
always uses the host page size. I hope I can draft up a RFC today.
From: Tianyu Lan <hidden> Date: 2021-08-20 09:32:40
On 8/20/2021 2:11 AM, Michael Kelley wrote:
quoted
}
+
+/*
+ * hv_map_memory - map memory to extra space in the AMD SEV-SNP Isolation VM.
+ */
+void *hv_map_memory(void *addr, unsigned long size)
+{
+ unsigned long *pfns = kcalloc(size / HV_HYP_PAGE_SIZE,
+ sizeof(unsigned long), GFP_KERNEL);
+ void *vaddr;
+ int i;
+
+ if (!pfns)
+ return NULL;
+
+ for (i = 0; i < size / HV_HYP_PAGE_SIZE; i++)
+ pfns[i] = virt_to_hvpfn(addr + i * HV_HYP_PAGE_SIZE) +
+ (ms_hyperv.shared_gpa_boundary >> HV_HYP_PAGE_SHIFT);
+
+ vaddr = vmap_pfn(pfns, size / HV_HYP_PAGE_SIZE, PAGE_KERNEL_IO);
+ kfree(pfns);
+
+ return vaddr;
+}
This function is manipulating page tables in the guest VM. It is not involved
in communicating with Hyper-V, or passing PFNs to Hyper-V. The pfn array
contains guest PFNs, not Hyper-V PFNs. So it should use PAGE_SIZE
instead of HV_HYP_PAGE_SIZE, and similarly PAGE_SHIFT and virt_to_pfn().
If this code were ever to run on ARM64 in the future with PAGE_SIZE other
than 4 Kbytes, the use of PAGE_SIZE is correct choice.
OK. Will update with PAGE_SIZE.
quoted
+void __init hyperv_iommu_swiotlb_init(void)
+{
+ unsigned long bytes;
+
+ /*
+ * Allocate Hyper-V swiotlb bounce buffer at early place
+ * to reserve large contiguous memory.
+ */
+ hyperv_io_tlb_size = 256 * 1024 * 1024;
A hard coded size here seems problematic. The memory size of
Isolated VMs can vary by orders of magnitude. I see that
xen_swiotlb_init() uses swiotlb_size_or_default(), which at least
pays attention to the value specified on the kernel boot line.
Another example is sev_setup_arch(), which in the native case sets
the size to 6% of main memory, with a max of 1 Gbyte. This is
the case that's closer to Isolated VMs, so doing something
similar could be a good approach.
Yes, agree. It's better to keep bounce buffer size with AMD SEV.
From: Tianyu Lan <hidden> Date: 2021-08-09 17:57:48
From: Tianyu Lan <redacted>
In Isolation VM, all shared memory with host needs to mark visible
to host via hvcall. vmbus_establish_gpadl() has already done it for
storvsc rx/tx ring buffer. The page buffer used by vmbus_sendpacket_
mpb_desc() still need to handle. Use DMA API to map/umap these
memory during sending/receiving packet and Hyper-V DMA ops callback
will use swiotlb function to allocate bounce buffer and copy data
from/to bounce buffer.
Signed-off-by: Tianyu Lan <redacted>
---
drivers/scsi/storvsc_drv.c | 68 +++++++++++++++++++++++++++++++++++---
1 file changed, 63 insertions(+), 5 deletions(-)
@@ -1860,13 +1911,20 @@ static int storvsc_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *scmnd)put_cpu();if(ret==-EAGAIN){-if(payload_sz>sizeof(cmd_request->mpb))-kfree(payload);/* no more space */-returnSCSI_MLQUEUE_DEVICE_BUSY;+ret=SCSI_MLQUEUE_DEVICE_BUSY;+gotofree_dma_range;}return0;++free_dma_range:+kfree(cmd_request->dma_range);++free_payload:+if(payload_sz>sizeof(cmd_request->mpb))+kfree(payload);+returnret;}staticstructscsi_host_templatescsi_driver={
From: Michael Kelley <hidden> Date: 2021-08-19 18:17:58
From: Tianyu Lan <redacted> Sent: Monday, August 9, 2021 10:56 AM
Subject line tag should be "scsi: storvsc:"
In Isolation VM, all shared memory with host needs to mark visible
to host via hvcall. vmbus_establish_gpadl() has already done it for
storvsc rx/tx ring buffer. The page buffer used by vmbus_sendpacket_
mpb_desc() still need to handle. Use DMA API to map/umap these
s/need to handle/needs to be handled/
quoted hunk
memory during sending/receiving packet and Hyper-V DMA ops callback
will use swiotlb function to allocate bounce buffer and copy data
from/to bounce buffer.
Signed-off-by: Tianyu Lan <redacted>
---
drivers/scsi/storvsc_drv.c | 68 +++++++++++++++++++++++++++++++++++---
1 file changed, 63 insertions(+), 5 deletions(-)
This count is really the number of entries in the dma_range
array, right? If so, perhaps "dma_range_count" would be
a better name so that it is more tightly associated.
Each of these macros is used only once. IMHO, they don't
add a lot of value. Just coding dma_map/unmap_page()
inline would be fine and eliminate these lines of code.
With this patch, it appears that storvsc_queuecommand() is always
doing bounce buffering, even when running in a non-isolated VM.
The dma_range is always allocated, and the inner loop below does
the dma mapping for every I/O page. The corresponding code in
storvsc_on_channel_callback() that does the dma unmap allows for
the dma_range to be NULL, but that never happens.
+ if (!cmd_request->dma_range) {
+ ret = -ENOMEM;
The other memory allocation failure in this function returns
SCSI_MLQUEUE_DEVICE_BUSY. It may be debatable as to whether
that's the best approach, but that's a topic for a different patch. I
would suggest being consistent and using the same return code
here.
quoted hunk
+ goto free_payload;
+ }
for (i = 0; sgl != NULL; sgl = sg_next(sgl)) {
/*
@@ -1847,9 +1878,29 @@ static int storvsc_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *scmnd) * last sgl should be reached at the same time that * the PFN array is filled. */- while (hvpfns_to_add--)- payload->range.pfn_array[i++] = hvpfn++;+ while (hvpfns_to_add--) {+ size = min(HV_HYP_PAGE_SIZE - offset_in_hvpg,+ (unsigned long)length);+ dma = storvsc_dma_map(&dev->device, pfn_to_page(hvpfn++),+ offset_in_hvpg, size,+ scmnd->sc_data_direction);+ if (dma_mapping_error(&dev->device, dma)) {+ ret = -ENOMEM;
The typical error from dma_map_page() will be running out of
bounce buffer memory. This is a transient condition that should be
retried at the higher levels. So make sure to return an error code
that indicates the I/O should be resubmitted.
I'm not clear on why payload->range.offset needs to be set again.
Even after the dma mapping is done, doesn't the offset in the first
page have to be the same? If it wasn't the same, Hyper-V wouldn't
be able to process the PFN list correctly. In fact, couldn't the above
code just always set offset_in_hvpg = 0?
This line just saves the size of the dma_range array. Could
it be moved up with the code that allocates the dma_range
array? To me, it would make more sense to have all that
code together in one place.
}
The whole approach here is to do dma remapping on each individual page
of the I/O buffer. But wouldn't it be possible to use dma_map_sg() to map
each scatterlist entry as a unit? Each scatterlist entry describes a range of
physically contiguous memory. After dma_map_sg(), the resulting dma
address must also refer to a physically contiguous range in the swiotlb
bounce buffer memory. So at the top of the "for" loop over the scatterlist
entries, do dma_map_sg() if we're in an isolated VM. Then compute the
hvpfn value based on the dma address instead of sg_page(). But everything
else is the same, and the inner loop for populating the pfn_arry is unmodified.
Furthermore, the dma_range array that you've added is not needed, since
scatterlist entries already have a dma_address field for saving the mapped
address, and dma_unmap_sg() uses that field.
One thing: There's a maximum swiotlb mapping size, which I think works
out to be 256 Kbytes. See swiotlb_max_mapping_size(). We need to make
sure that we don't get a scatterlist entry bigger than this size. But I think
this already happens because you set the device->dma_mask field in
Patch 11 of this series. __scsi_init_queue checks for this setting and
sets max_sectors to limits transfers to the max mapping size.
quoted hunk
cmd_request->payload = payload;
@@ -1860,13 +1911,20 @@ static int storvsc_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *scmnd) put_cpu(); if (ret == -EAGAIN) {- if (payload_sz > sizeof(cmd_request->mpb))- kfree(payload); /* no more space */- return SCSI_MLQUEUE_DEVICE_BUSY;+ ret = SCSI_MLQUEUE_DEVICE_BUSY;+ goto free_dma_range; } return 0;++free_dma_range:+ kfree(cmd_request->dma_range);++free_payload:+ if (payload_sz > sizeof(cmd_request->mpb))+ kfree(payload);+ return ret; } static struct scsi_host_template scsi_driver = {--
Each of these macros is used only once. IMHO, they don't
add a lot of value. Just coding dma_map/unmap_page()
inline would be fine and eliminate these lines of code.
Yes, I had the same thought when looking over the code. Especially
as macros tend to further obsfucate the code (compared to actual helper
functions).
quoted
+ for (i = 0; i < request->hvpg_count; i++)
+ storvsc_dma_unmap(&device->device,
+ request->dma_range[i],
+ request->vstor_packet.vm_srb.data_in == READ_TYPE);
I think you can directly get the DMA direction as request->cmd->sc_data_direction.
With this patch, it appears that storvsc_queuecommand() is always
doing bounce buffering, even when running in a non-isolated VM.
The dma_range is always allocated, and the inner loop below does
the dma mapping for every I/O page. The corresponding code in
storvsc_on_channel_callback() that does the dma unmap allows for
the dma_range to be NULL, but that never happens.
Maybe I'm missing something in the hyperv code, but I don't think
dma_map_page would bounce buffer for the non-isolated case. It
will just return the physical address.
quoted
+ if (!cmd_request->dma_range) {
+ ret = -ENOMEM;
The other memory allocation failure in this function returns
SCSI_MLQUEUE_DEVICE_BUSY. It may be debatable as to whether
that's the best approach, but that's a topic for a different patch. I
would suggest being consistent and using the same return code
here.
Independent of if SCSI_MLQUEUE_DEVICE_BUSY is good (it it a common
pattern in SCSI drivers), ->queuecommand can't return normal
negative errnos. It must return the SCSI_MLQUEUE_* codes or 0.
We should probably change the return type of the method definition
to a suitable enum to make this more clear.
I'm not clear on why payload->range.offset needs to be set again.
Even after the dma mapping is done, doesn't the offset in the first
page have to be the same? If it wasn't the same, Hyper-V wouldn't
be able to process the PFN list correctly. In fact, couldn't the above
code just always set offset_in_hvpg = 0?
Careful. DMA mapping is supposed to keep the offset in the page, but
for that the DMA mapping code needs to know what the device considers a
"page". For that the driver needs to set the min_align_mask field in
struct device_dma_parameters.
The whole approach here is to do dma remapping on each individual page
of the I/O buffer. But wouldn't it be possible to use dma_map_sg() to map
each scatterlist entry as a unit? Each scatterlist entry describes a range of
physically contiguous memory. After dma_map_sg(), the resulting dma
address must also refer to a physically contiguous range in the swiotlb
bounce buffer memory. So at the top of the "for" loop over the scatterlist
entries, do dma_map_sg() if we're in an isolated VM. Then compute the
hvpfn value based on the dma address instead of sg_page(). But everything
else is the same, and the inner loop for populating the pfn_arry is unmodified.
Furthermore, the dma_range array that you've added is not needed, since
scatterlist entries already have a dma_address field for saving the mapped
address, and dma_unmap_sg() uses that field.
Yes, I think dma_map_sg is the right thing to use here, probably even
for the non-isolated case so that we can get the hv drivers out of their
little corner and into being more like a normal kernel driver. That
is, use the scsi_dma_map/scsi_dma_unmap helpers, and then iterate over
the dma addresses one page at a time using for_each_sg_dma_page.
One thing: There's a maximum swiotlb mapping size, which I think works
out to be 256 Kbytes. See swiotlb_max_mapping_size(). We need to make
sure that we don't get a scatterlist entry bigger than this size. But I think
this already happens because you set the device->dma_mask field in
Patch 11 of this series. __scsi_init_queue checks for this setting and
sets max_sectors to limits transfers to the max mapping size.
With this patch, it appears that storvsc_queuecommand() is always
doing bounce buffering, even when running in a non-isolated VM.
The dma_range is always allocated, and the inner loop below does
the dma mapping for every I/O page. The corresponding code in
storvsc_on_channel_callback() that does the dma unmap allows for
the dma_range to be NULL, but that never happens.
Maybe I'm missing something in the hyperv code, but I don't think
dma_map_page would bounce buffer for the non-isolated case. It
will just return the physical address.
OK, right. In the isolated VM case, the swiotlb is in force mode
and will do bounce buffering. In the non-isolated case,
dma_map_page_attrs() -> dma_direct_map_page() does a lot of
checking but eventually just returns the physical address. As this
patch is currently coded, it adds a fair amount of overhead
here in storvsc_queuecommand(), plus the overhead of the dma
mapping function deciding to use the identity mapping. But if
dma_map_sg() is used and the code is simplified a bit, the overhead
will be less in general and will be per sgl entry instead of per page.
I'm not clear on why payload->range.offset needs to be set again.
Even after the dma mapping is done, doesn't the offset in the first
page have to be the same? If it wasn't the same, Hyper-V wouldn't
be able to process the PFN list correctly. In fact, couldn't the above
code just always set offset_in_hvpg = 0?
Careful. DMA mapping is supposed to keep the offset in the page, but
for that the DMA mapping code needs to know what the device considers a
"page". For that the driver needs to set the min_align_mask field in
struct device_dma_parameters.
I see that the swiotlb code gets and uses the min_align_mask field. But
the NVME driver is the only driver that ever sets it, so the value is zero
in all other cases. Does swiotlb just use PAGE_SIZE in that that case? I
couldn't tell from a quick glance at the swiotlb code.
quoted
The whole approach here is to do dma remapping on each individual page
of the I/O buffer. But wouldn't it be possible to use dma_map_sg() to map
each scatterlist entry as a unit? Each scatterlist entry describes a range of
physically contiguous memory. After dma_map_sg(), the resulting dma
address must also refer to a physically contiguous range in the swiotlb
bounce buffer memory. So at the top of the "for" loop over the scatterlist
entries, do dma_map_sg() if we're in an isolated VM. Then compute the
hvpfn value based on the dma address instead of sg_page(). But everything
else is the same, and the inner loop for populating the pfn_arry is unmodified.
Furthermore, the dma_range array that you've added is not needed, since
scatterlist entries already have a dma_address field for saving the mapped
address, and dma_unmap_sg() uses that field.
Yes, I think dma_map_sg is the right thing to use here, probably even
for the non-isolated case so that we can get the hv drivers out of their
little corner and into being more like a normal kernel driver. That
is, use the scsi_dma_map/scsi_dma_unmap helpers, and then iterate over
the dma addresses one page at a time using for_each_sg_dma_page.
Doing some broader revisions to the Hyper-V storvsc driver is up next on
my to-do list. Rather than significantly modifying the non-isolated case in
this patch set, I'd suggest factoring it into my broader revisions.
Michael
On Fri, Aug 20, 2021 at 03:40:08PM +0000, Michael Kelley wrote:
I see that the swiotlb code gets and uses the min_align_mask field. But
the NVME driver is the only driver that ever sets it, so the value is zero
in all other cases. Does swiotlb just use PAGE_SIZE in that that case? I
couldn't tell from a quick glance at the swiotlb code.
The encoding isn't all that common. I only know it for the RDMA memory
registration format, and RDMA in general does not interact very well
with swiotlb (although the in-kernel drivers should work fine, it is
userspace RDMA that is the problem). It seems recently a new driver
using the format (mpi3mr) also showed up. All these should probably set
the min_align_mask.