[PATCH v2 0/7] misc fixes to PV extentions code

[PATCH v2 0/7] misc fixes to PV extentions code

[Zhenzhong Duan]

[PATCH v2 1/7]  x86/xen: Mark xen_hvm_need_lapic() and xen_hvm_need_lapic() as __init
[PATCH v2 2/7]  x86/jailhouse: Mark jailhouse_x2apic_available as __init

Above two patches only add __init annotation to some functions, not
related to other patches. I didn't split the two out as following patches
need them to avoid conflicts.


[PATCH v2 3/7]  x86: Add nopv parameter to disable PV extensions
[PATCH v2 4/7]  Revert "xen: Introduce 'xen_nopv' to disable PV extensions for HVM guests."
[PATCH v2 5/7]  x86/xen: nopv parameter support for HVM guest

Above three patches add an unified nopv prameter used for most of hypervisor
platform except XEN PV/PVH, jailhouse. Those need PV extensions to work.

I revert 'xen_nopv' as it's same effect as nopv on XEN platform, there is also
an issue using 'xen_nopv' with PVH, we should ignore 'xen_nopv' for PVH.

[PATCH v2 6/7]  locking/spinlocks, paravirt, hyperv: Correct the hv_nopvspin case

This is a similar change as Commit e6fd28eb3522
("locking/spinlocks, paravirt, xen: Correct the xen_nopvspin case"), but for
hyperv.

[PATCH v2 7/7]  Revert "x86/paravirt: Set up the virt_spin_lock_key after static keys get initialized"

This revert an old change which is unnecessory now, I think the original change is smarter.


v2:
PATCH3 use 'ignore_nopv' for PVH/PV guest as suggested by Jgross.
PATCH5 new added one, specifically for HVM guest

Thanks
Zhenzhong

[PATCH v2 1/7] x86/xen: Mark xen_hvm_need_lapic() and xen_hvm_need_lapic() as __init

[Zhenzhong Duan]

.. as they are only called at early bootup stage. In fact, other
functions in x86_hyper_xen_hvm.init.* are all marked as __init.

Unexport xen_hvm_need_lapic as it's never used outside.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
---
 arch/x86/include/asm/xen/hypervisor.h | 6 +++---
 arch/x86/xen/enlighten_hvm.c          | 3 +--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
index 39171b3..42e1245 100644
--- a/arch/x86/include/asm/xen/hypervisor.h
+++ b/arch/x86/include/asm/xen/hypervisor.h
@@ -44,14 +44,14 @@ static inline uint32_t xen_cpuid_base(void)
 }
 
 #ifdef CONFIG_XEN
-extern bool xen_hvm_need_lapic(void);
+extern bool __init xen_hvm_need_lapic(void);
 
-static inline bool xen_x2apic_para_available(void)
+static inline bool __init xen_x2apic_para_available(void)
 {
 	return xen_hvm_need_lapic();
 }
 #else
-static inline bool xen_x2apic_para_available(void)
+static inline bool __init xen_x2apic_para_available(void)
 {
 	return (xen_cpuid_base() != 0);
 }
diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index 0e75642..ac4943c 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -218,7 +218,7 @@ static __init int xen_parse_nopv(char *arg)
 }
 early_param("xen_nopv", xen_parse_nopv);
 
-bool xen_hvm_need_lapic(void)
+bool __init xen_hvm_need_lapic(void)
 {
 	if (xen_nopv)
 		return false;
@@ -230,7 +230,6 @@ bool xen_hvm_need_lapic(void)
 		return false;
 	return true;
 }
-EXPORT_SYMBOL_GPL(xen_hvm_need_lapic);
 
 static uint32_t __init xen_platform_hvm(void)
 {
-- 
1.8.3.1

[PATCH v2 2/7] x86/jailhouse: Mark jailhouse_x2apic_available as __init

[Zhenzhong Duan]

.. as they are only called at early bootup stage.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: jailhouse-dev@googlegroups.com
---
 arch/x86/kernel/jailhouse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
index 1b2ee55..d96d563 100644
--- a/arch/x86/kernel/jailhouse.c
+++ b/arch/x86/kernel/jailhouse.c
@@ -203,7 +203,7 @@ bool jailhouse_paravirt(void)
 	return jailhouse_cpuid_base() != 0;
 }
 
