Re: [RFC PATCH 1/12] x86/Hyper-V: Add visibility parameter for vmbus_establish_gpadl()
From: Vitaly Kuznetsov <vkuznets@redhat.com>
Date: 2021-03-03 19:18:37
Also in:
lkml, netdev
Tianyu Lan [off-list ref] writes:
From: Tianyu Lan <redacted> Add visibility parameter for vmbus_establish_gpadl() and prepare to change host visibility when create gpadl for buffer.
"No functional change" as you don't actually use the parameter.
Signed-off-by: Sunil Muthuswamy <redacted> Co-Developed-by: Sunil Muthuswamy <redacted> Signed-off-by: Tianyu Lan <redacted>
Nit: Sunil's SoB looks misleading because the patch is from you, Co-Developed-by should be sufficient.
quoted hunk ↗ jump to hunk
--- arch/x86/include/asm/hyperv-tlfs.h | 9 +++++++++ drivers/hv/channel.c | 20 +++++++++++--------- drivers/net/hyperv/netvsc.c | 8 ++++++-- drivers/uio/uio_hv_generic.c | 7 +++++-- include/linux/hyperv.h | 3 ++- 5 files changed, 33 insertions(+), 14 deletions(-)diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h index e6cd3fee562b..fb1893a4c32b 100644 --- a/arch/x86/include/asm/hyperv-tlfs.h +++ b/arch/x86/include/asm/hyperv-tlfs.h@@ -236,6 +236,15 @@ enum hv_isolation_type { /* TSC invariant control */ #define HV_X64_MSR_TSC_INVARIANT_CONTROL 0x40000118 +/* Hyper-V GPA map flags */ +#define HV_MAP_GPA_PERMISSIONS_NONE 0x0 +#define HV_MAP_GPA_READABLE 0x1 +#define HV_MAP_GPA_WRITABLE 0x2 + +#define VMBUS_PAGE_VISIBLE_READ_ONLY HV_MAP_GPA_READABLE +#define VMBUS_PAGE_VISIBLE_READ_WRITE (HV_MAP_GPA_READABLE|HV_MAP_GPA_WRITABLE) +#define VMBUS_PAGE_NOT_VISIBLE HV_MAP_GPA_PERMISSIONS_NONE +
Are these x86-only? If not, then we should probably move these defines to include/asm-generic/hyperv-tlfs.h. In case they are, we should do something as we're using them from arch neutral places. Also, could you please add a comment stating that these flags define host's visibility of a page and not guest's (this seems to be not obvious at least to me).
quoted hunk ↗ jump to hunk
/* * Declare the MSR used to setup pages used to communicate with the hypervisor. */diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index 0bd202de7960..daa21cc72beb 100644 --- a/drivers/hv/channel.c +++ b/drivers/hv/channel.c@@ -242,7 +242,7 @@ EXPORT_SYMBOL_GPL(vmbus_send_modifychannel); */ static int create_gpadl_header(enum hv_gpadl_type type, void *kbuffer, u32 size, u32 send_offset, - struct vmbus_channel_msginfo **msginfo) + struct vmbus_channel_msginfo **msginfo, u32 visibility) { int i; int pagecount;@@ -391,7 +391,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) + u32 *gpadl_handle, u32 visibility) { struct vmbus_channel_gpadl_header *gpadlmsg; struct vmbus_channel_gpadl_body *gpadl_body;@@ -405,7 +405,8 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel, next_gpadl_handle = (atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1); - ret = create_gpadl_header(type, kbuffer, size, send_offset, &msginfo); + ret = create_gpadl_header(type, kbuffer, size, send_offset, + &msginfo, visibility); if (ret) return ret;@@ -496,10 +497,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, u32 *gpadl_handle, u32 visibility) { return __vmbus_establish_gpadl(channel, HV_GPADL_BUFFER, kbuffer, size, - 0U, gpadl_handle); + 0U, gpadl_handle, visibility); } EXPORT_SYMBOL_GPL(vmbus_establish_gpadl);@@ -610,10 +611,11 @@ static int __vmbus_open(struct vmbus_channel *newchannel, newchannel->ringbuffer_gpadlhandle = 0; err = __vmbus_establish_gpadl(newchannel, HV_GPADL_RING, - page_address(newchannel->ringbuffer_page), - (send_pages + recv_pages) << PAGE_SHIFT, - newchannel->ringbuffer_send_offset << PAGE_SHIFT, - &newchannel->ringbuffer_gpadlhandle); + page_address(newchannel->ringbuffer_page), + (send_pages + recv_pages) << PAGE_SHIFT, + newchannel->ringbuffer_send_offset << PAGE_SHIFT, + &newchannel->ringbuffer_gpadlhandle, + VMBUS_PAGE_VISIBLE_READ_WRITE);
Nit: I liked the original alignment more and we can avoid the unneeded code churn.
quoted hunk ↗ jump to hunk
if (err) goto error_clean_ring;diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index 2353623259f3..bb72c7578330 100644 --- a/drivers/net/hyperv/netvsc.c +++ b/drivers/net/hyperv/netvsc.c@@ -333,7 +333,8 @@ static int netvsc_init_buf(struct hv_device *device, */ ret = vmbus_establish_gpadl(device->channel, net_device->recv_buf, buf_size, - &net_device->recv_buf_gpadl_handle); + &net_device->recv_buf_gpadl_handle, + VMBUS_PAGE_VISIBLE_READ_WRITE); if (ret != 0) { netdev_err(ndev, "unable to establish receive buffer's gpadl\n");@@ -422,10 +423,13 @@ static int netvsc_init_buf(struct hv_device *device, /* Establish the gpadl handle for this buffer on this * channel. Note: This call uses the vmbus connection rather * than the channel to establish the gpadl handle. + * Send buffer should theoretically be only marked as "read-only", but + * the netvsp for some reason needs write capabilities on it. */ ret = vmbus_establish_gpadl(device->channel, net_device->send_buf, buf_size, - &net_device->send_buf_gpadl_handle); + &net_device->send_buf_gpadl_handle, + VMBUS_PAGE_VISIBLE_READ_WRITE); if (ret != 0) { netdev_err(ndev, "unable to establish send buffer's gpadl\n");diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c index 0330ba99730e..813a7bee5139 100644 --- a/drivers/uio/uio_hv_generic.c +++ b/drivers/uio/uio_hv_generic.c@@ -29,6 +29,7 @@ #include <linux/hyperv.h> #include <linux/vmalloc.h> #include <linux/slab.h> +#include <asm/mshyperv.h> #include "../hv/hyperv_vmbus.h"@@ -295,7 +296,8 @@ hv_uio_probe(struct hv_device *dev, } ret = vmbus_establish_gpadl(channel, pdata->recv_buf, - RECV_BUFFER_SIZE, &pdata->recv_gpadl); + RECV_BUFFER_SIZE, &pdata->recv_gpadl, + VMBUS_PAGE_VISIBLE_READ_WRITE); if (ret) goto fail_close;@@ -315,7 +317,8 @@ hv_uio_probe(struct hv_device *dev, } ret = vmbus_establish_gpadl(channel, pdata->send_buf, - SEND_BUFFER_SIZE, &pdata->send_gpadl); + SEND_BUFFER_SIZE, &pdata->send_gpadl, + VMBUS_PAGE_VISIBLE_READ_ONLY);
Actually, this is the only place where you use 'READ_ONLY' mapping -- which makes me wonder if it's actually worth it or we can hard-code VMBUS_PAGE_VISIBLE_READ_WRITE for now and avoid this additional parameter.
quoted hunk ↗ jump to hunk
if (ret) goto fail_close;diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index f1d74dcf0353..016fdca20d6e 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h@@ -1179,7 +1179,8 @@ 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); + u32 *gpadl_handle, + u32 visibility); extern int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle);
-- Vitaly