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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

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

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