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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
>>>
>>> --- 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