1. Patchew
  2. Xen
  3. [PATCH 0/4] x86: direct APIC vector adjustments
  4. View patch

[PATCH 1/4] x86/guest: move allocation of Xen upcall vector to init code

Jan Beulich posted 4 patches 3 weeks, 3 days ago
There is a newer version of this series
[PATCH 1/4] x86/guest: move allocation of Xen upcall vector to init code
Posted by Jan Beulich 3 weeks, 3 days ago 
There's no need to do this every time init_evtchn() is called. Just do it
once when setting up CPU0. Drop the assertion as well, as
alloc_hipriority_vector() (called by alloc_direct_apic_vector()) uses more
restrictive BUG_ON() anyway. Then evtchn_upcall_vector can also validly
become ro-after-init, just that it needs to move out of init_evtchn().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/guest/xen/xen.c
+++ b/xen/arch/x86/guest/xen/xen.c
@@ -233,16 +233,12 @@ static void cf_check xen_evtchn_upcall(v
     ack_APIC_irq();
 }
 
+static uint8_t __ro_after_init evtchn_upcall_vector;
+
 static int init_evtchn(void)
 {
-    static uint8_t evtchn_upcall_vector;
     int rc;
 
-    if ( !evtchn_upcall_vector )
-        alloc_direct_apic_vector(&evtchn_upcall_vector, xen_evtchn_upcall);
-
-    ASSERT(evtchn_upcall_vector);
-
     rc = xen_hypercall_set_evtchn_upcall_vector(this_cpu(vcpu_id),
                                                 evtchn_upcall_vector);
     if ( rc )
@@ -293,6 +289,8 @@ static void __init cf_check setup(void)
                XEN_LEGACY_MAX_VCPUS);
     }
 
+    alloc_direct_apic_vector(&evtchn_upcall_vector, xen_evtchn_upcall);
+
     BUG_ON(init_evtchn());
 }
Re: [PATCH 1/4] x86/guest: move allocation of Xen upcall vector to init code
Posted by Andrew Cooper 3 weeks, 2 days ago 
On 19/11/2025 10:50 am, Jan Beulich wrote:
> There's no need to do this every time init_evtchn() is called. Just do it
> once when setting up CPU0. Drop the assertion as well, as
> alloc_hipriority_vector() (called by alloc_direct_apic_vector()) uses more
> restrictive BUG_ON() anyway. Then evtchn_upcall_vector can also validly
> become ro-after-init, just that it needs to move out of init_evtchn().
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/guest/xen/xen.c
> +++ b/xen/arch/x86/guest/xen/xen.c
> @@ -233,16 +233,12 @@ static void cf_check xen_evtchn_upcall(v
>      ack_APIC_irq();
>  }
>  
> +static uint8_t __ro_after_init evtchn_upcall_vector;
> +
>  static int init_evtchn(void)
>  {
> -    static uint8_t evtchn_upcall_vector;
>      int rc;
>  
> -    if ( !evtchn_upcall_vector )
> -        alloc_direct_apic_vector(&evtchn_upcall_vector, xen_evtchn_upcall);
> -
> -    ASSERT(evtchn_upcall_vector);
> -
>      rc = xen_hypercall_set_evtchn_upcall_vector(this_cpu(vcpu_id),
>                                                  evtchn_upcall_vector);
>      if ( rc )
> @@ -293,6 +289,8 @@ static void __init cf_check setup(void)
>                 XEN_LEGACY_MAX_VCPUS);
>      }
>  
> +    alloc_direct_apic_vector(&evtchn_upcall_vector, xen_evtchn_upcall);
> +
>      BUG_ON(init_evtchn());
>  }
>  
>

This patch is fine, but it would be nicer to split init_evtchn() into
bsp_init_evtchn() and percpu_init_evtchn().

Just out of context in init_evtchn(), there's a check for CPU0 that also
ought to move into bsp_init_evtchn() (and therefore into __init), at
which point the percpu simplifies to a single hypercall, and we keep
subsystem specifics out of setup().

