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

[Dov Murik <dovmurik@linux.ibm.com>]

On 01/11/2021 16:25, Tom Lendacky wrote:
> On 11/1/21 5:21 AM, 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>
> 
> Just a few minor comments/questions below, otherwise:
> 
> Acked-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");
> 
> Not sure if warn or info would be better, either way works for me,
> though, since this allows the guest to boot now.

OK.

> 
> Unrelated to this change, but do we explicitly want to say OVMF? Can't
> another BIOS/UEFI implementation have this support?
> 

I don't mind saying "guest firmware" or "VM firmware" or "guest flash"
instead.  Note that the function name has "ovmf" in it...


> and should guid be GUID?

I agree; if we change "OVMF" I'll fix that as well.

-Dov


> 
> Thanks,
> Tom
> 
>>           return false;
>>       }
>>       area = (SevHashTableDescriptor *)data;
>>