lkml.org 
[lkml]   [2021]   [Sep]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH V5 07/12] Drivers: hv: vmbus: Add SNP support for VMbus channel initiate message
    Date
    From: Tianyu Lan <ltykernel@gmail.com> Sent: Tuesday, September 14, 2021 6:39 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 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..edd8f7dd169f 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()) {
    > + vmbus_connection.monitor_pages_pa[0] +=
    > + ms_hyperv.shared_gpa_boundary;
    > + vmbus_connection.monitor_pages_pa[1] +=
    > + ms_hyperv.shared_gpa_boundary;
    > +
    > + 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[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);
    > +
    > + }
    > +

    This all looks good. To me, this is a lot clearer to have all the mapping
    and encryption/decryption handled in one place.

    > 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..560cba916d1d 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];
    > + unsigned long monitor_pages_pa[2];

    The type of this field really should be phys_addr_t. In addition to
    just making semantic sense, then it will match the return type from
    virt_to_phys() and the input arg to memremap() since resource_size_t
    is typedef'ed as phys_addr_t.

    > struct list_head chn_msg_list;
    > spinlock_t channelmsg_lock;
    >
    > --
    > 2.25.1

    \
     
     \ /
      Last update: 2021-09-15 17:43    [W:4.210 / U:0.220 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site