| Message ID | 20210930130545.1210298-8-ltykernel@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | x86/Hyper-V: Add Hyper-V Isolation VM support(First part) | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
From: Tianyu Lan <ltykernel@gmail.com> Sent: Thursday, September 30, 2021 6:06 AM > > 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_original[] in the struct vmbus_connection > to store monitor page virtual address returned by hv_alloc_hyperv_ > zeroed_page() and free monitor page via monitor_pages_original in > the vmbus_disconnect(). The monitor_pages[] is to used to access > monitor page and it is initialized to be equal with monitor_pages_ > original. The monitor_pages[] will be overridden in the isolation VM > with va of extra address. Introduce monitor_pages_pa[] to store > monitor pages' physical address and use it to populate pa in the > initiate msg. > > Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com> > --- > Change since v5: > * change vmbus_connection.monitor_pages_pa type from > unsigned long to phys_addr_t > * Plus vmbus_connection.monitor_pages_pa with ms_hyperv. > shared_gpa_boundary only in the IVM with AMD SEV. > > Change since v4: > * Introduce monitor_pages_pa[] to store monitor pages' physical > address and use it to populate pa in the initiate msg. > * Move code of mapping moniter pages in extra address into > vmbus_connect(). > > Change since v3: > * Rename monitor_pages_va with monitor_pages_original > * free monitor page via monitor_pages_original and > monitor_pages is used to access monitor page. > > Change since v1: > * Not remap monitor pages in the non-SNP isolation VM. > --- > drivers/hv/connection.c | 90 ++++++++++++++++++++++++++++++++++++--- > drivers/hv/hyperv_vmbus.h | 2 + > 2 files changed, 86 insertions(+), 6 deletions(-) > > diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c > index 8820ae68f20f..7fac8d99541c 100644 > --- a/drivers/hv/connection.c > +++ b/drivers/hv/connection.c > @@ -19,6 +19,8 @@ > #include <linux/vmalloc.h> > #include <linux/hyperv.h> > #include <linux/export.h> > +#include <linux/io.h> > +#include <linux/set_memory.h> > #include <asm/mshyperv.h> > > #include "hyperv_vmbus.h" > @@ -102,8 +104,9 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version) > vmbus_connection.msg_conn_id = VMBUS_MESSAGE_CONNECTION_ID; > } > > - msg->monitor_page1 = virt_to_phys(vmbus_connection.monitor_pages[0]); > - msg->monitor_page2 = virt_to_phys(vmbus_connection.monitor_pages[1]); > + msg->monitor_page1 = vmbus_connection.monitor_pages_pa[0]; > + msg->monitor_page2 = vmbus_connection.monitor_pages_pa[1]; > + > msg->target_vcpu = hv_cpu_number_to_vp_number(VMBUS_CONNECT_CPU); > > /* > @@ -216,6 +219,65 @@ int vmbus_connect(void) > goto cleanup; > } > > + vmbus_connection.monitor_pages_original[0] > + = vmbus_connection.monitor_pages[0]; > + vmbus_connection.monitor_pages_original[1] > + = vmbus_connection.monitor_pages[1]; > + vmbus_connection.monitor_pages_pa[0] > + = virt_to_phys(vmbus_connection.monitor_pages[0]); > + vmbus_connection.monitor_pages_pa[1] > + = virt_to_phys(vmbus_connection.monitor_pages[1]); > + > + if (hv_is_isolation_supported()) { > + ret = set_memory_decrypted((unsigned long) > + vmbus_connection.monitor_pages[0], > + 1); > + ret |= set_memory_decrypted((unsigned long) > + vmbus_connection.monitor_pages[1], > + 1); > + if (ret) > + goto cleanup; > + > + /* > + * Isolation VM with AMD SNP needs to access monitor page via > + * address space above shared gpa boundary. > + */ > + if (hv_isolation_type_snp()) { > + vmbus_connection.monitor_pages_pa[0] += > + ms_hyperv.shared_gpa_boundary; > + vmbus_connection.monitor_pages_pa[1] += > + ms_hyperv.shared_gpa_boundary; > + > + vmbus_connection.monitor_pages[0] > + = memremap(vmbus_connection.monitor_pages_pa[0], > + HV_HYP_PAGE_SIZE, > + MEMREMAP_WB); > + if (!vmbus_connection.monitor_pages[0]) { > + ret = -ENOMEM; > + goto cleanup; > + } > + > + vmbus_connection.monitor_pages[1] > + = memremap(vmbus_connection.monitor_pages_pa[1], > + HV_HYP_PAGE_SIZE, > + MEMREMAP_WB); > + if (!vmbus_connection.monitor_pages[1]) { > + ret = -ENOMEM; > + goto cleanup; > + } > + } > + > + /* > + * Set memory host visibility hvcall smears memory > + * and so zero monitor pages here. > + */ > + memset(vmbus_connection.monitor_pages[0], 0x00, > + HV_HYP_PAGE_SIZE); > + memset(vmbus_connection.monitor_pages[1], 0x00, > + HV_HYP_PAGE_SIZE); > + > + } > + > msginfo = kzalloc(sizeof(*msginfo) + > sizeof(struct vmbus_channel_initiate_contact), > GFP_KERNEL); > @@ -303,10 +365,26 @@ void vmbus_disconnect(void) > vmbus_connection.int_page = NULL; > } > > - hv_free_hyperv_page((unsigned long)vmbus_connection.monitor_pages[0]); > - hv_free_hyperv_page((unsigned long)vmbus_connection.monitor_pages[1]); > - vmbus_connection.monitor_pages[0] = NULL; > - vmbus_connection.monitor_pages[1] = NULL; > + if (hv_is_isolation_supported()) { > + memunmap(vmbus_connection.monitor_pages[0]); > + memunmap(vmbus_connection.monitor_pages[1]); The matching memremap() calls are made in vmbus_connect() only in the SNP case. In the non-SNP case, monitor_pages and monitor_pages_original are the same, so you would be doing an unmap, and then doing the set_memory_encrypted() and hv_free_hyperv_page() using an address that is no longer mapped, which seems wrong. Looking at memunmap(), it might be a no-op in this case, but even if it is, making them conditional on the SNP case might be a safer thing to do, and it would make the code more symmetrical. > + > + set_memory_encrypted((unsigned long) > + vmbus_connection.monitor_pages_original[0], > + 1); > + set_memory_encrypted((unsigned long) > + vmbus_connection.monitor_pages_original[1], > + 1); > + } > + > + hv_free_hyperv_page((unsigned long) > + vmbus_connection.monitor_pages_original[0]); > + hv_free_hyperv_page((unsigned long) > + vmbus_connection.monitor_pages_original[1]); > + vmbus_connection.monitor_pages_original[0] = > + vmbus_connection.monitor_pages[0] = NULL; > + vmbus_connection.monitor_pages_original[1] = > + vmbus_connection.monitor_pages[1] = NULL; > } > > /* > diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h > index 42f3d9d123a1..d0a5232a1c3e 100644 > --- a/drivers/hv/hyperv_vmbus.h > +++ b/drivers/hv/hyperv_vmbus.h > @@ -240,6 +240,8 @@ struct vmbus_connection { > * is child->parent notification > */ > struct hv_monitor_page *monitor_pages[2]; > + void *monitor_pages_original[2]; > + phys_addr_t monitor_pages_pa[2]; > struct list_head chn_msg_list; > spinlock_t channelmsg_lock; > > -- > 2.25.1
Hi Michael:
Thanks for your review.
On 10/2/2021 9:26 PM, Michael Kelley wrote:
>> @@ -303,10 +365,26 @@ void vmbus_disconnect(void)
>> vmbus_connection.int_page = NULL;
>> }
>>
>> - hv_free_hyperv_page((unsigned long)vmbus_connection.monitor_pages[0]);
>> - hv_free_hyperv_page((unsigned long)vmbus_connection.monitor_pages[1]);
>> - vmbus_connection.monitor_pages[0] = NULL;
>> - vmbus_connection.monitor_pages[1] = NULL;
>> + if (hv_is_isolation_supported()) {
>> + memunmap(vmbus_connection.monitor_pages[0]);
>> + memunmap(vmbus_connection.monitor_pages[1]);
> The matching memremap() calls are made in vmbus_connect() only in the
> SNP case. In the non-SNP case, monitor_pages and monitor_pages_original
> are the same, so you would be doing an unmap, and then doing the
> set_memory_encrypted() and hv_free_hyperv_page() using an address
> that is no longer mapped, which seems wrong. Looking at memunmap(),
> it might be a no-op in this case, but even if it is, making them conditional
> on the SNP case might be a safer thing to do, and it would make the code
> more symmetrical.
>
Yes, memumap() does nothing is the non-SNP CVM and so I didn't check CVM
type here. I will add the check in the next version.
Thanks.
From: Tianyu Lan <ltykernel@gmail.com> Sent: Saturday, October 2, 2021 7:40 AM > > > On 10/2/2021 9:26 PM, Michael Kelley wrote: > >> @@ -303,10 +365,26 @@ void vmbus_disconnect(void) > >> vmbus_connection.int_page = NULL; > >> } > >> > >> - hv_free_hyperv_page((unsigned long)vmbus_connection.monitor_pages[0]); > >> - hv_free_hyperv_page((unsigned long)vmbus_connection.monitor_pages[1]); > >> - vmbus_connection.monitor_pages[0] = NULL; > >> - vmbus_connection.monitor_pages[1] = NULL; > >> + if (hv_is_isolation_supported()) { > >> + memunmap(vmbus_connection.monitor_pages[0]); > >> + memunmap(vmbus_connection.monitor_pages[1]); > > The matching memremap() calls are made in vmbus_connect() only in the > > SNP case. In the non-SNP case, monitor_pages and monitor_pages_original > > are the same, so you would be doing an unmap, and then doing the > > set_memory_encrypted() and hv_free_hyperv_page() using an address > > that is no longer mapped, which seems wrong. Looking at memunmap(), > > it might be a no-op in this case, but even if it is, making them conditional > > on the SNP case might be a safer thing to do, and it would make the code > > more symmetrical. > > > > Yes, memumap() does nothing is the non-SNP CVM and so I didn't check CVM > type here. I will add the check in the next version. > > Thanks. > I would also be OK with just adding a comment to that effect, just so someone looking at the code in the future understands that there's not a problem. Your call. Michael
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c index 8820ae68f20f..7fac8d99541c 100644 --- a/drivers/hv/connection.c +++ b/drivers/hv/connection.c @@ -19,6 +19,8 @@ #include <linux/vmalloc.h> #include <linux/hyperv.h> #include <linux/export.h> +#include <linux/io.h> +#include <linux/set_memory.h> #include <asm/mshyperv.h> #include "hyperv_vmbus.h" @@ -102,8 +104,9 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version) vmbus_connection.msg_conn_id = VMBUS_MESSAGE_CONNECTION_ID; } - msg->monitor_page1 = virt_to_phys(vmbus_connection.monitor_pages[0]); - msg->monitor_page2 = virt_to_phys(vmbus_connection.monitor_pages[1]); + msg->monitor_page1 = vmbus_connection.monitor_pages_pa[0]; + msg->monitor_page2 = vmbus_connection.monitor_pages_pa[1]; + msg->target_vcpu = hv_cpu_number_to_vp_number(VMBUS_CONNECT_CPU); /* @@ -216,6 +219,65 @@ int vmbus_connect(void) goto cleanup; } + vmbus_connection.monitor_pages_original[0] + = vmbus_connection.monitor_pages[0]; + vmbus_connection.monitor_pages_original[1] + = vmbus_connection.monitor_pages[1]; + vmbus_connection.monitor_pages_pa[0] + = virt_to_phys(vmbus_connection.monitor_pages[0]); + vmbus_connection.monitor_pages_pa[1] + = virt_to_phys(vmbus_connection.monitor_pages[1]); + + if (hv_is_isolation_supported()) { + ret = set_memory_decrypted((unsigned long) + vmbus_connection.monitor_pages[0], + 1); + ret |= set_memory_decrypted((unsigned long) + vmbus_connection.monitor_pages[1], + 1); + if (ret) + goto cleanup; + + /* + * Isolation VM with AMD SNP needs to access monitor page via + * address space above shared gpa boundary. + */ + if (hv_isolation_type_snp()) { + vmbus_connection.monitor_pages_pa[0] += + ms_hyperv.shared_gpa_boundary; + vmbus_connection.monitor_pages_pa[1] += + ms_hyperv.shared_gpa_boundary; + + vmbus_connection.monitor_pages[0] + = memremap(vmbus_connection.monitor_pages_pa[0], + HV_HYP_PAGE_SIZE, + MEMREMAP_WB); + if (!vmbus_connection.monitor_pages[0]) { + ret = -ENOMEM; + goto cleanup; + } + + vmbus_connection.monitor_pages[1] + = memremap(vmbus_connection.monitor_pages_pa[1], + HV_HYP_PAGE_SIZE, + MEMREMAP_WB); + if (!vmbus_connection.monitor_pages[1]) { + ret = -ENOMEM; + goto cleanup; + } + } + + /* + * Set memory host visibility hvcall smears memory + * and so zero monitor pages here. + */ + memset(vmbus_connection.monitor_pages[0], 0x00, + HV_HYP_PAGE_SIZE); + memset(vmbus_connection.monitor_pages[1], 0x00, + HV_HYP_PAGE_SIZE); + + } + msginfo = kzalloc(sizeof(*msginfo) + sizeof(struct vmbus_channel_initiate_contact), GFP_KERNEL); @@ -303,10 +365,26 @@ void vmbus_disconnect(void) vmbus_connection.int_page = NULL; } - hv_free_hyperv_page((unsigned long)vmbus_connection.monitor_pages[0]); - hv_free_hyperv_page((unsigned long)vmbus_connection.monitor_pages[1]); - vmbus_connection.monitor_pages[0] = NULL; - vmbus_connection.monitor_pages[1] = NULL; + if (hv_is_isolation_supported()) { + memunmap(vmbus_connection.monitor_pages[0]); + memunmap(vmbus_connection.monitor_pages[1]); + + set_memory_encrypted((unsigned long) + vmbus_connection.monitor_pages_original[0], + 1); + set_memory_encrypted((unsigned long) + vmbus_connection.monitor_pages_original[1], + 1); + } + + hv_free_hyperv_page((unsigned long) + vmbus_connection.monitor_pages_original[0]); + hv_free_hyperv_page((unsigned long) + vmbus_connection.monitor_pages_original[1]); + vmbus_connection.monitor_pages_original[0] = + vmbus_connection.monitor_pages[0] = NULL; + vmbus_connection.monitor_pages_original[1] = + vmbus_connection.monitor_pages[1] = NULL; } /* diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h index 42f3d9d123a1..d0a5232a1c3e 100644 --- a/drivers/hv/hyperv_vmbus.h +++ b/drivers/hv/hyperv_vmbus.h @@ -240,6 +240,8 @@ struct vmbus_connection { * is child->parent notification */ struct hv_monitor_page *monitor_pages[2]; + void *monitor_pages_original[2]; + phys_addr_t monitor_pages_pa[2]; struct list_head chn_msg_list; spinlock_t channelmsg_lock;