Re: [PATCH v1 04/26] x86/sev: Add the host SEV-SNP initialization support

[Borislav Petkov]

On Tue, Jan 09, 2024 at 12:56:17PM +0100, Jeremi Piotrowski wrote:
> Can we please not assume I am acting in bad faith.

No you're not acting with bad faith.

What you're doing, in my experience so far is, you come with some weird
HV + guest models which has been invented somewhere, behind some closed
doors, then you come with some desire that the upstream kernel should
support it and you're not even documenting it properly and I'm left with
asking questions all the time, what is this, what's the use case,
blabla.

Don't take this personally - I guess this is all due to NDAs,
development schedules, and whatever else and yes, I've heard it all.

But just because you want this, we're not going to jump on it and
support it unconditionally. It needs to integrate properly with the rest
of the kernel and if it doesn't, it is not going upstream. That simple.

> I am explicitly trying to integrate nicely with AMD's KVM SNP host
> patches to cover an additional usecase and get something upstreamable.

And yet I still have no clue what your use case is. I always have to go
ask behind the scenes and get some half-answers about *maybe* this is
what they support.

Looking at the patch you pointed at I see there a proper explanation of
your nested SNP stuff. Finally!

>From now on, please make sure your use case is properly explained
before you come with patches.

> The RMP in nested SNP is only used for kernel bookkeeping and so its
> allocation is optional. KVM could do without reading the RMP directly
> altogether (by tracking the assigned bit somewhere) but that would be
> a design change and I'd rather see the KVM SNP host patches merged in
> their current shape. Which is why the patch I linked allocates
> a (shadow) RMP from the kernel.

At least three issues I see with that:

- the allocation can fail so it is a lot more convenient when the
  firmware prepares it

- the RMP_BASE and RMP_END writes need to be verified they actially did
  set up the RMP range because if they haven't, you might as well
  throw SNP security out of the window. In general, letting the kernel
  do the RMP allocation needs to be verified very very thoroughly.

- a future feature might make this more complicated

> I would very much appreciate if we would not prevent that usecase from
> working - that's why I've been reviewing and testing multiple
> revisions of these patches and providing feedback all along.

I very much appreciate the help but we need to get the main SNP host
stuff in first and then we can talk about modifications.

-- 
Regards/Gruss,
    Boris.

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