On Thu, Jul 10, 2025, Sohil Mehta wrote:
> On 7/8/2025 11:37 AM, Sean Christopherson wrote:
>
> > This patch is buggy. There are at least two implementations of
> > ->send_IPI_mask()
> > that this breaks:
> >
>
> Thank you for point this out. I should have been more diligent.
>
>
> > Looking at all of this again, shoving the NMI source information into the
> > @vector
> > is quite brittle. Nothing forces implementations to handle embedded
> > delivery
> > mode information.
> >
>
> I agree. There is already some confusion with NMI_VECTOR and APIC_DM_NMI
> used interchangeably sometimes. Adding the new NMI-source vectors with
> the encoded delivery mode makes it worse.
>
>
> > One thought would be to pass a small struct (by value), and then provide
> > macros
> > to generate the structure for a specific vector. That provides some amount
> > of
> > type safety and should make it a bit harder to pass in garbage, without
> > making
> > the callers any less readable.
> >
> > struct apic_ipi {
> > u8 vector;
> > u8 type;
> > };
> >
>
> I am fine with this approach. Though, the changes would be massive since
> we have quite a few interfaces and a lot of "struct apic".
It'd definitely be big, but it doesn't seem like it'd be overwhelmingly painful.
Though it's certainly enough churn that I wouldn't do anything until there's a
consensus one way or the other :-)
> .send_IPI
> .send_IPI_mask
> .send_IPI_mask_allbutself
> .send_IPI_allbutself
> .send_IPI_all
> .send_IPI_self
>
>
> An option I was considering was whether we should avoid exposing the raw
> delivery mode to the callers since it is mainly an APIC internal thing.
> The callers should only have to say NMI or IRQ along with the vector and
> let the APIC code figure out how to generate it.
>
> One option is to add a separate set of send_IPI_NMI APIs parallel to
> send_IPI ones that we have. But then we would end with >10 ways to
> generate IPIs.
Yeah, that idea crossed my mind too, and I came to the same conclusion.
> Another way would be to assign the NMI vectors in a different range and
> use the range to differentiate between IRQ and NMI.
>
> For example:
> IRQ => 0x0-0xFF
> NMI => 0x10000-0x1000F.
>
> However, this would still be fragile and probably have similar issues to
> the one you pointed out.
>
> >
> > static __always_inline void __apic_send_IPI_self(struct apic_ipi ipi)
>
> Taking a step back:
>
> Since we are considering changing the interface, would it be worth
> consolidating the multiple send_IPI APIs into one or two? Mainly, by
> moving the destination information from the function name to the
> function parameter.
>
> apic_send_IPI(DEST, MASK, TYPE, VECTOR)
>
> DEST => self, all, allbutself, mask, maskbutself
>
> MASK => cpumask
>
> TYPE => IRQ, NMI
>
> VECTOR => Vector number specific to the type.
>
> I like the single line IPI invocation. All of this can still be passed
> in a neat "struct apic_ipi" with a macro helping the callers fill the
> struct.
>
> These interfaces are decades old. So, maybe I am being too ambitious and
> this isn't practically feasible. Thoughts/Suggestions?
I suspect making DEST a parameter will be a net negative. Many (most?)
implementations
will likely de-multiplex the DEST on the back end, i.e. the amount of churn will
be roughly the same, and we might end up with *more* code due to multiple
implemenations having to do the fan out.
I think we'd also end up with slightly less readable code in the callers.
> Note: Another part of me says there are only a handful of NMI IPI usages
> and the heavy lifting isn't worth it. We should fix the bugs, improve
> testing and use the existing approach since it is the least invasive :)
FWIW, I think the churn would be worthwhile in the long run. But I'm also not
volunteering to do said work...
