Re: [PATCH v8 29/40] x86/compressed/64: add support for SEV-SNP CPUID table in #VC handlers

[Michael Roth <michael.roth@amd.com>]

On Fri, Jan 14, 2022 at 05:13:30PM +0100, Borislav Petkov wrote:
> On Thu, Jan 13, 2022 at 10:39:13AM -0600, Michael Roth wrote:
> > I was thinking a future hypervisor/spec might make use of this field for
> > new functionality, while still wanting to be backward-compatible with
> > existing guests, so it would be better to not enforce 0. The firmware
> > ABI does indeed document it as must-be-zero,
> 
> Maybe there's a good reason for that.
> 
> > by that seems to be more of a constraint on what a hypervisor is
> > currently allowed to place in the CPUID table, rather than something
> > the guest is meant to enforce/rely on.
> 
> So imagine whoever creates those, starts putting stuff in those fields.
> Then, in the future, the spec decides to rename those reserved/unused
> fields into something else and starts putting concrete values in them.
> I.e., it starts using them for something.
> 
> But, now the spec breaks existing usage because those fields are already
> in use. And by then it doesn't matter what the spec says - existing
> usage makes it an ABI.
> 
> So we start doing expensive and ugly workarounds just so that we don't
> break the old, undocumented use which the spec simply silently allowed,
> and accomodate that new feature the spec adds.
> 
> So no, what you're thinking is a bad bad idea.

I can certainly see that argument. I'll add checks to enforce these
cases. If it breaks an existing hypervisor implementation, it's probably
better to have that happen early based on this initial reference
implementation rather than down the road when we actually need these
fields for something.

> 
> > snp_cpuid_info_create() (which sets snp_cpuid_initialized) only gets
> > called if firmware indicates this is an SNP guests (via the cc_blob), but
> > the #VC handler still needs to know whether or not it should use the SNP
> > CPUID table still SEV-ES will still make use of it, so it uses
> > snp_cpuid_active() to make that determination.
> 
> So I went and applied the rest of the series. And I think you mean
> do_vc_no_ghcb() and it doing snp_cpuid().

Yes that's correct.

> 
> Then, looking at sev_enable() and it calling snp_init(), you fail
> further init if there's any discrepancy in the supplied data - CPUID,
> SEV status MSR, etc.
> 
> So, practically, what you wanna test in all those places is whether
> you're a SNP guest or not. Which we already have:
> 
> 	sev_status & MSR_AMD64_SEV_SNP_ENABLED
> 
> so, unless I'm missing something, you don't need yet another
> <bla>_active() helper.

Unfortunately, in sev_enable(), between the point where snp_init() is
called, and sev_status is actually set, there are a number of cpuid
intructions which will make use of do_vc_no_ghcb() prior to sev_status
being set (and it needs to happen in that order to set sev_status
appropriately). After that point, snp_cpuid_active() would no longer be
necessary, but during that span some indicator is needed in case this
is just an SEV-ES guest trigger cpuid #VCs.

> 
> > This code is calculating the total XSAVE buffer size for whatever
> > features are enabled by the guest's XCR0/XSS registers. Those feature
> > bits correspond to the 0xD subleaves 2-63, which advertise the buffer
> > size for each particular feature. So that check needs to ignore anything
> > outside that range (including 0xD subleafs 0 and 1, which would normally
> > provide this total size dynamically based on current values of XCR0/XSS,
> > but here are instead calculated "manually" since we are not relying on
> > the XCR0_IN/XSS_IN fields in the table (due to the reasons mentioned
> > earlier in this thread).
> 
> Yah, the gist of that needs to be as a comment of that line as it is not
> obvious (at least to me).
> 
> > Not duplicate entries (though there's technically nothing in the spec
> > that says you can't), but I was more concerned here with multiple
> > entries corresponding to different combination of XCR0_IN/XSS_IN.
> > There's no good reason for a hypervisor to use those fields for anything
> > other than 0xD subleaves 0 and 1, but a hypervisor could in theory encode
> > 1 "duplicate" sub-leaf for each possible combination of XCR0_IN/XSS_IN,
> > similar to what it might do for subleaves 0 and 1, and not violate the
> > spec.
> 
> 
> Ditto. Also a comment ontop please.

Will do.

> 
> > The current spec is a bit open-ended in some of these areas so the guest
> > code is trying to be as agnostic as possible to the underlying implementation
> > so there's less chance of breakage running on one hypervisor verses
> > another. We're working on updating the spec to encourage better
> > interoperability, but that would likely only be enforceable for future
> > firmware versions/guests.
> 
> This has the same theoretical problem as the reserved/unused fields. If
> you don't enforce it, people will do whatever and once it is implemented
> in hypervisors and it has become an ABI, you can't change it anymore.
> 
> So I'd very strongly suggest you tighten in up upfront and only allow
> stuff later, when it makes sense. Not the other way around.

Yah, I was trying to avoid causing issues for other early
implementations trying to make do with the current open-ended spec, but
that's probably a losing battle and it's probably better to try to get
everyone using the proposed reference implementation early on. I'll tighten
the code up in this regard and also send a new version of reference
implementation document to SNP mailing list for awareness.

> 
> Thx.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&amp;data=04%7C01%7Cmichael.roth%40amd.com%7Cb2fefc5c0458441d2c9508d9d778cf46%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637777736172270637%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=lCq1wBm8iyW1AOorhL35om6gU9GEypzCksiFZUI3H%2Fk%3D&amp;reserved=0