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é (berra...@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 <dovmu...@linux.ibm.com>
>>>> Reported-by: Tom Lendacky <thomas.lenda...@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
