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

[Tyler Fanelli <tfanelli@redhat.com>]

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)