-static bool jailhouse_x2apic_available(void)
+static bool __init jailhouse_x2apic_available(void)
 {
 	/*
 	 * The x2APIC is only available if the root cell enabled it. Jailhouse
-- 
1.8.3.1

[PATCH v2 3/7] x86: Add nopv parameter to disable PV extensions

[Zhenzhong Duan]

In virtualization environment, PV extensions (drivers, interrupts,
timers, etc) are enabled in the majority of use cases which is the
best option.

However, in some cases (kexec not fully working, benchmarking)
we want to disable PV extensions. As such introduce the
'nopv' parameter that will do it.

There are guest types which just won't work without PV extensions,
like Xen PV, Xen PVH and jailhouse. add a "ignore_nopv" member to
struct hypervisor_x86 set to true for those guest types and call
the detect functions only if nopv is false or ignore_nopv is true.

There is already 'xen_nopv' parameter for XEN platform but not for
others. 'xen_nopv' can then be removed with this change.

Suggested-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: xen-devel@lists.xenproject.org
---
 Documentation/admin-guide/kernel-parameters.txt |  5 +++++
 arch/x86/include/asm/hypervisor.h               |  3 +++
 arch/x86/kernel/cpu/hypervisor.c                | 11 +++++++++++
 arch/x86/kernel/jailhouse.c                     |  1 +
 arch/x86/xen/enlighten_pv.c                     |  1 +
 5 files changed, 21 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 138f666..21e08af 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5268,6 +5268,11 @@
 			improve timer resolution at the expense of processing
 			more timer interrupts.
 
+	nopv=		[X86,XEN,KVM,HYPER_V,VMWARE]
+			Disables the PV optimizations forcing the guest to run
+			as generic guest with no PV drivers. Currently support
+			XEN HVM, KVM, HYPER_V and VMWARE guest.
+
 	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/hypervisor.h b/arch/x86/include/asm/hypervisor.h
index 8c5aaba..d75d2ea 100644
--- a/arch/x86/include/asm/hypervisor.h
+++ b/arch/x86/include/asm/hypervisor.h
@@ -52,6 +52,9 @@ struct hypervisor_x86 {
 
 	/* runtime callbacks */
 	struct x86_hyper_runtime runtime;
+
+	/* ignore nopv parameter */
+	bool ignore_nopv;
 };
 
 extern enum x86_hypervisor_type x86_hyper_type;
diff --git a/arch/x86/kernel/cpu/hypervisor.c b/arch/x86/kernel/cpu/hypervisor.c
index 479ca47..337ff07 100644
--- a/arch/x86/kernel/cpu/hypervisor.c
+++ b/arch/x86/kernel/cpu/hypervisor.c
@@ -54,6 +54,14 @@
 enum x86_hypervisor_type x86_hyper_type;
 EXPORT_SYMBOL(x86_hyper_type);
 
+bool __initdata nopv;
+static __init int parse_nopv(char *arg)
+{
+	nopv = true;
+	return 0;
+}
+early_param("nopv", parse_nopv);
+
 static inline const struct hypervisor_x86 * __init
 detect_hypervisor_vendor(void)
 {
@@ -61,6 +69,9 @@
 	uint32_t pri, max_pri = 0;
 
 	for (p = hypervisors; p < hypervisors + ARRAY_SIZE(hypervisors); p++) {
+		if (unlikely(nopv) && !(*p)->ignore_nopv)
+			continue;
+
 		pri = (*p)->detect();
 		if (pri > max_pri) {
 			max_pri = pri;
diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
index d96d563..880329f 100644
--- a/arch/x86/kernel/jailhouse.c
+++ b/arch/x86/kernel/jailhouse.c
@@ -217,4 +217,5 @@ static bool __init jailhouse_x2apic_available(void)
 	.detect			= jailhouse_detect,
 	.init.init_platform	= jailhouse_init_platform,
 	.init.x2apic_available	= jailhouse_x2apic_available,
+	.ignore_nopv		= true,
 };
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 4722ba2..5d16824 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -1463,4 +1463,5 @@ static uint32_t __init xen_platform_pv(void)
 	.detect                 = xen_platform_pv,
 	.type			= X86_HYPER_XEN_PV,
 	.runtime.pin_vcpu       = xen_pin_vcpu,
+	.ignore_nopv		= true,
 };
-- 
1.8.3.1

[PATCH v2 4/7] Revert "xen: Introduce 'xen_nopv' to disable PV extensions for HVM guests."

[Zhenzhong Duan]

This reverts commit 8d693b911bb9c57009c24cb1772d205b84c7985c.

Instead we use an unified parameter 'nopv' for all the hypervisor
platforms.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: xen-devel@lists.xenproject.org
---
 Documentation/admin-guide/kernel-parameters.txt |  4 ----
 arch/x86/xen/enlighten_hvm.c                    | 12 +-----------
 2 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 21e08af..d5c3dcc 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5251,10 +5251,6 @@
 			Disables the ticketlock slowpath using Xen PV
 			optimizations.
 
-	xen_nopv	[X86]
-			Disables the PV optimizations forcing the HVM guest to
-			run as generic HVM guest with no PV drivers.
-
 	xen_scrub_pages=	[XEN]
 			Boolean option to control scrubbing pages before giving them back
 			to Xen, for use by other domains. Can be also changed at runtime
diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index ac4943c..7fcb4ea 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -210,18 +210,8 @@ static void __init xen_hvm_guest_init(void)
 #endif
 }
 
