On 4/17/2025 4:20 PM, Thomas Gleixner wrote: > On Thu, Apr 17 2025 at 14:46, Neeraj Upadhyay wrote: >> Add update_vector callback to set/clear ALLOWED_IRR field in >> a vCPU's APIC backing page for external vectors. The ALLOWED_IRR >> field indicates the interrupt vectors which the guest allows the >> hypervisor to send (typically for emulated devices). Interrupt >> vectors used exclusively by the guest itself and the vectors which >> are not emulated by the hypervisor, such as IPI vectors, are part >> of system vectors and are not set in the ALLOWED_IRR. > > Please structure changelogs properly in paragraphs: > > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog > Ok >> arch/x86/include/asm/apic.h | 9 +++++ >> arch/x86/kernel/apic/vector.c | 53 ++++++++++++++++++++++------- >> arch/x86/kernel/apic/x2apic_savic.c | 35 +++++++++++++++++++ > > And split this patch up into two: > > 1) Do the modifications in vector.c which is what the $Subject line > says > > 2) Add the SAVIC specific bits > Ok >> @@ -471,6 +473,12 @@ static __always_inline bool apic_id_valid(u32 apic_id) >> return apic_id <= apic->max_apic_id; >> } >> >> +static __always_inline void apic_update_vector(unsigned int cpu, unsigned int vector, bool set) >> +{ >> + if (apic->update_vector) >> + apic->update_vector(cpu, vector, set); >> +} > > This is in the public header because it can? > apic_update_vector() is needed for some system vectors which are emulated/injected by Hypervisor. Patch 8 calls it for lapic timer. HYPERVISOR_CALLBACK_VECTOR would need it for hyperv. This patch series does not call apic_update_vector() for HYPERVISOR_CALLBACK_VECTOR though. Given that currently this callback is not used outside of apic code, do I need to add it to arch/x86/kernel/apic/local.h or just remove it and use conditional call in current callsites? >> -static void apic_update_vector(struct irq_data *irqd, unsigned int newvec, >> - unsigned int newcpu) >> +static int irq_alloc_vector(const struct cpumask *dest, bool resvd, unsigned int *cpu) >> +{ >> + int vector; >> + >> + vector = irq_matrix_alloc(vector_matrix, dest, resvd, cpu); > > int vector = irq_matrix_alloc(...); > Ok >> + >> + if (vector >= 0) >> + apic_update_vector(*cpu, vector, true); >> + >> + return vector; >> +} >> + >> +static int irq_alloc_managed_vector(unsigned int *cpu) >> +{ >> + int vector; >> + >> + vector = irq_matrix_alloc_managed(vector_matrix, vector_searchmask, cpu); >> + >> + if (vector >= 0) >> + apic_update_vector(*cpu, vector, true); >> + >> + return vector; >> +} > > I completely fail to see the value of these two functions. Each of them > has exactly _ONE_ call site and both sites invoke apic_update_vector() Ok, this was done to associate apic->update_vector() calls with the setup (irq_matrix_alloc()) and teardown (irq_matrix_free()) of vector, which was my understanding from your suggestion here: https://lore.kernel.org/lkml/87jz8i31dv.ffs@tglx/ . Given that there is single callsite for each and vector_configure_legacy() calls apic->update_vector() outside of alloc path, adding it to apic_update_irq_cfg(), as you suggest handles all callers. > when the allocation succeeded. Why can't you just do the obvious and > leave the existing code alone and add > > if (apic->update_vector) > apic->update_vector(); > > into apic_update_vector()? But then you have another place where you > need the update, which does not invoke apic_update_vector(). > > Now if you look deeper, then you notice that all places which invoke > apic_update_vector() invoke apic_update_irq_cfg(), which is also called > at this other place, no? > Yes, makes sense. apic_update_irq_cfg() is called for MANAGED_IRQ_SHUTDOWN_VECTOR also. I am not aware of the use case of that vector. Whethere it is an interrupt which is injected by Hypervisor. static void vector_assign_managed_shutdown(struct irq_data *irqd) { unsigned int cpu = cpumask_first(cpu_online_mask); apic_update_irq_cfg(irqd, MANAGED_IRQ_SHUTDOWN_VECTOR, cpu); } >> +static void irq_free_vector(unsigned int cpu, unsigned int vector, bool managed) >> +{ >> + apic_update_vector(cpu, vector, false); >> + irq_matrix_free(vector_matrix, cpu, vector, managed); >> +} > > This one makes sense, but please name it: apic_free_vector() > Ok > Something like the uncompiled below, no? > Ok, makes sense. Looks good. - Neeraj > Thanks, > > tglx > --- > --- a/arch/x86/kernel/apic/vector.c > +++ b/arch/x86/kernel/apic/vector.c > @@ -134,9 +134,19 @@ static void apic_update_irq_cfg(struct i > > apicd->hw_irq_cfg.vector = vector; > apicd->hw_irq_cfg.dest_apicid = apic->calc_dest_apicid(cpu); > + > + if (apic->update_vector) > + apic->update_vector(cpu, vector, true); > + > irq_data_update_effective_affinity(irqd, cpumask_of(cpu)); > - trace_vector_config(irqd->irq, vector, cpu, > - apicd->hw_irq_cfg.dest_apicid); > + trace_vector_config(irqd->irq, vector, cpu, apicd->hw_irq_cfg.dest_apicid); > +} > + > +static void apic_free_vector(unsigned int cpu, unsigned int vector, bool managed) > +{ > + if (apic->update_vector) > + apic->update_vector(cpu, vector, false); > + irq_matrix_free(vector_matrix, cpu, vector, managed); > } > > static void apic_update_vector(struct irq_data *irqd, unsigned int newvec, > @@ -174,8 +184,7 @@ static void apic_update_vector(struct ir > apicd->prev_cpu = apicd->cpu; > WARN_ON_ONCE(apicd->cpu == newcpu); > } else { > - irq_matrix_free(vector_matrix, apicd->cpu, apicd->vector, > - managed); > + apic_free_vector(apicd->cpu, apicd->vector, managed); > } > > setnew: > @@ -183,6 +192,7 @@ static void apic_update_vector(struct ir > apicd->cpu = newcpu; > BUG_ON(!IS_ERR_OR_NULL(per_cpu(vector_irq, newcpu)[newvec])); > per_cpu(vector_irq, newcpu)[newvec] = desc; > + apic_update_irq_cfg(irqd, newvec, newcpu); > } > > static void vector_assign_managed_shutdown(struct irq_data *irqd) > @@ -261,8 +271,6 @@ assign_vector_locked(struct irq_data *ir > if (vector < 0) > return vector; > apic_update_vector(irqd, vector, cpu); > - apic_update_irq_cfg(irqd, vector, cpu); > - > return 0; > } > > @@ -338,7 +346,6 @@ assign_managed_vector(struct irq_data *i > if (vector < 0) > return vector; > apic_update_vector(irqd, vector, cpu); > - apic_update_irq_cfg(irqd, vector, cpu); > return 0; > } > > @@ -357,7 +364,7 @@ static void clear_irq_vector(struct irq_ > apicd->prev_cpu); > > per_cpu(vector_irq, apicd->cpu)[vector] = VECTOR_SHUTDOWN; > - irq_matrix_free(vector_matrix, apicd->cpu, vector, managed); > + apic_free_vector(apicd->cpu, vector, managed); > apicd->vector = 0; > > /* Clean up move in progress */ > @@ -366,7 +373,7 @@ static void clear_irq_vector(struct irq_ > return; > > per_cpu(vector_irq, apicd->prev_cpu)[vector] = VECTOR_SHUTDOWN; > - irq_matrix_free(vector_matrix, apicd->prev_cpu, vector, managed); > + apic_free_vector(apicd->prev_cpu, vector, managed); > apicd->prev_vector = 0; > apicd->move_in_progress = 0; > hlist_del_init(&apicd->clist); > @@ -905,7 +912,7 @@ static void free_moved_vector(struct api > * affinity mask comes online. > */ > trace_vector_free_moved(apicd->irq, cpu, vector, managed); > - irq_matrix_free(vector_matrix, cpu, vector, managed); > + apic_free_vector(cpu, vector, managed); > per_cpu(vector_irq, cpu)[vector] = VECTOR_UNUSED; > hlist_del_init(&apicd->clist); > apicd->prev_vector = 0;
