[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/i386/pc: Document pc_system_ovmf_table_find
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH] hw/i386/pc: Document pc_system_ovmf_table_find |
Date: |
Tue, 29 Jun 2021 09:11:26 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 |
On 6/29/21 7:56 AM, Dov Murik wrote:
> On 29/06/2021 1:03, Tom Lendacky wrote:
>> On 6/22/21 7:58 AM, Dov Murik wrote:
>>> +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.
>>
>> Right, what would be best is to differentiate between an OVMF table that
>> isn't present in the flash vs the fact that pc_system_parse_ovmf_flash()
>> wasn't called, asserting only on the latter.
>>
>
> [+cc James who wrote this code]
>
>
> Thanks Tom; I agree. To achieve that, we need one of these:
>
> (a) add a 'static bool ovmf_table_parsed' which will be set to true at
> the beginning of pc_system_parse_ovmf_flash(). Then, at the beginning of
> pc_system_ovmf_table_find add: assert(ovmf_table_parsed).
>
> (b) (ab)use our existing ovmf_table_len static variable: initialize it
> to -1 (meaning that we haven't parsed the OVMF flash yet). When looking
> for the table set it to 0 (meaning that OVMF table doesn't exist or is
> invalid). When a proper table is found and copied to ovmf_table, then
> set it to the real length (>= 0). At the beginning of
> pc_system_ovmf_table_find add: assert(ovmf_table_len != -1). (this -1
> can be #define OVMF_FLASH_NOT_PARSED -1).
>
>
> Phil, Tom, James: which do you prefer? other options? Rust enum? ;-)
Since we are discussing code that should not be called, I don't have
strong preference as long as we keep the code easy to review :)
With that in mind, (a) seems simpler.
Regards,
Phil.