~Andrew
Re: [PATCH 1/4] x86/guest: move allocation of Xen upcall vector to init code
Posted by Andrew Cooper 3 weeks, 2 days ago 
On 20/11/2025 11:01 am, Andrew Cooper wrote:
> On 19/11/2025 10:50 am, Jan Beulich wrote:
>> There's no need to do this every time init_evtchn() is called. Just do it
>> once when setting up CPU0. Drop the assertion as well, as
>> alloc_hipriority_vector() (called by alloc_direct_apic_vector()) uses more
>> restrictive BUG_ON() anyway. Then evtchn_upcall_vector can also validly
>> become ro-after-init, just that it needs to move out of init_evtchn().
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/guest/xen/xen.c
>> +++ b/xen/arch/x86/guest/xen/xen.c
>> @@ -233,16 +233,12 @@ static void cf_check xen_evtchn_upcall(v
>>      ack_APIC_irq();
>>  }
>>  
>> +static uint8_t __ro_after_init evtchn_upcall_vector;
>> +
>>  static int init_evtchn(void)
>>  {
>> -    static uint8_t evtchn_upcall_vector;
>>      int rc;
>>  
>> -    if ( !evtchn_upcall_vector )
>> -        alloc_direct_apic_vector(&evtchn_upcall_vector, xen_evtchn_upcall);
>> -
>> -    ASSERT(evtchn_upcall_vector);
>> -
>>      rc = xen_hypercall_set_evtchn_upcall_vector(this_cpu(vcpu_id),
>>                                                  evtchn_upcall_vector);
>>      if ( rc )
>> @@ -293,6 +289,8 @@ static void __init cf_check setup(void)
>>                 XEN_LEGACY_MAX_VCPUS);
>>      }
>>  
>> +    alloc_direct_apic_vector(&evtchn_upcall_vector, xen_evtchn_upcall);
>> +
>>      BUG_ON(init_evtchn());
>>  }
>>  
>>
> This patch is fine, but it would be nicer to split init_evtchn() into
> bsp_init_evtchn() and percpu_init_evtchn().
>
> Just out of context in init_evtchn(), there's a check for CPU0 that also
> ought to move into bsp_init_evtchn() (and therefore into __init), at
> which point the percpu simplifies to a single hypercall, and we keep
> subsystem specifics out of setup().

No, scratch that.  HVM_PARAM_CALLBACK_IRQ is not in the list of HVM
Params that migration moves on migrate (see write_hvm_params() in
xg_sr_save_x86_hvm.c).

Everything is awful.

Could you include a comment such as /* HVM_PARAM_CALLBACK_IRQ is not
moved on migrate, so has to be set up again on resume. */ to make it
clear why that piece of logic needs to stay in a non-init function?

~Andrew
Re: [PATCH 1/4] x86/guest: move allocation of Xen upcall vector to init code
Posted by Jan Beulich 3 weeks, 2 days ago 
On 20.11.2025 12:07, Andrew Cooper wrote:
> On 20/11/2025 11:01 am, Andrew Cooper wrote:
>> On 19/11/2025 10:50 am, Jan Beulich wrote:
>>> There's no need to do this every time init_evtchn() is called. Just do it
>>> once when setting up CPU0. Drop the assertion as well, as
>>> alloc_hipriority_vector() (called by alloc_direct_apic_vector()) uses more
>>> restrictive BUG_ON() anyway. Then evtchn_upcall_vector can also validly
>>> become ro-after-init, just that it needs to move out of init_evtchn().
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/guest/xen/xen.c
>>> +++ b/xen/arch/x86/guest/xen/xen.c
>>> @@ -233,16 +233,12 @@ static void cf_check xen_evtchn_upcall(v
>>>      ack_APIC_irq();
>>>  }
>>>  
>>> +static uint8_t __ro_after_init evtchn_upcall_vector;
>>> +
>>>  static int init_evtchn(void)
>>>  {
>>> -    static uint8_t evtchn_upcall_vector;
>>>      int rc;
>>>  
>>> -    if ( !evtchn_upcall_vector )
>>> -        alloc_direct_apic_vector(&evtchn_upcall_vector, xen_evtchn_upcall);
>>> -
>>> -    ASSERT(evtchn_upcall_vector);
>>> -
>>>      rc = xen_hypercall_set_evtchn_upcall_vector(this_cpu(vcpu_id),
>>>                                                  evtchn_upcall_vector);
>>>      if ( rc )
>>> @@ -293,6 +289,8 @@ static void __init cf_check setup(void)
>>>                 XEN_LEGACY_MAX_VCPUS);
>>>      }
>>>  
>>> +    alloc_direct_apic_vector(&evtchn_upcall_vector, xen_evtchn_upcall);
>>> +
>>>      BUG_ON(init_evtchn());
>>>  }
>>>  
>>>
>> This patch is fine, but it would be nicer to split init_evtchn() into
>> bsp_init_evtchn() and percpu_init_evtchn().
>>
>> Just out of context in init_evtchn(), there's a check for CPU0 that also
>> ought to move into bsp_init_evtchn() (and therefore into __init), at
>> which point the percpu simplifies to a single hypercall, and we keep
>> subsystem specifics out of setup().
> 
> No, scratch that.  HVM_PARAM_CALLBACK_IRQ is not in the list of HVM
> Params that migration moves on migrate (see write_hvm_params() in
> xg_sr_save_x86_hvm.c).
> 
> Everything is awful.
> 
> Could you include a comment such as /* HVM_PARAM_CALLBACK_IRQ is not
> moved on migrate, so has to be set up again on resume. */ to make it
> clear why that piece of logic needs to stay in a non-init function?

