Drivers: hv: vmbus: enable VMBus protocol version 5.0
PATCH 1/1 Drivers: hv: vmbus: enable VMBus protocol version 5.0
From: Dexuan Cui <[email protected]>
With VMBus protocol 5.0, we're able to better support new features, e.g. running two or more VMBus drivers simultaneously in a single VM -- note:
we can't simply load the current VMBus driver twice, instead, a secondary VMBus driver must be implemented.
This patch adds the support for the new VMBus protocol, which is available on new Windows hosts, by:
1) We still use SINT2 for compatibility;
2) We must use Connection ID 4 for the Initiate Contact Message, and for subsequent messages, we must use the Message Connection ID field in the host-returned VersionResponse Message.
Notes for developers of the secondary VMBus driver:
1) Must use VMBus protocol 5.0 as well;
2) Must use a different SINT number that is not in use.
3) Must use Connection ID 4 for the Initiate Contact Message, and for subsequent messages, must use the Message Connection ID field in the host-returned VersionResponse Message.
4) It's possible that the primary VMBus driver using protocol version 4.0 can work with a secondary VMBus driver using protocol version 5.0, but it's recommended that both should use 5.0 for new Hyper-V features in the future.
--- drivers/hv/connection.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- drivers/hv/hyperv_vmbus.h | 3 +++ include/linux/hyperv.h | 26 ++++++++++++++++++++++++-- 3 files changed, 69 insertions(+), 4 deletions(-)
drivers/hv/connection.c +42 -2
@@ -63,6 +63,9 @@ static __u32 vmbus_get_next_version(__u32 current_version)case (VERSION_WIN10 ):return VERSION_WIN8_1 ; +case (VERSION_WIN10_V5 ): +return VERSION_WIN10 ; +case (VERSION_WS2008 ):default :return VERSION_INVAL ;
@@ -80,9 +83,29 @@ static int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, msg = (struct vmbus_channel_initiate_contact *)msginfo->msg; +memset (msg,0 ,sizeof (*msg)); msg->header.msgtype =CHANNELMSG_INITIATE_CONTACT ; msg->vmbus_version_requested = version; - msg->interrupt_page =virt_to_phys (vmbus_connection.int_page); + +/* +* VMBus protocol 5.0 (VERSION_WIN10_V5) requires that we must use +* VMBUS_MESSAGE_CONNECTION_ID_4 for the Initiate Contact Message, +* and for subsequent messages, we must use the Message Connection ID +* field in the host-returned Version Response Message. And, with +* VERSION_WIN10_V5, we don't use msg->interrupt_page, but we tell +* the host explicitly that we still use VMBUS_MESSAGE_SINT(2) for +* compatibility. +* +* On old hosts, we should always use VMBUS_MESSAGE_CONNECTION_ID (1). +*/ +if (version >=VERSION_WIN10_V5 ) { + msg->msg_sint =VMBUS_MESSAGE_SINT ; + vmbus_connection.msg_conn_id =VMBUS_MESSAGE_CONNECTION_ID_4 ; + }else { + msg->interrupt_page =virt_to_phys (vmbus_connection.int_page); + 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 ]);/*
@@ -137,6 +160,10 @@ static int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo,/* Check if successful */ if (msginfo->response.version_response.version_supported) { vmbus_connection.conn_state =CONNECTED ; + +if (version >=VERSION_WIN10_V5 ) + vmbus_connection.msg_conn_id = + msginfo->response.version_response.msg_conn_id; }else {return -ECONNREFUSED ; }
@@ -354,13 +381,14 @@ void vmbus_on_event(unsigned long data)*/ int vmbus_post_msg (void *buffer,size_t buflen,bool can_sleep) { +struct vmbus_channel_message_header *hdr;union hv_connection_id conn_id;int ret =0 ;int retries =0 ;u32 usec =1 ; conn_id.asu32 =0 ; - conn_id.u.id =VMBUS_MESSAGE_CONNECTION_ID ; + conn_id.u.id = vmbus_connection.msg_conn_id;/* * hv_post_message() can have transient failures because of
@@ -372,6 +400,18 @@ int vmbus_post_msg(void *buffer, size_t buflen, bool can_sleep)switch (ret) {case HV_STATUS_INVALID_CONNECTION_ID : +/* +* See vmbus_negotiate_version(): VMBus protocol 5.0 +* requires that we must use +* VMBUS_MESSAGE_CONNECTION_ID_4 for the Initiate +* Contact message, but on old hosts that only +* support VMBus protocol 4.0 or lower, here we get +* HV_STATUS_INVALID_CONNECTION_ID and we should +* return an error immediately without retrying. +*/ + hdr = (struct vmbus_channel_message_header *)buffer; +if (hdr->msgtype ==CHANNELMSG_INITIATE_CONTACT ) +return -EINVAL ;/* * We could get this if we send messages too * frequently.
drivers/hv/hyperv_vmbus.h +3 -0
@@ -187,6 +187,7 @@ struct hv_input_post_message {enum {VMBUS_MESSAGE_CONNECTION_ID =1 , +VMBUS_MESSAGE_CONNECTION_ID_4 =4 ,VMBUS_MESSAGE_PORT_ID =1 ,VMBUS_EVENT_CONNECTION_ID =2 ,VMBUS_EVENT_PORT_ID =2 ,
@@ -302,6 +303,8 @@ struct vmbus_connection {*/ int connect_cpu; +u32 msg_conn_id; +atomic_t offer_in_progress;enum vmbus_connect_state conn_state;
include/linux/hyperv.h +24 -2
@@ -163,6 +163,7 @@ static inline u32 hv_get_bytes_to_write(const struct hv_ring_buffer_info *rbi)* 2 . 4 (Windows 8) * 3 . 0 (Windows 8 R2) * 4 . 0 (Windows 10) +* 5 . 0 (Newer Windows 10) */ #define VERSION_WS2008 ((0 << 16) | (13))
@@ -170,10 +171,11 @@ static inline u32 hv_get_bytes_to_write(const struct hv_ring_buffer_info *rbi)#define VERSION_WIN8 ((2 << 16) | (4))#define VERSION_WIN8_1 ((3 << 16) | (0))#define VERSION_WIN10 ((4 << 16) | (0)) +#define VERSION_WIN10_V5 ((5 << 16) | (0))#define VERSION_INVAL -1 -#define VERSION_CURRENT VERSION_WIN10 +#define VERSION_CURRENT VERSION_WIN10_V5/* Make maximum size of pipe payload of 16K */ #define MAX_PIPE_DATA_PAYLOAD (sizeof(u8) * 16384)
@@ -570,7 +572,14 @@ struct vmbus_channel_initiate_contact {struct vmbus_channel_message_header header;u32 vmbus_version_requested;u32 target_vcpu;/* The VCPU the host should respond to */ -u64 interrupt_page; +union { +u64 interrupt_page; +struct { +u8 msg_sint; +u8 padding1[3 ]; +u32 padding2; + }; + };u64 monitor_page1;u64 monitor_page2; } __packed;
@@ -585,6 +594,19 @@ struct vmbus_channel_tl_connect_request {struct vmbus_channel_version_response {struct vmbus_channel_message_header header;u8 version_supported; + +u8 connection_state; +u16 padding; + +/* +* On new hosts that support VMBus protocol 5.0, we must use +* VMBUS_MESSAGE_CONNECTION_ID_4 for the Initiate Contact Message, +* and for subsequent messages, we must use the Message Connection ID +* field in the host-returned Version Response Message. +* +* On old hosts, we should always use VMBUS_MESSAGE_CONNECTION_ID (1). +*/ +u32 msg_conn_id; } __packed;enum vmbus_channel_state {
Signature · 1 line
2.15.1
Quoted 18 lines · On Sat, 12 May 2018 02:30:33 -0700 [email protected] wrote:
int
vmbus_post_msg(void *buffer, size_t buflen, bool can_sleep){drivers/hv/connection.c +2
-1+struct vmbus_channel_message_header *hdr;union hv_connection_id conn_id;int ret =0 ;int retries =0 ;u32 usec =1 ; conn_id.asu32 =0 ; - conn_id.u.id =VMBUS_MESSAGE_CONNECTION_ID ; + conn_id.u.id = vmbus_connection.msg_conn_id;/* * hv_post_message() can have transient failures because of @@ -372,6 +400,18 @@ int vmbus_post_msg(void *buffer, size_t buflen, bool can_sleep)switch (ret) {case HV_STATUS_INVALID_CONNECTION_ID :
drivers/hv/connection.c int vmbus_post_msg(void *buffer, size_t buflen, bool can_sleep) · line 412
Quoted 10 lines
drivers/hv/connection.c +10
-0+/* +* See vmbus_negotiate_version(): VMBus protocol 5.0 +* requires that we must use +* VMBUS_MESSAGE_CONNECTION_ID_4 for the Initiate +* Contact message, but on old hosts that only +* support VMBus protocol 4.0 or lower, here we get +* HV_STATUS_INVALID_CONNECTION_ID and we should +* return an error immediately without retrying. +*/ + hdr = (struct vmbus_channel_message_header *)buffer;
Hate to pick o the details, but buffer is void * so cast is not necessary here.
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Dexuan Cui
maintainerQuoted 10 lines
From: devel <[email protected]> On Behalf Of Stephen Hemminger
Sent: Sunday, May 13, 2018 10:24Review int vmbus_post_msg(void *buffer, size_t buflen,
Quoted 2 lines
...
(quoted diff) +0
-0@@ -372,6 +400,18 @@ int vmbus_post_msg(void *buffer, size_t buflen,bool
can_sleep)drivers/hv/connection.c int vmbus_post_msg(void *buffer, size_t buflen, bool can_sleep) · line 412
Quoted 2 lines
...
drivers/hv/connection.c +1
-0+ hdr = (struct vmbus_channel_message_header *)buffer;Hate to pick o the details, but buffer is void * so cast is not necessary here.
Yes, it's unnecessary in C, though it's necessary in C++.
I found the patch went into char-misc 4 hours ago, so it looks we may as well leave it as is. IMHO an explicit cast is not a bad thing. :-)
Thanks,
-- Dexuan
Quoted 11 lines · On Mon, 14 May 2018 18:14:15 +0000 Dexuan Cui <[email protected]> wrote:
Quoted 10 lines
From: devel <[email protected]> On Behalf Of Stephen Hemminger
Sent: Sunday, May 13, 2018 10:24Review int vmbus_post_msg(void *buffer, size_t buflen,
Quoted 2 lines
...
(quoted diff) +0
-0@@ -372,6 +400,18 @@ int vmbus_post_msg(void *buffer, size_t buflen,bool
can_sleep)drivers/hv/connection.c int vmbus_post_msg(void *buffer, size_t buflen, bool can_sleep) · line 412
Quoted 2 lines
...
drivers/hv/connection.c +1
-0+ hdr = (struct vmbus_channel_message_header *)buffer;Hate to pick o the details, but buffer is void * so cast is not necessary here.
Quoted 7 lines
Yes, it's unnecessary in C, though it's necessary in C++.
I found the patch went into char-misc 4 hours ago, so it looks we may as well leave it as is. IMHO an explicit cast is not a bad thing. :-)
Thanks,
-- Dexuan
Kernel developers like to be concise. In fact there is a smatch script that perodically gets run and more cleanup patches get sent.
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Dexuan Cui
maintainerQuoted 8 lines
From: Stephen Hemminger <[email protected]>
Sent: Monday, May 14, 2018 11:18
To: Dexuan Cui <[email protected]>Quoted 5 lines
Quoted 2 lines
...
Hate to pick o the details, but buffer is void * so cast is not necessary here.Yes, it's unnecessary in C, though it's necessary in C++.
Quoted 8 lines
Quoted 5 lines
I found the patch went into char-misc 4 hours ago, so it looks we may as well leave it as is. IMHO an explicit cast is not a bad thing. :-)
Thanks,
-- DexuanKernel developers like to be concise. In fact there is a smatch script that perodically gets run and more cleanup patches get sent.
I checked the "git log" and confimed you're correct: there are a lot of patches that removed the cast from "void *". :-)
Then let me post a small patch for this.
-- Dexuan
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Quoted 16 lines · Stephen Hemminger
Quoted 13 lines · On Mon, 14 May 2018 18:14:15 +0000 Dexuan Cui <[email protected]> wrote:
Quoted 10 lines
From: devel <[email protected]> On Behalf Of Stephen Hemminger
Sent: Sunday, May 13, 2018 10:24Review int vmbus_post_msg(void *buffer, size_t buflen,
Quoted 2 lines
...
(quoted diff) +0
-0@@ -372,6 +400,18 @@ int vmbus_post_msg(void *buffer, size_t buflen,bool
can_sleep)drivers/hv/connection.c int vmbus_post_msg(void *buffer, size_t buflen, bool can_sleep) · line 412
Quoted 2 lines
...
drivers/hv/connection.c +1
-0+ hdr = (struct vmbus_channel_message_header *)buffer;Hate to pick o the details, but buffer is void * so cast is not necessary here.
Yes, it's unnecessary in C, though it's necessary in C++.
Quoted 8 lines
Quoted 5 lines
I found the patch went into char-misc 4 hours ago, so it looks we may as well leave it as is. IMHO an explicit cast is not a bad thing. :-)
Thanks,
-- DexuanKernel developers like to be concise. In fact there is a smatch script that perodically gets run and more cleanup patches get sent.
It's a Coccinelle script, not Smatch. Coccinelle generates patches automatically so it's a better tool for cleanup than Smatch.
I would generate a lot more Smatch information if there was a way to integrate it easily into a code editor. For example, we could highlight unecessary casts or pointer dereferences where Smatch wasn't 100% sure if it was correct. Or you could hover over function name to see what resources it allocates.
regards,
dan carpenter
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel