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é <phi...@redhat.com> >>>>> Signed-off-by: Dov Murik <dovmu...@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.
