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

[PATCH 1/6] 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>
---
 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 2/6] 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: 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 3/6] 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 is already 'xen_nopv' parameter for XEN platform but not for
others. 'xen_nopv' can then be removed with this change.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: xen-devel@lists.xenproject.org
---
 Documentation/admin-guide/kernel-parameters.txt |  4 ++++
 arch/x86/kernel/cpu/hypervisor.c                | 11 +++++++++++
 2 files changed, 15 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 138f666..b352f36 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5268,6 +5268,10 @@
 			improve timer resolution at the expense of processing
 			more timer interrupts.
 
+	nopv=		[X86]
+			Disables the PV optimizations forcing the guest to run
+			as generic guest with no PV drivers.
+
 	xirc2ps_cs=	[NET,PCMCIA]
 			Format:
 			<irq>,<irq_mask>,<io>,<full_duplex>,<do_sound>,<lockup_hack>[,<irq2>[,<irq3>[,<irq4>]]]
diff --git a/arch/x86/kernel/cpu/hypervisor.c b/arch/x86/kernel/cpu/hypervisor.c
index 479ca47..4f2c875 100644
--- a/arch/x86/kernel/cpu/hypervisor.c
+++ b/arch/x86/kernel/cpu/hypervisor.c
@@ -85,10 +85,21 @@ static void __init copy_array(const void *src, void *target, unsigned int size)
 			to[i] = from[i];
 }
 
+static bool nopv;
+static __init int xen_parse_nopv(char *arg)
+{
+	nopv = true;
+	return 0;
+}
+early_param("nopv", xen_parse_nopv);
+
 void __init init_hypervisor_platform(void)
 {
 	const struct hypervisor_x86 *h;
 
+	if (nopv)
+		return;
+
 	h = detect_hypervisor_vendor();
 
 	if (!h)
-- 
1.8.3.1

[PATCH 4/6] 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: 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 b352f36..ebb75c1 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 5/6] 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: "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: 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..210495b 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 (!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 6/6] 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: 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 1/6] x86/xen: Mark xen_hvm_need_lapic() and xen_hvm_need_lapic() as __init

[Juergen Gross]

On 23.06.19 15:01, Zhenzhong Duan wrote:
> .. 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>


Juergen

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

[Juergen Gross]

On 23.06.19 15:01, 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 is already 'xen_nopv' parameter for XEN platform but not for
> others. 'xen_nopv' can then be removed with this change.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Cc: xen-devel@lists.xenproject.org
> ---
>   Documentation/admin-guide/kernel-parameters.txt |  4 ++++
>   arch/x86/kernel/cpu/hypervisor.c                | 11 +++++++++++
>   2 files changed, 15 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 138f666..b352f36 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5268,6 +5268,10 @@
>   			improve timer resolution at the expense of processing
>   			more timer interrupts.
>   
> +	nopv=		[X86]
> +			Disables the PV optimizations forcing the guest to run
> +			as generic guest with no PV drivers.
> +
>   	xirc2ps_cs=	[NET,PCMCIA]
>   			Format:
>   			<irq>,<irq_mask>,<io>,<full_duplex>,<do_sound>,<lockup_hack>[,<irq2>[,<irq3>[,<irq4>]]]
> diff --git a/arch/x86/kernel/cpu/hypervisor.c b/arch/x86/kernel/cpu/hypervisor.c
> index 479ca47..4f2c875 100644
> --- a/arch/x86/kernel/cpu/hypervisor.c
> +++ b/arch/x86/kernel/cpu/hypervisor.c
> @@ -85,10 +85,21 @@ static void __init copy_array(const void *src, void *target, unsigned int size)
>   			to[i] = from[i];
>   }
>   
> +static bool nopv;
> +static __init int xen_parse_nopv(char *arg)

You really don't want to use the "xen_" prefix here.


Juergen

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

[Juergen Gross]

