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

[Kalra, Ashish]

On 9/8/2026 8:52 PM, Borislav Petkov wrote:
> On Tue, Sep 08, 2026 at 03:21:31PM -0500, Kalra, Ashish wrote:
>>>> Suggested-by: Thomas Lendacky <thomas.lendacky@xxxxxxx>
>>>> Suggested-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
>>>> Suggested-by: K Prateek Nayak <kprateek.nayak@xxxxxxx>
>>>> Suggested-by: Borislav Petkov (AMD) <bp@xxxxxxxxx>
>>>> Reviewed-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>
>>>> Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
>>>
>>> R-by's need to get dropped when a patch changes in more or less significant
>>> way.
>>
>> Tom and Dave gave their R-by's on v12/v13 series, so probably i will keep their
>> R-b's.
> 
> Tom gave you a R-by on v12 before I asked you to axe off a bunch of stuff from
> that patch. Dave gave you a R-by to this revision which is still under
> discussion and you wanna keep it regardless.
> 
> There's a R-by Ackerley which you carry at least since v5:
> 
> https://lore.kernel.org/r/6f1ec3d8ebcf3aaceccc099c07d0deb545dd4ab9.1779133590.git.ashish.kalra@xxxxxxx
> 
> to which Ackerley is STILL GIVING YOU review feedback ON THAT SAME THREAD:
> 
> https://lore.kernel.org/r/CAEvNRgGfyb7zvZ1u1j7YLomD%2BJdAxnVW36gtvNG9gxgZ80vMyQ@xxxxxxxxxxxxxx
> 
> and yet you're still debating.
> 
> From where I'm standing, it looks like you don't understand how those tags
> should be used.
> 
> Do you need to go refresh up on the docs:
> 
> "Both Tested-by and Reviewed-by tags, once received on mailing list from tester
> or reviewer, should be added by author to the applicable patches when sending
> next versions.  However if the patch has changed substantially in following
> version, these tags might not be applicable anymore and thus should be removed.
> Usually removal of someone's Acked-by, Tested-by or Reviewed-by tags should be
> mentioned in the patch changelog with an explanation (after the '---'
> separator)."
> 
> ?
> 
> You're using Suggested-by tags also willy-nilly:
> 
> "A Suggested-by: tag indicates that the patch idea is suggested by the person
> named and ensures credit to the person for the idea: if we diligently credit
> our idea reporters, they will, hopefully, be inspired to help us again in the
> future. Note, this is one of only three tags you might be able to use without
> explicit permission of the person named (see 'Tagging people requires
> permission' below for details)."
> 
> So all 4 people have suggested this patch?
> 
> No, ofc not. You have simply received review comments from them which you've
> decided to integrate into your patch. This doesn't need a Suggested-by tag.
> This is normal patch review process. You should try it sometimes.
> 
> And this is damn well documented but you're still debating.
> 
> Well, you can debate all you want - those patches are not going anywhere until
> you do them right. This is solely your call.

Thanks, Boris for the explanation and for quoting the documentation, I'll get these right in v14.

Tags: you're right, I was crediting reviewers as if they'd suggested the approach, when they were just reviewing.

I'll drop the Reviewed-by tags that no longer apply to this patch -Tom's (from v12, before the rework you asked for) and Ackerley's
and note the removals in the changelog under the '---'. 

On Suggested-by, I'll keep it only where the approach was actually proposed and drop the ones that
were really just review comments, including my mis-tagging of your own feedback. 

I'll apply the same cleanup across the rest of the series.

> 
>> So the fix is: keep the setup allocated once, but make setup always
>> (re)program RMPOPT_BASE — then we never need to clear it on disable, and the
>> cycle case is covered on re-init. Proposed fix: 
> 
> Yes, you basically do the *minimal* work that is absolutely necessary and
> leave everything else untouched because it is unnecessary complication to all
> the code and if one is going to toggle SNP and hotplug, then one has bigger
> problems than some leftover facilities.

Re-init / hotplug: agreed - I'll do only the minimal work that's
necessary and leave the rest untouched. Setup programs RMPOPT_BASE once
and simply re-arms the pass on re-init; the disable path only cancels
the pending work - no tearing down the queue or clearing MSRs/state.
And I'll drop the re-init reprogramming I was defending: as you say, if
SNP and hotplug are being toggled underneath this, that's a bigger
problem than some leftover facilities.

> 
>> A full‑physmem RMPOPT pass is a warm‑up scan plus an IPI fan‑out over up to 2TB. The default system_wq is per‑CPU and concurrency‑managed,
>> a long‑running item there runs on the queueing CPU's worker pool and can stall (or be stalled by) other work on that pool. So we wanted it
>> off system_wq and use a dedicated RMPOPT specific workqueue.
>>
>> Another thing i looked at is for long-running unbound work, probably the standard shared queue is system_unbound_wq, which is probably built
>> for this use case and won't clog the per-CPU system_wq.
> 
> You can't:
> 
> system_unbound_wq = alloc_workqueue("events_unbound", WQ_UNBOUND | __WQ_DEPRECATED, WQ_MAX_ACTIVE);
> 								   ^^^^^^^^^^^^^^^
> 
> 	__WQ_DEPRECATED         = 1 << 19, /* internal: workqueue is deprecated */
> 
> So if you do an unbound workqueue and then block migration around it, you're
> basically doing a WQ_PERCPU one. So why don't you do one of those and drop the
> migration toggles around it?
> 
> All this talking is to get you to get the hint that *whatever* you do, it
> needs to have a good comment above it explaining why it has been chosen this
> way. Or put that info in the commit message.
> 
> So that people who look at that code in the future, can change it after
> knowing why.
> 
> And you should not do excessive commenting - it suffices if you put a couple
> of key comments which explain non-trivial things only. The rest people can
> figure out by simply reading the code.
> 

Workqueue: As you indicated system_unbound_wq is __WQ_DEPRECATED, so I
won't use it. And you're right that migrate_disable() around the local
scan makes an unbound queue pointless, so I'll switch to a WQ_PERCPU
workqueue and drop the migration toggles. I'll add a short comment on
why it's per-CPU (the warm-up scan runs pinned and primes the shared
RMPOPT table so the on_each_cpu_mask() fan-out is only cache hits), and
trim the redundant comments rather than over-commenting.

Will respin with all of this.

Thanks,
Ashish