Linux-HyperV List
 help / color / mirror / Atom feed
* Re: [RFC PATCH 08/13] vsock: move vsock_insert_unbound() in the vsock_create()
From: Stefano Garzarella @ 2019-10-10  9:52 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: netdev, Sasha Levin, linux-hyperv, Stephen Hemminger, kvm,
	Michael S. Tsirkin, Dexuan Cui, Haiyang Zhang, linux-kernel,
	virtualization, Stefan Hajnoczi, David S. Miller, Jorgen Hansen
In-Reply-To: <20191009123423.GI5747@stefanha-x1.localdomain>

On Wed, Oct 09, 2019 at 01:34:23PM +0100, Stefan Hajnoczi wrote:
> On Fri, Sep 27, 2019 at 01:26:58PM +0200, Stefano Garzarella wrote:
> > vsock_insert_unbound() was called only when 'sock' parameter of
> > __vsock_create() was not null. This only happened when
> > __vsock_create() was called by vsock_create().
> > 
> > In order to simplify the multi-transports support, this patch
> > moves vsock_insert_unbound() at the end of vsock_create().
> > 
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> >  net/vmw_vsock/af_vsock.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> Maybe transports shouldn't call __vsock_create() directly.  They always
> pass NULL as the parent socket, so we could have a more specific
> function that transports call without a parent sock argument.  This
> would eliminate any concern over moving vsock_insert_unbound() out of
> this function.  In any case, I've checked the code and this patch is
> correct.

Yes, I agree with you, I can add a new patch to do this cleaning.

> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Thanks,
Stefano

^ permalink raw reply

* [PATCH v4] x86/hyperv: make vapic support x2apic mode
From: Roman Kagan @ 2019-10-10 12:33 UTC (permalink / raw)
  To: Michael Kelley, Lan Tianyu, Joerg Roedel, K. Y. Srinivasan,
	Haiyang Zhang, Stephen Hemminger, Sasha Levin, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86@kernel.org,
	linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
	Vitaly Kuznetsov
  Cc: kvm@vger.kernel.org

Now that there's Hyper-V IOMMU driver, Linux can switch to x2apic mode
when supported by the vcpus.

However, the apic access functions for Hyper-V enlightened apic assume
xapic mode only.

As a result, Linux fails to bring up secondary cpus when run as a guest
in QEMU/KVM with both hv_apic and x2apic enabled.

According to Michael Kelley, when in x2apic mode, the Hyper-V synthetic
apic MSRs behave exactly the same as the corresponding architectural
x2apic MSRs, so there's no need to override the apic accessors.  The
only exception is hv_apic_eoi_write, which benefits from lazy EOI when
available; however, its implementation works for both xapic and x2apic
modes.

Fixes: 29217a474683 ("iommu/hyper-v: Add Hyper-V stub IOMMU driver")
Fixes: 6b48cb5f8347 ("X86/Hyper-V: Enlighten APIC access")
Cc: stable@vger.kernel.org
Suggested-by: Michael Kelley <mikelley@microsoft.com>
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
v3 -> v4:
- adjust the log message [Vitaly, Michael]

v2 -> v3:
- do not introduce x2apic-capable hv_apic accessors; leave original
  x2apic accessors instead

v1 -> v2:
- add ifdefs to handle !CONFIG_X86_X2APIC

 arch/x86/hyperv/hv_apic.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/arch/x86/hyperv/hv_apic.c b/arch/x86/hyperv/hv_apic.c
index 5c056b8aebef..e01078e93dd3 100644
--- a/arch/x86/hyperv/hv_apic.c
+++ b/arch/x86/hyperv/hv_apic.c
@@ -260,11 +260,21 @@ void __init hv_apic_init(void)
 	}
 
 	if (ms_hyperv.hints & HV_X64_APIC_ACCESS_RECOMMENDED) {
-		pr_info("Hyper-V: Using MSR based APIC access\n");
+		pr_info("Hyper-V: Using enlightened APIC (%s mode)",
+			x2apic_enabled() ? "x2apic" : "xapic");
+		/*
+		 * With x2apic, architectural x2apic MSRs are equivalent to the
+		 * respective synthetic MSRs, so there's no need to override
+		 * the apic accessors.  The only exception is
+		 * hv_apic_eoi_write, because it benefits from lazy EOI when
+		 * available, but it works for both xapic and x2apic modes.
+		 */
 		apic_set_eoi_write(hv_apic_eoi_write);
-		apic->read      = hv_apic_read;
-		apic->write     = hv_apic_write;
-		apic->icr_write = hv_apic_icr_write;
-		apic->icr_read  = hv_apic_icr_read;
+		if (!x2apic_enabled()) {
+			apic->read      = hv_apic_read;
+			apic->write     = hv_apic_write;
+			apic->icr_write = hv_apic_icr_write;
+			apic->icr_read  = hv_apic_icr_read;
+		}
 	}
 }
-- 
2.21.0


^ permalink raw reply related

* Re: [RFC PATCH 10/13] vsock: add multi-transports support
From: Stefano Garzarella @ 2019-10-10 12:55 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: netdev, Sasha Levin, linux-hyperv, Stephen Hemminger, kvm,
	Michael S. Tsirkin, Dexuan Cui, Haiyang Zhang, linux-kernel,
	virtualization, Stefan Hajnoczi, David S. Miller, Jorgen Hansen
In-Reply-To: <20191009131123.GK5747@stefanha-x1.localdomain>

On Wed, Oct 09, 2019 at 02:11:23PM +0100, Stefan Hajnoczi wrote:
> On Fri, Sep 27, 2019 at 01:27:00PM +0200, Stefano Garzarella wrote:
> > RFC:
> > - I'd like to move MODULE_ALIAS_NETPROTO(PF_VSOCK) to af_vsock.c.
> >   @Jorgen could this break the VMware products?
> 
> What will cause the vmw_vsock_vmci_transport.ko module to be loaded
> after you remove MODULE_ALIAS_NETPROTO(PF_VSOCK)?  Perhaps
> drivers/misc/vmw_vmci/vmci_guest.c:vmci_guest_probe_device() could do
> something when the guest driver loads.

Good idea, maybe we can call some function provided by vmci_transport
to register it as a guest (I'll remove the type from the transport
and I add it as a parameter of vsock_core_register())

>                                         There would need to be something
> equivalent for the host side too.

Maybe in the vmci_host_do_init_context().

> 
> This will solve another issue too.  Today the VMCI transport can be
> loaded if an application creates an AF_VSOCK socket during early boot
> before the virtio transport has been probed.  This happens because the
> VMCI transport uses MODULE_ALIAS_NETPROTO(PF_VSOCK) *and* it does not
> probe whether this system is actually a VMware guest.
> 
> If we instead load the core af_vsock.ko module and transports are only
> loaded based on hardware feature probing (e.g. the presence of VMware
> guest mode, a virtio PCI adapter, etc) then transports will be
> well-behaved.

Yes, I completely agree with you. I'll try to follow your suggestion,

> 
> > - DGRAM sockets are handled as before, I don't know if make sense work
> >   on it now, or when another transport will support DGRAM. The big
> >   issues here is that we cannot link 1-1 a socket to transport as
> >   for stream sockets since DGRAM is not connection-oriented.
> 
> Let's ignore DGRAM for now since only VMCI supports it and we therefore
> do not require multi-transpor) support.

Okay :)

> 
> > diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> > index 86f8f463e01a..2a081d19e20d 100644
> > --- a/include/net/af_vsock.h
> > +++ b/include/net/af_vsock.h
> > @@ -94,7 +94,13 @@ struct vsock_transport_send_notify_data {
> >  	u64 data2; /* Transport-defined. */
> >  };
> >  
> > +#define VSOCK_TRANSPORT_F_H2G		0x00000001
> > +#define VSOCK_TRANSPORT_F_G2H		0x00000002
> > +#define VSOCK_TRANSPORT_F_DGRAM		0x00000004
> 
> Documentation comments, please.

I'll fix!