On 23.06.19 15:01, 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 is already 'xen_nopv' parameter for XEN platform but not for
> others. 'xen_nopv' can then be removed with this change.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Cc: xen-devel@lists.xenproject.org
> ---
>   Documentation/admin-guide/kernel-parameters.txt |  4 ++++
>   arch/x86/kernel/cpu/hypervisor.c                | 11 +++++++++++
>   2 files changed, 15 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 138f666..b352f36 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5268,6 +5268,10 @@
>   			improve timer resolution at the expense of processing
>   			more timer interrupts.
>   
> +	nopv=		[X86]
> +			Disables the PV optimizations forcing the guest to run
> +			as generic guest with no PV drivers.
> +
>   	xirc2ps_cs=	[NET,PCMCIA]
>   			Format:
>   			<irq>,<irq_mask>,<io>,<full_duplex>,<do_sound>,<lockup_hack>[,<irq2>[,<irq3>[,<irq4>]]]
> diff --git a/arch/x86/kernel/cpu/hypervisor.c b/arch/x86/kernel/cpu/hypervisor.c
> index 479ca47..4f2c875 100644
> --- a/arch/x86/kernel/cpu/hypervisor.c
> +++ b/arch/x86/kernel/cpu/hypervisor.c
> @@ -85,10 +85,21 @@ static void __init copy_array(const void *src, void *target, unsigned int size)
>   			to[i] = from[i];
>   }
>   
> +static bool nopv;
> +static __init int xen_parse_nopv(char *arg)
> +{
> +	nopv = true;
> +	return 0;
> +}
> +early_param("nopv", xen_parse_nopv);
> +
>   void __init init_hypervisor_platform(void)
>   {
>   	const struct hypervisor_x86 *h;
>   
> +	if (nopv)
> +		return;
> +

Oh, this is no good idea.

There are guest types which just won't work without pv interfaces, like
Xen PV and Xen PVH. Letting them fail due to just a wrong command line
parameter is not nice, especially as the failure might be very hard to
track down to the issue for the user.

I guess you could add a "ignore_nopv" member to struct hypervisor_x86
set to true for the mentioned guest types and call the detect functions
only if nopv is false or ignore_nopv is true.


Juergen

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

[Zhenzhong Duan]

On 2019/6/24 21:18, Juergen Gross wrote:
> On 23.06.19 15:01, 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 is already 'xen_nopv' parameter for XEN platform but not for
>> others. 'xen_nopv' can then be removed with this change.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>> Cc: xen-devel@lists.xenproject.org
>> ---
>>   Documentation/admin-guide/kernel-parameters.txt |  4 ++++
>>   arch/x86/kernel/cpu/hypervisor.c                | 11 +++++++++++
>>   2 files changed, 15 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt 
>> b/Documentation/admin-guide/kernel-parameters.txt
>> index 138f666..b352f36 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -5268,6 +5268,10 @@
>>               improve timer resolution at the expense of processing
>>               more timer interrupts.
>>   +    nopv=        [X86]
>> +            Disables the PV optimizations forcing the guest to run
>> +            as generic guest with no PV drivers.
>> +
>>       xirc2ps_cs=    [NET,PCMCIA]
>>               Format:
>> <irq>,<irq_mask>,<io>,<full_duplex>,<do_sound>,<lockup_hack>[,<irq2>[,<irq3>[,<irq4>]]]
>> diff --git a/arch/x86/kernel/cpu/hypervisor.c 
>> b/arch/x86/kernel/cpu/hypervisor.c
>> index 479ca47..4f2c875 100644
>> --- a/arch/x86/kernel/cpu/hypervisor.c
>> +++ b/arch/x86/kernel/cpu/hypervisor.c
>> @@ -85,10 +85,21 @@ static void __init copy_array(const void *src, 
>> void *target, unsigned int size)
>>               to[i] = from[i];
>>   }
>>   +static bool nopv;
>> +static __init int xen_parse_nopv(char *arg)
>> +{
>> +    nopv = true;
>> +    return 0;
>> +}
>> +early_param("nopv", xen_parse_nopv);
>> +
>>   void __init init_hypervisor_platform(void)
>>   {
>>       const struct hypervisor_x86 *h;
>>   +    if (nopv)
>> +        return;
>> +
>
> Oh, this is no good idea.
>
> There are guest types which just won't work without pv interfaces, like
> Xen PV and Xen PVH. Letting them fail due to just a wrong command line
> parameter is not nice, especially as the failure might be very hard to
> track down to the issue for the user.
Yes, thanks for catching.
>
> I guess you could add a "ignore_nopv" member to struct hypervisor_x86
> set to true for the mentioned guest types and call the detect functions
> only if nopv is false or ignore_nopv is true.

I think your suggestion is good, I'll rework it based on your suggestion.

Thanks

Zhenzhong

Re: [PATCH 1/6] x86/xen: Mark xen_hvm_need_lapic() and xen_hvm_need_lapic() as __init

[Thomas Gleixner]

On Sun, 23 Jun 2019, Zhenzhong Duan wrote:

> .. 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.

Can you please provide a cover letter for your patch series and explain
what this whole exercise is about.

I'm seing some __init anotations in various files and then functional xen
changes which seem to be completely unrelated and independent.

Please split such stuff, e.g. the unrelated __init annotations so they can
be picked up and merged and the real functional stuff is then self
contained.

If the __init annotation touches the same code as the functional changes,
then keep them together, but please do not burden reviewers and maintainers
with guess work to decode what is what.

Thanks,

	tglx

Re: [PATCH 1/6] x86/xen: Mark xen_hvm_need_lapic() and xen_hvm_need_lapic() as __init

[Zhenzhong Duan]

On 2019/6/24 21:58, Thomas Gleixner wrote:
> On Sun, 23 Jun 2019, Zhenzhong Duan wrote:
>
>> .. 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.
> Can you please provide a cover letter for your patch series and explain
> what this whole exercise is about.

Sure, I'll send a new ones with changes you suggested and the cover letter.

Thanks

Zhenzhong