Re: [PATCH] hw/i386/pc: Document pc_system_ovmf_table_find
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. >
Re: [PATCH] hw/i386/pc: Document pc_system_ovmf_table_find
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é
> Signed-off-by: Dov Murik
> ---
> 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é
>>>
>>> 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
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é > Signed-off-by: Dov Murik > --- > 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
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é
Signed-off-by: Dov Murik
---
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é
>>>
>>
>> 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
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é
>>> Signed-off-by: Dov Murik
>>> ---
>>> 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é
>>
>
> 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
+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é
>> Signed-off-by: Dov Murik
>> ---
>> 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é
>
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
On 6/22/21 2:44 PM, Dov Murik wrote:
> Suggested-by: Philippe Mathieu-Daudé
> Signed-off-by: Dov Murik
> ---
> 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é
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)
> {
>
[PATCH] hw/i386/pc: Document pc_system_ovmf_table_find
Suggested-by: Philippe Mathieu-Daudé
Signed-off-by: Dov Murik
---
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