-static bool xen_nopv;
-static __init int xen_parse_nopv(char *arg)
-{
-       xen_nopv = true;
-       return 0;
-}
-early_param("xen_nopv", xen_parse_nopv);
-
 bool __init xen_hvm_need_lapic(void)
 {
-	if (xen_nopv)
-		return false;
 	if (xen_pv_domain())
 		return false;
 	if (!xen_hvm_domain())
@@ -233,7 +223,7 @@ bool __init xen_hvm_need_lapic(void)
 
 static uint32_t __init xen_platform_hvm(void)
 {
-	if (xen_pv_domain() || xen_nopv)
+	if (xen_pv_domain())
 		return 0;
 
 	return xen_cpuid_base();
-- 
1.8.3.1

[PATCH v2 5/7] x86/xen: nopv parameter support for HVM guest

[Zhenzhong Duan]

PVH guest needs PV extentions to work, so nopv parameter is ignored
for PVH but not for HVM guest.

In order for nopv parameter to take effect for HVM guest, we need to
distinguish between PVH and HVM guest early in hypervisor detection
code. By moving the detection of PVH in xen_platform_hvm(),
xen_pvh_domain() could be used for that purpose.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: xen-devel@lists.xenproject.org
---
 arch/x86/xen/enlighten_hvm.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index 7fcb4ea..26939e7 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -25,6 +25,7 @@
 #include "mmu.h"
 #include "smp.h"
 
+extern bool nopv;
 static unsigned long shared_info_pfn;
 
 void xen_hvm_init_shared_info(void)
@@ -226,20 +227,24 @@ static uint32_t __init xen_platform_hvm(void)
 	if (xen_pv_domain())
 		return 0;
 
+#ifdef CONFIG_XEN_PVH
+	/* Test for PVH domain (PVH boot path taken overrides ACPI flags). */
+	if (!x86_platform.legacy.rtc && x86_platform.legacy.no_vga)
+		xen_pvh = true;
+#endif
+
+	if (!xen_pvh_domain() && nopv)
+		return 0;
+
 	return xen_cpuid_base();
 }
 
 static __init void xen_hvm_guest_late_init(void)
 {
 #ifdef CONFIG_XEN_PVH
-	/* Test for PVH domain (PVH boot path taken overrides ACPI flags). */
-	if (!xen_pvh &&
-	    (x86_platform.legacy.rtc || !x86_platform.legacy.no_vga))
+	if (!xen_pvh)
 		return;
 
-	/* PVH detected. */
-	xen_pvh = true;
-
 	/* Make sure we don't fall back to (default) ACPI_IRQ_MODEL_PIC. */
 	if (!nr_ioapics && acpi_irq_model == ACPI_IRQ_MODEL_PIC)
 		acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM;
@@ -258,4 +263,5 @@ static __init void xen_hvm_guest_late_init(void)
 	.init.init_mem_mapping	= xen_hvm_init_mem_mapping,
 	.init.guest_late_init	= xen_hvm_guest_late_init,
 	.runtime.pin_vcpu       = xen_pin_vcpu,
+	.ignore_nopv            = true,
 };
-- 
1.8.3.1

[PATCH v2 6/7] locking/spinlocks, paravirt, hyperv: Correct the hv_nopvspin case

[Zhenzhong Duan]

With the boot parameter "hv_nopvspin" specified a Hyperv guest should
not make use of paravirt spinlocks, but behave as if running on bare
metal. This is not true, however, as the qspinlock code will fall back
to a test-and-set scheme when it is detecting a hypervisor.

In order to avoid this disable the virt_spin_lock_key.

Same change for XEN is already in Commit e6fd28eb3522
("locking/spinlocks, paravirt, xen: Correct the xen_nopvspin case")

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: Waiman Long <longman@redhat.com>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Cc: Sasha Levin <sashal@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: linux-hyperv@vger.kernel.org
---
 arch/x86/hyperv/hv_spinlock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/hyperv/hv_spinlock.c b/arch/x86/hyperv/hv_spinlock.c
index 07f21a0..d90b4b0 100644
--- a/arch/x86/hyperv/hv_spinlock.c
+++ b/arch/x86/hyperv/hv_spinlock.c
@@ -64,6 +64,9 @@ __visible bool hv_vcpu_is_preempted(int vcpu)
 
 void __init hv_init_spinlocks(void)
 {
+	if (unlikely(!hv_pvspin))
+		static_branch_disable(&virt_spin_lock_key);
+
 	if (!hv_pvspin || !apic ||
 	    !(ms_hyperv.hints & HV_X64_CLUSTER_IPI_RECOMMENDED) ||
 	    !(ms_hyperv.features & HV_X64_MSR_GUEST_IDLE_AVAILABLE)) {
-- 
1.8.3.1

[PATCH v2 7/7] Revert "x86/paravirt: Set up the virt_spin_lock_key after static keys get initialized"

[Zhenzhong Duan]

This reverts commit ca5d376e17072c1b60c3fee66f3be58ef018952d.

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.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: Waiman Long <longman@redhat.com>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Dou Liyang <douly.fnst@cn.fujitsu.com>
---
 arch/x86/kernel/smpboot.c | 3 +--
 arch/x86/xen/spinlock.c   | 6 ++----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 362dd89..44472ca 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1308,8 +1308,6 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
 	pr_info("CPU0: ");
 	print_cpu_info(&cpu_data(0));
 
-	native_pv_lock_init();
-
 	uv_system_init();
 
 	set_mtrr_aps_delayed_init();
@@ -1339,6 +1337,7 @@ void __init native_smp_prepare_boot_cpu(void)
 	/* already set me in cpu_online_mask in boot_cpu_init() */
 	cpumask_set_cpu(me, cpu_callout_mask);
 	cpu_set_state_online(me);
+	native_pv_lock_init();
 }
 
 void __init calculate_max_logical_packages(void)
diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 3776122..6deb490 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -68,11 +68,8 @@ void xen_init_lock_cpu(int cpu)
 	int irq;
 	char *name;
 
-	if (!xen_pvspin) {
-		if (cpu == 0)
-			static_branch_disable(&virt_spin_lock_key);
+	if (!xen_pvspin)
 		return;
-	}
 
 	WARN(per_cpu(lock_kicker_irq, cpu) >= 0, "spinlock on CPU%d exists on IRQ%d!\n",
 	     cpu, per_cpu(lock_kicker_irq, cpu));
@@ -124,6 +121,7 @@ void __init xen_init_spinlocks(void)
 
 	if (!xen_pvspin) {
 		printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
+		static_branch_disable(&virt_spin_lock_key);
 		return;
 	}
 	printk(KERN_DEBUG "xen: PV spinlocks enabled\n");
-- 
1.8.3.1

Re: [PATCH v2 5/7] x86/xen: nopv parameter support for HVM guest

[Juergen Gross]

On 24.06.19 14:02, Zhenzhong Duan wrote:
> PVH guest needs PV extentions to work, so nopv parameter is ignored
> for PVH but not for HVM guest.
> 
> In order for nopv parameter to take effect for HVM guest, we need to
> distinguish between PVH and HVM guest early in hypervisor detection
> code. By moving the detection of PVH in xen_platform_hvm(),
> xen_pvh_domain() could be used for that purpose.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: xen-devel@lists.xenproject.org
> ---
>   arch/x86/xen/enlighten_hvm.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
> index 7fcb4ea..26939e7 100644
> --- a/arch/x86/xen/enlighten_hvm.c
> +++ b/arch/x86/xen/enlighten_hvm.c
> @@ -25,6 +25,7 @@
>   #include "mmu.h"
>   #include "smp.h"
>   
> +extern bool nopv;
>   static unsigned long shared_info_pfn;
>   
>   void xen_hvm_init_shared_info(void)
> @@ -226,20 +227,24 @@ static uint32_t __init xen_platform_hvm(void)
>   	if (xen_pv_domain())
>   		return 0;
>   
> +#ifdef CONFIG_XEN_PVH
> +	/* Test for PVH domain (PVH boot path taken overrides ACPI flags). */
> +	if (!x86_platform.legacy.rtc && x86_platform.legacy.no_vga)
> +		xen_pvh = true;

Sorry, this won't work, as ACPI tables are scanned only some time later.

You can test for xen_pvh being true here (for the case where the guest
has been booted via the Xen-PVH boot entry) and handle that case, but
the case of a PVH guest started via the normal boot entry (like via
grub2) and nopv specified is difficult. The only idea I have right now
would be to use another struct hypervisor_x86 for that case which will
only be used for Xen HVM/PVH _and_ nopv specified. It should be a copy
of the bare metal variant, but a special guest_late_init member issuing
a big fat warning in case PVH is being detected.


Juergen

Re: [PATCH v2 5/7] x86/xen: nopv parameter support for HVM guest

[Zhenzhong Duan]

On 2019/6/25 20:31, Juergen Gross wrote:
> On 24.06.19 14:02, Zhenzhong Duan wrote:
>> PVH guest needs PV extentions to work, so nopv parameter is ignored
>> for PVH but not for HVM guest.
>>
>> In order for nopv parameter to take effect for HVM guest, we need to
>> distinguish between PVH and HVM guest early in hypervisor detection
>> code. By moving the detection of PVH in xen_platform_hvm(),
>> xen_pvh_domain() could be used for that purpose.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: Juergen Gross <jgross@suse.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: xen-devel@lists.xenproject.org
>> ---
>>   arch/x86/xen/enlighten_hvm.c | 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
>> index 7fcb4ea..26939e7 100644
>> --- a/arch/x86/xen/enlighten_hvm.c
>> +++ b/arch/x86/xen/enlighten_hvm.c
>> @@ -25,6 +25,7 @@
>>   #include "mmu.h"
>>   #include "smp.h"
>>   +extern bool nopv;
>>   static unsigned long shared_info_pfn;
>>     void xen_hvm_init_shared_info(void)
>> @@ -226,20 +227,24 @@ static uint32_t __init xen_platform_hvm(void)
>>       if (xen_pv_domain())
>>           return 0;
>>   +#ifdef CONFIG_XEN_PVH
>> +    /* Test for PVH domain (PVH boot path taken overrides ACPI 
>> flags). */
>> +    if (!x86_platform.legacy.rtc && x86_platform.legacy.no_vga)
>> +        xen_pvh = true;
>
> Sorry, this won't work, as ACPI tables are scanned only some time later.
Hmm, right. Thanks for point out.
>
> You can test for xen_pvh being true here (for the case where the guest
> has been booted via the Xen-PVH boot entry) and handle that case, but
> the case of a PVH guest started via the normal boot entry (like via
> grub2) and nopv specified is difficult. The only idea I have right now
> would be to use another struct hypervisor_x86 for that case which will
> only be used for Xen HVM/PVH _and_ nopv specified. It should be a copy
> of the bare metal variant, but a special guest_late_init member issuing
> a big fat warning in case PVH is being detected.

After that warning, I guess PVH will run into hang finally? If it's 
true, BUG() is better?

Adding another hypervisor_x86 is a bit redundant, I think of below change.

I'll test it tomorrow. But appreciate your suggestion whether it's 
feasible. Thanks

--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -25,6 +25,7 @@
  #include "mmu.h"
  #include "smp.h"

+extern bool nopv;
  static unsigned long shared_info_pfn;

  void xen_hvm_init_shared_info(void)
@@ -221,11 +222,37 @@ bool __init xen_hvm_need_lapic(void)
         return true;
  }

+static __init void xen_hvm_nopv_guest_late_init(void)
+{
+#ifdef CONFIG_XEN_PVH
+       if (x86_platform.legacy.rtc || !x86_platform.legacy.no_vga)
+               return;
+
+       /* PVH detected. */
+       xen_pvh = true;
+
+       printk(KERN_CRIT "nopv parameter isn't supported in PVH guest\n");
+       BUG();
+#endif
+}
+
+
  static uint32_t __init xen_platform_hvm(void)
  {
         if (xen_pv_domain())
                 return 0;

+       if (xen_pvh_domain() && nopv)
+       {
+       /* guest booting via the Xen-PVH boot entry goes here */
+               printk(KERN_INFO "nopv parameter is ignored in PVH 
guest\n");
+       }
+       else if (nopv)
+       {
+       /* guest booting via normal boot entry (like via grub2) goes here */
+               x86_init.hyper.guest_late_init = 
xen_hvm_nopv_guest_late_init;
+               return 0;
+       }
         return xen_cpuid_base();
  }

@@ -258,4 +285,5 @@ static __init void xen_hvm_guest_late_init(void)
         .init.init_mem_mapping  = xen_hvm_init_mem_mapping,
         .init.guest_late_init   = xen_hvm_guest_late_init,
         .runtime.pin_vcpu       = xen_pin_vcpu,
+       .ignore_nopv            = true,
  };

Re: [PATCH v2 5/7] x86/xen: nopv parameter support for HVM guest

[Juergen Gross]

On 26.06.19 10:56, Zhenzhong Duan wrote:
> 
> On 2019/6/25 20:31, Juergen Gross wrote:
>> On 24.06.19 14:02, Zhenzhong Duan wrote:
>>> PVH guest needs PV extentions to work, so nopv parameter is ignored
>>> for PVH but not for HVM guest.
>>>
>>> In order for nopv parameter to take effect for HVM guest, we need to
>>> distinguish between PVH and HVM guest early in hypervisor detection
>>> code. By moving the detection of PVH in xen_platform_hvm(),
>>> xen_pvh_domain() could be used for that purpose.
>>>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> Cc: Juergen Gross <jgross@suse.com>
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Ingo Molnar <mingo@redhat.com>
>>> Cc: Borislav Petkov <bp@alien8.de>
>>> Cc: xen-devel@lists.xenproject.org
>>> ---
>>>   arch/x86/xen/enlighten_hvm.c | 18 ++++++++++++------
>>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
>>> index 7fcb4ea..26939e7 100644
>>> --- a/arch/x86/xen/enlighten_hvm.c
>>> +++ b/arch/x86/xen/enlighten_hvm.c
>>> @@ -25,6 +25,7 @@
>>>   #include "mmu.h"
>>>   #include "smp.h"
>>>   +extern bool nopv;
>>>   static unsigned long shared_info_pfn;
>>>     void xen_hvm_init_shared_info(void)
>>> @@ -226,20 +227,24 @@ static uint32_t __init xen_platform_hvm(void)
>>>       if (xen_pv_domain())
>>>           return 0;
>>>   +#ifdef CONFIG_XEN_PVH
>>> +    /* Test for PVH domain (PVH boot path taken overrides ACPI 
>>> flags). */
>>> +    if (!x86_platform.legacy.rtc && x86_platform.legacy.no_vga)
>>> +        xen_pvh = true;
>>
>> Sorry, this won't work, as ACPI tables are scanned only some time later.
> Hmm, right. Thanks for point out.
>>
>> You can test for xen_pvh being true here (for the case where the guest
>> has been booted via the Xen-PVH boot entry) and handle that case, but
>> the case of a PVH guest started via the normal boot entry (like via
>> grub2) and nopv specified is difficult. The only idea I have right now
>> would be to use another struct hypervisor_x86 for that case which will
>> only be used for Xen HVM/PVH _and_ nopv specified. It should be a copy
>> of the bare metal variant, but a special guest_late_init member issuing
>> a big fat warning in case PVH is being detected.
> 
> After that warning, I guess PVH will run into hang finally? If it's 
> true, BUG() is better?
> 
> Adding another hypervisor_x86 is a bit redundant, I think of below change.
> 
> I'll test it tomorrow. But appreciate your suggestion whether it's 
> feasible. Thanks

Yes, this seems to be a viable option.

> 
> --- a/arch/x86/xen/enlighten_hvm.c
> +++ b/arch/x86/xen/enlighten_hvm.c
> @@ -25,6 +25,7 @@
>   #include "mmu.h"
>   #include "smp.h"
> 
> +extern bool nopv;
>   static unsigned long shared_info_pfn;
> 
>   void xen_hvm_init_shared_info(void)
> @@ -221,11 +222,37 @@ bool __init xen_hvm_need_lapic(void)
>          return true;
>   }
> 
> +static __init void xen_hvm_nopv_guest_late_init(void)
> +{
> +#ifdef CONFIG_XEN_PVH
> +       if (x86_platform.legacy.rtc || !x86_platform.legacy.no_vga)
> +               return;
> +
> +       /* PVH detected. */
> +       xen_pvh = true;
> +
> +       printk(KERN_CRIT "nopv parameter isn't supported in PVH guest\n");
> +       BUG();
> +#endif
> +}
> +
> +
>   static uint32_t __init xen_platform_hvm(void)
>   {
>          if (xen_pv_domain())
>                  return 0;
> 
> +       if (xen_pvh_domain() && nopv)
> +       {
> +       /* guest booting via the Xen-PVH boot entry goes here */

Mind adjusting indentation of that comment?

> +               printk(KERN_INFO "nopv parameter is ignored in PVH 
> guest\n");
> +       }
> +       else if (nopv)
> +       {
> +       /* guest booting via normal boot entry (like via grub2) goes 
> here */

Same again?

With those corrected and no other changes you can add my:

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

Re: [PATCH v2 3/7] x86: Add nopv parameter to disable PV extensions

[Juergen Gross]

On 24.06.19 14:02, Zhenzhong Duan wrote:
> In virtualization environment, PV extensions (drivers, interrupts,
> timers, etc) are enabled in the majority of use cases which is the
> best option.
> 
> However, in some cases (kexec not fully working, benchmarking)
> we want to disable PV extensions. As such introduce the
> 'nopv' parameter that will do it.
> 
> There are guest types which just won't work without PV extensions,
> like Xen PV, Xen PVH and jailhouse. add a "ignore_nopv" member to
> struct hypervisor_x86 set to true for those guest types and call
> the detect functions only if nopv is false or ignore_nopv is true.
> 
> There is already 'xen_nopv' parameter for XEN platform but not for
> others. 'xen_nopv' can then be removed with this change.
> 
> Suggested-by: Juergen Gross <jgross@suse.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: xen-devel@lists.xenproject.org

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

Re: [PATCH v2 4/7] Revert "xen: Introduce 'xen_nopv' to disable PV extensions for HVM guests."

[Juergen Gross]

On 24.06.19 14:02, Zhenzhong Duan wrote:
> This reverts commit 8d693b911bb9c57009c24cb1772d205b84c7985c.
> 
> Instead we use an unified parameter 'nopv' for all the hypervisor
> platforms.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: xen-devel@lists.xenproject.org

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

Re: [PATCH v2 0/7] misc fixes to PV extentions code

[Thomas Gleixner]

On Mon, 24 Jun 2019, Zhenzhong Duan wrote:

> [PATCH v2 1/7]  x86/xen: Mark xen_hvm_need_lapic() and xen_hvm_need_lapic() as __init
> [PATCH v2 2/7]  x86/jailhouse: Mark jailhouse_x2apic_available as __init
> 
> Above two patches only add __init annotation to some functions, not
> related to other patches. I didn't split the two out as following patches
> need them to avoid conflicts.

Not really. Just the XEN one conflicts. The jailhoise one is independent.

> [PATCH v2 3/7]  x86: Add nopv parameter to disable PV extensions
> [PATCH v2 4/7]  Revert "xen: Introduce 'xen_nopv' to disable PV extensions for HVM guests."
> [PATCH v2 5/7]  x86/xen: nopv parameter support for HVM guest
> 
> Above three patches add an unified nopv prameter used for most of hypervisor
> platform except XEN PV/PVH, jailhouse. Those need PV extensions to work.
> 
> I revert 'xen_nopv' as it's same effect as nopv on XEN platform, there is also
> an issue using 'xen_nopv' with PVH, we should ignore 'xen_nopv' for PVH.

Well, command line options are ABI. You cannot nilly willy remove them as
it might break existing setups.

The fact that nopv can replace xen_nopv is not a justification.

Also if there is an issue with xen_nopv and PVH then this issue needs to be
fixed first especially if that issue exists on older kernels.

> [PATCH v2 6/7]  locking/spinlocks, paravirt, hyperv: Correct the hv_nopvspin case
> 
> This is a similar change as Commit e6fd28eb3522
> ("locking/spinlocks, paravirt, xen: Correct the xen_nopvspin case"), but for
> hyperv.

This looks like an independent bug fix. Bug fixes should either be posted
seperately or at least at the beginning of the series. And this one clearly
is self contained so why hiding it in the middle of a pile of other
patches?

> [PATCH v2 7/7]  Revert "x86/paravirt: Set up the virt_spin_lock_key after static keys get initialized"
> 
> This revert an old change which is unnecessory now, I think the original change is smarter.

Again, this has nothing to do with the meat of this series which deals with
the command line parameters and is completely independent.

So you give a list of patches with some explanation for them, but the cover
letter should provide the big picture. The details of the patches need to
be in the changelog.

  1) Describe the context

     The paravirtualization whatever lack whatever they lack and have the
     shortcoming A, B, C.

     For a consistent admin experience a common command line parameter set
     across all PV guest implementations is a better choice, yada, yada
     yada.

  2) Describe the changes as overview

     To achieve this introduce a new nopv parameter which is usable by
     all PV guest implementation.

     While analyzing the PV guest code several bugs were found and
     fixed. (Patches 1 - 3). They can be applied independent of the
     functional changes, but they are kept in the series as the functional
     changes depend on them.

Documentation/process/submitting-patches.rst clearly explains why it is a
bad idea to send random collections of patches especially if some patches
are independent and contain bug fixes.

These rules exist for a reason and are not subject to personal
interpretation. You want your patches to be reviewed and merged, so pretty
please make the life of those who need to do that as easy as possible.

It's not the job of reviewers and maintainers to distangle your randomly
ordered patch series.

Thanks,

	tglx

Re: [PATCH v2 4/7] Revert "xen: Introduce 'xen_nopv' to disable PV extensions for HVM guests."

[Thomas Gleixner]

On Wed, 26 Jun 2019, Juergen Gross wrote:
> On 24.06.19 14:02, Zhenzhong Duan wrote:
> > This reverts commit 8d693b911bb9c57009c24cb1772d205b84c7985c.
> > 
> > Instead we use an unified parameter 'nopv' for all the hypervisor
> > platforms.
> > 
> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Cc: Juergen Gross <jgross@suse.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: xen-devel@lists.xenproject.org
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>

You're really sure that you want to break existing setups which might use
that already?

Command line parameters are ABI. You can map xen_nopv to the new shiny nopv
implementation but removing it?

Your decision, but you've been told :)

Thanks,

	tglx

Re: [PATCH v2 0/7] misc fixes to PV extentions code

[Zhenzhong Duan]

On 2019/6/26 21:39, Thomas Gleixner wrote:
> Documentation/process/submitting-patches.rst clearly explains why it is a
> bad idea to send random collections of patches especially if some patches
> are independent and contain bug fixes.
>
> These rules exist for a reason and are not subject to personal
> interpretation. You want your patches to be reviewed and merged, so pretty
> please make the life of those who need to do that as easy as possible.
>
> It's not the job of reviewers and maintainers to distangle your randomly
> ordered patch series.

Ok，understood.  I'll send independent and unrelated patch seperately.

Thanks

Zhenzhong

Re: [PATCH v2 0/7] misc fixes to PV extentions code

[Thomas Gleixner]

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

Zhenzhong,

On Thu, 27 Jun 2019, Zhenzhong Duan wrote:
> On 2019/6/26 21:39, Thomas Gleixner wrote:
> > Documentation/process/submitting-patches.rst clearly explains why it is a
> > bad idea to send random collections of patches especially if some patches
> > are independent and contain bug fixes.
> > 
> > These rules exist for a reason and are not subject to personal
> > interpretation. You want your patches to be reviewed and merged, so pretty
> > please make the life of those who need to do that as easy as possible.
> > 
> > It's not the job of reviewers and maintainers to distangle your randomly
> > ordered patch series.
> 
> Ok，understood.  I'll send independent and unrelated patch seperately.

Much appreciated.

Thanks,

	tglx

Re: [PATCH v2 6/7] locking/spinlocks, paravirt, hyperv: Correct the hv_nopvspin case

[Sasha Levin]

On Mon, Jun 24, 2019 at 08:02:58PM +0800, Zhenzhong Duan wrote:
>With the boot parameter "hv_nopvspin" specified a Hyperv guest should
>not make use of paravirt spinlocks, but behave as if running on bare
>metal. This is not true, however, as the qspinlock code will fall back
>to a test-and-set scheme when it is detecting a hypervisor.
>
>In order to avoid this disable the virt_spin_lock_key.
>
>Same change for XEN is already in Commit e6fd28eb3522
>("locking/spinlocks, paravirt, xen: Correct the xen_nopvspin case")
>
>Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>Cc: Waiman Long <longman@redhat.com>
>Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
>Cc: "K. Y. Srinivasan" <kys@microsoft.com>
>Cc: Haiyang Zhang <haiyangz@microsoft.com>
>Cc: Stephen Hemminger <sthemmin@microsoft.com>
>Cc: Sasha Levin <sashal@kernel.org>
>Cc: Thomas Gleixner <tglx@linutronix.de>
>Cc: Ingo Molnar <mingo@redhat.com>
>Cc: Borislav Petkov <bp@alien8.de>
>Cc: linux-hyperv@vger.kernel.org
>---
> arch/x86/hyperv/hv_spinlock.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/arch/x86/hyperv/hv_spinlock.c b/arch/x86/hyperv/hv_spinlock.c
>index 07f21a0..d90b4b0 100644
>--- a/arch/x86/hyperv/hv_spinlock.c
>+++ b/arch/x86/hyperv/hv_spinlock.c
>@@ -64,6 +64,9 @@ __visible bool hv_vcpu_is_preempted(int vcpu)
>
> void __init hv_init_spinlocks(void)
> {
>+	if (unlikely(!hv_pvspin))
>+		static_branch_disable(&virt_spin_lock_key);

This should be combined in the conditional under it, which already
attempts to disable PV spinlocks, note how hv_pvspin is checked there.
hc_pvspin isn't the only reason we would disable PV spinlocks on hyperv.

Also, there's no need for the unlikely() here, it's only getting called
once...

--
Thanks,
Sasha

Re: [PATCH v2 6/7] locking/spinlocks, paravirt, hyperv: Correct the hv_nopvspin case

[Zhenzhong Duan]

On 2019/6/28 6:28, Sasha Levin wrote:
> On Mon, Jun 24, 2019 at 08:02:58PM +0800, Zhenzhong Duan wrote:
>> With the boot parameter "hv_nopvspin" specified a Hyperv guest should
>> not make use of paravirt spinlocks, but behave as if running on bare
>> metal. This is not true, however, as the qspinlock code will fall back
>> to a test-and-set scheme when it is detecting a hypervisor.
>>
>> In order to avoid this disable the virt_spin_lock_key.
>>
>> Same change for XEN is already in Commit e6fd28eb3522
>> ("locking/spinlocks, paravirt, xen: Correct the xen_nopvspin case")
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>> Cc: Waiman Long <longman@redhat.com>
>> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
>> Cc: "K. Y. Srinivasan" <kys@microsoft.com>
>> Cc: Haiyang Zhang <haiyangz@microsoft.com>
>> Cc: Stephen Hemminger <sthemmin@microsoft.com>
>> Cc: Sasha Levin <sashal@kernel.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: linux-hyperv@vger.kernel.org
>> ---
>> arch/x86/hyperv/hv_spinlock.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/x86/hyperv/hv_spinlock.c 
>> b/arch/x86/hyperv/hv_spinlock.c
>> index 07f21a0..d90b4b0 100644
>> --- a/arch/x86/hyperv/hv_spinlock.c
>> +++ b/arch/x86/hyperv/hv_spinlock.c
>> @@ -64,6 +64,9 @@ __visible bool hv_vcpu_is_preempted(int vcpu)
>>
>> void __init hv_init_spinlocks(void)
>> {
>> +    if (unlikely(!hv_pvspin))
>> +        static_branch_disable(&virt_spin_lock_key);
>
> This should be combined in the conditional under it, which already
> attempts to disable PV spinlocks, note how hv_pvspin is checked there.
> hc_pvspin isn't the only reason we would disable PV spinlocks on hyperv.

In virt_spin_lock() there is a comment as below. The test-and-set spinlock

is an optimization to hypervisor platform when PV spinlock is unsupported.

         /*
          * On hypervisors without PARAVIRT_SPINLOCKS support we fall
          * back to a Test-and-Set spinlock, because fair locks have
          * horrible lock 'holder' preemption issues.
          */


So my understanding is:

If hv_pvspin=0 by command line, we want to behave as if running on bare 
metal(the fair locks path).

Though there is performance regression, but it's not that important when 
we use hv_pvspin=0.

If PV spinlock is disabled by other reasons, we prefer the optimization 
path.

>
> Also, there's no need for the unlikely() here, it's only getting called
> once...

Ok, I'll removed it.


Thanks

Zhenzhong