Re: [PATCH] hw/i386/pc: Document pc_system_ovmf_table_find

From: Dov Murik <dovmurik@linux.ibm.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>, qemu-devel@nongnu.org
Cc: Tom Lendacky <thomas.lendacky@amd.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH] hw/i386/pc: Document pc_system_ovmf_table_find
Date: Tue, 22 Jun 2021 15:58:11 +0300	[thread overview]
Message-ID: <d5fbda1c-69dc-35b6-388e-443a697c2fdf@linux.ibm.com> (raw)
In-Reply-To: <838caecc-6d4a-6257-147e-fbef4148f679@redhat.com>

+cc: Tom Lendacky

On 22/06/2021 15:47, Philippe Mathieu-Daudé wrote:
> On 6/22/21 2:44 PM, Dov Murik wrote:
>> Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>>  hw/i386/pc_sysfw.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
>> index 6ce37a2b05..e8d20cb83f 100644
>> --- a/hw/i386/pc_sysfw.c
>> +++ b/hw/i386/pc_sysfw.c
>> @@ -176,6 +176,20 @@ static void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size)
>>      ovmf_table += tot_len;
>>  }
>>  
>> +/**
>> + * pc_system_ovmf_table_find - Find the data associated with an entry in OVMF's
>> + * reset vector GUIDed table.
>> + *
>> + * @entry: GUID string of the entry to lookup
>> + * @data: Filled with a pointer to the entry's value (if not NULL)
>> + * @data_len: Filled with the length of the entry's value (if not NULL). Pass
>> + *            NULL here if the length of data is known.
>> + *
>> + * Note that this function must be called after the OVMF table was found and
>> + * copied by pc_system_parse_ovmf_flash().
> 
> What about replacing this comment by:
> 
>   assert(ovmf_table && ovmf_table_len);
> 

I think this will break things: in target/i386/sev.c we have SEV-ES code
that calls pc_system_ovmf_table_find() and can deal with the case when
there's no OVMF table.  An assert will break it.

> Otherwise,
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 

Thanks!

-Dov

> Thanks!
> 
>> + *
>> + * Return: true if the entry was found in the OVMF table; false otherwise.
>> + */
>>  bool pc_system_ovmf_table_find(const char *entry, uint8_t **data,
>>                                 int *data_len)
>>  {
>>
> 