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

[Tyler Fanelli <tfanelli@redhat.com>]

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)