RE: [PATCH V3 04/13] HV: Mark vmbus ring buffer visible to host in Isolation VM

[Michael Kelley <mikelley@xxxxxxxxxxxxx>]

From: Tianyu Lan <ltykernel@xxxxxxxxx> Sent: Monday, August 9, 2021 10:56 AM
> Subject: [PATCH V3 04/13] HV: Mark vmbus ring buffer visible to host in 
> Isolation VM
> 

Use tag "Drivers: hv: vmbus:" in the Subject line.

> Mark vmbus ring buffer visible with set_memory_decrypted() when
> establish gpadl handle.
> 
> Signed-off-by: Tianyu Lan <Tianyu.Lan@xxxxxxxxxxxxx>
> ---
>  drivers/hv/channel.c   | 44 ++++++++++++++++++++++++++++++++++++++++--
>  include/linux/hyperv.h | 11 +++++++++++
>  2 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index f3761c73b074..4c4717c26240 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>
> 
> @@ -465,7 +466,14 @@ static int __vmbus_establish_gpadl(struct vmbus_channel 
> *channel,
>       struct list_head *curr;
>       u32 next_gpadl_handle;
>       unsigned long flags;
> -     int ret = 0;
> +     int ret = 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)
>               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.

> +             return ret;
> +     }
> +
>       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;
> +

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.

>  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((unsigned long)kbuffer,
> +                                  HVPFN_UP(size));
> +             atomic_dec(&channel->gpadl_index);
> +     }
> +
>       return ret;
>  }
> 
> @@ -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.

> +
> +     channel->gpadl_array[i].gpadlhandle = 0;
> +     atomic_dec(&channel->gpadl_index);

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.

> +
>       return ret;
>  }
>  EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl);
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index ddc8713ce57b..90b542597143 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -803,6 +803,14 @@ struct vmbus_device {
> 
>  #define VMBUS_DEFAULT_MAX_PKT_SIZE 4096
> 
> +struct vmbus_gpadl {
> +     u32 gpadlhandle;
> +     u32 size;
> +     void *buffer;
> +};
> +
> +#define VMBUS_GPADL_RANGE_COUNT              3
> +
>  struct vmbus_channel {
>       struct list_head listentry;
> 
> @@ -823,6 +831,9 @@ struct vmbus_channel {
>       struct completion rescind_event;
> 
>       u32 ringbuffer_gpadlhandle;
> +     /* GPADL_RING and Send/Receive GPADL_BUFFER. */
> +     struct vmbus_gpadl gpadl_array[VMBUS_GPADL_RANGE_COUNT];
> +     atomic_t gpadl_index;
> 
>       /* Allocated memory for ring buffer */
>       struct page *ringbuffer_page;
> --
> 2.25.1