| Message ID | 20171204212832.130100-2-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi: Enable basic sandbox support for EFI loader | expand |
On 12/04/2017 10:28 PM, Simon Glass wrote: > This function can fail but gives no indication of failure. Update it to > return an error when something goes wrong. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > Changes in v2: > - Update return type of efi_smbios_register() to efi_status_t > - Use return value of efi_install_configuration_table > > include/efi_loader.h | 9 ++++++++- > lib/efi_loader/efi_smbios.c | 7 ++++--- > 2 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index 1b92edbd77..35f8f84401 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -164,7 +164,14 @@ int efi_gop_register(void); > /* Called by bootefi to make the network interface available */ > int efi_net_register(void); > /* Called by bootefi to make SMBIOS tables available */ > -void efi_smbios_register(void); > +/** > + * efi_smbios_register() - write out SMBIOS tables > + * > + * Called by bootefi to make SMBIOS tables available > + * > + * @return 0 if OK, -ENOMEM if no memory is available for the tables > + */ > +efi_status_t efi_smbios_register(void); > > struct efi_simple_file_system_protocol * > efi_fs_from_path(struct efi_device_path *fp); > diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c > index ac412e7362..67f71892ca 100644 > --- a/lib/efi_loader/efi_smbios.c > +++ b/lib/efi_loader/efi_smbios.c > @@ -13,7 +13,7 @@ > > static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID; > > -void efi_smbios_register(void) > +efi_status_t efi_smbios_register(void) > { > /* Map within the low 32 bits, to allow for 32bit SMBIOS tables */ > uint64_t dmi = 0xffffffff; > @@ -22,11 +22,12 @@ void efi_smbios_register(void) > int memtype = EFI_RUNTIME_SERVICES_DATA; > > if (efi_allocate_pages(1, memtype, pages, &dmi) != EFI_SUCCESS) Please, return the value returned from efi_allocate_pages(). The function has a lot of different failure modes. > - return; > + return EFI_OUT_OF_RESOURCES; > > /* Generate SMBIOS tables */ > write_smbios_table(dmi); We should add a comment explaining why we do not use the return value of write_smbios_table(). My understanding is: write_smbios_table returns dmi rounded up to a multiple of 16. efi_allocate_pages returns a 4096 aligned address. So we do expect that the return value equals the parameter apssed to write_smbios_table(). Best regards Heinrich > > /* And expose them to our EFI payload */ > - efi_install_configuration_table(&smbios_guid, (void*)(uintptr_t)dmi); > + return efi_install_configuration_table(&smbios_guid, > + (void *)(uintptr_t)dmi); > } >
Hi Heinrich, On 4 December 2017 at 15:15, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > On 12/04/2017 10:28 PM, Simon Glass wrote: >> >> This function can fail but gives no indication of failure. Update it to >> return an error when something goes wrong. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> >> Changes in v2: >> - Update return type of efi_smbios_register() to efi_status_t >> - Use return value of efi_install_configuration_table >> >> include/efi_loader.h | 9 ++++++++- >> lib/efi_loader/efi_smbios.c | 7 ++++--- >> 2 files changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/include/efi_loader.h b/include/efi_loader.h >> index 1b92edbd77..35f8f84401 100644 >> --- a/include/efi_loader.h >> +++ b/include/efi_loader.h >> @@ -164,7 +164,14 @@ int efi_gop_register(void); >> /* Called by bootefi to make the network interface available */ >> int efi_net_register(void); >> /* Called by bootefi to make SMBIOS tables available */ >> -void efi_smbios_register(void); >> +/** >> + * efi_smbios_register() - write out SMBIOS tables >> + * >> + * Called by bootefi to make SMBIOS tables available >> + * >> + * @return 0 if OK, -ENOMEM if no memory is available for the tables >> + */ >> +efi_status_t efi_smbios_register(void); >> struct efi_simple_file_system_protocol * >> efi_fs_from_path(struct efi_device_path *fp); >> diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c >> index ac412e7362..67f71892ca 100644 >> --- a/lib/efi_loader/efi_smbios.c >> +++ b/lib/efi_loader/efi_smbios.c >> @@ -13,7 +13,7 @@ >> static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID; >> -void efi_smbios_register(void) >> +efi_status_t efi_smbios_register(void) >> { >> /* Map within the low 32 bits, to allow for 32bit SMBIOS tables */ >> uint64_t dmi = 0xffffffff; >> @@ -22,11 +22,12 @@ void efi_smbios_register(void) >> int memtype = EFI_RUNTIME_SERVICES_DATA; >> if (efi_allocate_pages(1, memtype, pages, &dmi) != EFI_SUCCESS) > > > Please, return the value returned from efi_allocate_pages(). The function > has a lot of different failure modes. OK, will do. But looking at efi_allocate_pages() there is no documentation as to the return value. > >> - return; >> + return EFI_OUT_OF_RESOURCES; >> /* Generate SMBIOS tables */ >> write_smbios_table(dmi); > > We should add a comment explaining why we do not use the return value of > write_smbios_table(). My understanding is: > write_smbios_table returns dmi rounded up to a multiple of 16. > efi_allocate_pages returns a 4096 aligned address. So we do expect that the > return value equals the parameter apssed to write_smbios_table(). OK will do. Regards, Simon
diff --git a/include/efi_loader.h b/include/efi_loader.h index 1b92edbd77..35f8f84401 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -164,7 +164,14 @@ int efi_gop_register(void); /* Called by bootefi to make the network interface available */ int efi_net_register(void); /* Called by bootefi to make SMBIOS tables available */ -void efi_smbios_register(void); +/** + * efi_smbios_register() - write out SMBIOS tables + * + * Called by bootefi to make SMBIOS tables available + * + * @return 0 if OK, -ENOMEM if no memory is available for the tables + */ +efi_status_t efi_smbios_register(void); struct efi_simple_file_system_protocol * efi_fs_from_path(struct efi_device_path *fp); diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c index ac412e7362..67f71892ca 100644 --- a/lib/efi_loader/efi_smbios.c +++ b/lib/efi_loader/efi_smbios.c @@ -13,7 +13,7 @@ static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID; -void efi_smbios_register(void) +efi_status_t efi_smbios_register(void) { /* Map within the low 32 bits, to allow for 32bit SMBIOS tables */ uint64_t dmi = 0xffffffff; @@ -22,11 +22,12 @@ void efi_smbios_register(void) int memtype = EFI_RUNTIME_SERVICES_DATA; if (efi_allocate_pages(1, memtype, pages, &dmi) != EFI_SUCCESS) - return; + return EFI_OUT_OF_RESOURCES; /* Generate SMBIOS tables */ write_smbios_table(dmi); /* And expose them to our EFI payload */ - efi_install_configuration_table(&smbios_guid, (void*)(uintptr_t)dmi); + return efi_install_configuration_table(&smbios_guid, + (void *)(uintptr_t)dmi); }
This function can fail but gives no indication of failure. Update it to return an error when something goes wrong. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v2: - Update return type of efi_smbios_register() to efi_status_t - Use return value of efi_install_configuration_table include/efi_loader.h | 9 ++++++++- lib/efi_loader/efi_smbios.c | 7 ++++--- 2 files changed, 12 insertions(+), 4 deletions(-)