| Message ID | 20210622124419.3008278-1-dovmurik@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/i386/pc: Document pc_system_ovmf_table_find | expand |
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) > { >
+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) >> { >> >
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) >>> { >>> >>
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) >>>> { >>>> >>>
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.
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) >>>>> { >>>>> >>>>
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. >
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) {
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(+)