Re: [PATCH v3 03/14] x86/sev: Check for the presence of an SVSM in the SNP Secrets page

[Borislav Petkov]

On Thu, Apr 18, 2024 at 04:17:36PM -0500, Tom Lendacky wrote:
> Do you want it added as a in this patch or in a documentation patch at the
> end of the series?

Either way's fine.

> > Why was that thing ever called "_layout" and not simply
> > snp_secrets_page?
> > 
> > Fix it?
> 
> Sure, I can change that as a pre-patch to the series.

Ack.

> 
> > 
> > > +	u64 caa;
> > > +
> > > +	BUILD_BUG_ON(sizeof(*secrets_page) != PAGE_SIZE);
> > 
> > Put it in the header under the struct definition I guess.
> 
> It can't stand on it's own in the header file. I'd have to put it in a
> #define or an inline function and then use that in some code. So it's
> probably best to keep it here.

You can always put it in an inline function in the header to move this
macro out of the way but ok, one macro is not too nasty yet. :-)

> > Uff, duplication.
> > 
> > Let's put them in sev-shared.c pls and avoid that.
> 
> Ok, but it will require moving some functions after the inclusion of
> sev-shared.c and then (later) adding some advance function declarations.

I guess I'll have to see it.

I get the feeling that this sev-shared.c is starting to get problematic
and we have to do some dancing to get it all to work nicely.

In this particular case, those decompressor and kernel proper variables
should probably be passed explicitly to the shared function or returned
from it so that there's no "magic" fitting of the shared function
touching external variables of the same name and thus those names are
kept the same and it all becomes fragile.

IOW:

	svsm_ca = setup_svsm_ca(...);
	svsm_ca_pa = (unsigned long)svsm_ca;

or whatever needs to happen. But you get the idea...

Thx.

-- 
Regards/Gruss,
    Boris.

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