Re: [RFC PATCH v3 5/7] x86/mm: Introduce Remote Action Request

[Rik van Riel <riel@surriel.com>]

On Thu, 2025-06-05 at 21:54 +0300, Nadav Amit wrote:
> Just few small things that jump out…
> 
> > On 5 Jun 2025, at 19:35, Rik van Riel <riel@surriel.com> wrote:
> > 
> > +void rar_cpu_init(void)
> > +{
> > +	u64 r;
> > +	u8 *bitmap;
> > +	int max_payloads;
> > +	int this_cpu = smp_processor_id();
> > +
> > +	cpumask_clear(&per_cpu(rar_cpu_mask, this_cpu));
> > +
> > +	/* The MSR contains N defining the max [0-N] rar payload
> > slots. */
> > +	rdmsrl(MSR_IA32_RAR_INFO, r);
> > +	max_payloads = (r >> 32) + 1;
> > +
> > +	/* If this CPU supports less than RAR_MAX_PAYLOADS, lower
> > our limit. */
> > +	if (max_payloads < rar_max_payloads)
> > +		rar_max_payloads = max_payloads;
> 
> Unless I am missing something, this looks very racy.
> 
All the CPUs in the system should support the same
number rar_max_payloads, since they share the same
rar_action table.

> BTW: should rar_max_payloads be ro_after_init?
> 
> > +	pr_info_once("RAR: support %d payloads\n", max_payloads);
> > +
> > +	bitmap = (u8 *)per_cpu(rar_action, this_cpu);
> 
> this_cpu_ptr() would be cleaner (here and when using rar_cpu_mask).

A CPU cannot start using the rar_action table until
the wrmsl below.

That should ensure there is no race here.

Thank you for the cleanup ideas. I'll apply those for v4.

> > +	/*
> > +	 * Allow RAR events to be processed while interrupts are
> > disabled on
> > +	 * a target CPU. This prevents "pileups" where many CPUs
> > are waiting
> > +	 * on one CPU that has IRQs blocked for too long, and
> > should reduce
> > +	 * contention on the rar_payload table.
> > +	 */
> > +	r = RAR_CTRL_ENABLE | RAR_CTRL_IGNORE_IF;
> 
> Do we really need r ?

I suppose not. The original code added another
value into r, but that hangs hard with today's
microcode :)

> 
> > +	wrmsrl(MSR_IA32_RAR_CTRL, r);
> > +}
> 
> 

-- 
All Rights Reversed.