| Message ID | 20210630054625.3424178-2-dovmurik@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/i386/pc: Clarify pc_system_ovmf_table_find usage | expand |
On 6/30/21 7:46 AM, Dov Murik wrote: > Add assertion in pc_system_ovmf_table_find that verifies that the flash > was indeed previously parsed (looking for the OVMF table) by > pc_system_parse_ovmf_flash. > > Now pc_system_ovmf_table_find distinguishes between "no one called > pc_system_parse_ovmf_flash" (which will abort due to assertion failure) > and "the flash was parsed but no OVMF table was found, or it is invalid" > (which will return false). > > Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> > --- > hw/i386/pc_sysfw.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Thank you Dov, Phil.
On 30/06/2021 11:23, Philippe Mathieu-Daudé wrote: > On 6/30/21 7:46 AM, Dov Murik wrote: >> Add assertion in pc_system_ovmf_table_find that verifies that the flash >> was indeed previously parsed (looking for the OVMF table) by >> pc_system_parse_ovmf_flash. >> >> Now pc_system_ovmf_table_find distinguishes between "no one called >> pc_system_parse_ovmf_flash" (which will abort due to assertion failure) >> and "the flash was parsed but no OVMF table was found, or it is invalid" >> (which will return false). >> >> Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> >> --- >> hw/i386/pc_sysfw.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Thanks Phil for the constructive review process. -Dov
On 6/30/21 12:46 AM, Dov Murik wrote: > Add assertion in pc_system_ovmf_table_find that verifies that the flash > was indeed previously parsed (looking for the OVMF table) by > pc_system_parse_ovmf_flash. > > Now pc_system_ovmf_table_find distinguishes between "no one called > pc_system_parse_ovmf_flash" (which will abort due to assertion failure) > and "the flash was parsed but no OVMF table was found, or it is invalid" > (which will return false). > > Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> Does the qemu coding style prefer not initializing the bool to false since it will default to that? Otherwise, Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > hw/i386/pc_sysfw.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c > index 6ce37a2b05..9eac36b830 100644 > --- a/hw/i386/pc_sysfw.c > +++ b/hw/i386/pc_sysfw.c > @@ -126,6 +126,7 @@ void pc_system_flash_cleanup_unused(PCMachineState *pcms) > > #define OVMF_TABLE_FOOTER_GUID "96b582de-1fb2-45f7-baea-a366c55a082d" > > +static bool ovmf_flash_parsed = false; > static uint8_t *ovmf_table; > static int ovmf_table_len; > > @@ -136,10 +137,12 @@ static void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size) > int tot_len; > > /* should only be called once */ > - if (ovmf_table) { > + if (ovmf_flash_parsed) { > return; > } > > + ovmf_flash_parsed = true; > + > if (flash_size < TARGET_PAGE_SIZE) { > return; > } > @@ -183,6 +186,8 @@ bool pc_system_ovmf_table_find(const char *entry, uint8_t **data, > int tot_len = ovmf_table_len; > QemuUUID entry_guid; > > + assert(ovmf_flash_parsed); > + > if (qemu_uuid_parse(entry, &entry_guid) < 0) { > return false; > } >
On 6/30/21 3:38 PM, Tom Lendacky wrote: > On 6/30/21 12:46 AM, Dov Murik wrote: >> Add assertion in pc_system_ovmf_table_find that verifies that the flash >> was indeed previously parsed (looking for the OVMF table) by >> pc_system_parse_ovmf_flash. >> >> Now pc_system_ovmf_table_find distinguishes between "no one called >> pc_system_parse_ovmf_flash" (which will abort due to assertion failure) >> and "the flash was parsed but no OVMF table was found, or it is invalid" >> (which will return false). >> >> Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> > > Does the qemu coding style prefer not initializing the bool to false since > it will default to that? Indeed, you are right, and checkpatch will block this patch: ERROR: do not initialise statics to 0 or NULL #33: FILE: hw/i386/pc_sysfw.c:129: +static bool ovmf_flash_parsed = false; total: 1 errors, 0 warnings, 28 lines checked > Otherwise, > > Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> > >> --- >> hw/i386/pc_sysfw.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c >> index 6ce37a2b05..9eac36b830 100644 >> --- a/hw/i386/pc_sysfw.c >> +++ b/hw/i386/pc_sysfw.c >> @@ -126,6 +126,7 @@ void pc_system_flash_cleanup_unused(PCMachineState *pcms) >> >> #define OVMF_TABLE_FOOTER_GUID "96b582de-1fb2-45f7-baea-a366c55a082d" >> >> +static bool ovmf_flash_parsed = false; >> static uint8_t *ovmf_table; >> static int ovmf_table_len; >> >> @@ -136,10 +137,12 @@ static void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size) >> int tot_len; >> >> /* should only be called once */ >> - if (ovmf_table) { >> + if (ovmf_flash_parsed) { >> return; >> } >> >> + ovmf_flash_parsed = true; >> + >> if (flash_size < TARGET_PAGE_SIZE) { >> return; >> } >> @@ -183,6 +186,8 @@ bool pc_system_ovmf_table_find(const char *entry, uint8_t **data, >> int tot_len = ovmf_table_len; >> QemuUUID entry_guid; >> >> + assert(ovmf_flash_parsed); >> + >> if (qemu_uuid_parse(entry, &entry_guid) < 0) { >> return false; >> } >> >
On 30/06/2021 17:35, Philippe Mathieu-Daudé wrote: > On 6/30/21 3:38 PM, Tom Lendacky wrote: >> On 6/30/21 12:46 AM, Dov Murik wrote: >>> Add assertion in pc_system_ovmf_table_find that verifies that the flash >>> was indeed previously parsed (looking for the OVMF table) by >>> pc_system_parse_ovmf_flash. >>> >>> Now pc_system_ovmf_table_find distinguishes between "no one called >>> pc_system_parse_ovmf_flash" (which will abort due to assertion failure) >>> and "the flash was parsed but no OVMF table was found, or it is invalid" >>> (which will return false). >>> >>> Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> >> >> Does the qemu coding style prefer not initializing the bool to false since >> it will default to that? > > Indeed, you are right, and checkpatch will block this patch: > > ERROR: do not initialise statics to 0 or NULL > #33: FILE: hw/i386/pc_sysfw.c:129: > +static bool ovmf_flash_parsed = false; > > total: 1 errors, 0 warnings, 28 lines checked oops, missed that in my flow. Sent a v3 series with this fix. > >> Otherwise, >> >> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> Thanks, Tom! -Dov
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c index 6ce37a2b05..9eac36b830 100644 --- a/hw/i386/pc_sysfw.c +++ b/hw/i386/pc_sysfw.c @@ -126,6 +126,7 @@ void pc_system_flash_cleanup_unused(PCMachineState *pcms) #define OVMF_TABLE_FOOTER_GUID "96b582de-1fb2-45f7-baea-a366c55a082d" +static bool ovmf_flash_parsed = false; static uint8_t *ovmf_table; static int ovmf_table_len; @@ -136,10 +137,12 @@ static void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size) int tot_len; /* should only be called once */ - if (ovmf_table) { + if (ovmf_flash_parsed) { return; } + ovmf_flash_parsed = true; + if (flash_size < TARGET_PAGE_SIZE) { return; } @@ -183,6 +186,8 @@ bool pc_system_ovmf_table_find(const char *entry, uint8_t **data, int tot_len = ovmf_table_len; QemuUUID entry_guid; + assert(ovmf_flash_parsed); + if (qemu_uuid_parse(entry, &entry_guid) < 0) { return false; }
Add assertion in pc_system_ovmf_table_find that verifies that the flash was indeed previously parsed (looking for the OVMF table) by pc_system_parse_ovmf_flash. Now pc_system_ovmf_table_find distinguishes between "no one called pc_system_parse_ovmf_flash" (which will abort due to assertion failure) and "the flash was parsed but no OVMF table was found, or it is invalid" (which will return false). Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> --- hw/i386/pc_sysfw.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)