Drivers: hv: vmbus: Fix mmio management
Subsystems (2)
cover Drivers: hv: vmbus: Fix mmio management
Fix mmio management for vmbus devices. Also included is a patch to fix an issue in KVP.
Jake Oshins (5):
hv: Make a function to free mmio regions through vmbus
hv: Lock access to hyperv_mmio resource tree
hv: Use new vmbus_mmio_free() from client drivers.
hv: Reverse order of resources in hyperv_mmio
hv: Track allocations of children of hv_vmbus in private resource
tree
Vitaly Kuznetsov (1):
Drivers: hv: kvp: fix IP Failover
drivers/hv/hv_kvp.c | 30 +++++++++++++++++++++ drivers/hv/hyperv_vmbus.h | 5 +++ drivers/hv/vmbus_drv.c | 56 ++++++++++++++++++++++++++++++++++----- drivers/video/fbdev/hyperv_fb.c | 4 +- include/linux/hyperv.h | 2 +- 5 files changed, 87 insertions(+), 10 deletions(-)
Signature · 1 line
1.7.4.1
PATCH 1/6 Drivers: hv: kvp: fix IP Failover
From: Vitaly Kuznetsov <[email protected]>
Hyper-V VMs can be replicated to another hosts and there is a feature to set different IP for replicas, it is called 'Failover TCP/IP'. When such guest starts Hyper-V host sends it KVP_OP_SET_IP_INFO message as soon as we finish negotiation procedure. The problem is that it can happen (and it actually happens) before userspace daemon connects and we reply with HV_E_FAIL to the message. As there are no repetitions we fail to set the requested IP.
Solve the issue by postponing our reply to the negotiation message till userspace daemon is connected. We can't wait too long as there is a host-side timeout (cca. 75 seconds) and if we fail to reply in this time frame the whole KVP service will become inactive. The solution is not ideal - if it takes userspace daemon more than 60 seconds to connect IP Failover will still fail but I don't see a solution with our current separation between kernel and userspace parts.
Other two modules (VSS and FCOPY) don't require such delay, leave them untouched.
--- drivers/hv/hv_kvp.c | 30 ++++++++++++++++++++++++++++++ drivers/hv/hyperv_vmbus.h | 5 +++++ 2 files changed, 35 insertions(+), 0 deletions(-)
drivers/hv/hv_kvp.c +30 -0
@@ -78,9 +78,11 @@ static void kvp_send_key(struct work_struct *dummy);static void kvp_respond_to_host (struct hv_kvp_msg *msg,int error);static void kvp_timeout_func (struct work_struct *dummy); +static void kvp_host_handshake_func (struct work_struct *dummy);static void kvp_register (int ); static DECLARE_DELAYED_WORK(kvp_timeout_work, kvp_timeout_func); +static DECLARE_DELAYED_WORK(kvp_host_handshake_work, kvp_host_handshake_func); static DECLARE_WORK(kvp_sendkey_work, kvp_send_key);static const char kvp_devname[] ="vmbus/hv_kvp" ;
@@ -130,6 +132,11 @@ static void kvp_timeout_func(struct work_struct *dummy)hv_poll_channel (kvp_transaction.recv_channel, kvp_poll_wrapper); } +static void kvp_host_handshake_func (struct work_struct *dummy) +{ +hv_poll_channel (kvp_transaction.recv_channel, hv_kvp_onchannelcallback); +} +static int kvp_handle_handshake (struct hv_kvp_msg *msg) {switch (msg->kvp_hdr.operation) {
@@ -154,6 +161,12 @@ static int kvp_handle_handshake(struct hv_kvp_msg *msg)pr_debug ("KVP: userspace daemon ver. %d registered\n" ,KVP_OP_REGISTER );kvp_register (dm_reg_value); + +/* +* If we're still negotiating with the host cancel the timeout +* work to not poll the channel twice. +*/ +cancel_delayed_work_sync (&kvp_host_handshake_work);hv_poll_channel (kvp_transaction.recv_channel, kvp_poll_wrapper);return 0 ;
@@ -594,7 +607,22 @@ void hv_kvp_onchannelcallback(void *context)struct icmsg_negotiate *negop =NULL ;int util_fw_version;int kvp_srv_version; +static enum {NEGO_NOT_STARTED , +NEGO_IN_PROGRESS , +NEGO_FINISHED } host_negotiatied =NEGO_NOT_STARTED ; +if (host_negotiatied ==NEGO_NOT_STARTED && + kvp_transaction.state <HVUTIL_READY ) { +/* +* If userspace daemon is not connected and host is asking +* us to negotiate we need to delay to not lose messages. +* This is important for Failover IP setting. +*/ + host_negotiatied =NEGO_IN_PROGRESS ; +schedule_delayed_work (&kvp_host_handshake_work, +HV_UTIL_NEGO_TIMEOUT *HZ ); +return ; + }if (kvp_transaction.state >HVUTIL_READY )return ;
@@ -672,6 +700,8 @@ void hv_kvp_onchannelcallback(void *context)vmbus_sendpacket (channel, recv_buffer, recvlen, requestid,VM_PKT_DATA_INBAND ,0 ); + + host_negotiatied =NEGO_FINISHED ; } }
drivers/hv/hyperv_vmbus.h +5 -0
@@ -36,6 +36,11 @@#define HV_UTIL_TIMEOUT 30/* +* Timeout for guest-host handshake for services. +*/ +#define HV_UTIL_NEGO_TIMEOUT 60 + +/* * The below CPUID leaves are present if VersionAndFeatures.HypervisorPresent * is set by CPUID(HVCPUID_VERSION_FEATURES). */
Signature · 1 line
1.7.4.1
PATCH 2/6 hv: Make a function to free mmio regions through vmbus
From: Jake Oshins <[email protected]>
This patch introduces a function that reverses everything done by vmbus_allocate_mmio(). Existing code just called release_mem_region(). Future patches in this series require a more complex sequence of actions, so this function is introduced to wrap those actions.
--- drivers/hv/vmbus_drv.c | 15 +++++++++++++++ include/linux/hyperv.h | 2 +- 2 files changed, 16 insertions(+), 1 deletions(-)
drivers/hv/vmbus_drv.c +15 -0
@@ -1180,6 +1180,21 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,EXPORT_SYMBOL_GPL (vmbus_allocate_mmio);/** +* vmbus_free_mmio() - Free a memory-mapped I/O range. +* @start: Base address of region to release. +* @size: Size of the range to be allocated +* +* This function releases anything requested by +* vmbus_mmio_allocate(). +*/ +void vmbus_free_mmio (resource_size_t start,resource_size_t size) +{ +release_mem_region (start, size); + +} +EXPORT_SYMBOL_GPL (vmbus_free_mmio); + +/** * vmbus_cpu_number_to_vp_number() - Map CPU to VP. * @cpu_number: CPU number in Linux terms *
include/linux/hyperv.h +1 -1
@@ -1091,7 +1091,7 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj, resource_size_t min, resource_size_t max, resource_size_t size, resource_size_t align, bool fb_overlap_ok); - +void vmbus_free_mmio (resource_size_t start,resource_size_t size);int vmbus_cpu_number_to_vp_number (int cpu_number);u64 hv_do_hypercall (u64 control,void *input,void *output);
Signature · 1 line
1.7.4.1
PATCH 3/6 hv: Lock access to hyperv_mmio resource tree
In reply to K. Y. SrinivasanFrom: Jake Oshins <[email protected]>
In existing code, this tree of resources is created in single-threaded code and never modified after it is created, and thus needs no locking. This patch introduces a semaphore for tree access, as other patches in this series introduce run-time modifications of this resource tree which can happen on multiple threads.
--- drivers/hv/vmbus_drv.c | 16 ++++++++++++---- 1 files changed, 12 insertions(+), 4 deletions(-)
drivers/hv/vmbus_drv.c +12 -4
@@ -102,6 +102,7 @@ static struct notifier_block hyperv_panic_block = { };struct resource *hyperv_mmio; +DEFINE_SEMAPHORE (hyperv_mmio_lock);static int vmbus_exists (void ) {
@@ -1132,7 +1133,10 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,resource_size_t range_min, range_max, start, local_min, local_max;const char *dev_n =dev_name (&device_obj->device);u32 fb_end = screen_info.lfb_base + (screen_info.lfb_size <<1 ); -int i; +int i, retval; + + retval = -ENXIO ; +down (&hyperv_mmio_lock);for (iter = hyperv_mmio; iter; iter = iter->sibling) {if ((iter->start >= max) || (iter->end <= min))
@@ -1169,13 +1173,17 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj,for (; start + size -1 <= local_max; start += align) { *new =request_mem_region_exclusive (start, size, dev_n); -if (*new) -return 0 ; +if (*new) { + retval =0 ; + goto exit; + } } } } -return -ENXIO ; +exit: +up (&hyperv_mmio_lock); +return retval; }EXPORT_SYMBOL_GPL (vmbus_allocate_mmio);
Signature · 1 line
1.7.4.1
PATCH 4/6 hv: Use new vmbus_mmio_free() from client drivers.
In reply to K. Y. SrinivasanFrom: Jake Oshins <[email protected]>
This patch modifies all the callers of vmbus_mmio_allocate() to call vmbus_mmio_free() instead of release_mem_region().
--- drivers/video/fbdev/hyperv_fb.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/video/fbdev/hyperv_fb.c +2 -2
@@ -743,7 +743,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info) err3:iounmap (fb_virt); err2: -release_mem_region (par->mem->start, screen_fb_size); +vmbus_free_mmio (par->mem->start, screen_fb_size); par->mem =NULL ; err1:if (!gen2vm)
@@ -758,7 +758,7 @@ static void hvfb_putmem(struct fb_info *info)struct hvfb_par *par = info->par;iounmap (info->screen_base); -release_mem_region (par->mem->start, screen_fb_size); +vmbus_free_mmio (par->mem->start, screen_fb_size); par->mem =NULL ; }
Signature · 1 line
1.7.4.1
PATCH 5/6 hv: Reverse order of resources in hyperv_mmio
In reply to K. Y. SrinivasanFrom: Jake Oshins <[email protected]>
A patch later in this series allocates child nodes in this resource tree. For that to work, this tree needs to be sorted in ascending order.
--- drivers/hv/vmbus_drv.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
drivers/hv/vmbus_drv.c +1 -2
@@ -1049,7 +1049,6 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx) new_res->end = end;/* -* Stick ranges from higher in address space at the front of the list. * If two ranges are adjacent, merge them. */ do {
@@ -1070,7 +1069,7 @@ static acpi_status vmbus_walk_resources(struct acpi_resource *res, void *ctx)break ; } -if ((*old_res)->end < new_res->start) { +if ((*old_res)->start > new_res->end) { new_res->sibling = *old_res;if (prev_res) (*prev_res)->sibling = new_res;
Signature · 1 line
1.7.4.1
PATCH 6/6 hv: Track allocations of children of hv_vmbus in private resource tree
In reply to K. Y. SrinivasanFrom: Jake Oshins <[email protected]>
This patch changes vmbus_allocate_mmio() and vmbus_free_mmio() so that when child paravirtual devices allocate memory-mapped I/O space, they allocate it privately from a resource tree pointed at by hyperv_mmio and also by the public resource tree iomem_resource. This allows the region to be marked as "busy" in the private tree, but a "bridge window" in the public tree, guaranteeing that no two bridge windows will overlap each other but while also allowing the PCI device children of the bridge windows to overlap that window.
One might conclude that this belongs in the pnp layer, rather than in this driver. Rafael Wysocki, the maintainter of the pnp layer, has previously asked that we not modify the pnp layer as it is considered deprecated. This patch is thus essentially a workaround.
--- drivers/hv/vmbus_drv.c | 22 +++++++++++++++++++++- 1 files changed, 21 insertions(+), 1 deletions(-)
drivers/hv/vmbus_drv.c +21 -1
@@ -1128,7 +1128,7 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj, resource_size_t size, resource_size_t align, bool fb_overlap_ok) { - struct resource *iter; + struct resource *iter, *shadow;resource_size_t range_min, range_max, start, local_min, local_max;const char *dev_n =dev_name (&device_obj->device);u32 fb_end = screen_info.lfb_base + (screen_info.lfb_size <<1 );
@@ -1170,12 +1170,22 @@ int vmbus_allocate_mmio(struct resource **new, struct hv_device *device_obj, start = (local_min + align -1 ) & ~(align -1 );for (; start + size -1 <= local_max; start += align) { + shadow =__request_region (iter, start, + size, +NULL , +IORESOURCE_BUSY ); +if (!shadow) +continue ; + *new =request_mem_region_exclusive (start, size, dev_n);if (*new) { + shadow->name = (char *)*new; retval =0 ; goto exit; } + +__release_region (iter, start, size); } } }
@@ -1196,7 +1206,17 @@ EXPORT_SYMBOL_GPL(vmbus_allocate_mmio);*/ void vmbus_free_mmio (resource_size_t start,resource_size_t size) { +struct resource *iter; + +down (&hyperv_mmio_lock); +for (iter = hyperv_mmio; iter; iter = iter->sibling) { +if ((iter->start >= start + size) || (iter->end <= start)) +continue ; + +__release_region (iter, start, size); + }release_mem_region (start, size); +up (&hyperv_mmio_lock); }EXPORT_SYMBOL_GPL (vmbus_free_mmio);
Signature · 1 line
1.7.4.1
Re: [PATCH 1/6] Drivers: hv: kvp: fix IP Failover
In reply to K. Y. SrinivasanQuoted 18 lines · K. Y. Srinivasan
From: Vitaly Kuznetsov <[email protected]>
Hyper-V VMs can be replicated to another hosts and there is a feature to set different IP for replicas, it is called 'Failover TCP/IP'. When such guest starts Hyper-V host sends it
KVP_OP_SET_IP_INFOmessage as soon as we finish negotiation procedure. The problem is that it can happen (and it actually happens) before userspace daemon connects and we reply withHV_E_FAILto the message. As there are no repetitions we fail to set the requested IP.Solve the issue by postponing our reply to the negotiation message till userspace daemon is connected. We can't wait too long as there is a host-side timeout (cca. 75 seconds) and if we fail to reply in this time frame the whole KVP service will become inactive. The solution is not ideal - if it takes userspace daemon more than 60 seconds to connect IP Failover will still fail but I don't see a solution with our current separation between kernel and userspace parts.
Quoted 5 lines
Other two modules (VSS and FCOPY) don't require such delay, leave them untouched.
Signed-off-by Vitaly Kuznetsov <[email protected]>Signed-off-by K. Y. Srinivasan <[email protected]>
An issue was found with this patch: we don't cancel kvp_host_handshake_work on module unload and if the work is still pending we end up crashing. As far as I can see this series didn't make it to char-misc tree so instead of sending a patch to this patch I'll send v2 for the original issue.
Sorry for the inconvenience.
Quoted 94 lines
--- drivers/hv/hv_kvp.c | 30 ++++++++++++++++++++++++++++++ drivers/hv/hyperv_vmbus.h | 5 +++++ 2 files changed, 35 insertions(+), 0 deletions(-)drivers/hv/hv_kvp.c +30
-0@@ -78,9 +78,11 @@ static void kvp_send_key(struct work_struct *dummy);static void kvp_respond_to_host (struct hv_kvp_msg *msg,int error);static void kvp_timeout_func (struct work_struct *dummy); +static void kvp_host_handshake_func (struct work_struct *dummy);static void kvp_register (int ); static DECLARE_DELAYED_WORK(kvp_timeout_work, kvp_timeout_func); +static DECLARE_DELAYED_WORK(kvp_host_handshake_work, kvp_host_handshake_func); static DECLARE_WORK(kvp_sendkey_work, kvp_send_key);static const char kvp_devname[] ="vmbus/hv_kvp" ;@@ -130,6 +132,11 @@ static void kvp_timeout_func(struct work_struct *dummy)hv_poll_channel (kvp_transaction.recv_channel, kvp_poll_wrapper); } +static void kvp_host_handshake_func (struct work_struct *dummy) +{ +hv_poll_channel (kvp_transaction.recv_channel, hv_kvp_onchannelcallback); +} +static int kvp_handle_handshake (struct hv_kvp_msg *msg) {switch (msg->kvp_hdr.operation) {@@ -154,6 +161,12 @@ static int kvp_handle_handshake(struct hv_kvp_msg *msg)pr_debug ("KVP: userspace daemon ver. %d registered\n" ,KVP_OP_REGISTER );kvp_register (dm_reg_value); + +/* +* If we're still negotiating with the host cancel the timeout +* work to not poll the channel twice. +*/ +cancel_delayed_work_sync (&kvp_host_handshake_work);hv_poll_channel (kvp_transaction.recv_channel, kvp_poll_wrapper);return 0 ;@@ -594,7 +607,22 @@ void hv_kvp_onchannelcallback(void *context)struct icmsg_negotiate *negop =NULL ;int util_fw_version;int kvp_srv_version; +static enum {NEGO_NOT_STARTED , +NEGO_IN_PROGRESS , +NEGO_FINISHED } host_negotiatied =NEGO_NOT_STARTED ; +if (host_negotiatied ==NEGO_NOT_STARTED && + kvp_transaction.state <HVUTIL_READY ) { +/* +* If userspace daemon is not connected and host is asking +* us to negotiate we need to delay to not lose messages. +* This is important for Failover IP setting. +*/ + host_negotiatied =NEGO_IN_PROGRESS ; +schedule_delayed_work (&kvp_host_handshake_work, +HV_UTIL_NEGO_TIMEOUT *HZ ); +return ; + }if (kvp_transaction.state >HVUTIL_READY )return ;@@ -672,6 +700,8 @@ void hv_kvp_onchannelcallback(void *context)vmbus_sendpacket (channel, recv_buffer, recvlen, requestid,VM_PKT_DATA_INBAND ,0 ); + + host_negotiatied =NEGO_FINISHED ; } }drivers/hv/hyperv_vmbus.h +5
-0@@ -36,6 +36,11 @@#define HV_UTIL_TIMEOUT 30/* +* Timeout for guest-host handshake for services. +*/ +#define HV_UTIL_NEGO_TIMEOUT 60 + +/* * The below CPUID leaves are present if VersionAndFeatures.HypervisorPresent * is set by CPUID(HVCPUID_VERSION_FEATURES). */
Signature · 1 line
Vitaly
Re: [PATCH 1/6] Drivers: hv: kvp: fix IP Failover
Quoted 26 lines · Vitaly Kuznetsov
Quoted 18 lines · K. Y. Srinivasan
From: Vitaly Kuznetsov <[email protected]>
Hyper-V VMs can be replicated to another hosts and there is a feature to set different IP for replicas, it is called 'Failover TCP/IP'. When such guest starts Hyper-V host sends it
KVP_OP_SET_IP_INFOmessage as soon as we finish negotiation procedure. The problem is that it can happen (and it actually happens) before userspace daemon connects and we reply withHV_E_FAILto the message. As there are no repetitions we fail to set the requested IP.Solve the issue by postponing our reply to the negotiation message till userspace daemon is connected. We can't wait too long as there is a host-side timeout (cca. 75 seconds) and if we fail to reply in this time frame the whole KVP service will become inactive. The solution is not ideal - if it takes userspace daemon more than 60 seconds to connect IP Failover will still fail but I don't see a solution with our current separation between kernel and userspace parts.
Quoted 5 lines
Other two modules (VSS and FCOPY) don't require such delay, leave them untouched.
Signed-off-by Vitaly Kuznetsov <[email protected]>Signed-off-by K. Y. Srinivasan <[email protected]>
Quoted 7 lines
An issue was found with this patch: we don't cancel
kvp_host_handshake_workon module unload and if the work is still pending we end up crashing. As far as I can see this series didn't make it to char-misc tree so instead of sending a patch to this patch I'll send v2 for the original issue.Sorry for the inconvenience.
Ok, I'll drop this whole series. K.Y., can you resend this series after fixing this up?
thanks,
greg k-h
RE: [PATCH 1/6] Drivers: hv: kvp: fix IP Failover
Quoted 48 lines
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Thursday, March 31, 2016 1:19 PM
To: Vitaly Kuznetsov <[email protected]>
Cc: KY Srinivasan <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH 1/6] Drivers: hv: kvp: fix IP FailoverQuoted 7 lines · Vitaly Kuznetsov
Quoted 5 lines · K. Y. Srinivasan
From: Vitaly Kuznetsov <[email protected]>
Hyper-V VMs can be replicated to another hosts and there is a feature to set different IP for replicas, it is called 'Failover TCP/IP'. When such guest starts Hyper-V host sends it
KVP_OP_SET_IP_INFOmessageas soon
Quoted 1 line
Quoted 1 line
as we finish negotiation procedure. The problem is that it can happen
(and
Quoted 1 line
Quoted 1 line
it actually happens) before userspace daemon connects and we reply
with
Quoted 11 lines
Quoted 11 lines
HV_E_FAILto the message. As there are no repetitions we fail to set the requested IP.Solve the issue by postponing our reply to the negotiation message till userspace daemon is connected. We can't wait too long as there is a host-side timeout (cca. 75 seconds) and if we fail to reply in this time frame the whole KVP service will become inactive. The solution is not ideal - if it takes userspace daemon more than 60 seconds to connect IP Failover will still fail but I don't see a solution with our current separation between kernel and userspace parts.
Quoted 1 line
Quoted 1 line
Other two modules (VSS and FCOPY) don't require such delay, leave
them
Quoted 11 lines
Quoted 4 lines
untouched.
Signed-off-by Vitaly Kuznetsov <[email protected]>Signed-off-by K. Y. Srinivasan <[email protected]>An issue was found with this patch: we don't cancel
kvp_host_handshake_workon module unload and if the work is still pending we end up crashing. As far as I can see this series didn't make it to char-misc tree so instead of sending a patch to this patch I'll send v2 for the original issue.
Quoted 4 lines
Quoted 1 line
Sorry for the inconvenience.
Ok, I'll drop this whole series. K.Y., can you resend this series after fixing this up?
Will do.
Thanks,
K. Y
Quoted 4 lines
thanks,
greg k-h