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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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?

~Andrew