| Message ID | 20220913102705.65506-18-clombard@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Implement MCTP and PLDM features | expand |
On Tue, Sep 13, 2022 at 12:27:01PM +0200, Christophe Lombard wrote: > Use the GetFruRecordByOptionReq command to retrieve the bmc information > with: "FRU Field Type": Version > "FRU Record Set Identifier": 1, > "FRU Record Type": "General(1)" > and update the "bmc-firmware-version" device-tree field. > > Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com> Reviewed-by: Abhishek Singh Tomar <abhishek@linux.ibm.com> > --- > core/pldm/pldm-fru-requests.c | 60 +++++++++++++++++++++++++++++++++++ > core/pldm/pldm.h | 1 + > include/pldm.h | 5 +++ > 3 files changed, 66 insertions(+) > > diff --git a/core/pldm/pldm-fru-requests.c b/core/pldm/pldm-fru-requests.c > index 02c9b968..b07a7b1a 100644 > --- a/core/pldm/pldm-fru-requests.c > +++ b/core/pldm/pldm-fru-requests.c > @@ -15,6 +15,7 @@ static void *fru_record_table; > static size_t fru_record_length; > > static bool fru_ready; > +static char *bmc_version = NULL; > > static void fru_init_complete(bool success) > { > @@ -33,6 +34,16 @@ static void fru_init_complete(bool success) > fru_ready = true; > } > > +int pldm_fru_get_bmc_version(void *bv) > +{ > + if (bmc_version == NULL) > + return OPAL_HARDWARE; > + > + memcpy(bv, bmc_version, strlen(bmc_version) + 1); > + > + return OPAL_SUCCESS; > +} > + > static int get_fru_record_table_req(void **record_table_data, > size_t *record_table_length) > { > @@ -115,6 +126,55 @@ static int get_fru_record_table_req(void **record_table_data, > return OPAL_SUCCESS; > } > > +int pldm_fru_dt_add_bmc_version(void) > +{ > + struct pldm_fru_record_data_format *data; > + struct pldm_fru_record_tlv *tlv; > + struct dt_node *dt_fw_version; > + uint8_t *record_table; > + size_t record_size; > + > + dt_fw_version = dt_find_by_name(dt_root, "ibm,firmware-versions"); > + if (!dt_fw_version) > + return OPAL_SUCCESS; > + > + /* retrieve the bmc information with > + * "FRU Record Set Identifier": 1, > + * "FRU Record Type": "General(1)" > + * "FRU Field Type": Version > + * > + * we can not know size of the record table got by options > + * in advance, but it must be less than the source table. So > + * it's safe to use sizeof the source table. > + */ > + record_table = zalloc(fru_record_length); > + record_size = fru_record_length; > + get_fru_record_by_option( > + fru_record_table, > + fru_record_length - PLDM_GET_FRU_RECORD_TABLE_MIN_RESP_BYTES, > + record_table, > + &record_size, > + 1, > + PLDM_FRU_RECORD_TYPE_GENERAL, > + PLDM_FRU_FIELD_TYPE_VERSION); > + > + /* get tlv value */ > + data = (struct pldm_fru_record_data_format *)record_table; > + tlv = (struct pldm_fru_record_tlv *)data->tlvs; > + prlog(PR_DEBUG, "%s - value: %s\n", __func__, tlv->value); > + > + dt_add_property_string(dt_fw_version, "bmc-firmware-version", > + tlv->value); > + > + /* store the bmc version */ > + bmc_version = zalloc(tlv->length + 1); > + memcpy(bmc_version, tlv->value, tlv->length); > + > + free(record_table); > + > + return OPAL_SUCCESS; > +} > + > int pldm_fru_init(void) > { > int rc; > diff --git a/core/pldm/pldm.h b/core/pldm/pldm.h > index f9e9db40..5ee53cb9 100644 > --- a/core/pldm/pldm.h > +++ b/core/pldm/pldm.h > @@ -47,6 +47,7 @@ int pldm_responder_handle_request(struct pldm_rx_data *rx); > int pldm_responder_init(void); > > /* Requester support */ > +int pldm_fru_get_bmc_version(void *bv); > int pldm_fru_init(void); > > int pldm_bios_find_lid_by_attr_name(const char *name, char **lid); > diff --git a/include/pldm.h b/include/pldm.h > index 3e653aa3..693c1ae5 100644 > --- a/include/pldm.h > +++ b/include/pldm.h > @@ -25,4 +25,9 @@ int pldm_platform_power_off(void); > */ > int pldm_platform_restart(void); > > +/** > + * Update the firmware version device-tree field > + */ > +int pldm_fru_dt_add_bmc_version(void); > + > #endif /* __PLDM_H__ */ > -- > 2.37.3 > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
On 13/09/2022 12:27, Christophe Lombard wrote: > Use the GetFruRecordByOptionReq command to retrieve the bmc information > with: "FRU Field Type": Version > "FRU Record Set Identifier": 1, > "FRU Record Type": "General(1)" > and update the "bmc-firmware-version" device-tree field. > > Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com> > --- > core/pldm/pldm-fru-requests.c | 60 +++++++++++++++++++++++++++++++++++ > core/pldm/pldm.h | 1 + > include/pldm.h | 5 +++ > 3 files changed, 66 insertions(+) > > diff --git a/core/pldm/pldm-fru-requests.c b/core/pldm/pldm-fru-requests.c > index 02c9b968..b07a7b1a 100644 > --- a/core/pldm/pldm-fru-requests.c > +++ b/core/pldm/pldm-fru-requests.c > @@ -15,6 +15,7 @@ static void *fru_record_table; > static size_t fru_record_length; > > static bool fru_ready; > +static char *bmc_version = NULL; > > static void fru_init_complete(bool success) > { > @@ -33,6 +34,16 @@ static void fru_init_complete(bool success) > fru_ready = true; > } > > +int pldm_fru_get_bmc_version(void *bv) That's a dangerous API as we don't know what has been allocated and we're going to write over it an unknown size. Should at the minimum take an extra parameter specifying the buffer size. > +{ > + if (bmc_version == NULL) > + return OPAL_HARDWARE; > + > + memcpy(bv, bmc_version, strlen(bmc_version) + 1); > + > + return OPAL_SUCCESS; > +} > + > static int get_fru_record_table_req(void **record_table_data, > size_t *record_table_length) > { > @@ -115,6 +126,55 @@ static int get_fru_record_table_req(void **record_table_data, > return OPAL_SUCCESS; > } > > +int pldm_fru_dt_add_bmc_version(void) > +{ > + struct pldm_fru_record_data_format *data; > + struct pldm_fru_record_tlv *tlv; > + struct dt_node *dt_fw_version; > + uint8_t *record_table; > + size_t record_size; > + > + dt_fw_version = dt_find_by_name(dt_root, "ibm,firmware-versions"); > + if (!dt_fw_version) > + return OPAL_SUCCESS; > + > + /* retrieve the bmc information with > + * "FRU Record Set Identifier": 1, > + * "FRU Record Type": "General(1)" > + * "FRU Field Type": Version > + * > + * we can not know size of the record table got by options > + * in advance, but it must be less than the source table. So > + * it's safe to use sizeof the source table. > + */ > + record_table = zalloc(fru_record_length); We need to verify that the init went fine and fru_record_table/fru_record_length are defined. It's a general pattern in all those functions with static data. It's the only time I've seen where you don't verify it first, but it's probably worth double-checking all of them. > + record_size = fru_record_length; > + get_fru_record_by_option( > + fru_record_table, > + fru_record_length - PLDM_GET_FRU_RECORD_TABLE_MIN_RESP_BYTES, PLDM_GET_FRU_RECORD_TABLE_MIN_RESP_BYTES was already deducted in decode_get_fru_record_table_resp(), so fru_record_length should be the correct length. At least, that's what I understand :) > + record_table, > + &record_size, > + 1, > + PLDM_FRU_RECORD_TYPE_GENERAL, > + PLDM_FRU_FIELD_TYPE_VERSION); > + > + /* get tlv value */ We need to check 'record_size' and make sure we have (at least) one entry in the record table. Fred
Le 12/04/2023 à 17:41, Frederic Barrat a écrit : > > > On 13/09/2022 12:27, Christophe Lombard wrote: >> Use the GetFruRecordByOptionReq command to retrieve the bmc information >> with: "FRU Field Type": Version >> "FRU Record Set Identifier": 1, >> "FRU Record Type": "General(1)" >> and update the "bmc-firmware-version" device-tree field. >> >> Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com> >> --- >> core/pldm/pldm-fru-requests.c | 60 +++++++++++++++++++++++++++++++++++ >> core/pldm/pldm.h | 1 + >> include/pldm.h | 5 +++ >> 3 files changed, 66 insertions(+) >> >> diff --git a/core/pldm/pldm-fru-requests.c >> b/core/pldm/pldm-fru-requests.c >> index 02c9b968..b07a7b1a 100644 >> --- a/core/pldm/pldm-fru-requests.c >> +++ b/core/pldm/pldm-fru-requests.c >> @@ -15,6 +15,7 @@ static void *fru_record_table; >> static size_t fru_record_length; >> static bool fru_ready; >> +static char *bmc_version = NULL; >> static void fru_init_complete(bool success) >> { >> @@ -33,6 +34,16 @@ static void fru_init_complete(bool success) >> fru_ready = true; >> } >> +int pldm_fru_get_bmc_version(void *bv) > > > That's a dangerous API as we don't know what has been allocated and > we're going to write over it an unknown size. Should at the minimum > take an extra parameter specifying the buffer size. > You are right. We have to new parameters to control the memcpy operation. Thanks > >> +{ >> + if (bmc_version == NULL) >> + return OPAL_HARDWARE; >> + >> + memcpy(bv, bmc_version, strlen(bmc_version) + 1); > + >> + return OPAL_SUCCESS; >> +} >> + >> static int get_fru_record_table_req(void **record_table_data, >> size_t *record_table_length) >> { >> @@ -115,6 +126,55 @@ static int get_fru_record_table_req(void >> **record_table_data, >> return OPAL_SUCCESS; >> } >> +int pldm_fru_dt_add_bmc_version(void) >> +{ >> + struct pldm_fru_record_data_format *data; >> + struct pldm_fru_record_tlv *tlv; >> + struct dt_node *dt_fw_version; >> + uint8_t *record_table; >> + size_t record_size; >> + >> + dt_fw_version = dt_find_by_name(dt_root, "ibm,firmware-versions"); >> + if (!dt_fw_version) >> + return OPAL_SUCCESS; >> + >> + /* retrieve the bmc information with >> + * "FRU Record Set Identifier": 1, >> + * "FRU Record Type": "General(1)" >> + * "FRU Field Type": Version >> + * >> + * we can not know size of the record table got by options >> + * in advance, but it must be less than the source table. So >> + * it's safe to use sizeof the source table. >> + */ >> + record_table = zalloc(fru_record_length); > > > We need to verify that the init went fine and > fru_record_table/fru_record_length are defined. It's a general pattern > in all those functions with static data. It's the only time I've seen > where you don't verify it first, but it's probably worth > double-checking all of them. > > That's true. We need to check that we got successfully the pldm fru data using fru_ready. thanks >> + record_size = fru_record_length; >> + get_fru_record_by_option( >> + fru_record_table, >> + fru_record_length - >> PLDM_GET_FRU_RECORD_TABLE_MIN_RESP_BYTES, > > > PLDM_GET_FRU_RECORD_TABLE_MIN_RESP_BYTES was already deducted in > decode_get_fru_record_table_resp(), so fru_record_length should be the > correct length. At least, that's what I understand :) > Really, I don't remember. You should certainly right, but I need to dobble check it. Thanks > >> + record_table, >> + &record_size, >> + 1, >> + PLDM_FRU_RECORD_TYPE_GENERAL, >> + PLDM_FRU_FIELD_TYPE_VERSION); >> + >> + /* get tlv value */ > > > We need to check 'record_size' and make sure we have (at least) one > entry in the record table. > will do. thanks > Fred
diff --git a/core/pldm/pldm-fru-requests.c b/core/pldm/pldm-fru-requests.c index 02c9b968..b07a7b1a 100644 --- a/core/pldm/pldm-fru-requests.c +++ b/core/pldm/pldm-fru-requests.c @@ -15,6 +15,7 @@ static void *fru_record_table; static size_t fru_record_length; static bool fru_ready; +static char *bmc_version = NULL; static void fru_init_complete(bool success) { @@ -33,6 +34,16 @@ static void fru_init_complete(bool success) fru_ready = true; } +int pldm_fru_get_bmc_version(void *bv) +{ + if (bmc_version == NULL) + return OPAL_HARDWARE; + + memcpy(bv, bmc_version, strlen(bmc_version) + 1); + + return OPAL_SUCCESS; +} + static int get_fru_record_table_req(void **record_table_data, size_t *record_table_length) { @@ -115,6 +126,55 @@ static int get_fru_record_table_req(void **record_table_data, return OPAL_SUCCESS; } +int pldm_fru_dt_add_bmc_version(void) +{ + struct pldm_fru_record_data_format *data; + struct pldm_fru_record_tlv *tlv; + struct dt_node *dt_fw_version; + uint8_t *record_table; + size_t record_size; + + dt_fw_version = dt_find_by_name(dt_root, "ibm,firmware-versions"); + if (!dt_fw_version) + return OPAL_SUCCESS; + + /* retrieve the bmc information with + * "FRU Record Set Identifier": 1, + * "FRU Record Type": "General(1)" + * "FRU Field Type": Version + * + * we can not know size of the record table got by options + * in advance, but it must be less than the source table. So + * it's safe to use sizeof the source table. + */ + record_table = zalloc(fru_record_length); + record_size = fru_record_length; + get_fru_record_by_option( + fru_record_table, + fru_record_length - PLDM_GET_FRU_RECORD_TABLE_MIN_RESP_BYTES, + record_table, + &record_size, + 1, + PLDM_FRU_RECORD_TYPE_GENERAL, + PLDM_FRU_FIELD_TYPE_VERSION); + + /* get tlv value */ + data = (struct pldm_fru_record_data_format *)record_table; + tlv = (struct pldm_fru_record_tlv *)data->tlvs; + prlog(PR_DEBUG, "%s - value: %s\n", __func__, tlv->value); + + dt_add_property_string(dt_fw_version, "bmc-firmware-version", + tlv->value); + + /* store the bmc version */ + bmc_version = zalloc(tlv->length + 1); + memcpy(bmc_version, tlv->value, tlv->length); + + free(record_table); + + return OPAL_SUCCESS; +} + int pldm_fru_init(void) { int rc; diff --git a/core/pldm/pldm.h b/core/pldm/pldm.h index f9e9db40..5ee53cb9 100644 --- a/core/pldm/pldm.h +++ b/core/pldm/pldm.h @@ -47,6 +47,7 @@ int pldm_responder_handle_request(struct pldm_rx_data *rx); int pldm_responder_init(void); /* Requester support */ +int pldm_fru_get_bmc_version(void *bv); int pldm_fru_init(void); int pldm_bios_find_lid_by_attr_name(const char *name, char **lid); diff --git a/include/pldm.h b/include/pldm.h index 3e653aa3..693c1ae5 100644 --- a/include/pldm.h +++ b/include/pldm.h @@ -25,4 +25,9 @@ int pldm_platform_power_off(void); */ int pldm_platform_restart(void); +/** + * Update the firmware version device-tree field + */ +int pldm_fru_dt_add_bmc_version(void); + #endif /* __PLDM_H__ */
Use the GetFruRecordByOptionReq command to retrieve the bmc information with: "FRU Field Type": Version "FRU Record Set Identifier": 1, "FRU Record Type": "General(1)" and update the "bmc-firmware-version" device-tree field. Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com> --- core/pldm/pldm-fru-requests.c | 60 +++++++++++++++++++++++++++++++++++ core/pldm/pldm.h | 1 + include/pldm.h | 5 +++ 3 files changed, 66 insertions(+)