| Message ID | 20210809175620.720923-5-ltykernel@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | x86/Hyper-V: Add Hyper-V Isolation VM support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
From: Tianyu Lan <ltykernel@gmail.com> 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@microsoft.com> > --- > 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
On 8/13/2021 6:20 AM, Michael Kelley wrote: >> @@ -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. > >> @@ -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. > >> @@ -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 Yes, agree. Will update. Thanks.
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"); + 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; + 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"); + + channel->gpadl_array[i].gpadlhandle = 0; + atomic_dec(&channel->gpadl_index); + 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;