Re: [PATCH v8 4/7] x86/sev: Add support to perform RMP optimizations asynchronously

[Ackerley Tng]

"Kalra, Ashish" <ashish.kalra@xxxxxxx> writes:

>
> [...snip...]
>
>
> Yes, a simpler implementation will be like this:
> ...
>
>  	if (!alloc_cpumask_var(&follower_mask, GFP_KERNEL))

Perhaps have a WARN_ON_ONCE() here so we know rmpopt was not performed?
Not a huge deal without though.

>                 return;
>
>  	cpumask_copy(follower_mask, &rmpopt_cpumask);
>
>         /*
>          * The current CPU's core always has RMPOPT_BASE programmed
>          * (snp_prepare() required all CPUs online at setup and CPU hotplug
>          * is disabled while SNP is active), so it can always be the leader.
>          * RMPOPT_BASE is per-core; exclude this core from the followers.
>          */
>         migrate_disable();
>         cpumask_andnot(follower_mask, follower_mask,
>                        topology_sibling_cpumask(smp_processor_id()));
>
>         for (pa = rmpopt_pa_start; pa < rmpopt_pa_end; pa += SZ_1G) {
>                 rmpopt(pa);
>                 cond_resched();
>         }
>         migrate_enable();
>
>         cpus_read_lock();
>         for (pa = rmpopt_pa_start; pa < rmpopt_pa_end; pa += SZ_1G) {
>                 on_each_cpu_mask(follower_mask, rmpopt_smp, (void *)pa, true);
>                 cond_resched();
>         }
>         cpus_read_unlock();
>
>         free_cpumask_var(follower_mask);
>
>

Definitely better than the version in the original patch :) Thanks!

>  Here, the leader exclusion must use the sibling mask, not clear_cpu(this_cpu). That's why my collapsed version uses:
>
>         cpumask_andnot(follower_mask, follower_mask,
>                        topology_sibling_cpumask(smp_processor_id()));
>
>   - If this_cpu is a primary: its sibling mask contains itself (the primary) -> andnot removes this core's primary from the followers.
>
>   - If this_cpu is a secondary: it isn't in follower_mask at all, but its sibling mask contains its primary, which is in
>   follower_mask -> andnot still removes this core's primary.
>
>   So either way the current core is dropped from the followers. (The old code needed two branches because case #1 used
>   clear_cpu(this_cpu) — only correct when this_cpu is the primary — while case #2 used the sibling andnot. The single andnot works for
>   both cases).
>
> Thanks,
> Ashish
>
>>> +		goto followers;
>>> +	}
>>> +
>>> +	migrate_enable();
>>> +