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