| Message ID | 20230301091523.18384-3-masahisa.kojima@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | FMP versioning support | expand |
On Wed, Mar 01, 2023 at 06:15:20PM +0900, Masahisa Kojima wrote: > Current FMP->GetImageInfo() always return 0 for the firmware > version, user can not identify which firmware version is currently > running through the EFI interface. > > This commit reads the "FmpStateXXXX" EFI variable, then fills the > firmware version, lowest supported version, last attempt version > and last attempt status in FMP->GetImageInfo(). > > Now FMP->GetImageInfo() and ESRT have the meaningful version number. > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > --- > No update since v1 > > lib/efi_loader/efi_firmware.c | 30 ++++++++++++++++++++++++++---- > 1 file changed, 26 insertions(+), 4 deletions(-) > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > index d1afafb052..ead20fa914 100644 > --- a/lib/efi_loader/efi_firmware.c > +++ b/lib/efi_loader/efi_firmware.c > @@ -173,13 +173,38 @@ static efi_status_t efi_fill_image_desc_array( > *package_version_name = NULL; /* not supported */ > > for (i = 0; i < num_image_type_guids; i++) { > + u16 varname[13]; /* u"FmpStateXXXX" */ > + efi_status_t ret; > + efi_uintn_t size; > + struct fmp_state var_state = { 0 }; > + > image_info[i].image_index = fw_array[i].image_index; > image_info[i].image_type_id = fw_array[i].image_type_id; > image_info[i].image_id = fw_array[i].image_index; > > image_info[i].image_id_name = fw_array[i].fw_name; > > - image_info[i].version = 0; /* not supported */ > + efi_create_indexed_name(varname, sizeof(varname), "FmpState", > + fw_array[i].image_index); Don't we have to think of the systems where multiple FMP drivers are used? In those cases, 'image_index' doesn't work as an unique ID. It is unlikely under the current code, but we should consider any future extension. -Takahiro Akashi > + size = sizeof(var_state); > + ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL, > + &size, &var_state, NULL); > + if (ret == EFI_SUCCESS) { > + image_info[i].version = var_state.fw_version; > + image_info[i].lowest_supported_image_version = > + var_state.lowest_supported_version; > + image_info[i].last_attempt_version = > + var_state.last_attempt_version; > + image_info[i].last_attempt_status = > + var_state.last_attempt_status; > + } else { > + image_info[i].version = 0; > + image_info[i].lowest_supported_image_version = 0; > + image_info[i].last_attempt_version = 0; > + image_info[i].last_attempt_status = > + LAST_ATTEMPT_STATUS_SUCCESS; > + } > + > image_info[i].version_name = NULL; /* not supported */ > image_info[i].size = 0; > image_info[i].attributes_supported = > @@ -193,9 +218,6 @@ static efi_status_t efi_fill_image_desc_array( > image_info[0].attributes_setting |= > IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED; > > - image_info[i].lowest_supported_image_version = 0; > - image_info[i].last_attempt_version = 0; > - image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; > image_info[i].hardware_instance = 1; > image_info[i].dependencies = NULL; > } > -- > 2.17.1 >
On Thu, 2 Mar 2023 at 14:16, Takahiro Akashi <takahiro.akashi@linaro.org> wrote: > > On Wed, Mar 01, 2023 at 06:15:20PM +0900, Masahisa Kojima wrote: > > Current FMP->GetImageInfo() always return 0 for the firmware > > version, user can not identify which firmware version is currently > > running through the EFI interface. > > > > This commit reads the "FmpStateXXXX" EFI variable, then fills the > > firmware version, lowest supported version, last attempt version > > and last attempt status in FMP->GetImageInfo(). > > > > Now FMP->GetImageInfo() and ESRT have the meaningful version number. > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > --- > > No update since v1 > > > > lib/efi_loader/efi_firmware.c | 30 ++++++++++++++++++++++++++---- > > 1 file changed, 26 insertions(+), 4 deletions(-) > > > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > index d1afafb052..ead20fa914 100644 > > --- a/lib/efi_loader/efi_firmware.c > > +++ b/lib/efi_loader/efi_firmware.c > > @@ -173,13 +173,38 @@ static efi_status_t efi_fill_image_desc_array( > > *package_version_name = NULL; /* not supported */ > > > > for (i = 0; i < num_image_type_guids; i++) { > > + u16 varname[13]; /* u"FmpStateXXXX" */ > > + efi_status_t ret; > > + efi_uintn_t size; > > + struct fmp_state var_state = { 0 }; > > + > > image_info[i].image_index = fw_array[i].image_index; > > image_info[i].image_type_id = fw_array[i].image_type_id; > > image_info[i].image_id = fw_array[i].image_index; > > > > image_info[i].image_id_name = fw_array[i].fw_name; > > > > - image_info[i].version = 0; /* not supported */ > > + efi_create_indexed_name(varname, sizeof(varname), "FmpState", > > + fw_array[i].image_index); > > Don't we have to think of the systems where multiple FMP drivers are > used? In those cases, 'image_index' doesn't work as an unique ID. > It is unlikely under the current code, but we should consider > any future extension. I assume that other FMP drivers implement their own version management. Thanks, Masahisa Kojima > > -Takahiro Akashi > > > > + size = sizeof(var_state); > > + ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL, > > + &size, &var_state, NULL); > > + if (ret == EFI_SUCCESS) { > > + image_info[i].version = var_state.fw_version; > > + image_info[i].lowest_supported_image_version = > > + var_state.lowest_supported_version; > > + image_info[i].last_attempt_version = > > + var_state.last_attempt_version; > > + image_info[i].last_attempt_status = > > + var_state.last_attempt_status; > > + } else { > > + image_info[i].version = 0; > > + image_info[i].lowest_supported_image_version = 0; > > + image_info[i].last_attempt_version = 0; > > + image_info[i].last_attempt_status = > > + LAST_ATTEMPT_STATUS_SUCCESS; > > + } > > + > > image_info[i].version_name = NULL; /* not supported */ > > image_info[i].size = 0; > > image_info[i].attributes_supported = > > @@ -193,9 +218,6 @@ static efi_status_t efi_fill_image_desc_array( > > image_info[0].attributes_setting |= > > IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED; > > > > - image_info[i].lowest_supported_image_version = 0; > > - image_info[i].last_attempt_version = 0; > > - image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; > > image_info[i].hardware_instance = 1; > > image_info[i].dependencies = NULL; > > } > > -- > > 2.17.1 > >
On Thu, Mar 02, 2023 at 07:05:50PM +0900, Masahisa Kojima wrote: > On Thu, 2 Mar 2023 at 14:16, Takahiro Akashi <takahiro.akashi@linaro.org> wrote: > > > > On Wed, Mar 01, 2023 at 06:15:20PM +0900, Masahisa Kojima wrote: > > > Current FMP->GetImageInfo() always return 0 for the firmware > > > version, user can not identify which firmware version is currently > > > running through the EFI interface. > > > > > > This commit reads the "FmpStateXXXX" EFI variable, then fills the > > > firmware version, lowest supported version, last attempt version > > > and last attempt status in FMP->GetImageInfo(). > > > > > > Now FMP->GetImageInfo() and ESRT have the meaningful version number. > > > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > > --- > > > No update since v1 > > > > > > lib/efi_loader/efi_firmware.c | 30 ++++++++++++++++++++++++++---- > > > 1 file changed, 26 insertions(+), 4 deletions(-) > > > > > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > > index d1afafb052..ead20fa914 100644 > > > --- a/lib/efi_loader/efi_firmware.c > > > +++ b/lib/efi_loader/efi_firmware.c > > > @@ -173,13 +173,38 @@ static efi_status_t efi_fill_image_desc_array( > > > *package_version_name = NULL; /* not supported */ > > > > > > for (i = 0; i < num_image_type_guids; i++) { > > > + u16 varname[13]; /* u"FmpStateXXXX" */ > > > + efi_status_t ret; > > > + efi_uintn_t size; > > > + struct fmp_state var_state = { 0 }; > > > + > > > image_info[i].image_index = fw_array[i].image_index; > > > image_info[i].image_type_id = fw_array[i].image_type_id; > > > image_info[i].image_id = fw_array[i].image_index; > > > > > > image_info[i].image_id_name = fw_array[i].fw_name; > > > > > > - image_info[i].version = 0; /* not supported */ > > > + efi_create_indexed_name(varname, sizeof(varname), "FmpState", > > > + fw_array[i].image_index); > > > > Don't we have to think of the systems where multiple FMP drivers are > > used? In those cases, 'image_index' doesn't work as an unique ID. > > It is unlikely under the current code, but we should consider > > any future extension. > > I assume that other FMP drivers implement their own version management. I don't have a strong opinion, but my idea about a variable name is to use 'image_type_id' as a 'vendor' field which allows for making the variable globally unique. -Takahiro Akashi > Thanks, > Masahisa Kojima > > > > > -Takahiro Akashi > > > > > > > + size = sizeof(var_state); > > > + ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL, > > > + &size, &var_state, NULL); > > > + if (ret == EFI_SUCCESS) { > > > + image_info[i].version = var_state.fw_version; > > > + image_info[i].lowest_supported_image_version = > > > + var_state.lowest_supported_version; > > > + image_info[i].last_attempt_version = > > > + var_state.last_attempt_version; > > > + image_info[i].last_attempt_status = > > > + var_state.last_attempt_status; > > > + } else { > > > + image_info[i].version = 0; > > > + image_info[i].lowest_supported_image_version = 0; > > > + image_info[i].last_attempt_version = 0; > > > + image_info[i].last_attempt_status = > > > + LAST_ATTEMPT_STATUS_SUCCESS; > > > + } > > > + > > > image_info[i].version_name = NULL; /* not supported */ > > > image_info[i].size = 0; > > > image_info[i].attributes_supported = > > > @@ -193,9 +218,6 @@ static efi_status_t efi_fill_image_desc_array( > > > image_info[0].attributes_setting |= > > > IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED; > > > > > > - image_info[i].lowest_supported_image_version = 0; > > > - image_info[i].last_attempt_version = 0; > > > - image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; > > > image_info[i].hardware_instance = 1; > > > image_info[i].dependencies = NULL; > > > } > > > -- > > > 2.17.1 > > >
Hi Akashi-san, On Fri, 3 Mar 2023 at 09:10, Takahiro Akashi <takahiro.akashi@linaro.org> wrote: > > On Thu, Mar 02, 2023 at 07:05:50PM +0900, Masahisa Kojima wrote: > > On Thu, 2 Mar 2023 at 14:16, Takahiro Akashi <takahiro.akashi@linaro.org> wrote: > > > > > > On Wed, Mar 01, 2023 at 06:15:20PM +0900, Masahisa Kojima wrote: > > > > Current FMP->GetImageInfo() always return 0 for the firmware > > > > version, user can not identify which firmware version is currently > > > > running through the EFI interface. > > > > > > > > This commit reads the "FmpStateXXXX" EFI variable, then fills the > > > > firmware version, lowest supported version, last attempt version > > > > and last attempt status in FMP->GetImageInfo(). > > > > > > > > Now FMP->GetImageInfo() and ESRT have the meaningful version number. > > > > > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> > > > > --- > > > > No update since v1 > > > > > > > > lib/efi_loader/efi_firmware.c | 30 ++++++++++++++++++++++++++---- > > > > 1 file changed, 26 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > > > > index d1afafb052..ead20fa914 100644 > > > > --- a/lib/efi_loader/efi_firmware.c > > > > +++ b/lib/efi_loader/efi_firmware.c > > > > @@ -173,13 +173,38 @@ static efi_status_t efi_fill_image_desc_array( > > > > *package_version_name = NULL; /* not supported */ > > > > > > > > for (i = 0; i < num_image_type_guids; i++) { > > > > + u16 varname[13]; /* u"FmpStateXXXX" */ > > > > + efi_status_t ret; > > > > + efi_uintn_t size; > > > > + struct fmp_state var_state = { 0 }; > > > > + > > > > image_info[i].image_index = fw_array[i].image_index; > > > > image_info[i].image_type_id = fw_array[i].image_type_id; > > > > image_info[i].image_id = fw_array[i].image_index; > > > > > > > > image_info[i].image_id_name = fw_array[i].fw_name; > > > > > > > > - image_info[i].version = 0; /* not supported */ > > > > + efi_create_indexed_name(varname, sizeof(varname), "FmpState", > > > > + fw_array[i].image_index); > > > > > > Don't we have to think of the systems where multiple FMP drivers are > > > used? In those cases, 'image_index' doesn't work as an unique ID. > > > It is unlikely under the current code, but we should consider > > > any future extension. > > > > I assume that other FMP drivers implement their own version management. > > I don't have a strong opinion, but my idea about a variable name is > to use 'image_type_id' as a 'vendor' field which allows for making the variable > globally unique. Thank you for the suggestion, I agree to use image_type_id as vendor field. Regards, Masahisa Kojima > > -Takahiro Akashi > > > > Thanks, > > Masahisa Kojima > > > > > > > > -Takahiro Akashi > > > > > > > > > > + size = sizeof(var_state); > > > > + ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL, > > > > + &size, &var_state, NULL); > > > > + if (ret == EFI_SUCCESS) { > > > > + image_info[i].version = var_state.fw_version; > > > > + image_info[i].lowest_supported_image_version = > > > > + var_state.lowest_supported_version; > > > > + image_info[i].last_attempt_version = > > > > + var_state.last_attempt_version; > > > > + image_info[i].last_attempt_status = > > > > + var_state.last_attempt_status; > > > > + } else { > > > > + image_info[i].version = 0; > > > > + image_info[i].lowest_supported_image_version = 0; > > > > + image_info[i].last_attempt_version = 0; > > > > + image_info[i].last_attempt_status = > > > > + LAST_ATTEMPT_STATUS_SUCCESS; > > > > + } > > > > + > > > > image_info[i].version_name = NULL; /* not supported */ > > > > image_info[i].size = 0; > > > > image_info[i].attributes_supported = > > > > @@ -193,9 +218,6 @@ static efi_status_t efi_fill_image_desc_array( > > > > image_info[0].attributes_setting |= > > > > IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED; > > > > > > > > - image_info[i].lowest_supported_image_version = 0; > > > > - image_info[i].last_attempt_version = 0; > > > > - image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; > > > > image_info[i].hardware_instance = 1; > > > > image_info[i].dependencies = NULL; > > > > } > > > > -- > > > > 2.17.1 > > > >
diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index d1afafb052..ead20fa914 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -173,13 +173,38 @@ static efi_status_t efi_fill_image_desc_array( *package_version_name = NULL; /* not supported */ for (i = 0; i < num_image_type_guids; i++) { + u16 varname[13]; /* u"FmpStateXXXX" */ + efi_status_t ret; + efi_uintn_t size; + struct fmp_state var_state = { 0 }; + image_info[i].image_index = fw_array[i].image_index; image_info[i].image_type_id = fw_array[i].image_type_id; image_info[i].image_id = fw_array[i].image_index; image_info[i].image_id_name = fw_array[i].fw_name; - image_info[i].version = 0; /* not supported */ + efi_create_indexed_name(varname, sizeof(varname), "FmpState", + fw_array[i].image_index); + size = sizeof(var_state); + ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL, + &size, &var_state, NULL); + if (ret == EFI_SUCCESS) { + image_info[i].version = var_state.fw_version; + image_info[i].lowest_supported_image_version = + var_state.lowest_supported_version; + image_info[i].last_attempt_version = + var_state.last_attempt_version; + image_info[i].last_attempt_status = + var_state.last_attempt_status; + } else { + image_info[i].version = 0; + image_info[i].lowest_supported_image_version = 0; + image_info[i].last_attempt_version = 0; + image_info[i].last_attempt_status = + LAST_ATTEMPT_STATUS_SUCCESS; + } + image_info[i].version_name = NULL; /* not supported */ image_info[i].size = 0; image_info[i].attributes_supported = @@ -193,9 +218,6 @@ static efi_status_t efi_fill_image_desc_array( image_info[0].attributes_setting |= IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED; - image_info[i].lowest_supported_image_version = 0; - image_info[i].last_attempt_version = 0; - image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; image_info[i].hardware_instance = 1; image_info[i].dependencies = NULL; }
Current FMP->GetImageInfo() always return 0 for the firmware version, user can not identify which firmware version is currently running through the EFI interface. This commit reads the "FmpStateXXXX" EFI variable, then fills the firmware version, lowest supported version, last attempt version and last attempt status in FMP->GetImageInfo(). Now FMP->GetImageInfo() and ESRT have the meaningful version number. Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org> --- No update since v1 lib/efi_loader/efi_firmware.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-)