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

[Borislav Petkov]

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.

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

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

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette