[PATCH] xen: setup pv irq ops vector earlier

[PATCH] xen: setup pv irq ops vector earlier

[Juergen Gross]

Setting pv_irq_ops for Xen PV domains should be done as early as
possible in order to support e.g. very early printk() usage.

Remove the no longer necessary conditional in xen_init_irq_ops()
from PVH V1 times to make clear this is a PV only function.

Cc: <stable@vger.kernel.org> # 4.14
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/xen/enlighten_pv.c | 3 +--
 arch/x86/xen/irq.c          | 4 +---
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 8d4e2e1ae60b..0f4cd9e5bed4 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -1213,6 +1213,7 @@ asmlinkage __visible void __init xen_start_kernel(void)
 	pv_info = xen_info;
 	pv_init_ops.patch = paravirt_patch_default;
 	pv_cpu_ops = xen_cpu_ops;
+	xen_init_irq_ops();
 
 	x86_platform.get_nmi_reason = xen_get_nmi_reason;
 
@@ -1249,8 +1250,6 @@ asmlinkage __visible void __init xen_start_kernel(void)
 	get_cpu_cap(&boot_cpu_data);
 	x86_configure_nx();
 
-	xen_init_irq_ops();
-
 	/* Let's presume PV guests always boot on vCPU with id 0. */
 	per_cpu(xen_vcpu_id, 0) = 0;
 
diff --git a/arch/x86/xen/irq.c b/arch/x86/xen/irq.c
index 74179852e46c..7515a19fd324 100644
--- a/arch/x86/xen/irq.c
+++ b/arch/x86/xen/irq.c
@@ -128,8 +128,6 @@ static const struct pv_irq_ops xen_irq_ops __initconst = {
 
 void __init xen_init_irq_ops(void)
 {
-	/* For PVH we use default pv_irq_ops settings. */
-	if (!xen_feature(XENFEAT_hvm_callback_vector))
-		pv_irq_ops = xen_irq_ops;
+	pv_irq_ops = xen_irq_ops;
 	x86_init.irqs.intr_init = xen_init_IRQ;
 }
-- 
2.13.7

Re: [Xen-devel] [PATCH] xen: setup pv irq ops vector earlier

[Jan Beulich]

>>> On 02.07.18 at 12:00, <jgross@suse.com> wrote:
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -1213,6 +1213,7 @@ asmlinkage __visible void __init xen_start_kernel(void)
>  	pv_info = xen_info;
>  	pv_init_ops.patch = paravirt_patch_default;
>  	pv_cpu_ops = xen_cpu_ops;
> +	xen_init_irq_ops();

Isn't this still too late? xen_setup_machphys_mapping(), for example,
has a WARN_ON(), which implies multiple printk()s.

Jan

Re: [Xen-devel] [PATCH] xen: setup pv irq ops vector earlier

[Juergen Gross]

On 02/07/18 13:18, Jan Beulich wrote:
>>>> On 02.07.18 at 12:00, <jgross@suse.com> wrote:
>> --- a/arch/x86/xen/enlighten_pv.c
>> +++ b/arch/x86/xen/enlighten_pv.c
>> @@ -1213,6 +1213,7 @@ asmlinkage __visible void __init xen_start_kernel(void)
>>  	pv_info = xen_info;
>>  	pv_init_ops.patch = paravirt_patch_default;
>>  	pv_cpu_ops = xen_cpu_ops;
>> +	xen_init_irq_ops();
> 
> Isn't this still too late? xen_setup_machphys_mapping(), for example,
> has a WARN_ON(), which implies multiple printk()s.

Seems as if it would be a good idea to move calling
xen_setup_machphys_mapping() into xen_init_mmu_ops(). There is really
no need to do it earlier.


Juergen

Re: [PATCH] xen: setup pv irq ops vector earlier

[Boris Ostrovsky]

On 07/02/2018 06:00 AM, Juergen Gross wrote:
> Setting pv_irq_ops for Xen PV domains should be done as early as
> possible in order to support e.g. very early printk() usage.

Will printk() work as result of this move? We still, for example,
haven't set up console.

This will (probably) allow us not to crash (due to STI and such) but I
am not sure "support" is the right term here.

-boris


>
> Remove the no longer necessary conditional in xen_init_irq_ops()
> from PVH V1 times to make clear this is a PV only function.
>
> Cc: <stable@vger.kernel.org> # 4.14
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/xen/enlighten_pv.c | 3 +--
>  arch/x86/xen/irq.c          | 4 +---
>  2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index 8d4e2e1ae60b..0f4cd9e5bed4 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -1213,6 +1213,7 @@ asmlinkage __visible void __init xen_start_kernel(void)
>  	pv_info = xen_info;
>  	pv_init_ops.patch = paravirt_patch_default;
>  	pv_cpu_ops = xen_cpu_ops;
> +	xen_init_irq_ops();
>  
>  	x86_platform.get_nmi_reason = xen_get_nmi_reason;
>  
> @@ -1249,8 +1250,6 @@ asmlinkage __visible void __init xen_start_kernel(void)
>  	get_cpu_cap(&boot_cpu_data);
>  	x86_configure_nx();
>  
> -	xen_init_irq_ops();
> -
>  	/* Let's presume PV guests always boot on vCPU with id 0. */
>  	per_cpu(xen_vcpu_id, 0) = 0;
>  
> diff --git a/arch/x86/xen/irq.c b/arch/x86/xen/irq.c
> index 74179852e46c..7515a19fd324 100644
> --- a/arch/x86/xen/irq.c
> +++ b/arch/x86/xen/irq.c
> @@ -128,8 +128,6 @@ static const struct pv_irq_ops xen_irq_ops __initconst = {
>  
>  void __init xen_init_irq_ops(void)
>  {
> -	/* For PVH we use default pv_irq_ops settings. */
> -	if (!xen_feature(XENFEAT_hvm_callback_vector))
> -		pv_irq_ops = xen_irq_ops;
> +	pv_irq_ops = xen_irq_ops;
>  	x86_init.irqs.intr_init = xen_init_IRQ;
>  }

Re: [PATCH] xen: setup pv irq ops vector earlier

[Juergen Gross]

On 11/07/18 00:26, Boris Ostrovsky wrote:
> On 07/02/2018 06:00 AM, Juergen Gross wrote:
>> Setting pv_irq_ops for Xen PV domains should be done as early as
>> possible in order to support e.g. very early printk() usage.
> 
> Will printk() work as result of this move? We still, for example,
> haven't set up console.

It will print to the kernel print buffer, so the output will be
available later.

> This will (probably) allow us not to crash (due to STI and such) but I
> am not sure "support" is the right term here.

Not crashing is big plus IMO. :-)


Juergen

Re: [PATCH] xen: setup pv irq ops vector earlier

[Boris Ostrovsky]

On 07/11/2018 01:08 AM, Juergen Gross wrote:
> On 11/07/18 00:26, Boris Ostrovsky wrote:
>> On 07/02/2018 06:00 AM, Juergen Gross wrote:
>>> Setting pv_irq_ops for Xen PV domains should be done as early as
>>> possible in order to support e.g. very early printk() usage.
>> Will printk() work as result of this move? We still, for example,
>> haven't set up console.
> It will print to the kernel print buffer, so the output will be
> available later.

Right, haven't thought about it.


>
>> This will (probably) allow us not to crash (due to STI and such) but I
>> am not sure "support" is the right term here.
> Not crashing is big plus IMO. :-)

Actually, no, this is not sufficient.

You need to move xen_vcpu_info_reset(0) up as well. Otherwise you will
not be able to dereference the percpu xen_vcpu in xen_save_fl().


-boris