On Thu, Jul 10, 2025, Sohil Mehta wrote:

On 7/8/2025 11:37 AM, Sean Christopherson wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

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