Re: [PATCH v14 4/5] x86/sev: Perform RMP optimizations asynchronously

["Kalra, Ashish" <ashish.kalra@amd.com>]

Hello Boris,

On 9/14/2026 6:15 PM, Borislav Petkov wrote:
> On Mon, Sep 14, 2026 at 03:00:04PM -0500, Kalra, Ashish wrote:
>> Thanks, Boris. Splitting setup from start and collapsing to a single export makes sense — a couple of constraints from the RMPOPT
>> spec shape how it has to be done.
>>
>> RMPOPT_BASE can only be written (RMPOPT_EN set) when SYSCFG[SnpEn] and RMP_CFG[SegmentedRmpEn] are both 1; otherwise the access
>> #GP(0)s. So the MSR programming can't run from an init‑time initcall — SnpEn is 0 then and it would #GP. The software setup can,
>> though, so the split becomes:
>>
>>   - an initcall in this file does the software setup — allocate the workqueue and INIT_DELAYED_WORK(), no export;
>>   - snp_enable_rmpopt() (the single export) programs RMPOPT_BASE on the primary threads and queues the pass. ccp calls it after it
>>   has enabled SNP, and kvm‑amd calls it on teardown.
> 
> This programming sure sounds like something we don't need to repeat each time
> we enable RMPOPT...
> 
> rmpopt_pa_start is practically static so I'd love it if those MSRs are written
> once and that's it. The question is, do they keep their value when we disable
> SNP?
> 
> And I can basically imagine the answer from hw folks: "yeah, yeah, maybe, but
> to be on the safe side, you should always write them after having enabled
> SNP."
> 
> Because if not, I'd be perfectly fine with us setting them on the *first* SNP
> init and not touching them again.
> 
> IOW, this pseudo:
> 
> 	if (!rdmsr(RMPOPT_BASE))
> 		wrmsr(RMPOPT_BASE, ...):
> 
> This should probably be in the snp_enable_rmpopt() function anyway as it
> should do what we want.
> 
>> The same spec text makes that single entry point safe to call repeatedly: RMPOPT_BASE_ADDR is read‑only once RMPOPT_EN is 1 (and
>> RMPOPT_EN can't be cleared while SnpEn is 1), so a later call's write is a probably a no‑op rather than a reprogram. If we want
>> to avoid even the redundant IPIs, snp_enable_rmpopt() can read RMPOPT_BASE and skip programming when RMPOPT_EN is already set — a
>> hardware‑state check instead of an if (rmpopt_wq).
> 
> Yap, or that. Sounds ok to me if it works.
> 
>> On clearing X86_FEATURE_RMPOPT when the allocation fails: that hits the problem we ran into in earlier revisions — the workqueue
>> allocation is at initcall time, after alternatives are patched, where setup_clear_cpu_cap() isn't reliable (static_cpu_has() is
>> already baked in), so clearing the cap won't flip rmpopt_capable(). The setup/enable split removes most of the if (rmpopt_wq)
>> checks anyway; the only one left is a single guard in snp_enable_rmpopt() for the (rare) allocation‑failure case, which I will
>> probably like to keep rather than rely on clearing the feature.
> 
> Or, you can introduce that bool rmpopt_enabled and clear it and test it in
> rmpopt_capable(). As long as it stays a static var, only visible in this
> compilation unit and not exported, that's good enough.
> 
>> I'll respin as v15 with the setup/enable split once we settle the feature‑clear question and the RMPOPT_BASE MSR programming
>> question (i.e., skipping it if RMPOPT_EN is already set).
> 
> Thx.
> 
> Btw, you can respin the last two patches only and send them as a reply to that
> thread - I have applied the first 3 already so no need to resend them again.
> 

One thing to sort out first, since you've already applied p1‑p3: the setup/enable rework reshapes a few things that landed
in p3 — snp_setup_rmpopt() becomes the single snp_enable_rmpopt() export, rmpopt_capable() now uses the rmpopt_enabled
guard, and the ccp caller in sev-dev.c changes with the rename. I can carry all of that as deltas in the respun p4 (i.e.
p3 adds snp_setup_rmpopt() and p4 renames/reworks it right after), but if you'd rather it land cleanly I can refresh p3
too so it introduces the final shape. Which would you prefer — p4 deltas over what's applied, or a refreshed p3 as well?

Thanks,
Ashish