Re: [PATCH] sev: allow capabilities to check for SEV-ES support

[Daniel P . Berrangé]

On Tue, Nov 16, 2021 at 11:58:12AM -0500, Tyler Fanelli wrote:
> On 11/16/21 10:53 AM, Daniel P. Berrangé wrote:
> > On Tue, Nov 16, 2021 at 10:29:35AM -0500, Tyler Fanelli wrote:
> > > On 11/16/21 4:17 AM, Daniel P. Berrangé wrote:
> > > > On Mon, Nov 15, 2021 at 02:38:04PM -0500, Tyler Fanelli wrote:
> > > > > Probe for SEV-ES and SEV-SNP capabilities to distinguish between Rome,
> > > > > Naples, and Milan processors. Use the CPUID function to probe if a
> > > > > processor is capable of running SEV-ES or SEV-SNP, rather than if it
> > > > > actually is running SEV-ES or SEV-SNP.
> > > > > 
> > > > > Signed-off-by: Tyler Fanelli <tfanelli@redhat.com>
> > > > > ---
> > > > >    qapi/misc-target.json | 11 +++++++++--
> > > > >    target/i386/sev.c     |  6 ++++--
> > > > >    2 files changed, 13 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> > > > > index 5aa2b95b7d..c3e9bce12b 100644
> > > > > --- a/qapi/misc-target.json
> > > > > +++ b/qapi/misc-target.json
> > > > > @@ -182,13 +182,19 @@
> > > > >    # @reduced-phys-bits: Number of physical Address bit reduction 
> > > > > when SEV is
> > > > >    #                     enabled
> > > > >    #
> > > > > +# @es: SEV-ES capability of the machine.
> > > > > +#
> > > > > +# @snp: SEV-SNP capability of the machine.
> > > > > +#
> > > > >    # Since: 2.12
> > > > >    ##
> > > > >    { 'struct': 'SevCapability',
> > > > >      'data': { 'pdh': 'str',
> > > > >                'cert-chain': 'str',
> > > > >                'cbitpos': 'int',
> > > > > -            'reduced-phys-bits': 'int'},
> > > > > +            'reduced-phys-bits': 'int',
> > > > > +            'es': 'bool',
> > > > > +            'snp': 'bool'},
> > > > >      'if': 'TARGET_I386' }
> > > > >    ##
> > > > > @@ -205,7 +211,8 @@
> > > > >    #
> > > > >    # -> { "execute": "query-sev-capabilities" }
> > > > >    # <- { "return": { "pdh": "8CCDD8DDD", "cert-chain": "888CCCDDDEE",
> > > > > -#                  "cbitpos": 47, "reduced-phys-bits": 5}}
> > > > > +#                  "cbitpos": 47, "reduced-phys-bits": 5
> > > > > +#                  "es": false, "snp": false}}
> > > > We've previously had patches posted to support SNP in QEMU
> > > > 
> > > >     https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg04761.html
> > > > 
> > > > and this included an update to query-sev for reporting info
> > > > about the VM instance.
> > > > 
> > > > Your patch is updating query-sev-capabilities, which is a
> > > > counterpart for detecting host capabilities separate from
> > > > a guest instance.
> > > Yes, that's because with this patch, I'm more interested in determining
> > > which AMD processor is running on a host, and less if ES or SNP is 
> > > actually
> > > running on a guest instance or not.
> > > > None the less I wonder if the same design questions from
> > > > query-sev apply. ie do we need to have the ability to
> > > > report any SNP specific information fields, if so we need
> > > > to use a discriminated union of structs, not just bool
> > > > flags.
> > > > 
> > > > More generally I'm some what wary of adding this to
> > > > query-sev-capabilities at all, unless it is part of the
> > > > main SEV-SNP series.
> > > > 
> > > > Also what's the intended usage for the mgmt app from just
> > > > having these boolean fields ? Are they other more explicit
> > > > feature flags we should be reporting, instead of what are
> > > > essentially SEV generation codenames.
> > > If by "mgmt app" you're referring to sevctl, in order to determine which
> > > certificate chain to use (Naples vs Rome vs Milan ARK/ASK) we must query
> > > which processor we are running on. Although sevctl has a feature which can
> > > do this already, we cannot guarantee that sevctl is running on the same 
> > > host
> > > that a VM is running on, so we must query this capability from QEMU. My
> > > logic was determining the processor would have been the following:
> > I'm not really talking about a specific, rather any tool which wants
> > to deal with SEV and QEMU, whether libvirt or an app using libvirt,
> > or something else using QEMU directly.
> 
> Ah, my mistake.
> 
> > Where does the actual cert chain payload come from ? Is that something
> > the app has to acquire out of band, or can the full cert chain be
> > acquired from the hardware itself ?
> 
> The cert chain (or the ARK/ASK specifically) comes from AMD's KDS, yet
> sevctl is able to cache the values, and has them on-hand when needed. This
> patch would tell sevctl *which* of the cert chains to use (Naples vs Rome vs
> Milan chain). If need be, I could just focus on Naples and Rome processors
> for now and bring support for SNP (Milan processors) later on when it is
> more mature.
> 
> > > !es && !snp --> Naples
> > > 
> > > es && !snp --> Rome
> > > 
> > > es && snp --> Milan
> > This approach isn't future proof if subsequent generations introduce
> > new certs. It feels like we should be explicitly reporting something
> > about the certs rather than relying on every app to re-implement tihs
> > logic.
> 
> Alright, like an encoding of which processor generation the host is running
> on?

IIUC (from looking at sev-tool), the certificates can be acquired
from

   https://developer.amd.com/wp-content/resources/ask_ark_{gen}.cert

where {gen} is one of "milan", "naples", "rome".

With this in mind, I'd think that query-sev-capabilities could just
report the required certificate name. e.g.

  { 'enum': 'SevAskArkCertName',
    'data': ['milan', 'naples', 'rome'] }

and then report it in SevCapability struct with

    "ask-ark-cert-name":  "SevAskArkCertName"


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|