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

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

[Tyler Fanelli]

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}}
 #
 ##
 { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
diff --git a/target/i386/sev.c b/target/i386/sev.c
index eede07f11d..6d78dcd744 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -506,7 +506,7 @@ static SevCapability *sev_get_capabilities(Error **errp)
     guchar *pdh_data = NULL;
     guchar *cert_chain_data = NULL;
     size_t pdh_len = 0, cert_chain_len = 0;
-    uint32_t ebx;
+    uint32_t eax, ebx;
     int fd;
 
     if (!kvm_enabled()) {
@@ -534,8 +534,10 @@ static SevCapability *sev_get_capabilities(Error **errp)
     cap->pdh = g_base64_encode(pdh_data, pdh_len);
     cap->cert_chain = g_base64_encode(cert_chain_data, cert_chain_len);
 
-    host_cpuid(0x8000001F, 0, NULL, &ebx, NULL, NULL);
+    host_cpuid(0x8000001F, 0, &eax, &ebx, NULL, NULL);
     cap->cbitpos = ebx & 0x3f;
+    cap->es = (eax & 0x8) ? true : false;
+    cap->snp = (eax & 0x10) ? true : false;
 
     /*
      * When SEV feature is enabled, we loose one bit in guest physical
-- 
2.31.1

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

[Eric Blake]

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

Missing '(since 7.0)' tags on the new members.

>  # 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}}

Invalid JSON, as you missed the comma needed after 5.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Daniel P. Berrangé]

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.

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.


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

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

[Tyler Fanelli]

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:

!es && !snp --> Naples

es && !snp --> Rome

es && snp --> Milan

>
>
> Regards,
> Daniel

Tyler.

-- 
Tyler Fanelli (tfanelli)

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

[Daniel P. Berrangé]

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.

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 ? 

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

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

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

[Tyler Fanelli]

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?

>
> Regards,
> Daniel

Tyler.

-- 
Tyler Fanelli (tfanelli)

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

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

[Tyler Fanelli]

On 11/16/21 12:23 PM, Daniel P. Berrangé wrote:
> 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"

That seems reasonable to me, I'll give it a try and submit a v2 patch.

>
> Regards,
> Daniel


-- 
Tyler Fanelli (tfanelli)