Message-ID: <YeGhKll2fTcTr2wS@zn.tnic>
Date:   Fri, 14 Jan 2022 17:13:30 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Michael Roth <michael.roth@....com>
Cc:     Brijesh Singh <brijesh.singh@....com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        linux-efi@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        linux-coco@...ts.linux.dev, linux-mm@...ck.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Joerg Roedel <jroedel@...e.de>,
        Tom Lendacky <thomas.lendacky@....com>,
        "H. Peter Anvin" <hpa@...or.com>, Ard Biesheuvel <ardb@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Jim Mattson <jmattson@...gle.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Sergio Lopez <slp@...hat.com>, Peter Gonda <pgonda@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
        David Rientjes <rientjes@...gle.com>,
        Dov Murik <dovmurik@...ux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@....com>,
        Vlastimil Babka <vbabka@...e.cz>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Andi Kleen <ak@...ux.intel.com>,
        "Dr . David Alan Gilbert" <dgilbert@...hat.com>,
        tony.luck@...el.com, marcorr@...gle.com,
        sathyanarayanan.kuppuswamy@...ux.intel.com
Subject: Re: [PATCH v8 29/40] x86/compressed/64: add support for SEV-SNP
 CPUID table in #VC handlers

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.

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

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.

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

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

Thx.

-- 
Regards/Gruss,
    Boris.

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

Powered by blists - more mailing lists