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

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

Hello Boris,

On 9/11/2026 8:53 PM, Borislav Petkov wrote:
> On Thu, Sep 10, 2026 at 10:00:08PM +0000, Ashish Kalra wrote:
>>  void snp_setup_rmpopt(void)
>>  {
>>  	u64 rmpopt_base;
>> @@ -591,6 +648,30 @@ void snp_setup_rmpopt(void)
>>  	if (!rmpopt_capable())
>>  		return;
>>  
>> +	guard(mutex)(&rmpopt_wq_mutex);
>> +
>> +	/*
>> +	 * Set up once: the workqueue and RMPOPT_BASE MSRs are left in place on
>> +	 * shutdown, so a later re-initialization just re-queues the optimization
>> +	 * pass rather than redoing the setup.
>> +	 */
>> +	if (rmpopt_wq) {
>> +		queue_delayed_work(rmpopt_wq, &rmpopt_delayed_work, 0);
>> +		return;
>> +	}
> 
> No, this is not how this is done. This is a *setup* function but you also use
> it to start the workqueue if it has been allocated already. So it should
> either setup or start but not both.
> 
> So what you do is, you try to allocate the workqueue. If it fails, you clear
> X86_FEATURE_RMPOPT so that rmpopt_capable() is false and that can be your
> start_workqueue function.
> 
> This way you get rid of all that
> 
> 	if (rmpopt_wq) 
> 
> sprinkles everywhere.
> 
>> +
>> +	/*
>> +	 * Use a dedicated per-CPU workqueue so the potentially lengthy warm-up
>> +	 * scan does not tie up a shared workqueue worker.
>> +	 */
>> +	rmpopt_wq = alloc_workqueue("rmpopt_wq", WQ_PERCPU, 1);
>> +	if (!rmpopt_wq) {
>> +		pr_err("Failed to allocate RMPOPT workqueue\n");
>> +		return;
>> +	}
>> +
>> +	INIT_DELAYED_WORK(&rmpopt_delayed_work, do_rmpopt_work);
>> +
>>  	rmpopt_pa_start = ALIGN_DOWN(PFN_PHYS(min_low_pfn), SZ_1G);
>>  	rmpopt_base = rmpopt_pa_start | MSR_AMD64_RMPOPT_ENABLE;
>>  
>> @@ -600,6 +681,15 @@ void snp_setup_rmpopt(void)
>>  	 */
>>  	for_each_cpu(cpu, cpu_primary_thread_mask)
>>  		wrmsrq_on_cpu(cpu, MSR_AMD64_RMPOPT_BASE, rmpopt_base);
>> +
>> +	rmpopt_pa_end = ALIGN(PFN_PHYS(max_pfn), SZ_1G);
>> +
>> +	if ((rmpopt_pa_end - rmpopt_pa_start) > SZ_2T)
>> +		rmpopt_pa_end = rmpopt_pa_start + SZ_2T;
>> +
>> +	queue_delayed_work(rmpopt_wq, &rmpopt_delayed_work, 0);
>> +
>> +	pr_info("RMPOPT optimizations enabled\n");
>>  }
>>  EXPORT_SYMBOL_FOR_MODULES(snp_setup_rmpopt, "ccp");
> 
> There is no ccp driver patch calling this so this export needs to happen when
> you're actually adding the ccp code.
> 
> Same thing for the snp_rmpopt_all_physmem() export to kvm-amd.
> 
> Looking at this more, I would like to get rid of the snp_setup_rmpopt() export
> and have this function do the necessary setup stuff from an initcall in this
> file. This way you set up the stuff at kernel init time and have everything
> ready to go.
> 
> Then the ccp will *only* call a function which is called snp_enable_rmpopt()
> after it has enabled SNP. That function simply enables the workqueue.
> 
> And then kvm-amd can call that function too so we end up with one export.
> 

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.

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

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.

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

> Oh, and you can zap those comments while at it:

Yes, i will fix the comments as below.

Thanks,
Ashish

> 
> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index ca99617142be..c8ba71431a5e 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
> @@ -619,7 +619,6 @@ static void rmpopt(u64 pa)
>  		     : "memory", "cc");
>  }
>  
> -/* on_each_cpu() callback: optimize the whole RMPOPT range on this CPU. */
>  static void rmpopt_scan_range(void *arg)
>  {
>  	u64 pa;
> @@ -632,7 +631,7 @@ static void do_rmpopt_work(struct work_struct *work)
>  {
>  	/*
>  	 * Warm up the RMPOPT cache on this pinned per-CPU worker with interrupts
> -	 * on, so the IRQ-disabled fan-out below only issues cache-hit RMPOPTs.
> +	 * enabled, so the IRQ-disabled fan-out below only issues cache-hit RMPOPTs.
>  	 */
>  	rmpopt_scan_range(NULL);
>  
> @@ -649,11 +648,6 @@ void snp_setup_rmpopt(void)
>  
>  	guard(mutex)(&rmpopt_wq_mutex);
>  
> -	/*
> -	 * Set up once: the workqueue and RMPOPT_BASE MSRs are left in place on
> -	 * shutdown, so a later re-initialization just re-queues the optimization
> -	 * pass rather than redoing the setup.
> -	 */
>  	if (rmpopt_wq) {
>  		queue_delayed_work(rmpopt_wq, &rmpopt_delayed_work, 0);
>  		return;
>