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 <[email protected]> >>>> >>>> --- 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 <[email protected]>