It's pretty much unrelated to the change here, but yes, sure, I can add
such a comment while touching the function.

Jan
Re: [PATCH 1/4] x86/guest: move allocation of Xen upcall vector to init code
Posted by Andrew Cooper 3 weeks, 2 days ago 
On 20/11/2025 12:08 pm, Jan Beulich wrote:
> On 20.11.2025 12:07, Andrew Cooper wrote:
>> On 20/11/2025 11:01 am, Andrew Cooper wrote:
>>> On 19/11/2025 10:50 am, Jan Beulich wrote:
>>>> There's no need to do this every time init_evtchn() is called. Just do it
>>>> once when setting up CPU0. Drop the assertion as well, as
>>>> alloc_hipriority_vector() (called by alloc_direct_apic_vector()) uses more
>>>> restrictive BUG_ON() anyway. Then evtchn_upcall_vector can also validly
>>>> become ro-after-init, just that it needs to move out of init_evtchn().
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/xen/arch/x86/guest/xen/xen.c
>>>> +++ b/xen/arch/x86/guest/xen/xen.c
>>>> @@ -233,16 +233,12 @@ static void cf_check xen_evtchn_upcall(v
>>>>      ack_APIC_irq();
>>>>  }
>>>>  
>>>> +static uint8_t __ro_after_init evtchn_upcall_vector;
>>>> +
>>>>  static int init_evtchn(void)
>>>>  {
>>>> -    static uint8_t evtchn_upcall_vector;
>>>>      int rc;
>>>>  
>>>> -    if ( !evtchn_upcall_vector )
>>>> -        alloc_direct_apic_vector(&evtchn_upcall_vector, xen_evtchn_upcall);
>>>> -
>>>> -    ASSERT(evtchn_upcall_vector);
>>>> -
>>>>      rc = xen_hypercall_set_evtchn_upcall_vector(this_cpu(vcpu_id),
>>>>                                                  evtchn_upcall_vector);
>>>>      if ( rc )
>>>> @@ -293,6 +289,8 @@ static void __init cf_check setup(void)
>>>>                 XEN_LEGACY_MAX_VCPUS);
>>>>      }
>>>>  
>>>> +    alloc_direct_apic_vector(&evtchn_upcall_vector, xen_evtchn_upcall);
>>>> +
>>>>      BUG_ON(init_evtchn());
>>>>  }
>>>>  
>>>>
>>> This patch is fine, but it would be nicer to split init_evtchn() into
>>> bsp_init_evtchn() and percpu_init_evtchn().
>>>
>>> Just out of context in init_evtchn(), there's a check for CPU0 that also
>>> ought to move into bsp_init_evtchn() (and therefore into __init), at
>>> which point the percpu simplifies to a single hypercall, and we keep
>>> subsystem specifics out of setup().
>> No, scratch that.  HVM_PARAM_CALLBACK_IRQ is not in the list of HVM
>> Params that migration moves on migrate (see write_hvm_params() in
>> xg_sr_save_x86_hvm.c).
>>
>> Everything is awful.
>>
>> Could you include a comment such as /* HVM_PARAM_CALLBACK_IRQ is not
>> moved on migrate, so has to be set up again on resume. */ to make it
>> clear why that piece of logic needs to stay in a non-init function?
> It's pretty much unrelated to the change here, but yes, sure, I can add
> such a comment while touching the function.

Yes please.  It's relevant to judging which code should move out of
init_evtchn().

With that done, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.