> 
> > +void vsock_core_unregister(const struct vsock_transport *t)
> > +{
> > +	mutex_lock(&vsock_register_mutex);
> > +
> > +	/* RFC-TODO: maybe we should check if there are open sockets
> > +	 * assigned to that transport and avoid the unregistration
> > +	 */
> 
> If unregister() is only called from module_exit() functions then holding
> a reference to the transport module would be enough to prevent this
> case.  The transport could only be removed once all sockets have been
> destroyed (and dropped their transport module reference).

Yes. I did this in
"[RFC PATCH 12/13] vsock: prevent transport modules unloading".

Maybe I can merge it in this patch...

Thanks,
Stefano

^ permalink raw reply

* Re: [RFC PATCH 11/13] vsock: add 'transport_hg' to handle g2h\h2g transports
From: Stefano Garzarella @ 2019-10-10 13:04 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: netdev, Sasha Levin, linux-hyperv, Stephen Hemminger, kvm,
	Michael S. Tsirkin, Dexuan Cui, Haiyang Zhang, linux-kernel,
	virtualization, Stefan Hajnoczi, David S. Miller, Jorgen Hansen
In-Reply-To: <20191009131643.GL5747@stefanha-x1.localdomain>

On Wed, Oct 09, 2019 at 02:16:43PM +0100, Stefan Hajnoczi wrote:
> On Fri, Sep 27, 2019 at 01:27:01PM +0200, Stefano Garzarella wrote:
> > VMCI transport provides both g2h and h2g behaviors in a single
> > transport.
> > We are able to set (or not) the g2h behavior, detecting if we
> > are in a VMware guest (or not), but the h2g feature is always set.
> > This prevents to load other h2g transports while we are in a
> > VMware guest.
> 
> In the vhost_vsock.ko case we only register the h2g transport when
> userspace has loaded the module (by opening /dev/vhost-vsock).
> 
> VMCI has something kind of similar: /dev/vmci and the
> vmci_host_active_users counter.  Maybe we can use this instead of
> introducing the transport_hg concept?

Yes, maybe we can register the host in the vmci_host_do_init_context().

I also don't like a lot the transport_hg concept, so I'll try to found
an alternative.

Thanks,
Stefano

^ permalink raw reply

* Re: [PATCH] mm/resource: Move child to new resource when release mem region.
From: Dave Hansen @ 2019-10-10 14:29 UTC (permalink / raw)
  To: lantianyu1986, dan.j.williams, dave.hansen, mingo, mpe,
	pasha.tatashin, osalvador, richardw.yang, Tianyu.Lan,
	christophe.leroy, bp, rdunlap, michael.h.kelley, kys, sashal
  Cc: linux-kernel, vkuznets, linux-hyperv
In-Reply-To: <20191010072856.20079-1-Tianyu.Lan@microsoft.com>

On 10/10/19 12:28 AM, lantianyu1986@gmail.com wrote:
> When release mem region, old mem region may be splited to
> two regions. Current allocate new struct resource for high
> end mem region but not move child resources whose ranges are
> in the high end range to new resource. When adjust old mem
> region's range, adjust_resource() detects child region's range
> is out of new range and return error. Move child resources to
> high end resource before adjusting old mem range.

From the comment, it appears the old code intended to have the behavior
that you are changing.  Could you explain _why_ this has become a
problem for you?


^ permalink raw reply

* [PATCH v2 0/3] Drivers: hv: vmbus: Miscellaneous improvements
From: Andrea Parri @ 2019-10-10 15:45 UTC (permalink / raw)
  To: linux-kernel, linux-hyperv, netdev
  Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
	David S . Miller, Michael Kelley, Vitaly Kuznetsov, Dexuan Cui,
	Andrea Parri

Hi all,

The patchset:

  - refactors the VMBus negotiation code by introducing the table of
    VMBus protocol versions (patch 1/3),

  - enables VMBus protocol version 4.1, 5.1 and 5.2 (patch 2/3),

  - introduces a module parameter to cap the VMBus protocol versions
    which a guest can negotiate with the hypervisor (patch 3/3).

Thanks,
  Andrea

---

Changes since v1 ([1]):
  - remove the VERSION_INVAL macro (Vitaly Kuznetsov and Dexuan Cui)
  - make the table of VMBus protocol versions static (Dexuan Cui)
  - enable VMBus protocol version 4.1 (Michael Kelley)
  - introduce module parameter to cap the VMBus version (Dexuan Cui)

[1] https://lkml.kernel.org/r/20191007163115.26197-1-parri.andrea@gmail.com

Andrea Parri (3):
  Drivers: hv: vmbus: Introduce table of VMBus protocol versions
  Drivers: hv: vmbus: Enable VMBus protocol versions 4.1, 5.1 and 5.2
  Drivers: hv: vmbus: Add module parameter to cap the VMBus version

 drivers/hv/connection.c          | 68 ++++++++++++++++----------------
 drivers/hv/vmbus_drv.c           |  3 +-
 drivers/net/hyperv/netvsc.c      |  6 +--
 include/linux/hyperv.h           | 12 +++---
 net/vmw_vsock/hyperv_transport.c |  4 +-
 5 files changed, 48 insertions(+), 45 deletions(-)

-- 
2.23.0


^ permalink raw reply

* [PATCH v2 2/3] Drivers: hv: vmbus: Enable VMBus protocol versions 4.1, 5.1 and 5.2
From: Andrea Parri @ 2019-10-10 15:45 UTC (permalink / raw)
  To: linux-kernel, linux-hyperv, netdev
  Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
	David S . Miller, Michael Kelley, Vitaly Kuznetsov, Dexuan Cui,
	Andrea Parri
In-Reply-To: <20191010154600.23875-1-parri.andrea@gmail.com>

Hyper-V has added VMBus protocol versions 5.1 and 5.2 in recent release
versions.  Allow Linux guests to negotiate these new protocol versions
on versions of Hyper-V that support them.  While on this, also allow
guests to negotiate the VMBus protocol version 4.1 (which was missing).

Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
---
 drivers/hv/connection.c          | 15 +++++++++------
 drivers/net/hyperv/netvsc.c      |  6 +++---
 include/linux/hyperv.h           |  8 +++++++-
 net/vmw_vsock/hyperv_transport.c |  4 ++--
 4 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index b1f805426e6b4..2f6961ac8c996 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -44,8 +44,11 @@ EXPORT_SYMBOL_GPL(vmbus_proto_version);
  * Table of VMBus versions listed from newest to oldest.
  */
 static __u32 vmbus_versions[] = {
+	VERSION_WIN10_V5_2,
+	VERSION_WIN10_V5_1,
 	VERSION_WIN10_V5,
-	VERSION_WIN10,
+	VERSION_WIN10_V4_1,
+	VERSION_WIN10_V4,
 	VERSION_WIN8_1,
 	VERSION_WIN8,
 	VERSION_WIN7,
@@ -68,12 +71,12 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
 	msg->vmbus_version_requested = version;
 
 	/*
-	 * VMBus protocol 5.0 (VERSION_WIN10_V5) requires that we must use
-	 * VMBUS_MESSAGE_CONNECTION_ID_4 for the Initiate Contact Message,
+	 * VMBus protocol 5.0 (VERSION_WIN10_V5) and higher require 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
+	 * VERSION_WIN10_V5 and higher, 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).
@@ -399,7 +402,7 @@ int vmbus_post_msg(void *buffer, size_t buflen, bool can_sleep)
 		case HV_STATUS_INVALID_CONNECTION_ID:
 			/*
 			 * See vmbus_negotiate_version(): VMBus protocol 5.0
-			 * requires that we must use
+			 * and higher require 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
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index d22a36fc7a7c6..d4c1a776b314a 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -624,11 +624,11 @@ void netvsc_device_remove(struct hv_device *device)
 	 * receive buffer GPADL. Do the same for send buffer.
 	 */
 	netvsc_revoke_recv_buf(device, net_device, ndev);
-	if (vmbus_proto_version < VERSION_WIN10)
+	if (vmbus_proto_version < VERSION_WIN10_V4)
 		netvsc_teardown_recv_gpadl(device, net_device, ndev);
 
 	netvsc_revoke_send_buf(device, net_device, ndev);
-	if (vmbus_proto_version < VERSION_WIN10)
+	if (vmbus_proto_version < VERSION_WIN10_V4)
 		netvsc_teardown_send_gpadl(device, net_device, ndev);
 
 	RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
@@ -650,7 +650,7 @@ void netvsc_device_remove(struct hv_device *device)
 	 * If host is Win2016 or higher then we do the GPADL tear down
 	 * here after VMBus is closed.
 	*/
-	if (vmbus_proto_version >= VERSION_WIN10) {
+	if (vmbus_proto_version >= VERSION_WIN10_V4) {
 		netvsc_teardown_recv_gpadl(device, net_device, ndev);
 		netvsc_teardown_send_gpadl(device, net_device, ndev);
 	}
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index c08b62dbd151f..a4f80e30b0207 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -182,15 +182,21 @@ static inline u32 hv_get_avail_to_write_percent(
  * 2 . 4  (Windows 8)
  * 3 . 0  (Windows 8 R2)
  * 4 . 0  (Windows 10)
+ * 4 . 1  (Windows 10 RS3)
  * 5 . 0  (Newer Windows 10)
+ * 5 . 1  (Windows 10 RS4)
+ * 5 . 2  (Windows Server 2019, RS5)
  */
 
 #define VERSION_WS2008  ((0 << 16) | (13))
 #define VERSION_WIN7    ((1 << 16) | (1))
 #define VERSION_WIN8    ((2 << 16) | (4))
 #define VERSION_WIN8_1    ((3 << 16) | (0))
-#define VERSION_WIN10	((4 << 16) | (0))
+#define VERSION_WIN10_V4 ((4 << 16) | (0))
+#define VERSION_WIN10_V4_1 ((4 << 16) | (1))
 #define VERSION_WIN10_V5 ((5 << 16) | (0))
+#define VERSION_WIN10_V5_1 ((5 << 16) | (1))
+#define VERSION_WIN10_V5_2 ((5 << 16) | (2))
 
 /* Make maximum size of pipe payload of 16K */
 #define MAX_PIPE_DATA_PAYLOAD		(sizeof(u8) * 16384)
diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index c443db7af8d4a..cb0dbae4de14a 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -14,7 +14,7 @@
 #include <net/sock.h>
 #include <net/af_vsock.h>
 
-/* Older (VMBUS version 'VERSION_WIN10' or before) Windows hosts have some
+/* Older (VMBUS version 'VERSION_WIN10_V4' or before) Windows hosts have some
  * stricter requirements on the hv_sock ring buffer size of six 4K pages. Newer
  * hosts don't have this limitation; but, keep the defaults the same for compat.
  */
@@ -955,7 +955,7 @@ static int __init hvs_init(void)
 {
 	int ret;
 
-	if (vmbus_proto_version < VERSION_WIN10)
+	if (vmbus_proto_version < VERSION_WIN10_V4)
 		return -ENODEV;
 
 	ret = vmbus_driver_register(&hvs_drv);
-- 
2.23.0


^ permalink raw reply related

* [PATCH v2 3/3] Drivers: hv: vmbus: Add module parameter to cap the VMBus version
From: Andrea Parri @ 2019-10-10 15:46 UTC (permalink / raw)
  To: linux-kernel, linux-hyperv, netdev
  Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
	David S . Miller, Michael Kelley, Vitaly Kuznetsov, Dexuan Cui,
	Andrea Parri
In-Reply-To: <20191010154600.23875-1-parri.andrea@gmail.com>

Currently, Linux guests negotiate the VMBus version with Hyper-V
and use the highest available VMBus version they can connect to.
This has some drawbacks: by using the highest available version,
certain code paths are never executed and can not be tested when
the guest runs on the newest host.

Add the module parameter "max_version", to upper-bound the VMBus
versions guests can negotiate.

Suggested-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
---
 drivers/hv/connection.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 2f6961ac8c996..f60d7330ff3fd 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -14,6 +14,7 @@
 #include <linux/wait.h>
 #include <linux/delay.h>
 #include <linux/mm.h>
+#include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/hyperv.h>
@@ -55,6 +56,16 @@ static __u32 vmbus_versions[] = {
 	VERSION_WS2008
 };
 
+/*
+ * Maximal VMBus protocol version guests can negotiate.  Useful to cap the
+ * VMBus version for testing and debugging purpose.
+ */
+static uint max_version = VERSION_WIN10_V5_2;
+
+module_param(max_version, uint, S_IRUGO);
+MODULE_PARM_DESC(max_version,
+		 "Maximal VMBus protocol version which can be negotiated");
+
 int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
 {
 	int ret = 0;
@@ -237,6 +248,8 @@ int vmbus_connect(void)
 
 	for (i = 0; i < ARRAY_SIZE(vmbus_versions); i++) {
 		version = vmbus_versions[i];
+		if (version > max_version)
+			continue;
 
 		ret = vmbus_negotiate_version(msginfo, version);
 		if (ret == -ETIMEDOUT)
-- 
2.23.0


^ permalink raw reply related

* [PATCH v2 1/3] Drivers: hv: vmbus: Introduce table of VMBus protocol versions
From: Andrea Parri @ 2019-10-10 15:45 UTC (permalink / raw)
  To: linux-kernel, linux-hyperv, netdev
  Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
	David S . Miller, Michael Kelley, Vitaly Kuznetsov, Dexuan Cui,
	Andrea Parri
In-Reply-To: <20191010154600.23875-1-parri.andrea@gmail.com>

The technique used to get the next VMBus version seems increasisly
clumsy as the number of VMBus versions increases.  Performance is
not a concern since this is only done once during system boot; it's
just that we'll end up with more lines of code than is really needed.

As an alternative, introduce a table with the version numbers listed
in order (from the most recent to the oldest).  vmbus_connect() loops
through the versions listed in the table until it gets an accepted
connection or gets to the end of the table (invalid version).

Suggested-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
---
 drivers/hv/connection.c | 46 ++++++++++++++---------------------------
 drivers/hv/vmbus_drv.c  |  3 +--
 include/linux/hyperv.h  |  4 ----
 3 files changed, 17 insertions(+), 36 deletions(-)

diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 6e4c015783ffc..b1f805426e6b4 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -40,29 +40,17 @@ EXPORT_SYMBOL_GPL(vmbus_connection);
 __u32 vmbus_proto_version;
 EXPORT_SYMBOL_GPL(vmbus_proto_version);
 
-static __u32 vmbus_get_next_version(__u32 current_version)
-{
-	switch (current_version) {
-	case (VERSION_WIN7):
-		return VERSION_WS2008;
-
-	case (VERSION_WIN8):
-		return VERSION_WIN7;
-
-	case (VERSION_WIN8_1):
-		return VERSION_WIN8;
-
-	case (VERSION_WIN10):
-		return VERSION_WIN8_1;
-
-	case (VERSION_WIN10_V5):
-		return VERSION_WIN10;
-
-	case (VERSION_WS2008):
-	default:
-		return VERSION_INVAL;
-	}
-}
+/*
+ * Table of VMBus versions listed from newest to oldest.
+ */
+static __u32 vmbus_versions[] = {
+	VERSION_WIN10_V5,
+	VERSION_WIN10,
+	VERSION_WIN8_1,
+	VERSION_WIN8,
+	VERSION_WIN7,
+	VERSION_WS2008
+};
 
 int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
 {
@@ -169,8 +157,8 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
  */
 int vmbus_connect(void)
 {
-	int ret = 0;
 	struct vmbus_channel_msginfo *msginfo = NULL;
+	int i, ret = 0;
 	__u32 version;
 
 	/* Initialize the vmbus connection */
@@ -244,20 +232,18 @@ int vmbus_connect(void)
 	 * version.
 	 */
 
-	version = VERSION_CURRENT;
+	for (i = 0; i < ARRAY_SIZE(vmbus_versions); i++) {
+		version = vmbus_versions[i];
 
-	do {
 		ret = vmbus_negotiate_version(msginfo, version);
 		if (ret == -ETIMEDOUT)
 			goto cleanup;
 
 		if (vmbus_connection.conn_state == CONNECTED)
 			break;
+	}
 
-		version = vmbus_get_next_version(version);
-	} while (version != VERSION_INVAL);
-
-	if (version == VERSION_INVAL)
+	if (vmbus_connection.conn_state != CONNECTED)
 		goto cleanup;
 
 	vmbus_proto_version = version;
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 391f0b225c9ae..a0cd65ab9a950 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2215,8 +2215,7 @@ static int vmbus_bus_resume(struct device *dev)
 	 * We only use the 'vmbus_proto_version', which was in use before
 	 * hibernation, to re-negotiate with the host.
 	 */
-	if (vmbus_proto_version == VERSION_INVAL ||
-	    vmbus_proto_version == 0) {
+	if (!vmbus_proto_version) {
 		pr_err("Invalid proto version = 0x%x\n", vmbus_proto_version);
 		return -EINVAL;
 	}
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index b4a017093b697..c08b62dbd151f 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -192,10 +192,6 @@ static inline u32 hv_get_avail_to_write_percent(
 #define VERSION_WIN10	((4 << 16) | (0))
 #define VERSION_WIN10_V5 ((5 << 16) | (0))
 
-#define VERSION_INVAL -1
-
-#define VERSION_CURRENT VERSION_WIN10_V5
-
 /* Make maximum size of pipe payload of 16K */
 #define MAX_PIPE_DATA_PAYLOAD		(sizeof(u8) * 16384)
 
-- 
2.23.0


^ permalink raw reply related

* Re: [PATCH v2] PCI: PM: Move to D0 before calling pci_legacy_resume_early()
From: Rafael J. Wysocki @ 2019-10-10 16:45 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Dexuan Cui, lorenzo.pieralisi@arm.com, linux-pci@vger.kernel.org,
	Michael Kelley, linux-hyperv@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	driverdev-devel@linuxdriverproject.org, Sasha Levin,
	Haiyang Zhang, KY Srinivasan, olaf@aepfle.de, apw@canonical.com,
	jasowang@redhat.com, vkuznets, marcelo.cerri@canonical.com,
	Stephen Hemminger, jackm@mellanox.com
In-Reply-To: <20191008195624.GA198287@google.com>

On 10/8/2019 9:56 PM, Bjorn Helgaas wrote:
> On Tue, Oct 08, 2019 at 07:32:27PM +0200, Rafael J. Wysocki wrote:
>> On 10/7/2019 8:57 PM, Dexuan Cui wrote:
>>>> -----Original Message-----
>>>> From: Bjorn Helgaas <helgaas@kernel.org>
>>>> Sent: Monday, October 7, 2019 6:24 AM
>>>> To: Dexuan Cui <decui@microsoft.com>
>>>> Cc: lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; Michael Kelley
>>>> <mikelley@microsoft.com>; linux-hyperv@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; driverdev-devel@linuxdriverproject.org; Sasha
>>>> Levin <Alexander.Levin@microsoft.com>; Haiyang Zhang
>>>> <haiyangz@microsoft.com>; KY Srinivasan <kys@microsoft.com>;
>>>> olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com; vkuznets
>>>> <vkuznets@redhat.com>; marcelo.cerri@canonical.com; Stephen Hemminger
>>>> <sthemmin@microsoft.com>; jackm@mellanox.com
>>>> Subject: Re: [PATCH v2] PCI: PM: Move to D0 before calling
>>>> pci_legacy_resume_early()
>>>>
>>>> On Wed, Aug 14, 2019 at 01:06:55AM +0000, Dexuan Cui wrote:
>>>>> In pci_legacy_suspend_late(), the device state is moved to PCI_UNKNOWN.
>>>>>
>>>>> In pci_pm_thaw_noirq(), the state is supposed to be moved back to PCI_D0,
>>>>> but the current code misses the pci_legacy_resume_early() path, so the
>>>>> state remains in PCI_UNKNOWN in that path. As a result, in the resume
>>>>> phase of hibernation, this causes an error for the Mellanox VF driver,
>>>>> which fails to enable MSI-X because pci_msi_supported() is false due
>>>>> to dev->current_state != PCI_D0:
>>>>>
>>>>> mlx4_core a6d1:00:02.0: Detected virtual function - running in slave mode
>>>>> mlx4_core a6d1:00:02.0: Sending reset
>>>>> mlx4_core a6d1:00:02.0: Sending vhcr0
>>>>> mlx4_core a6d1:00:02.0: HCA minimum page size:512
>>>>> mlx4_core a6d1:00:02.0: Timestamping is not supported in slave mode
>>>>> mlx4_core a6d1:00:02.0: INTx is not supported in multi-function mode,
>>>> aborting
>>>>> PM: dpm_run_callback(): pci_pm_thaw+0x0/0xd7 returns -95
>>>>> PM: Device a6d1:00:02.0 failed to thaw: error -95
>>>>>
>>>>> To be more accurate, the "resume" phase means the "thaw" callbacks which
>>>>> run before the system enters hibernation: when the user runs the command
>>>>> "echo disk > /sys/power/state" for hibernation, first the kernel "freezes"
>>>>> all the devices and creates a hibernation image, then the kernel "thaws"
>>>>> the devices including the disk/NIC, writes the memory to the disk, and
>>>>> powers down. This patch fixes the error message for the Mellanox VF driver
>>>>> in this phase.
> Wordsmithing nit: what the patch does is not "fix the error message";
> what it does is fix the *problem*, i.e., the fact that we can't
> operate the device because we can't enable MSI-X.  The message is only
> a symptom.
>
> IIUC the relevant part of the system hibernation sequence is:
>
>    pci_pm_freeze_noirq
>    pci_pm_thaw_noirq
>    pci_pm_thaw
>
> And the execution flow is:
>
>    pci_pm_freeze_noirq
>      if (pci_has_legacy_pm_support(pci_dev)) # true for mlx4
>        pci_legacy_suspend_late(dev, PMSG_FREEZE)
> 	pci_pm_set_unknown_state
> 	  dev->current_state = PCI_UNKNOWN  # <---
>    pci_pm_thaw_noirq
>      if (pci_has_legacy_pm_support(pci_dev)) # true
>        pci_legacy_resume_early(dev)          # noop; mlx4 doesn't implement
>    pci_pm_thaw                               # returns -95 EOPNOTSUPP
>      if (pci_has_legacy_pm_support(pci_dev)) # true
>        pci_legacy_resume
> 	drv->resume
> 	  mlx4_resume                       # mlx4_driver.resume (legacy)
> 	    mlx4_load_one
> 	      mlx4_enable_msi_x
> 		pci_enable_msix_range
> 		  __pci_enable_msix_range
> 		    __pci_enable_msix
> 		      if (!pci_msi_supported())
> 			if (dev->current_state != PCI_D0)  # <---
> 			  return 0
> 			return -EINVAL
> 		err = -EOPNOTSUPP
> 		"INTx is not supported ..."
>
> (These are just my notes; you don't need to put them all into the
> commit message.  I'm just sharing them in case I'm not understanding
> correctly.)
>
>>>>> When the system starts again, a fresh kernel starts to run, and when the
>>>>> kernel detects that a hibernation image was saved, the kernel "quiesces"
>>>>> the devices, and then "restores" the devices from the saved image. In this
>>>>> path:
>>>>> device_resume_noirq() -> ... ->
>>>>>     pci_pm_restore_noirq() ->
>>>>>       pci_pm_default_resume_early() ->
>>>>>         pci_power_up() moves the device states back to PCI_D0. This path is
>>>>> not broken and doesn't need my patch.
>>>>>
> The cc list suggests that this might be a fix for a user-reported
> problem.  Is there a launchpad or similar link you could include here?
>
> Should this be marked for stable?
>
>>>>> Signed-off-by: Dexuan Cui <decui@microsoft.com>
>>>> This looks like a bugfix for 5839ee7389e8 ("PCI / PM: Force devices to
>>>> D0 in pci_pm_thaw_noirq()") so maybe it should be marked for stable as
>>>> 5839ee7389e8 was?
>>>>
>>>> Rafael, could you confirm?
>> No, it is not a bug fix for that commit.  The underlying issue would be
>> there without that commit too.
> Oh, right, I dunno what I was thinking, sorry.
>
>>>>> --- a/drivers/pci/pci-driver.c
>>>>> +++ b/drivers/pci/pci-driver.c
>>>>> @@ -1074,15 +1074,16 @@ static int pci_pm_thaw_noirq(struct device
>>>> *dev)
>>>>>    			return error;
>>>>>    	}
>>>>>
>>>>> -	if (pci_has_legacy_pm_support(pci_dev))
>>>>> -		return pci_legacy_resume_early(dev);
>>>>> -
>>>>>    	/*
>>>>>    	 * pci_restore_state() requires the device to be in D0 (because of MSI
>>>>>    	 * restoration among other things), so force it into D0 in case the
>>>>>    	 * driver's "freeze" callbacks put it into a low-power state directly.
>>>>>    	 */
>>>>>    	pci_set_power_state(pci_dev, PCI_D0);
>>>>> +
>>>>> +	if (pci_has_legacy_pm_support(pci_dev))
>>>>> +		return pci_legacy_resume_early(dev);
>>>>> +
>>>>>    	pci_restore_state(pci_dev);
>>>>>
>>>>>    	if (drv && drv->pm && drv->pm->thaw_noirq)
>>>>> --
>>>>> 2.19.1
>>>>>
>> The patch looks reasonable to me, but the comment above the
>> pci_set_power_state() call needs to be updated too IMO.
> Hmm.
>
> 1) pci_restore_state() mainly writes config space, which doesn't
> require the device to be in D0.  The only thing I see that would
> require D0 is the MSI-X MMIO space, so to be more specific, the
> comment could say "restoring the MSI-X *MMIO* state requires the
> device to be in D0".
>
> But I think you meant some other comment change.  Did you mean
> something along the lines of "a legacy drv->resume_early() callback
> and pci_restore_state() both require the device to be in D0"?

Yes, I did.



^ permalink raw reply

* Re: [PATCH] hv_sock: use HV_HYP_PAGE_SIZE instead of PAGE_SIZE_4K
From: Sasha Levin @ 2019-10-10 17:06 UTC (permalink / raw)
  To: Michael Kelley, davem
  Cc: Himadri Pandya, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	davem@davemloft.net, linux-hyperv@vger.kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	himadri18.07
In-Reply-To: <20191004154817.GL17454@sasha-vm>

On Fri, Oct 04, 2019 at 11:48:17AM -0400, Sasha Levin wrote:
>On Wed, Jul 31, 2019 at 01:02:03AM +0000, Michael Kelley wrote:
>>From: Himadri Pandya <himadrispandya@gmail.com> Sent: Wednesday, July 24, 2019 10:11 PM
>>>
>>>Older windows hosts require the hv_sock ring buffer to be defined
>>>using 4K pages. This was achieved by using the symbol PAGE_SIZE_4K
>>>defined specifically for this purpose. But now we have a new symbol
>>>HV_HYP_PAGE_SIZE defined in hyperv-tlfs which can be used for this.
>>>
>>>This patch removes the definition of symbol PAGE_SIZE_4K and replaces
>>>its usage with the symbol HV_HYP_PAGE_SIZE. This patch also aligns
>>>sndbuf and rcvbuf to hyper-v specific page size using HV_HYP_PAGE_SIZE
>>>instead of the guest page size(PAGE_SIZE) as hyper-v expects the page
>>>size to be 4K and it might not be the case on ARM64 architecture.
>>>
>>>Signed-off-by: Himadri Pandya <himadri18.07@gmail.com>
>>>---
>>> net/vmw_vsock/hyperv_transport.c | 21 +++++++++++----------
>>> 1 file changed, 11 insertions(+), 10 deletions(-)
>>>
>>>diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
>>>index f2084e3f7aa4..ecb5d72d8010 100644
>>>--- a/net/vmw_vsock/hyperv_transport.c
>>>+++ b/net/vmw_vsock/hyperv_transport.c
>>>@@ -13,15 +13,16 @@
>>> #include <linux/hyperv.h>
>>> #include <net/sock.h>
>>> #include <net/af_vsock.h>
>>>+#include <asm/hyperv-tlfs.h>
>>>
>>
>>Reviewed-by:  Michael Kelley <mikelley@microsoft.com>
>>
>>This patch depends on a prerequisite patch in
>>
>>  git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/hyperv
>>
>>that defines HV_HYP_PAGE_SIZE.
>
>David, the above prerequisite patch is now upstream, so this patch
>should be good to go. Would you take it through the net tree or should I
>do it via the hyperv tree?

Ping?

-- 
Thanks,
Sasha

^ permalink raw reply

* Re: [PATCH] hv_sock: use HV_HYP_PAGE_SIZE instead of PAGE_SIZE_4K
From: Jakub Kicinski @ 2019-10-10 20:43 UTC (permalink / raw)
  To: Sasha Levin
  Cc: Michael Kelley, davem, Himadri Pandya, KY Srinivasan,
	Haiyang Zhang, Stephen Hemminger, linux-hyperv@vger.kernel.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	himadri18.07
In-Reply-To: <20191010170606.GA1396@sasha-vm>

On Thu, 10 Oct 2019 13:06:06 -0400, Sasha Levin wrote:
> On Fri, Oct 04, 2019 at 11:48:17AM -0400, Sasha Levin wrote:
> >On Wed, Jul 31, 2019 at 01:02:03AM +0000, Michael Kelley wrote:  
> >>From: Himadri Pandya <himadrispandya@gmail.com> Sent: Wednesday, July 24, 2019 10:11 PM  
> >>>
> >>>Older windows hosts require the hv_sock ring buffer to be defined
> >>>using 4K pages. This was achieved by using the symbol PAGE_SIZE_4K
> >>>defined specifically for this purpose. But now we have a new symbol
> >>>HV_HYP_PAGE_SIZE defined in hyperv-tlfs which can be used for this.
> >>>
> >>>This patch removes the definition of symbol PAGE_SIZE_4K and replaces
> >>>its usage with the symbol HV_HYP_PAGE_SIZE. This patch also aligns
> >>>sndbuf and rcvbuf to hyper-v specific page size using HV_HYP_PAGE_SIZE
> >>>instead of the guest page size(PAGE_SIZE) as hyper-v expects the page
> >>>size to be 4K and it might not be the case on ARM64 architecture.
> >>>
> >>>Signed-off-by: Himadri Pandya <himadri18.07@gmail.com>
> >>>---
> >>> net/vmw_vsock/hyperv_transport.c | 21 +++++++++++----------
> >>> 1 file changed, 11 insertions(+), 10 deletions(-)
> >>>
> >>>diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> >>>index f2084e3f7aa4..ecb5d72d8010 100644
> >>>--- a/net/vmw_vsock/hyperv_transport.c
> >>>+++ b/net/vmw_vsock/hyperv_transport.c
> >>>@@ -13,15 +13,16 @@
> >>> #include <linux/hyperv.h>
> >>> #include <net/sock.h>
> >>> #include <net/af_vsock.h>
> >>>+#include <asm/hyperv-tlfs.h>
> >>>  
> >>
> >>Reviewed-by:  Michael Kelley <mikelley@microsoft.com>
> >>
> >>This patch depends on a prerequisite patch in
> >>
> >>  git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/hyperv
> >>
> >>that defines HV_HYP_PAGE_SIZE.  
> >
> >David, the above prerequisite patch is now upstream, so this patch
> >should be good to go. Would you take it through the net tree or should I
> >do it via the hyperv tree?  
> 
> Ping?

Is this a fix? It's slightly unclear from the description of the patch.
I think the best course of action would be reposting it again, with
either [PATCH net] in the subject and a Fixes tag if it's a fix, or
[PATCH net-next] otherwise.

^ permalink raw reply

* Re: [RFC PATCH 07/13] vsock: handle buffer_size sockopts in the core
From: Stefan Hajnoczi @ 2019-10-11  8:27 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Stefan Hajnoczi, netdev, Sasha Levin, linux-hyperv,
	Stephen Hemminger, kvm, Michael S. Tsirkin, Dexuan Cui,
	Haiyang Zhang, linux-kernel, virtualization, David S. Miller,
	Jorgen Hansen
In-Reply-To: <20191010093254.aluys4hpsfcepb42@steredhat>

[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]

On Thu, Oct 10, 2019 at 11:32:54AM +0200, Stefano Garzarella wrote:
> On Wed, Oct 09, 2019 at 01:30:26PM +0100, Stefan Hajnoczi wrote:
> > On Fri, Sep 27, 2019 at 01:26:57PM +0200, Stefano Garzarella wrote:
> > Another issue is that this patch drops the VIRTIO_VSOCK_MAX_BUF_SIZE
> > limit that used to be enforced by virtio_transport_set_buffer_size().
> > Now the limit is only applied at socket init time.  If the buffer size
> > is changed later then VIRTIO_VSOCK_MAX_BUF_SIZE can be exceeded.  If
> > that doesn't matter, why even bother with VIRTIO_VSOCK_MAX_BUF_SIZE
> > here?
> > 
> 
> The .notify_buffer_size() should avoid this issue, since it allows the
> transport to limit the buffer size requested after the initialization.
> 
> But again the min set by the user can not be respected and in the
> previous implementation we forced it to VIRTIO_VSOCK_MAX_BUF_SIZE.
> 
> Now we don't limit the min, but we guarantee only that vsk->buffer_size
> is lower than VIRTIO_VSOCK_MAX_BUF_SIZE.
> 
> Can that be an acceptable compromise?

I think so.

Setting buffer sizes was never tested or used much by userspace
applications that I'm aware of.  We should probably include tests for
changing buffer sizes in the test suite.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply

* Re: [RFC PATCH 07/13] vsock: handle buffer_size sockopts in the core
From: Stefano Garzarella @ 2019-10-11  8:51 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: Stefan Hajnoczi, netdev, Sasha Levin, linux-hyperv,
	Stephen Hemminger, kvm, Michael S. Tsirkin, Dexuan Cui,
	Haiyang Zhang, linux-kernel, virtualization, David S. Miller,
	Jorgen Hansen
In-Reply-To: <20191011082714.GF12360@stefanha-x1.localdomain>

On Fri, Oct 11, 2019 at 09:27:14AM +0100, Stefan Hajnoczi wrote:
> On Thu, Oct 10, 2019 at 11:32:54AM +0200, Stefano Garzarella wrote:
> > On Wed, Oct 09, 2019 at 01:30:26PM +0100, Stefan Hajnoczi wrote:
> > > On Fri, Sep 27, 2019 at 01:26:57PM +0200, Stefano Garzarella wrote:
> > > Another issue is that this patch drops the VIRTIO_VSOCK_MAX_BUF_SIZE
> > > limit that used to be enforced by virtio_transport_set_buffer_size().
> > > Now the limit is only applied at socket init time.  If the buffer size
> > > is changed later then VIRTIO_VSOCK_MAX_BUF_SIZE can be exceeded.  If
> > > that doesn't matter, why even bother with VIRTIO_VSOCK_MAX_BUF_SIZE
> > > here?
> > > 
> > 
> > The .notify_buffer_size() should avoid this issue, since it allows the
> > transport to limit the buffer size requested after the initialization.
> > 
> > But again the min set by the user can not be respected and in the
> > previous implementation we forced it to VIRTIO_VSOCK_MAX_BUF_SIZE.
> > 
> > Now we don't limit the min, but we guarantee only that vsk->buffer_size
> > is lower than VIRTIO_VSOCK_MAX_BUF_SIZE.
> > 
> > Can that be an acceptable compromise?
> 
> I think so.
> 
> Setting buffer sizes was never tested or used much by userspace
> applications that I'm aware of.  We should probably include tests for
> changing buffer sizes in the test suite.

Good idea! We should add a test to check if min/max are respected,
playing a bit with these sockopt.

I'll do it in the test series!

Thanks,
Stefano

^ permalink raw reply

* RE: [PATCH] mm/resource: Move child to new resource when release mem region.
From: Tianyu Lan @ 2019-10-11 14:53 UTC (permalink / raw)
  To: Dave Hansen, lantianyu1986@gmail.com, dan.j.williams@intel.com,
	dave.hansen@linux.intel.com, mingo@kernel.org, mpe@ellerman.id.au,
	pasha.tatashin@soleen.com, osalvador@suse.de,
	richardw.yang@linux.intel.com, christophe.leroy@c-s.fr,
	bp@suse.de, rdunlap@infradead.org, Michael Kelley, KY Srinivasan,
	sashal@kernel.org
  Cc: linux-kernel@vger.kernel.org, vkuznets,
	linux-hyperv@vger.kernel.org
In-Reply-To: <77e08231-0687-8d8d-0faf-c490a8b510d4@intel.com>

On 10/10/2019 10:29 PM, Dave Hansen wrote:> On 10/10/19 12:28 AM, lantianyu1986@gmail.com wrote:
>> When release mem region, old mem region may be splited to
>> two regions. Current allocate new struct resource for high
>> end mem region but not move child resources whose ranges are
>> in the high end range to new resource. When adjust old mem
>> region's range, adjust_resource() detects child region's range
>> is out of new range and return error. Move child resources to
>> high end resource before adjusting old mem range.
> 
>  From the comment, it appears the old code intended to have the behavior
> that you are changing.  Could you explain _why_ this has become a
> problem for you?
Hi Dave:
    Thanks for your review. current code assumes that all children remain in
 the lower address entry for simplicity. For memory hot-remove, selecting
remove region via scanning system memory may hit case of child in the
higher address entry.

For example, the following output from /proc/iomem shows kernel code,
data and bss locate from 3a000000 to 3b5fffff and these resources are the
system ram resource's children. If the 39800000-39ffffff was selected as
remove range, the resource will be split into two ranges 00100000-397fffff
and 39800000-b87f1fff. Current code move kernel image related resources
under 00100000-397fffff resource. This will cause adjust_resource() return
error because children are not in the parent's range.

00100000-b87f1fff : System RAM
  3a000000-3ac00e80 : Kernel code
  3ac00e81-3b33883f : Kernel data
  3b4d3000-3b5fffff : Kernel bss




^ permalink raw reply

* [GIT PULL] Hyper-V commits for v5.4-rc
From: Sasha Levin @ 2019-10-11 15:01 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-kernel, linux-hyperv, kys, sthemmin, linux-kernel

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

The following changes since commit 54ecb8f7028c5eb3d740bb82b0f1d90f2df63c5c:

  Linux 5.4-rc1 (2019-09-30 10:35:40 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git tags/hyperv-fixes-signed

for you to fetch changes up to 83b50f83a96899f30c6369ef5988412fa2354ab2:

  Drivers: hv: vmbus: Fix harmless building warnings without CONFIG_PM_SLEEP (2019-10-01 14:49:45 -0400)

- ----------------------------------------------------------------
Two fixes from Dexuan Cui:

 - Fix for a (harmless) warning when building vmbus without
CONFIG_PM_SLEEP.
 - Fix for a memory leak (and optimization) in the hyperv mouse code.

- ----------------------------------------------------------------
Dexuan Cui (2):
      HID: hyperv: Use in-place iterator API in the channel callback
      Drivers: hv: vmbus: Fix harmless building warnings without CONFIG_PM_SLEEP

 drivers/hid/hid-hyperv.c | 56 +++++++++---------------------------------------
 drivers/hv/vmbus_drv.c   |  6 ++++++
 2 files changed, 16 insertions(+), 46 deletions(-)
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEE4n5dijQDou9mhzu83qZv95d3LNwFAl2gmN4ACgkQ3qZv95d3
LNxJ4Q/8DadwqP9vA75eReW1Ww55wxUprF4Vny+mtvaRYEEys4MKZwj4Dq/jSyon
D5X6WbphZZtAy+azhD5LKda6wsUZdSP4lJCX4QOq1nTGgFwzHEudXkshm0bJwJe9
B8nM7aAU0HuYi/RNBunEXoKUq5EZ7vNGaNs16iOWq30cnLhUSKNE/9fwTT3sC818
XhKJBKwZTDLYLzx8TXW3lgbhG6LG9ABKqnTLaKm0IiaGjvHSNjfdgh1hdWNOFdYE
zFPY9gT/Of7r6IM7O4G6BodGpvkkO6L+Gil6naAA9uDzasLM83NTlrFE26zYaf0l
n9diccfxOFnrstqR2yN82bKjgrfx9iv7tEODW66NeiWIvkJBzR2GfPdB9enHQ0dA
v3fTYny/XgaqnjIqj5WTIKwU+yGiwfiG0FjBhp/4ZFPRnfdg9ik0EoMkF7ZuK+37
OAbdoo6wDUns3Y49Z5WofK0okGXDKMHWuUlg2EmtQzsV5COUJyOz/bJTau0h2L+r
rp/ewkdp98+iozCyqRODDcBs9Za8pr/brME4+hYYvRiOCBunQU+RadOApev2jThe
GMNwwMclQo+AKGNmfPmq4Nul1stDpB6NIytud2pBxXqCZkAOadKRntgt/anc/1pt
XxEIfxVc6Ca1AqMOUoO3mxM6KoqjW6TmYx9it2sAk2aH9herwtM=
=9JAl
-----END PGP SIGNATURE-----

^ permalink raw reply

* Re: [GIT PULL] Hyper-V commits for v5.4-rc
From: pr-tracker-bot @ 2019-10-11 17:50 UTC (permalink / raw)
  To: Sasha Levin
  Cc: Linus Torvalds, linux-kernel, linux-hyperv, kys, sthemmin,
	linux-kernel
In-Reply-To: <20191011150106.44699206CD@mail.kernel.org>

The pull request you sent on Fri, 11 Oct 2019 11:01:05 -0400:

> git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git tags/hyperv-fixes-signed

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/56c642e2aa1c3be3e51e136eace6502aca8116ab

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker

^ permalink raw reply

* Re: [PATCH v5 0/5] Add a unified parameter "nopvspin"
From: Zhenzhong Duan @ 2019-10-12  8:40 UTC (permalink / raw)
  To: linux-kernel
  Cc: vkuznets, linux-hyperv, kvm, kys, haiyangz, sthemmin, sashal,
	tglx, mingo, bp, pbonzini, rkrcmar, sean.j.christopherson,
	wanpengli, jmattson, joro, boris.ostrovsky, jgross, sstabellini,
	peterz
In-Reply-To: <1570439071-9814-1-git-send-email-zhenzhong.duan@oracle.com>

The last two patches are reviewed, will any KVM expert be willing to 
review the first three patches?

They are all KVM related changes. Thanks

Zhenzhong

On 2019/10/7 17:04, Zhenzhong Duan wrote:
> There are cases folks want to disable spinlock optimization for
> debug/test purpose. Xen and hyperv already have parameters "xen_nopvspin"
> and "hv_nopvspin" to support that, but kvm doesn't.
>
> The first patch adds that feature to KVM guest with "nopvspin".
>
> For compatibility reason original parameters "xen_nopvspin" and
> "hv_nopvspin" are retained and marked obsolete.
>
> v5:
> PATCH1: new patch to revert a currently unnecessory commit,
>          code is simpler a bit after that change.         [Boris Ostrovsky]
> PATCH3: fold 'if' statement,add comments on virt_spin_lock_key,
>          reorder with PATCH2 to better reflect dependency
> PATCH4: fold 'if' statement, add Reviewed-by             [Boris Ostrovsky]
> PATCH5: add Reviewed-by                                  [Michael Kelley]
>
> v4:
> PATCH1: use variable name nopvspin instead of pvspin and
>          defined it as __initdata, changed print message,
>          updated patch description                     [Sean Christopherson]
> PATCH2: remove Suggested-by, use "kvm-guest:" prefix  [Sean Christopherson]
> PATCH3: make variable nopvsin and xen_pvspin coexist
>          remove Reviewed-by due to code change         [Sean Christopherson]
> PATCH4: make variable nopvsin and hv_pvspin coexist   [Sean Christopherson]
>
> v3:
> PATCH2: Fix indentation
>
> v2:
> PATCH1: pick the print code change into separate PATCH2,
>          updated patch description             [Vitaly Kuznetsov]
> PATCH2: new patch with print code change      [Vitaly Kuznetsov]
> PATCH3: add Reviewed-by                       [Juergen Gross]
>
> Zhenzhong Duan (5):
>    Revert "KVM: X86: Fix setup the virt_spin_lock_key before static key
>      get initialized"
>    x86/kvm: Change print code to use pr_*() format
>    x86/kvm: Add "nopvspin" parameter to disable PV spinlocks
>    xen: Mark "xen_nopvspin" parameter obsolete
>    x86/hyperv: Mark "hv_nopvspin" parameter obsolete
>
>   Documentation/admin-guide/kernel-parameters.txt | 14 +++++-
>   arch/x86/hyperv/hv_spinlock.c                   |  4 ++
>   arch/x86/include/asm/qspinlock.h                |  1 +
>   arch/x86/kernel/kvm.c                           | 63 ++++++++++++++-----------
>   arch/x86/xen/spinlock.c                         |  4 +-
>   kernel/locking/qspinlock.c                      |  7 +++
>   6 files changed, 62 insertions(+), 31 deletions(-)
>

^ permalink raw reply

* RE: [PATCH v2 2/3] Drivers: hv: vmbus: Enable VMBus protocol versions 4.1, 5.1 and 5.2
From: Michael Kelley @ 2019-10-12 13:34 UTC (permalink / raw)
  To: Andrea Parri, linux-kernel@vger.kernel.org,
	linux-hyperv@vger.kernel.org, netdev@vger.kernel.org
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
	David S . Miller, vkuznets, Dexuan Cui
In-Reply-To: <20191010154600.23875-3-parri.andrea@gmail.com>

From: Andrea Parri <parri.andrea@gmail.com> Sent: Thursday, October 10, 2019 8:46 AM
> 
> Hyper-V has added VMBus protocol versions 5.1 and 5.2 in recent release
> versions.  Allow Linux guests to negotiate these new protocol versions
> on versions of Hyper-V that support them.  While on this, also allow
> guests to negotiate the VMBus protocol version 4.1 (which was missing).
> 
> Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
> ---
>  drivers/hv/connection.c          | 15 +++++++++------
>  drivers/net/hyperv/netvsc.c      |  6 +++---
>  include/linux/hyperv.h           |  8 +++++++-
>  net/vmw_vsock/hyperv_transport.c |  4 ++--
>  4 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index c08b62dbd151f..a4f80e30b0207 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -182,15 +182,21 @@ static inline u32 hv_get_avail_to_write_percent(
>   * 2 . 4  (Windows 8)
>   * 3 . 0  (Windows 8 R2)
>   * 4 . 0  (Windows 10)
> + * 4 . 1  (Windows 10 RS3)
>   * 5 . 0  (Newer Windows 10)
> + * 5 . 1  (Windows 10 RS4)
> + * 5 . 2  (Windows Server 2019, RS5)
>   */
> 
>  #define VERSION_WS2008  ((0 << 16) | (13))
>  #define VERSION_WIN7    ((1 << 16) | (1))
>  #define VERSION_WIN8    ((2 << 16) | (4))
>  #define VERSION_WIN8_1    ((3 << 16) | (0))
> -#define VERSION_WIN10	((4 << 16) | (0))
> +#define VERSION_WIN10_V4 ((4 << 16) | (0))

I would recommend not changing the symbol name for version 4.0.
The change makes it more consistent with the later VERSION_WIN10_*
symbols, but it doesn't fundamentally add any clarity and I'm not sure
it's worth the churn in the other files that have to be touched. It's a
judgment call, and that's just my input.

> +#define VERSION_WIN10_V4_1 ((4 << 16) | (1))
>  #define VERSION_WIN10_V5 ((5 << 16) | (0))
> +#define VERSION_WIN10_V5_1 ((5 << 16) | (1))
> +#define VERSION_WIN10_V5_2 ((5 << 16) | (2))
> 

Michael

^ permalink raw reply

* RE: [PATCH v2 1/3] Drivers: hv: vmbus: Introduce table of VMBus protocol versions
From: Michael Kelley @ 2019-10-12 13:47 UTC (permalink / raw)
  To: Andrea Parri, linux-kernel@vger.kernel.org,
	linux-hyperv@vger.kernel.org, netdev@vger.kernel.org
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
	David S . Miller, vkuznets, Dexuan Cui
In-Reply-To: <20191010154600.23875-2-parri.andrea@gmail.com>

From: Andrea Parri <parri.andrea@gmail.com> Sent: Thursday, October 10, 2019 8:46 AM
> 
> The technique used to get the next VMBus version seems increasisly
> clumsy as the number of VMBus versions increases.  Performance is
> not a concern since this is only done once during system boot; it's
> just that we'll end up with more lines of code than is really needed.
> 
> As an alternative, introduce a table with the version numbers listed
> in order (from the most recent to the oldest).  vmbus_connect() loops
> through the versions listed in the table until it gets an accepted
> connection or gets to the end of the table (invalid version).
> 
> Suggested-by: Michael Kelley <mikelley@microsoft.com>
> Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
> ---
>  drivers/hv/connection.c | 46 ++++++++++++++---------------------------
>  drivers/hv/vmbus_drv.c  |  3 +--
>  include/linux/hyperv.h  |  4 ----
>  3 files changed, 17 insertions(+), 36 deletions(-)
> 
> @@ -244,20 +232,18 @@ int vmbus_connect(void)
>  	 * version.
>  	 */
> 
> -	version = VERSION_CURRENT;
> +	for (i = 0; i < ARRAY_SIZE(vmbus_versions); i++) {
> +		version = vmbus_versions[i];
> 
> -	do {
>  		ret = vmbus_negotiate_version(msginfo, version);
>  		if (ret == -ETIMEDOUT)
>  			goto cleanup;
> 
>  		if (vmbus_connection.conn_state == CONNECTED)
>  			break;
> +	}
> 
> -		version = vmbus_get_next_version(version);
> -	} while (version != VERSION_INVAL);
> -
> -	if (version == VERSION_INVAL)
> +	if (vmbus_connection.conn_state != CONNECTED)
>  		goto cleanup;
> 

This is a nit, but the loop exit path bugs me.  When a connection
is established, the loop is exited by the "break", and then
conn_state has to be tested again to decide whether the loop
exited due to getting a connection vs. hitting the end of the list.
Slightly cleaner in my mind would be:

	for (i=0; ; i++) {
		if (i == ARRAY_SIZE(vmbus_versions))
			goto cleanup;

		version  = vmbus_versions[i];
		ret = vmbus_negotiate_version(msginfo, version);
		if (ret == -ETIMEDOUT)
			goto cleanup;

		if (vmbus_connection.conn_state == CONNECTED)
			break;
	}

Michael

^ permalink raw reply

* [PATCH net-next v2] hv_sock: use HV_HYP_PAGE_SIZE for Hyper-V communication
From: Michael Kelley @ 2019-10-13  0:30 UTC (permalink / raw)
  To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	sashal@kernel.org, davem@davemloft.net,
	linux-hyperv@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
  Cc: himadrispandya@gmail.com

From: Himadri Pandya <himadrispandya@gmail.com>

Current code assumes PAGE_SIZE (the guest page size) is equal
to the page size used to communicate with Hyper-V (which is
always 4K). While this assumption is true on x86, it may not
be true for Hyper-V on other architectures. For example,
Linux on ARM64 may have PAGE_SIZE of 16K or 64K. A new symbol,
HV_HYP_PAGE_SIZE, has been previously introduced to use when
the Hyper-V page size is intended instead of the guest page size.

Make this code work on non-x86 architectures by using the new
HV_HYP_PAGE_SIZE symbol instead of PAGE_SIZE, where appropriate.
Also replace the now redundant PAGE_SIZE_4K with HV_HYP_PAGE_SIZE.
The change has no effect on x86, but lays the groundwork to run
on ARM64 and others.

Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
---

Changes in v2:
* Revised commit message and subject [Jakub Kicinski]

---
 net/vmw_vsock/hyperv_transport.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index 261521d..d2929ea 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -13,15 +13,16 @@
 #include <linux/hyperv.h>
 #include <net/sock.h>
 #include <net/af_vsock.h>
+#include <asm/hyperv-tlfs.h>
 
 /* Older (VMBUS version 'VERSION_WIN10' or before) Windows hosts have some
- * stricter requirements on the hv_sock ring buffer size of six 4K pages. Newer
- * hosts don't have this limitation; but, keep the defaults the same for compat.
+ * stricter requirements on the hv_sock ring buffer size of six 4K pages.
+ * hyperv-tlfs defines HV_HYP_PAGE_SIZE as 4K. Newer hosts don't have this
+ * limitation; but, keep the defaults the same for compat.
  */
-#define PAGE_SIZE_4K		4096
-#define RINGBUFFER_HVS_RCV_SIZE (PAGE_SIZE_4K * 6)
-#define RINGBUFFER_HVS_SND_SIZE (PAGE_SIZE_4K * 6)
-#define RINGBUFFER_HVS_MAX_SIZE (PAGE_SIZE_4K * 64)
+#define RINGBUFFER_HVS_RCV_SIZE (HV_HYP_PAGE_SIZE * 6)
+#define RINGBUFFER_HVS_SND_SIZE (HV_HYP_PAGE_SIZE * 6)
+#define RINGBUFFER_HVS_MAX_SIZE (HV_HYP_PAGE_SIZE * 64)
 
 /* The MTU is 16KB per the host side's design */
 #define HVS_MTU_SIZE		(1024 * 16)
@@ -54,7 +55,8 @@ struct hvs_recv_buf {
  * ringbuffer APIs that allow us to directly copy data from userspace buffer
  * to VMBus ringbuffer.
  */
-#define HVS_SEND_BUF_SIZE (PAGE_SIZE_4K - sizeof(struct vmpipe_proto_header))
+#define HVS_SEND_BUF_SIZE \
+		(HV_HYP_PAGE_SIZE - sizeof(struct vmpipe_proto_header))
 
 struct hvs_send_buf {
 	/* The header before the payload data */
@@ -393,10 +395,10 @@ static void hvs_open_connection(struct vmbus_channel *chan)
 	} else {
 		sndbuf = max_t(int, sk->sk_sndbuf, RINGBUFFER_HVS_SND_SIZE);
 		sndbuf = min_t(int, sndbuf, RINGBUFFER_HVS_MAX_SIZE);
-		sndbuf = ALIGN(sndbuf, PAGE_SIZE);
+		sndbuf = ALIGN(sndbuf, HV_HYP_PAGE_SIZE);
 		rcvbuf = max_t(int, sk->sk_rcvbuf, RINGBUFFER_HVS_RCV_SIZE);
 		rcvbuf = min_t(int, rcvbuf, RINGBUFFER_HVS_MAX_SIZE);
-		rcvbuf = ALIGN(rcvbuf, PAGE_SIZE);
+		rcvbuf = ALIGN(rcvbuf, HV_HYP_PAGE_SIZE);
 	}
 
 	ret = vmbus_open(chan, sndbuf, rcvbuf, NULL, 0, hvs_channel_cb,
@@ -670,7 +672,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk, struct msghdr *msg,
 	ssize_t ret = 0;
 	ssize_t bytes_written = 0;
 
-	BUILD_BUG_ON(sizeof(*send_buf) != PAGE_SIZE_4K);
+	BUILD_BUG_ON(sizeof(*send_buf) != HV_HYP_PAGE_SIZE);
 
 	send_buf = kmalloc(sizeof(*send_buf), GFP_KERNEL);
 	if (!send_buf)
-- 
1.8.3.1


^ permalink raw reply related

* Re: [PATCH v5 3/5] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks
From: Vitaly Kuznetsov @ 2019-10-13  9:02 UTC (permalink / raw)
  To: Zhenzhong Duan, linux-kernel
  Cc: linux-hyperv, kvm, kys, haiyangz, sthemmin, sashal, tglx, mingo,
	bp, pbonzini, rkrcmar, sean.j.christopherson, wanpengli, jmattson,
	joro, boris.ostrovsky, jgross, sstabellini, peterz,
	Zhenzhong Duan, Jonathan Corbet, H. Peter Anvin, Will Deacon
In-Reply-To: <1570439071-9814-4-git-send-email-zhenzhong.duan@oracle.com>

Zhenzhong Duan <zhenzhong.duan@oracle.com> writes:

> There are cases where a guest tries to switch spinlocks to bare metal
> behavior (e.g. by setting "xen_nopvspin" on XEN platform and
> "hv_nopvspin" on HYPER_V).
>
> That feature is missed on KVM, add a new parameter "nopvspin" to disable
> PV spinlocks for KVM guest.
>
> The new 'nopvspin' parameter will also replace Xen and Hyper-V specific
> parameters in future patches.
>
> Define variable nopvsin as global because it will be used in future
> patches as above.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krcmar <rkrcmar@redhat.com>
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> Cc: Wanpeng Li <wanpengli@tencent.com>
> Cc: Jim Mattson <jmattson@google.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Will Deacon <will@kernel.org>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  5 +++++
>  arch/x86/include/asm/qspinlock.h                |  1 +
>  arch/x86/kernel/kvm.c                           | 21 +++++++++++++++++----
>  kernel/locking/qspinlock.c                      |  7 +++++++
>  4 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index c7ac2f3..89d77ea 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5330,6 +5330,11 @@
>  			as generic guest with no PV drivers. Currently support
>  			XEN HVM, KVM, HYPER_V and VMWARE guest.
>  
> +	nopvspin	[X86,KVM]
> +			Disables the qspinlock slow path using PV optimizations
> +			which allow the hypervisor to 'idle' the guest on lock
> +			contention.
> +
>  	xirc2ps_cs=	[NET,PCMCIA]
>  			Format:
>  			<irq>,<irq_mask>,<io>,<full_duplex>,<do_sound>,<lockup_hack>[,<irq2>[,<irq3>[,<irq4>]]]
> diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
> index 444d6fd..d86ab94 100644
> --- a/arch/x86/include/asm/qspinlock.h
> +++ b/arch/x86/include/asm/qspinlock.h
> @@ -32,6 +32,7 @@ static __always_inline u32 queued_fetch_set_pending_acquire(struct qspinlock *lo
>  extern void __pv_init_lock_hash(void);
>  extern void __pv_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
>  extern void __raw_callee_save___pv_queued_spin_unlock(struct qspinlock *lock);
> +extern bool nopvspin;
>  
>  #define	queued_spin_unlock queued_spin_unlock
>  /**
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index ef836d6..6e14bd4 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -825,18 +825,31 @@ __visible bool __kvm_vcpu_is_preempted(long cpu)
>   */
>  void __init kvm_spinlock_init(void)
>  {
> -	/* Does host kernel support KVM_FEATURE_PV_UNHALT? */
> -	if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
> +	/*
> +	 * Disable PV qspinlocks if host kernel doesn't support
> +	 * KVM_FEATURE_PV_UNHALT feature or there is only 1 vCPU.
> +	 * virt_spin_lock_key is enabled to avoid lock holder
> +	 * preemption issue.
> +	 */
> +	if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) ||
> +	    num_possible_cpus() == 1) {
> +		pr_info("PV spinlocks disabled\n");

Why don't we need static_branch_disable(&virt_spin_lock_key) here?

Also, as you're printing the exact reason for PV spinlocks disablement
in other cases, I'd suggest separating "no host support" and "single
CPU" cases.

>  		return;
> +	}
>  
>  	if (kvm_para_has_hint(KVM_HINTS_REALTIME)) {
> +		pr_info("PV spinlocks disabled with KVM_HINTS_REALTIME hints.\n");
>  		static_branch_disable(&virt_spin_lock_key);
>  		return;
>  	}
>  
> -	/* Don't use the pvqspinlock code if there is only 1 vCPU. */
> -	if (num_possible_cpus() == 1)
> +	if (nopvspin) {
> +		pr_info("PV spinlocks disabled forced by \"nopvspin\" parameter.\n");

Nit: to make it sound better a comma is missing between 'disabled' and
'forced', or

"PV spinlocks forcefully disabled by ..." if you prefer.

> +		static_branch_disable(&virt_spin_lock_key);
>  		return;
> +	}
> +
> +	pr_info("PV spinlocks enabled\n");
>  
>  	__pv_init_lock_hash();
>  	pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
> index 2473f10..75193d6 100644
> --- a/kernel/locking/qspinlock.c
> +++ b/kernel/locking/qspinlock.c
> @@ -580,4 +580,11 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
>  #include "qspinlock_paravirt.h"
>  #include "qspinlock.c"
>  
> +bool nopvspin __initdata;
> +static __init int parse_nopvspin(char *arg)
> +{
> +	nopvspin = true;
> +	return 0;
> +}
> +early_param("nopvspin", parse_nopvspin);
>  #endif

-- 
Vitaly

^ permalink raw reply

* Re: [PATCH v5 2/5] x86/kvm: Change print code to use pr_*() format
From: Vitaly Kuznetsov @ 2019-10-13  9:06 UTC (permalink / raw)
  To: Zhenzhong Duan, linux-kernel
  Cc: linux-hyperv, kvm, kys, haiyangz, sthemmin, sashal, tglx, mingo,
	bp, pbonzini, rkrcmar, sean.j.christopherson, wanpengli, jmattson,
	joro, boris.ostrovsky, jgross, sstabellini, peterz,
	Zhenzhong Duan, H. Peter Anvin
In-Reply-To: <1570439071-9814-3-git-send-email-zhenzhong.duan@oracle.com>

Zhenzhong Duan <zhenzhong.duan@oracle.com> writes:

> pr_*() is preferred than printk(KERN_* ...), after change all the print
> in arch/x86/kernel/kvm.c will have "kvm_guest: xxx" style.
>
> No functional change.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krcmar <rkrcmar@redhat.com>
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> Cc: Wanpeng Li <wanpengli@tencent.com>
> Cc: Jim Mattson <jmattson@google.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> ---
>  arch/x86/kernel/kvm.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 3bc6a266..ef836d6 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -7,6 +7,8 @@
>   *   Authors: Anthony Liguori <aliguori@us.ibm.com>
>   */
>  
> +#define pr_fmt(fmt) "kvm_guest: " fmt
> +
>  #include <linux/context_tracking.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> @@ -286,8 +288,8 @@ static void kvm_register_steal_time(void)
>  		return;
>  
>  	wrmsrl(MSR_KVM_STEAL_TIME, (slow_virt_to_phys(st) | KVM_MSR_ENABLED));
> -	pr_info("kvm-stealtime: cpu %d, msr %llx\n",
> -		cpu, (unsigned long long) slow_virt_to_phys(st));
> +	pr_info("stealtime: cpu %d, msr %llx\n", cpu,
> +		(unsigned long long) slow_virt_to_phys(st));
>  }
>  
>  static DEFINE_PER_CPU_DECRYPTED(unsigned long, kvm_apic_eoi) = KVM_PV_EOI_DISABLED;
> @@ -321,8 +323,7 @@ static void kvm_guest_cpu_init(void)
>  
>  		wrmsrl(MSR_KVM_ASYNC_PF_EN, pa);
>  		__this_cpu_write(apf_reason.enabled, 1);
> -		printk(KERN_INFO"KVM setup async PF for cpu %d\n",
> -		       smp_processor_id());
> +		pr_info("setup async PF for cpu %d\n", smp_processor_id());
>  	}
>  
>  	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI)) {
> @@ -347,8 +348,7 @@ static void kvm_pv_disable_apf(void)
>  	wrmsrl(MSR_KVM_ASYNC_PF_EN, 0);
>  	__this_cpu_write(apf_reason.enabled, 0);
>  
> -	printk(KERN_INFO"Unregister pv shared memory for cpu %d\n",
> -	       smp_processor_id());
> +	pr_info("Unregister pv shared memory for cpu %d\n", smp_processor_id());
>  }
>  
>  static void kvm_pv_guest_cpu_reboot(void *unused)
> @@ -469,7 +469,8 @@ static void __send_ipi_mask(const struct cpumask *mask, int vector)
>  		} else {
>  			ret = kvm_hypercall4(KVM_HC_SEND_IPI, (unsigned long)ipi_bitmap,
>  				(unsigned long)(ipi_bitmap >> BITS_PER_LONG), min, icr);
> -			WARN_ONCE(ret < 0, "KVM: failed to send PV IPI: %ld", ret);
> +			WARN_ONCE(ret < 0, "kvm_guest: failed to send PV IPI: %ld",
> +				  ret);
>  			min = max = apic_id;
>  			ipi_bitmap = 0;
>  		}
> @@ -479,7 +480,8 @@ static void __send_ipi_mask(const struct cpumask *mask, int vector)
>  	if (ipi_bitmap) {
>  		ret = kvm_hypercall4(KVM_HC_SEND_IPI, (unsigned long)ipi_bitmap,
>  			(unsigned long)(ipi_bitmap >> BITS_PER_LONG), min, icr);
> -		WARN_ONCE(ret < 0, "KVM: failed to send PV IPI: %ld", ret);
> +		WARN_ONCE(ret < 0, "kvm_guest: failed to send PV IPI: %ld",
> +			  ret);
>  	}
>  
>  	local_irq_restore(flags);
> @@ -509,7 +511,7 @@ static void kvm_setup_pv_ipi(void)
>  {
>  	apic->send_IPI_mask = kvm_send_ipi_mask;
>  	apic->send_IPI_mask_allbutself = kvm_send_ipi_mask_allbutself;
> -	pr_info("KVM setup pv IPIs\n");
> +	pr_info("setup pv IPIs\n");

Not your fault but in WARN_ONCE() above we use 'PV' capitalized so I'd
suggest we converge on something: either capitalize them all or make
them all lowercase.

>  }
>  
>  static void kvm_smp_send_call_func_ipi(const struct cpumask *mask)
> @@ -631,11 +633,11 @@ static void __init kvm_guest_init(void)
>  	    !kvm_para_has_hint(KVM_HINTS_REALTIME) &&
>  	    kvm_para_has_feature(KVM_FEATURE_STEAL_TIME)) {
>  		smp_ops.send_call_func_ipi = kvm_smp_send_call_func_ipi;
> -		pr_info("KVM setup pv sched yield\n");
> +		pr_info("setup pv sched yield\n");

here

>  	}
>  	if (cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "x86/kvm:online",
>  				      kvm_cpu_online, kvm_cpu_down_prepare) < 0)
> -		pr_err("kvm_guest: Failed to install cpu hotplug callbacks\n");
> +		pr_err("failed to install cpu hotplug callbacks\n");
>  #else
>  	sev_map_percpu_data();
>  	kvm_guest_cpu_init();
> @@ -738,7 +740,7 @@ static __init int kvm_setup_pv_tlb_flush(void)
>  			zalloc_cpumask_var_node(per_cpu_ptr(&__pv_tlb_mask, cpu),
>  				GFP_KERNEL, cpu_to_node(cpu));
>  		}
> -		pr_info("KVM setup pv remote TLB flush\n");
> +		pr_info("setup pv remote TLB flush\n");

and here too.

>  	}
>  
>  	return 0;
> @@ -866,8 +868,8 @@ static void kvm_enable_host_haltpoll(void *i)
>  void arch_haltpoll_enable(unsigned int cpu)
>  {
>  	if (!kvm_para_has_feature(KVM_FEATURE_POLL_CONTROL)) {
> -		pr_err_once("kvm: host does not support poll control\n");
> -		pr_err_once("kvm: host upgrade recommended\n");
> +		pr_err_once("host does not support poll control\n");
> +		pr_err_once("host upgrade recommended\n");
>  		return;
>  	}

Other than the above,

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly

^ permalink raw reply

* Re: [PATCH v5 1/5] Revert "KVM: X86: Fix setup the virt_spin_lock_key before static key get initialized"
From: Vitaly Kuznetsov @ 2019-10-13  9:08 UTC (permalink / raw)
  To: Zhenzhong Duan, linux-kernel
  Cc: linux-hyperv, kvm, kys, haiyangz, sthemmin, sashal, tglx, mingo,
	bp, pbonzini, rkrcmar, sean.j.christopherson, wanpengli, jmattson,
	joro, boris.ostrovsky, jgross, sstabellini, peterz,
	Zhenzhong Duan, H. Peter Anvin
In-Reply-To: <1570439071-9814-2-git-send-email-zhenzhong.duan@oracle.com>

Zhenzhong Duan <zhenzhong.duan@oracle.com> writes:

> This reverts commit 34226b6b70980a8f81fff3c09a2c889f77edeeff.
>
> Commit 8990cac6e5ea ("x86/jump_label: Initialize static branching
> early") adds jump_label_init() call in setup_arch() to make static
> keys initialized early, so we could use the original simpler code
> again.
>
> The similar change for XEN is in commit 090d54bcbc54 ("Revert
> "x86/paravirt: Set up the virt_spin_lock_key after static keys get
> initialized"")
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krcmar <rkrcmar@redhat.com>
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> Cc: Wanpeng Li <wanpengli@tencent.com>
> Cc: Jim Mattson <jmattson@google.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> ---
>  arch/x86/kernel/kvm.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index e820568..3bc6a266 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -527,13 +527,6 @@ static void kvm_smp_send_call_func_ipi(const struct cpumask *mask)
>  	}
>  }
>  
> -static void __init kvm_smp_prepare_cpus(unsigned int max_cpus)
> -{
> -	native_smp_prepare_cpus(max_cpus);
> -	if (kvm_para_has_hint(KVM_HINTS_REALTIME))
> -		static_branch_disable(&virt_spin_lock_key);
> -}
> -
>  static void __init kvm_smp_prepare_boot_cpu(void)
>  {
>  	/*
> @@ -633,7 +626,6 @@ static void __init kvm_guest_init(void)
>  		apic_set_eoi_write(kvm_guest_apic_eoi_write);
>  
>  #ifdef CONFIG_SMP
> -	smp_ops.smp_prepare_cpus = kvm_smp_prepare_cpus;
>  	smp_ops.smp_prepare_boot_cpu = kvm_smp_prepare_boot_cpu;
>  	if (kvm_para_has_feature(KVM_FEATURE_PV_SCHED_YIELD) &&
>  	    !kvm_para_has_hint(KVM_HINTS_REALTIME) &&
> @@ -835,8 +827,10 @@ void __init kvm_spinlock_init(void)
>  	if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
>  		return;
>  
> -	if (kvm_para_has_hint(KVM_HINTS_REALTIME))
> +	if (kvm_para_has_hint(KVM_HINTS_REALTIME)) {
> +		static_branch_disable(&virt_spin_lock_key);
>  		return;
> +	}
>  
>  	/* Don't use the pvqspinlock code if there is only 1 vCPU. */
>  	if (num_possible_cpus() == 1)

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly

^ permalink raw reply

* Re: [PATCH v5 2/5] x86/kvm: Change print code to use pr_*() format
From: Zhenzhong Duan @ 2019-10-14  1:38 UTC (permalink / raw)
  To: Vitaly Kuznetsov, linux-kernel
  Cc: linux-hyperv, kvm, kys, haiyangz, sthemmin, sashal, tglx, mingo,
	bp, pbonzini, rkrcmar, sean.j.christopherson, wanpengli, jmattson,
	joro, boris.ostrovsky, jgross, sstabellini, peterz,
	H. Peter Anvin
In-Reply-To: <87lftp5819.fsf@vitty.brq.redhat.com>


On 2019/10/13 17:06, Vitaly Kuznetsov wrote:
> Zhenzhong Duan <zhenzhong.duan@oracle.com> writes:
>
>> pr_*() is preferred than printk(KERN_* ...), after change all the print
>> in arch/x86/kernel/kvm.c will have "kvm_guest: xxx" style.
>>
>> No functional change.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Radim Krcmar <rkrcmar@redhat.com>
>> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
>> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
>> Cc: Wanpeng Li <wanpengli@tencent.com>
>> Cc: Jim Mattson <jmattson@google.com>
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> ---
>>   arch/x86/kernel/kvm.c | 30 ++++++++++++++++--------------
>>   1 file changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> index 3bc6a266..ef836d6 100644
>> --- a/arch/x86/kernel/kvm.c
>> +++ b/arch/x86/kernel/kvm.c
>> @@ -7,6 +7,8 @@
>>    *   Authors: Anthony Liguori <aliguori@us.ibm.com>
>>    */
>>   
>> +#define pr_fmt(fmt) "kvm_guest: " fmt
>> +
>>   #include <linux/context_tracking.h>
>>   #include <linux/init.h>
>>   #include <linux/kernel.h>
>> @@ -286,8 +288,8 @@ static void kvm_register_steal_time(void)
>>   		return;
>>   
>>   	wrmsrl(MSR_KVM_STEAL_TIME, (slow_virt_to_phys(st) | KVM_MSR_ENABLED));
>> -	pr_info("kvm-stealtime: cpu %d, msr %llx\n",
>> -		cpu, (unsigned long long) slow_virt_to_phys(st));
>> +	pr_info("stealtime: cpu %d, msr %llx\n", cpu,
>> +		(unsigned long long) slow_virt_to_phys(st));
>>   }
>>   
>>   static DEFINE_PER_CPU_DECRYPTED(unsigned long, kvm_apic_eoi) = KVM_PV_EOI_DISABLED;
>> @@ -321,8 +323,7 @@ static void kvm_guest_cpu_init(void)
>>   
>>   		wrmsrl(MSR_KVM_ASYNC_PF_EN, pa);
>>   		__this_cpu_write(apf_reason.enabled, 1);
>> -		printk(KERN_INFO"KVM setup async PF for cpu %d\n",
>> -		       smp_processor_id());
>> +		pr_info("setup async PF for cpu %d\n", smp_processor_id());
>>   	}
>>   
>>   	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI)) {
>> @@ -347,8 +348,7 @@ static void kvm_pv_disable_apf(void)
>>   	wrmsrl(MSR_KVM_ASYNC_PF_EN, 0);
>>   	__this_cpu_write(apf_reason.enabled, 0);
>>   
>> -	printk(KERN_INFO"Unregister pv shared memory for cpu %d\n",
>> -	       smp_processor_id());
>> +	pr_info("Unregister pv shared memory for cpu %d\n", smp_processor_id());
>>   }
>>   
>>   static void kvm_pv_guest_cpu_reboot(void *unused)
>> @@ -469,7 +469,8 @@ static void __send_ipi_mask(const struct cpumask *mask, int vector)
>>   		} else {
>>   			ret = kvm_hypercall4(KVM_HC_SEND_IPI, (unsigned long)ipi_bitmap,
>>   				(unsigned long)(ipi_bitmap >> BITS_PER_LONG), min, icr);
>> -			WARN_ONCE(ret < 0, "KVM: failed to send PV IPI: %ld", ret);
>> +			WARN_ONCE(ret < 0, "kvm_guest: failed to send PV IPI: %ld",
>> +				  ret);
>>   			min = max = apic_id;
>>   			ipi_bitmap = 0;
>>   		}
>> @@ -479,7 +480,8 @@ static void __send_ipi_mask(const struct cpumask *mask, int vector)
>>   	if (ipi_bitmap) {
>>   		ret = kvm_hypercall4(KVM_HC_SEND_IPI, (unsigned long)ipi_bitmap,
>>   			(unsigned long)(ipi_bitmap >> BITS_PER_LONG), min, icr);
>> -		WARN_ONCE(ret < 0, "KVM: failed to send PV IPI: %ld", ret);
>> +		WARN_ONCE(ret < 0, "kvm_guest: failed to send PV IPI: %ld",
>> +			  ret);
>>   	}
>>   
>>   	local_irq_restore(flags);
>> @@ -509,7 +511,7 @@ static void kvm_setup_pv_ipi(void)
>>   {
>>   	apic->send_IPI_mask = kvm_send_ipi_mask;
>>   	apic->send_IPI_mask_allbutself = kvm_send_ipi_mask_allbutself;
>> -	pr_info("KVM setup pv IPIs\n");
>> +	pr_info("setup pv IPIs\n");
> Not your fault but in WARN_ONCE() above we use 'PV' capitalized so I'd
> suggest we converge on something: either capitalize them all or make
> them all lowercase.

Thanks for catching, will do with 'PV' for all print.

Zhenzhong


^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox