RE: [PATCH V5 04/12] Drivers: hv: vmbus: Mark vmbus ring buffer visible to host in Isolation VM

From: Michael Kelley
Date: Wed Sep 15 2021 - 11:40:32 EST

Next message: Michael Kelley: "RE: [PATCH V5 05/12] x86/hyperv: Add Write/Read MSR registers via ghcb page"
Previous message: Paul E. McKenney: "Re: [PATCH] kernel: in_irq() cleanup"
In reply to: Tianyu Lan: "[PATCH V5 04/12] Drivers: hv: vmbus: Mark vmbus ring buffer visible to host in Isolation VM"
Next in thread: Tianyu Lan: "[PATCH V5 06/12] x86/hyperv: Add ghcb hvcall support for SNP VM"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

From: Tianyu Lan <ltykernel@xxxxxxxxx> Sent: Tuesday, September 14, 2021 6:39 AM
>
> Mark vmbus ring buffer visible with set_memory_decrypted() when
> establish gpadl handle.
>
> Signed-off-by: Tianyu Lan <Tianyu.Lan@xxxxxxxxxxxxx>
> ---
> Change sincv v4
> * Change gpadl handle in netvsc and uio driver from u32 to
> struct vmbus_gpadl.
> * Change vmbus_establish_gpadl()'s gpadl_handle parameter
> to vmbus_gpadl data structure.
>
> Change since v3:
> * Change vmbus_teardown_gpadl() parameter and put gpadl handle,
> buffer and buffer size in the struct vmbus_gpadl.
> ---
> drivers/hv/channel.c | 54 ++++++++++++++++++++++++---------
> drivers/net/hyperv/hyperv_net.h | 5 +--
> drivers/net/hyperv/netvsc.c | 17 ++++++-----
> drivers/uio/uio_hv_generic.c | 20 ++++++------
> include/linux/hyperv.h | 12 ++++++--
> 5 files changed, 71 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index f3761c73b074..cf419eb1de77 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -17,6 +17,7 @@
> #include <linux/hyperv.h>
> #include <linux/uio.h>
> #include <linux/interrupt.h>
> +#include <linux/set_memory.h>
> #include <asm/page.h>
> #include <asm/mshyperv.h>
>
> @@ -456,7 +457,7 @@ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer,
> static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
> enum hv_gpadl_type type, void *kbuffer,
> u32 size, u32 send_offset,
> - u32 *gpadl_handle)
> + struct vmbus_gpadl *gpadl)
> {
> struct vmbus_channel_gpadl_header *gpadlmsg;
> struct vmbus_channel_gpadl_body *gpadl_body;
> @@ -474,6 +475,15 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
> if (ret)
> return ret;
>
> + ret = set_memory_decrypted((unsigned long)kbuffer,
> + HVPFN_UP(size));

This should be PFN_UP, not HVPFN_UP. The numpages parameter to
set_memory_decrypted() is in guest size pages, not Hyper-V size pages.

> + if (ret) {
> + dev_warn(&channel->device_obj->device,
> + "Failed to set host visibility for new GPADL %d.\n",
> + ret);
> + return ret;
> + }
> +
> init_completion(&msginfo->waitevent);
> msginfo->waiting_channel = channel;
>
> @@ -537,7 +547,10 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
> }
>
> /* At this point, we received the gpadl created msg */
> - *gpadl_handle = gpadlmsg->gpadl;
> + gpadl->gpadl_handle = gpadlmsg->gpadl;
> + gpadl->buffer = kbuffer;
> + gpadl->size = size;
> +
>
> cleanup:
> spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
> @@ -549,6 +562,11 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
> }
>
> kfree(msginfo);
> +
> + if (ret)
> + set_memory_encrypted((unsigned long)kbuffer,
> + HVPFN_UP(size));

Should be PFN_UP as noted on the previous call to set_memory_decrypted().

> +
> return ret;
> }
>
> @@ -561,10 +579,10 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
> * @gpadl_handle: some funky thing
> */
> int vmbus_establish_gpadl(struct vmbus_channel *channel, void *kbuffer,
> - u32 size, u32 *gpadl_handle)
> + u32 size, struct vmbus_gpadl *gpadl)
> {
> return __vmbus_establish_gpadl(channel, HV_GPADL_BUFFER, kbuffer, size,
> - 0U, gpadl_handle);
> + 0U, gpadl);
> }
> EXPORT_SYMBOL_GPL(vmbus_establish_gpadl);
>
> @@ -639,6 +657,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
> struct vmbus_channel_open_channel *open_msg;
> struct vmbus_channel_msginfo *open_info = NULL;
> struct page *page = newchannel->ringbuffer_page;
> + struct vmbus_gpadl gpadl;

