[PATCH] hw/i386/pc: Document pc_system_ovmf_table_find

[PATCH] hw/i386/pc: Document pc_system_ovmf_table_find

[Dov Murik]

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().
+ *
+ * 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)
 {
-- 
2.25.1

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

[Philippe Mathieu-Daudé]

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);

Otherwise,

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

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)
>  {
> 

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

[Dov Murik]

+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)
>>  {
>>
> 

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

[Tom Lendacky]

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.

Thanks,
Tom

> 
> 
>> 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)
>>>  {
>>>
>>

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

[Dov Murik]

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? ;-)


Thanks,
Dov


> Thanks,
> Tom
> 
>>
>>
>>> 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)
>>>>  {
>>>>
>>>

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

[Philippe Mathieu-Daudé]

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.

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

[Dov Murik]

On 29/06/2021 8:56, 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).

typo: That should be    (> 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? ;-)
> 
> 
> Thanks,
> Dov
> 
> 
>> Thanks,
>> Tom
>>
>>>
>>>
>>>> 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)
>>>>>  {
>>>>>
>>>>

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

[Tom Lendacky]

On 6/29/21 2:11 AM, Philippe Mathieu-Daudé wrote:
> 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:
>>

>> (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.

Yes, to me (a) seems simpler, too.

Thanks,
Tom

> 
> Regards,
> 
> Phil.
>