Re: [PATCH 1/3] sev/i386: Allow launching with -kernel if no OVMF hashes table found

[Dov Murik]

On 04/11/2021 20:22, Daniel P. Berrangé wrote:
> On Thu, Nov 04, 2021 at 06:18:10PM +0000, Dr. David Alan Gilbert wrote:
>> * Daniel P. Berrangé (berrange@redhat.com) wrote:
>>> On Mon, Nov 01, 2021 at 10:21:34AM +0000, Dov Murik wrote:
>>>> Commit cff03145ed3c ("sev/i386: Introduce sev_add_kernel_loader_hashes
>>>> for measured linux boot", 2021-09-30) introduced measured direct boot
>>>> with -kernel, using an OVMF-designated hashes table which QEMU fills.
>>>>
>>>> However, if OVMF doesn't designate such an area, QEMU would completely
>>>> abort the VM launch.  This breaks launching with -kernel using older
>>>> OVMF images which don't publish the SEV_HASH_TABLE_RV_GUID.
>>>>
>>>> Instead, just warn the user that -kernel was supplied by OVMF doesn't
>>>> specify the GUID for the hashes table.  The following warning will be
>>>> displayed during VM launch:
>>>>
>>>>     qemu-system-x86_64: warning: SEV: kernel specified but OVMF has no 
>>>> hash table guid
>>>>
>>>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>>>> Reported-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>> ---
>>>>  target/i386/sev.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>>>> index eede07f11d..682b8ccf6c 100644
>>>> --- a/target/i386/sev.c
>>>> +++ b/target/i386/sev.c
>>>> @@ -1204,7 +1204,7 @@ bool 
>>>> sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
>>>>      int aligned_len;
>>>>  
>>>>      if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
>>>> -        error_setg(errp, "SEV: kernel specified but OVMF has no hash 
>>>> table guid");
>>>> +        warn_report("SEV: kernel specified but OVMF has no hash table 
>>>> guid");
>>>>          return false;
>>>
>>> I'm pretty wary of doing this kind of thing.
>>>
>>> If someone is using QEMU and they required to have the hashes populated
>>> for their use case, they now don't get a fatal error if something goes
>>> wrong with the process. This is bad as it hides a serious mistake.
>>>
>>> If someone is using QEMU and they don't require to have the hashes
>>> populated and they knowingly use a firmware that doesn't support
>>> this, they'll now get a irrelevant warning every time they boot
>>> QEMU. This is bad because IME users will file bugs complaining
>>> about this bogus warning.
>>>
>>>
>>> If we genuinely need to support both uses cases, then we should have
>>> an explicit command line flag to request the desired behaviour.
>>>
>>> This could be a -machine option to indicate that the hashes must
>>> be populated.
>>>
>>>  - unset: try to populate hashes, *silently* ignore missing table
>>>           in ovmf
>>>  - set == on: try to populate hashes, report error on missing
>>>              table in ovmf
>>>   -set == off: never try to populate hashes, nor look for the
>>>                table in ovmf
>>
>> Or as a property on the sev-guest object.
> 
> Yep, I thought of that too, and I'm pretty undecided which is "best".
> 
> -machine makes sense as 'kernel' and 'initrd' are properties of
> the '-machine' and we're doing stuff related to the.
> 
> -object sev-guest makes sense as this is behaviour that's (currently)
> specific to SEV.

Thanks for the suggestions.

I'm going to go with '-object sev-guest,...,kernel_hashes=on' because
this whole function is defined in sev.c and only works with the AmdSev
OVMF target.

-Dov