I think this local variable was needed in a previous version of the patch, but
is now unused and should be deleted.

> u32 send_pages, recv_pages;
> unsigned long flags;
> int err;
> @@ -675,7 +694,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
> goto error_clean_ring;
>
> /* Establish the gpadl for the ring buffer */
> - newchannel->ringbuffer_gpadlhandle = 0;
> + newchannel->ringbuffer_gpadlhandle.gpadl_handle = 0;
>
> err = __vmbus_establish_gpadl(newchannel, HV_GPADL_RING,
> page_address(newchannel->ringbuffer_page),
> @@ -701,7 +720,8 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
> open_msg->header.msgtype = CHANNELMSG_OPENCHANNEL;
> open_msg->openid = newchannel->offermsg.child_relid;
> open_msg->child_relid = newchannel->offermsg.child_relid;
> - open_msg->ringbuffer_gpadlhandle = newchannel->ringbuffer_gpadlhandle;
> + open_msg->ringbuffer_gpadlhandle
> + = newchannel->ringbuffer_gpadlhandle.gpadl_handle;
> /*
> * The unit of ->downstream_ringbuffer_pageoffset is HV_HYP_PAGE and
> * the unit of ->ringbuffer_send_offset (i.e. send_pages) is PAGE, so
> @@ -759,8 +779,8 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
> error_free_info:
> kfree(open_info);
> error_free_gpadl:
> - vmbus_teardown_gpadl(newchannel, newchannel->ringbuffer_gpadlhandle);
> - newchannel->ringbuffer_gpadlhandle = 0;
> + vmbus_teardown_gpadl(newchannel, &newchannel->ringbuffer_gpadlhandle);
> + newchannel->ringbuffer_gpadlhandle.gpadl_handle = 0;

My previous comments had suggested letting vmbus_teardown_gpadl() set the
gpadl_handle to zero, avoiding the need for all the callers to set it to zero.
Did that not work for some reason? Just curious ....

> error_clean_ring:
> hv_ringbuffer_cleanup(&newchannel->outbound);
> hv_ringbuffer_cleanup(&newchannel->inbound);
> @@ -806,7 +826,7 @@ EXPORT_SYMBOL_GPL(vmbus_open);
> /*
> * vmbus_teardown_gpadl -Teardown the specified GPADL handle
> */
> -int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
> +int vmbus_teardown_gpadl(struct vmbus_channel *channel, struct vmbus_gpadl *gpadl)
> {
> struct vmbus_channel_gpadl_teardown *msg;
> struct vmbus_channel_msginfo *info;
> @@ -825,7 +845,7 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
>
> msg->header.msgtype = CHANNELMSG_GPADL_TEARDOWN;
> msg->child_relid = channel->offermsg.child_relid;
> - msg->gpadl = gpadl_handle;
> + msg->gpadl = gpadl->gpadl_handle;
>
> spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
> list_add_tail(&info->msglistentry,
> @@ -859,6 +879,12 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
> spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
>
> kfree(info);
> +
> + ret = set_memory_encrypted((unsigned long)gpadl->buffer,
> + HVPFN_UP(gpadl->size));

PFN_UP here as well.

> + if (ret)
> + pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret);
> +
> return ret;
> }
> EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl);
> @@ -896,6 +922,7 @@ void vmbus_reset_channel_cb(struct vmbus_channel *channel)
> static int vmbus_close_internal(struct vmbus_channel *channel)
> {
> struct vmbus_channel_close_channel *msg;
> + struct vmbus_gpadl gpadl;

I think this local variable was needed in a previous version of the patch, but
is now unused and should be deleted.

> int ret;
>
> vmbus_reset_channel_cb(channel);
> @@ -933,9 +960,8 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
> }
>
> /* Tear down the gpadl for the channel's ring buffer */
> - else if (channel->ringbuffer_gpadlhandle) {
> - ret = vmbus_teardown_gpadl(channel,
> - channel->ringbuffer_gpadlhandle);
> + else if (channel->ringbuffer_gpadlhandle.gpadl_handle) {
> + ret = vmbus_teardown_gpadl(channel, &channel->ringbuffer_gpadlhandle);
> if (ret) {
> pr_err("Close failed: teardown gpadl return %d\n", ret);
> /*
> @@ -944,7 +970,7 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
> */
> }
>
> - channel->ringbuffer_gpadlhandle = 0;
> + channel->ringbuffer_gpadlhandle.gpadl_handle = 0;
> }
>
> if (!ret)
> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
> index bc48855dff10..315278a7cf88 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -1075,14 +1075,15 @@ struct netvsc_device {
> /* Receive buffer allocated by us but manages by NetVSP */
> void *recv_buf;
> u32 recv_buf_size; /* allocated bytes */
> - u32 recv_buf_gpadl_handle;
> + struct vmbus_gpadl recv_buf_gpadl_handle;
> u32 recv_section_cnt;
> u32 recv_section_size;
> u32 recv_completion_cnt;
>
> /* Send buffer allocated by us */
> void *send_buf;
> - u32 send_buf_gpadl_handle;
> + u32 send_buf_size;
> + struct vmbus_gpadl send_buf_gpadl_handle;
> u32 send_section_cnt;
> u32 send_section_size;
> unsigned long *send_section_map;
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index 7bd935412853..1f87e570ed2b 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -278,9 +278,9 @@ static void netvsc_teardown_recv_gpadl(struct hv_device *device,
> {
> int ret;
>
> - if (net_device->recv_buf_gpadl_handle) {
> + if (net_device->recv_buf_gpadl_handle.gpadl_handle) {
> ret = vmbus_teardown_gpadl(device->channel,
> - net_device->recv_buf_gpadl_handle);
> + &net_device->recv_buf_gpadl_handle);
>
> /* If we failed here, we might as well return and have a leak
> * rather than continue and a bugchk
> @@ -290,7 +290,7 @@ static void netvsc_teardown_recv_gpadl(struct hv_device *device,
> "unable to teardown receive buffer's gpadl\n");
> return;
> }
> - net_device->recv_buf_gpadl_handle = 0;
> + net_device->recv_buf_gpadl_handle.gpadl_handle = 0;
> }
> }
>
> @@ -300,9 +300,9 @@ static void netvsc_teardown_send_gpadl(struct hv_device *device,
> {
> int ret;
>
> - if (net_device->send_buf_gpadl_handle) {
> + if (net_device->send_buf_gpadl_handle.gpadl_handle) {
> ret = vmbus_teardown_gpadl(device->channel,
> - net_device->send_buf_gpadl_handle);
> + &net_device->send_buf_gpadl_handle);
>
> /* If we failed here, we might as well return and have a leak
> * rather than continue and a bugchk
> @@ -312,7 +312,7 @@ static void netvsc_teardown_send_gpadl(struct hv_device *device,
> "unable to teardown send buffer's gpadl\n");
> return;
> }
> - net_device->send_buf_gpadl_handle = 0;
> + net_device->send_buf_gpadl_handle.gpadl_handle = 0;
> }
> }
>
> @@ -380,7 +380,7 @@ static int netvsc_init_buf(struct hv_device *device,
> memset(init_packet, 0, sizeof(struct nvsp_message));
> init_packet->hdr.msg_type = NVSP_MSG1_TYPE_SEND_RECV_BUF;
> init_packet->msg.v1_msg.send_recv_buf.
> - gpadl_handle = net_device->recv_buf_gpadl_handle;
> + gpadl_handle = net_device->recv_buf_gpadl_handle.gpadl_handle;
> init_packet->msg.v1_msg.
> send_recv_buf.id = NETVSC_RECEIVE_BUFFER_ID;
>
> @@ -463,6 +463,7 @@ static int netvsc_init_buf(struct hv_device *device,
> ret = -ENOMEM;
> goto cleanup;
> }
> + net_device->send_buf_size = buf_size;
>
> /* Establish the gpadl handle for this buffer on this
> * channel. Note: This call uses the vmbus connection rather
> @@ -482,7 +483,7 @@ static int netvsc_init_buf(struct hv_device *device,
> memset(init_packet, 0, sizeof(struct nvsp_message));
> init_packet->hdr.msg_type = NVSP_MSG1_TYPE_SEND_SEND_BUF;
> init_packet->msg.v1_msg.send_send_buf.gpadl_handle =
> - net_device->send_buf_gpadl_handle;
> + net_device->send_buf_gpadl_handle.gpadl_handle;
> init_packet->msg.v1_msg.send_send_buf.id = NETVSC_SEND_BUFFER_ID;
>
> trace_nvsp_send(ndev, init_packet);
> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> index 652fe2547587..548243dcd895 100644
> --- a/drivers/uio/uio_hv_generic.c
> +++ b/drivers/uio/uio_hv_generic.c
> @@ -58,11 +58,11 @@ struct hv_uio_private_data {
> atomic_t refcnt;
>
> void *recv_buf;
> - u32 recv_gpadl;
> + struct vmbus_gpadl recv_gpadl;
> char recv_name[32]; /* "recv_4294967295" */
>
> void *send_buf;
> - u32 send_gpadl;
> + struct vmbus_gpadl send_gpadl;
> char send_name[32];
> };
>
> @@ -179,15 +179,15 @@ hv_uio_new_channel(struct vmbus_channel *new_sc)
> static void
> hv_uio_cleanup(struct hv_device *dev, struct hv_uio_private_data *pdata)
> {
> - if (pdata->send_gpadl) {
> - vmbus_teardown_gpadl(dev->channel, pdata->send_gpadl);
> - pdata->send_gpadl = 0;
> + if (pdata->send_gpadl.gpadl_handle) {
> + vmbus_teardown_gpadl(dev->channel, &pdata->send_gpadl);
> + pdata->send_gpadl.gpadl_handle = 0;
> vfree(pdata->send_buf);
> }
>
> - if (pdata->recv_gpadl) {
> - vmbus_teardown_gpadl(dev->channel, pdata->recv_gpadl);
> - pdata->recv_gpadl = 0;
> + if (pdata->recv_gpadl.gpadl_handle) {
> + vmbus_teardown_gpadl(dev->channel, &pdata->recv_gpadl);
> + pdata->recv_gpadl.gpadl_handle = 0;
> vfree(pdata->recv_buf);
> }
> }
> @@ -303,7 +303,7 @@ hv_uio_probe(struct hv_device *dev,
>
> /* put Global Physical Address Label in name */
> snprintf(pdata->recv_name, sizeof(pdata->recv_name),
> - "recv:%u", pdata->recv_gpadl);
> + "recv:%u", pdata->recv_gpadl.gpadl_handle);
> pdata->info.mem[RECV_BUF_MAP].name = pdata->recv_name;
> pdata->info.mem[RECV_BUF_MAP].addr
> = (uintptr_t)pdata->recv_buf;
> @@ -324,7 +324,7 @@ hv_uio_probe(struct hv_device *dev,
> }
>
> snprintf(pdata->send_name, sizeof(pdata->send_name),
> - "send:%u", pdata->send_gpadl);
> + "send:%u", pdata->send_gpadl.gpadl_handle);
> pdata->info.mem[SEND_BUF_MAP].name = pdata->send_name;
> pdata->info.mem[SEND_BUF_MAP].addr
> = (uintptr_t)pdata->send_buf;
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index ddc8713ce57b..a9e0bc3b1511 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -803,6 +803,12 @@ struct vmbus_device {
>
> #define VMBUS_DEFAULT_MAX_PKT_SIZE 4096
>
> +struct vmbus_gpadl {
> + u32 gpadl_handle;
> + u32 size;
> + void *buffer;
> +};
> +
> struct vmbus_channel {
> struct list_head listentry;
>
> @@ -822,7 +828,7 @@ struct vmbus_channel {
> bool rescind_ref; /* got rescind msg, got channel reference */
> struct completion rescind_event;
>
> - u32 ringbuffer_gpadlhandle;
> + struct vmbus_gpadl ringbuffer_gpadlhandle;
>
> /* Allocated memory for ring buffer */
> struct page *ringbuffer_page;
> @@ -1192,10 +1198,10 @@ extern int vmbus_sendpacket_mpb_desc(struct vmbus_channel *channel,
> extern int vmbus_establish_gpadl(struct vmbus_channel *channel,
> void *kbuffer,
> u32 size,
> - u32 *gpadl_handle);
> + struct vmbus_gpadl *gpadl);
>
> extern int vmbus_teardown_gpadl(struct vmbus_channel *channel,
> - u32 gpadl_handle);
> + struct vmbus_gpadl *gpadl);
>
> void vmbus_reset_channel_cb(struct vmbus_channel *channel);
>
> --
> 2.25.1

Next message: Michael Kelley: "RE: [PATCH V5 05/12] x86/hyperv: Add Write/Read MSR registers via ghcb page"
Previous message: Paul E. McKenney: "Re: [PATCH] kernel: in_irq() cleanup"
In reply to: Tianyu Lan: "[PATCH V5 04/12] Drivers: hv: vmbus: Mark vmbus ring buffer visible to host in Isolation VM"
Next in thread: Tianyu Lan: "[PATCH V5 06/12] x86/hyperv: Add ghcb hvcall support for SNP VM"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]