| Message ID | 20190326064532.9054-1-xin.li@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/1] hvmloader: allow overriding SMBIOS type 2 info | expand |
>>> On 26.03.19 at 07:45, <talons.lee@gmail.com> wrote: > Extend smbios type 2 struct to match specification, add support to > override strings from toolstack. > > Signed-off-by: Xin Li <xin.li@citrix.com> > > --- > CC: Igor Druzhinin <igor.druzhinin@citrix.com> > CC: Sergey Dyasli <sergey.dyasli@citrix.com> > CC: Andrew Cooper <andrew.cooper3@citrix.com> I wonder why I have not been Cc-ed. > @@ -518,7 +520,67 @@ smbios_type_2_init(void *start) > return (start + length); > } In the subject you say "overriding", but you add new information only when it couldn't be found via get_smbios_pt_struct(). Which in turn already is sort of tool stack provided, so a means to override things already exists. Please clarify this in title and/or description. > - /* Only present when passed in */ > + s = xenstore_read(HVM_XS_BASEBOARD_MANUFACTURER, NULL); > + if ( (s != NULL) && (*s != '\0') ) Is it really a good idea to key everything else off of the presence of this one string in xenstore? Shouldn't it rather be that the structure gets instantiated whenever any of the strings are there? > + { > + memset(p, 0, sizeof(*p)); > + p->header.type = 2; > + p->header.length = sizeof(struct smbios_type_2); > + p->header.handle = SMBIOS_HANDLE_TYPE2; > + p->feature_flags = 0x09; /* Board is a hosting board and replaceable */ Doesn't setting bit 3 sort of imply also setting bit 2? Yet do we really mean to mark the board as replaceable in the first place? > + p->chassis_handle = SMBIOS_HANDLE_TYPE3; > + p->board_type = 0x0a; /* Motherboard */ > + start += sizeof(struct smbios_type_2); > + > + strcpy((char *)start, s); There's at least one example in smbios_type_3_init() showing that casts like this one aren't needed. > --- a/tools/firmware/hvmloader/smbios_types.h > +++ b/tools/firmware/hvmloader/smbios_types.h > @@ -90,6 +90,12 @@ struct smbios_type_2 { > uint8_t product_name_str; > uint8_t version_str; > uint8_t serial_number_str; > + uint8_t asset_tag_str; > + uint8_t feature_flags; > + uint8_t location_in_chassis_str; > + uint16_t chassis_handle; > + uint8_t board_type; > + uint8_t contained_handle_count; uint16_t contained_handles[]; > --- a/xen/include/public/hvm/hvm_xs_strings.h > +++ b/xen/include/public/hvm/hvm_xs_strings.h > @@ -62,18 +62,24 @@ > /* The following xenstore values are used to override some of the default > * string values in the SMBIOS table constructed in hvmloader. > */ > -#define HVM_XS_BIOS_STRINGS "bios-strings" > -#define HVM_XS_BIOS_VENDOR "bios-strings/bios-vendor" > -#define HVM_XS_BIOS_VERSION "bios-strings/bios-version" > -#define HVM_XS_SYSTEM_MANUFACTURER "bios-strings/system-manufacturer" > -#define HVM_XS_SYSTEM_PRODUCT_NAME "bios-strings/system-product-name" > -#define HVM_XS_SYSTEM_VERSION "bios-strings/system-version" > -#define HVM_XS_SYSTEM_SERIAL_NUMBER "bios-strings/system-serial-number" > -#define HVM_XS_ENCLOSURE_MANUFACTURER "bios-strings/enclosure-manufacturer" > -#define HVM_XS_ENCLOSURE_SERIAL_NUMBER "bios-strings/enclosure-serial-number" > -#define HVM_XS_ENCLOSURE_ASSET_TAG "bios-strings/enclosure-asset-tag" > -#define HVM_XS_BATTERY_MANUFACTURER "bios-strings/battery-manufacturer" > -#define HVM_XS_BATTERY_DEVICE_NAME "bios-strings/battery-device-name" > +#define HVM_XS_BIOS_STRINGS "bios-strings" > +#define HVM_XS_BIOS_VENDOR "bios-strings/bios-vendor" > +#define HVM_XS_BIOS_VERSION "bios-strings/bios-version" > +#define HVM_XS_SYSTEM_MANUFACTURER "bios-strings/system-manufacturer" > +#define HVM_XS_SYSTEM_PRODUCT_NAME "bios-strings/system-product-name" > +#define HVM_XS_SYSTEM_VERSION "bios-strings/system-version" > +#define HVM_XS_SYSTEM_SERIAL_NUMBER "bios-strings/system-serial-number" > +#define HVM_XS_BASEBOARD_MANUFACTURER "bios-strings/baseboard-manufacturer" > +#define HVM_XS_BASEBOARD_PRODUCT_NAME "bios-strings/baseboard-product-name" > +#define HVM_XS_BASEBOARD_VERSION "bios-strings/baseboard-version" > +#define HVM_XS_BASEBOARD_SERIAL_NUMBER "bios-strings/baseboard-serial-number" > +#define HVM_XS_BASEBOARD_ASSET_TAG "bios-strings/baseboard-asset-tag" > +#define HVM_XS_BASEBOARD_LOCATION_IN_CHASSIS "bios-strings/baseboard-location-in-chassis" > +#define HVM_XS_ENCLOSURE_MANUFACTURER "bios-strings/enclosure-manufacturer" > +#define HVM_XS_ENCLOSURE_SERIAL_NUMBER "bios-strings/enclosure-serial-number" > +#define HVM_XS_ENCLOSURE_ASSET_TAG "bios-strings/enclosure-asset-tag" > +#define HVM_XS_BATTERY_MANUFACTURER "bios-strings/battery-manufacturer" > +#define HVM_XS_BATTERY_DEVICE_NAME "bios-strings/battery-device-name" To be honest I'd prefer if you avoided the re-formatting, accepting the one definition that then doesn't properly align with the rest. But if others think differently, so be it. Jan
Hi Jan,
Thanks for reviewing.
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, March 26, 2019 9:15 PM
> To: Xin Li <talons.lee@gmail.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Igor Druzhinin
> <igor.druzhinin@citrix.com>; Sergey Dyasli <sergey.dyasli@citrix.com>; Xin Li
> (Talons) <xin.li@citrix.com>; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH v1 1/1] hvmloader: allow overriding SMBIOS
> type 2 info
>
> >>> On 26.03.19 at 07:45, <talons.lee@gmail.com> wrote:
> > Extend smbios type 2 struct to match specification, add support to
> > override strings from toolstack.
> >
> > Signed-off-by: Xin Li <xin.li@citrix.com>
> >
> > ---
> > CC: Igor Druzhinin <igor.druzhinin@citrix.com>
> > CC: Sergey Dyasli <sergey.dyasli@citrix.com>
> > CC: Andrew Cooper <andrew.cooper3@citrix.com>
>
> I wonder why I have not been Cc-ed.
Sorry, adding you.
>
> > @@ -518,7 +520,67 @@ smbios_type_2_init(void *start)
> > return (start + length);
> > }
>
> In the subject you say "overriding", but you add new information only when
> it couldn't be found via get_smbios_pt_struct(). Which in turn already is sort
> of tool stack provided, so a means to override things already exists. Please
> clarify this in title and/or description.
OK. how about:
hvmloader: add SMBIOS type 2 info for customized string
Extend smbios type 2 struct to match specification, add support to
write it when customized string provided and no smbios passed in.
>
> > - /* Only present when passed in */
> > + s = xenstore_read(HVM_XS_BASEBOARD_MANUFACTURER, NULL);
> > + if ( (s != NULL) && (*s != '\0') )
>
> Is it really a good idea to key everything else off of the presence of this one
> string in xenstore? Shouldn't it rather be that the structure gets instantiated
> whenever any of the strings are there?
OK.
I wanted to avoid structure without manufacturer,
the app we met only read this field.
Can I iterate the 6 keys twice?
first iteration to decide if any string is provided,
the second iteration to initialize structure.
>
> > + {
> > + memset(p, 0, sizeof(*p));
> > + p->header.type = 2;
> > + p->header.length = sizeof(struct smbios_type_2);
> > + p->header.handle = SMBIOS_HANDLE_TYPE2;
> > + p->feature_flags = 0x09; /* Board is a hosting board and
> > + replaceable */
>
> Doesn't setting bit 3 sort of imply also setting bit 2? Yet do we really mean to
> mark the board as replaceable in the first place?
For the hosts I've checked, bit 3 is set but bit 2 isn't.
>
> > + p->chassis_handle = SMBIOS_HANDLE_TYPE3;
> > + p->board_type = 0x0a; /* Motherboard */
> > + start += sizeof(struct smbios_type_2);
> > +
> > + strcpy((char *)start, s);
>
> There's at least one example in smbios_type_3_init() showing that casts like
> this one aren't needed.
OK. Will remove this unnecessary cast for void*.
>
> > --- a/tools/firmware/hvmloader/smbios_types.h
> > +++ b/tools/firmware/hvmloader/smbios_types.h
> > @@ -90,6 +90,12 @@ struct smbios_type_2 {
> > uint8_t product_name_str;
> > uint8_t version_str;
> > uint8_t serial_number_str;
> > + uint8_t asset_tag_str;
> > + uint8_t feature_flags;
> > + uint8_t location_in_chassis_str;
> > + uint16_t chassis_handle;
> > + uint8_t board_type;
> > + uint8_t contained_handle_count;
>
> uint16_t contained_handles[];
Sure. Adding this.
>
> > --- a/xen/include/public/hvm/hvm_xs_strings.h
> > +++ b/xen/include/public/hvm/hvm_xs_strings.h
> > @@ -62,18 +62,24 @@
> > /* The following xenstore values are used to override some of the default
> > * string values in the SMBIOS table constructed in hvmloader.
> > */
> > -#define HVM_XS_BIOS_STRINGS "bios-strings"
> > -#define HVM_XS_BIOS_VENDOR "bios-strings/bios-vendor"
> > -#define HVM_XS_BIOS_VERSION "bios-strings/bios-version"
> > -#define HVM_XS_SYSTEM_MANUFACTURER "bios-strings/system-
> manufacturer"
> > -#define HVM_XS_SYSTEM_PRODUCT_NAME "bios-strings/system-
> product-name"
> > -#define HVM_XS_SYSTEM_VERSION "bios-strings/system-version"
> > -#define HVM_XS_SYSTEM_SERIAL_NUMBER "bios-strings/system-serial-
> number"
> > -#define HVM_XS_ENCLOSURE_MANUFACTURER "bios-strings/enclosure-
> manufacturer"
> > -#define HVM_XS_ENCLOSURE_SERIAL_NUMBER "bios-strings/enclosure-
> serial-number"
> > -#define HVM_XS_ENCLOSURE_ASSET_TAG "bios-strings/enclosure-
> asset-tag"
> > -#define HVM_XS_BATTERY_MANUFACTURER "bios-strings/battery-
> manufacturer"
> > -#define HVM_XS_BATTERY_DEVICE_NAME "bios-strings/battery-
> device-name"
> > +#define HVM_XS_BIOS_STRINGS "bios-strings"
> > +#define HVM_XS_BIOS_VENDOR "bios-strings/bios-vendor"
> > +#define HVM_XS_BIOS_VERSION "bios-strings/bios-version"
> > +#define HVM_XS_SYSTEM_MANUFACTURER "bios-strings/system-
> manufacturer"
> > +#define HVM_XS_SYSTEM_PRODUCT_NAME "bios-strings/system-
> product-name"
> > +#define HVM_XS_SYSTEM_VERSION "bios-strings/system-
> version"
> > +#define HVM_XS_SYSTEM_SERIAL_NUMBER "bios-strings/system-
> serial-number"
> > +#define HVM_XS_BASEBOARD_MANUFACTURER "bios-
> strings/baseboard-manufacturer"
> > +#define HVM_XS_BASEBOARD_PRODUCT_NAME "bios-
> strings/baseboard-product-name"
> > +#define HVM_XS_BASEBOARD_VERSION "bios-strings/baseboard-
> version"
> > +#define HVM_XS_BASEBOARD_SERIAL_NUMBER "bios-
> strings/baseboard-serial-number"
> > +#define HVM_XS_BASEBOARD_ASSET_TAG "bios-
> strings/baseboard-asset-tag"
> > +#define HVM_XS_BASEBOARD_LOCATION_IN_CHASSIS "bios-
> strings/baseboard-location-in-chassis"
> > +#define HVM_XS_ENCLOSURE_MANUFACTURER "bios-
> strings/enclosure-manufacturer"
> > +#define HVM_XS_ENCLOSURE_SERIAL_NUMBER "bios-
> strings/enclosure-serial-number"
> > +#define HVM_XS_ENCLOSURE_ASSET_TAG "bios-strings/enclosure-
> asset-tag"
> > +#define HVM_XS_BATTERY_MANUFACTURER "bios-strings/battery-
> manufacturer"
> > +#define HVM_XS_BATTERY_DEVICE_NAME "bios-strings/battery-
> device-name"
>
> To be honest I'd prefer if you avoided the re-formatting, accepting the one
> definition that then doesn't properly align with the rest. But if others think
> differently, so be it.
Can I keep this style? This seems fit current code style.
>
> Jan
>>> On 27.03.19 at 11:54, <xin.li@citrix.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Tuesday, March 26, 2019 9:15 PM >> >> >>> On 26.03.19 at 07:45, <talons.lee@gmail.com> wrote: >> > @@ -518,7 +520,67 @@ smbios_type_2_init(void *start) >> > return (start + length); >> > } >> >> In the subject you say "overriding", but you add new information only when >> it couldn't be found via get_smbios_pt_struct(). Which in turn already is sort >> of tool stack provided, so a means to override things already exists. Please >> clarify this in title and/or description. > OK. how about: > > hvmloader: add SMBIOS type 2 info for customized string > > Extend smbios type 2 struct to match specification, add support to > write it when customized string provided and no smbios passed in. Looks reasonable to me. >> > - /* Only present when passed in */ >> > + s = xenstore_read(HVM_XS_BASEBOARD_MANUFACTURER, NULL); >> > + if ( (s != NULL) && (*s != '\0') ) >> >> Is it really a good idea to key everything else off of the presence of this one >> string in xenstore? Shouldn't it rather be that the structure gets instantiated >> whenever any of the strings are there? > OK. > I wanted to avoid structure without manufacturer, > the app we met only read this field. > > Can I iterate the 6 keys twice? > first iteration to decide if any string is provided, > the second iteration to initialize structure. I'd suggest to avoid this. Fill the structure without the surrounding if(), and simply determine the function's return values based on whether counter is non-zero. >> > + { >> > + memset(p, 0, sizeof(*p)); >> > + p->header.type = 2; >> > + p->header.length = sizeof(struct smbios_type_2); >> > + p->header.handle = SMBIOS_HANDLE_TYPE2; >> > + p->feature_flags = 0x09; /* Board is a hosting board and >> > + replaceable */ >> >> Doesn't setting bit 3 sort of imply also setting bit 2? Yet do we really mean to >> mark the board as replaceable in the first place? > For the hosts I've checked, bit 3 is set but bit 2 isn't. But my reading of the spec suggests this implication; the question just is whether that's an implication for the producer to follow or the consumer. If on actual hardware it's observed as you say, I won't object you keeping it as is. >> > --- a/xen/include/public/hvm/hvm_xs_strings.h >> > +++ b/xen/include/public/hvm/hvm_xs_strings.h >> > @@ -62,18 +62,24 @@ >> > /* The following xenstore values are used to override some of the default >> > * string values in the SMBIOS table constructed in hvmloader. >> > */ >> > -#define HVM_XS_BIOS_STRINGS "bios-strings" >> > -#define HVM_XS_BIOS_VENDOR "bios-strings/bios-vendor" >> > -#define HVM_XS_BIOS_VERSION "bios-strings/bios-version" >> > -#define HVM_XS_SYSTEM_MANUFACTURER "bios-strings/system- >> manufacturer" >> > -#define HVM_XS_SYSTEM_PRODUCT_NAME "bios-strings/system- >> product-name" >> > -#define HVM_XS_SYSTEM_VERSION "bios-strings/system-version" >> > -#define HVM_XS_SYSTEM_SERIAL_NUMBER "bios-strings/system-serial- >> number" >> > -#define HVM_XS_ENCLOSURE_MANUFACTURER "bios-strings/enclosure- >> manufacturer" >> > -#define HVM_XS_ENCLOSURE_SERIAL_NUMBER "bios-strings/enclosure- >> serial-number" >> > -#define HVM_XS_ENCLOSURE_ASSET_TAG "bios-strings/enclosure- >> asset-tag" >> > -#define HVM_XS_BATTERY_MANUFACTURER "bios-strings/battery- >> manufacturer" >> > -#define HVM_XS_BATTERY_DEVICE_NAME "bios-strings/battery- >> device-name" >> > +#define HVM_XS_BIOS_STRINGS "bios-strings" >> > +#define HVM_XS_BIOS_VENDOR "bios-strings/bios-vendor" >> > +#define HVM_XS_BIOS_VERSION "bios-strings/bios-version" >> > +#define HVM_XS_SYSTEM_MANUFACTURER "bios-strings/system- >> manufacturer" >> > +#define HVM_XS_SYSTEM_PRODUCT_NAME "bios-strings/system- >> product-name" >> > +#define HVM_XS_SYSTEM_VERSION "bios-strings/system- >> version" >> > +#define HVM_XS_SYSTEM_SERIAL_NUMBER "bios-strings/system- >> serial-number" >> > +#define HVM_XS_BASEBOARD_MANUFACTURER "bios- >> strings/baseboard-manufacturer" >> > +#define HVM_XS_BASEBOARD_PRODUCT_NAME "bios- >> strings/baseboard-product-name" >> > +#define HVM_XS_BASEBOARD_VERSION "bios-strings/baseboard- >> version" >> > +#define HVM_XS_BASEBOARD_SERIAL_NUMBER "bios- >> strings/baseboard-serial-number" >> > +#define HVM_XS_BASEBOARD_ASSET_TAG "bios- >> strings/baseboard-asset-tag" >> > +#define HVM_XS_BASEBOARD_LOCATION_IN_CHASSIS "bios- >> strings/baseboard-location-in-chassis" >> > +#define HVM_XS_ENCLOSURE_MANUFACTURER "bios- >> strings/enclosure-manufacturer" >> > +#define HVM_XS_ENCLOSURE_SERIAL_NUMBER "bios- >> strings/enclosure-serial-number" >> > +#define HVM_XS_ENCLOSURE_ASSET_TAG "bios-strings/enclosure- >> asset-tag" >> > +#define HVM_XS_BATTERY_MANUFACTURER "bios-strings/battery- >> manufacturer" >> > +#define HVM_XS_BATTERY_DEVICE_NAME "bios-strings/battery- >> device-name" >> >> To be honest I'd prefer if you avoided the re-formatting, accepting the one >> definition that then doesn't properly align with the rest. But if others think >> differently, so be it. > Can I keep this style? This seems fit current code style. I'm afraid I don't understand the question in the light of me having asked to avoid the re-formatting. Is the question perhaps targeted at others, not me? Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Wednesday, March 27, 2019 7:24 PM > To: Xin Li (Talons) <xin.li@citrix.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Igor Druzhinin > <igor.druzhinin@citrix.com>; Sergey Dyasli <sergey.dyasli@citrix.com>; Xin Li > <talons.lee@gmail.com>; xen-devel@lists.xen.org > Subject: RE: [Xen-devel] [PATCH v1 1/1] hvmloader: allow overriding SMBIOS > type 2 info > >> > - /* Only present when passed in */ > >> > + s = xenstore_read(HVM_XS_BASEBOARD_MANUFACTURER, NULL); > >> > + if ( (s != NULL) && (*s != '\0') ) > >> > >> Is it really a good idea to key everything else off of the presence > >> of this one string in xenstore? Shouldn't it rather be that the > >> structure gets instantiated whenever any of the strings are there? > > OK. > > I wanted to avoid structure without manufacturer, the app we met only > > read this field. > > > > Can I iterate the 6 keys twice? > > first iteration to decide if any string is provided, the second > > iteration to initialize structure. > > I'd suggest to avoid this. Fill the structure without the surrounding if(), and > simply determine the function's return values based on whether counter is > non-zero. OK. Other structs can override this header. > >> > --- a/xen/include/public/hvm/hvm_xs_strings.h > >> > +++ b/xen/include/public/hvm/hvm_xs_strings.h > >> > @@ -62,18 +62,24 @@ > >> > /* The following xenstore values are used to override some of the > default > >> > * string values in the SMBIOS table constructed in hvmloader. > >> > */ > >> > -#define HVM_XS_BIOS_STRINGS "bios-strings" > >> > -#define HVM_XS_BIOS_VENDOR "bios-strings/bios-vendor" > >> > -#define HVM_XS_BIOS_VERSION "bios-strings/bios-version" > >> > -#define HVM_XS_SYSTEM_MANUFACTURER "bios-strings/system- > >> manufacturer" > >> > -#define HVM_XS_SYSTEM_PRODUCT_NAME "bios-strings/system- > >> product-name" > >> > -#define HVM_XS_SYSTEM_VERSION "bios-strings/system- > version" > >> > -#define HVM_XS_SYSTEM_SERIAL_NUMBER "bios-strings/system- > serial- > >> number" > >> > -#define HVM_XS_ENCLOSURE_MANUFACTURER "bios- > strings/enclosure- > >> manufacturer" > >> > -#define HVM_XS_ENCLOSURE_SERIAL_NUMBER "bios- > strings/enclosure- > >> serial-number" > >> > -#define HVM_XS_ENCLOSURE_ASSET_TAG "bios-strings/enclosure- > >> asset-tag" > >> > -#define HVM_XS_BATTERY_MANUFACTURER "bios-strings/battery- > >> manufacturer" > >> > -#define HVM_XS_BATTERY_DEVICE_NAME "bios-strings/battery- > >> device-name" > >> > +#define HVM_XS_BIOS_STRINGS "bios-strings" > >> > +#define HVM_XS_BIOS_VENDOR "bios-strings/bios-vendor" > >> > +#define HVM_XS_BIOS_VERSION "bios-strings/bios-version" > >> > +#define HVM_XS_SYSTEM_MANUFACTURER "bios- > strings/system- > >> manufacturer" > >> > +#define HVM_XS_SYSTEM_PRODUCT_NAME "bios- > strings/system- > >> product-name" > >> > +#define HVM_XS_SYSTEM_VERSION "bios-strings/system- > >> version" > >> > +#define HVM_XS_SYSTEM_SERIAL_NUMBER "bios- > strings/system- > >> serial-number" > >> > +#define HVM_XS_BASEBOARD_MANUFACTURER "bios- > >> strings/baseboard-manufacturer" > >> > +#define HVM_XS_BASEBOARD_PRODUCT_NAME "bios- > >> strings/baseboard-product-name" > >> > +#define HVM_XS_BASEBOARD_VERSION "bios- > strings/baseboard- > >> version" > >> > +#define HVM_XS_BASEBOARD_SERIAL_NUMBER "bios- > >> strings/baseboard-serial-number" > >> > +#define HVM_XS_BASEBOARD_ASSET_TAG "bios- > >> strings/baseboard-asset-tag" > >> > +#define HVM_XS_BASEBOARD_LOCATION_IN_CHASSIS "bios- > >> strings/baseboard-location-in-chassis" > >> > +#define HVM_XS_ENCLOSURE_MANUFACTURER "bios- > >> strings/enclosure-manufacturer" > >> > +#define HVM_XS_ENCLOSURE_SERIAL_NUMBER "bios- > >> strings/enclosure-serial-number" > >> > +#define HVM_XS_ENCLOSURE_ASSET_TAG "bios- > strings/enclosure- > >> asset-tag" > >> > +#define HVM_XS_BATTERY_MANUFACTURER "bios- > strings/battery- > >> manufacturer" > >> > +#define HVM_XS_BATTERY_DEVICE_NAME "bios-strings/battery- > >> device-name" > >> > >> To be honest I'd prefer if you avoided the re-formatting, accepting > >> the one definition that then doesn't properly align with the rest. > >> But if others think differently, so be it. > > Can I keep this style? This seems fit current code style. > > I'm afraid I don't understand the question in the light of me having asked to > avoid the re-formatting. Is the question perhaps targeted at others, not me? OK. Then HVM_XS_BASEBOARD_LOCATION_IN_CHASSIS won't align with the rest. > Jan
diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c index 40d8399be1..90cfe9fd10 100644 --- a/tools/firmware/hvmloader/smbios.c +++ b/tools/firmware/hvmloader/smbios.c @@ -497,9 +497,11 @@ static void * smbios_type_2_init(void *start) { struct smbios_type_2 *p = (struct smbios_type_2 *)start; + const char *s; uint8_t *ptr; void *pts; uint32_t length; + uint32_t counter = 0; pts = get_smbios_pt_struct(2, &length); if ( (pts != NULL)&&(length > 0) ) @@ -518,7 +520,67 @@ smbios_type_2_init(void *start) return (start + length); } - /* Only present when passed in */ + s = xenstore_read(HVM_XS_BASEBOARD_MANUFACTURER, NULL); + if ( (s != NULL) && (*s != '\0') ) + { + memset(p, 0, sizeof(*p)); + p->header.type = 2; + p->header.length = sizeof(struct smbios_type_2); + p->header.handle = SMBIOS_HANDLE_TYPE2; + p->feature_flags = 0x09; /* Board is a hosting board and replaceable */ + p->chassis_handle = SMBIOS_HANDLE_TYPE3; + p->board_type = 0x0a; /* Motherboard */ + start += sizeof(struct smbios_type_2); + + strcpy((char *)start, s); + start += strlen(s) + 1; + p->manufacturer_str = ++counter; + + s = xenstore_read(HVM_XS_BASEBOARD_PRODUCT_NAME, NULL); + if ( (s != NULL) && (*s != '\0') ) + { + strcpy((char *)start, s); + start += strlen(s) + 1; + p->product_name_str = ++counter; + } + + s = xenstore_read(HVM_XS_BASEBOARD_VERSION, NULL); + if ( (s != NULL) && (*s != '\0') ) + { + strcpy((char *)start, s); + start += strlen(s) + 1; + p->version_str = ++counter; + } + + s = xenstore_read(HVM_XS_BASEBOARD_SERIAL_NUMBER, NULL); + if ( (s != NULL) && (*s != '\0') ) + { + strcpy((char *)start, s); + start += strlen(s) + 1; + p->serial_number_str = ++counter; + } + + s = xenstore_read(HVM_XS_BASEBOARD_ASSET_TAG, NULL); + if ( (s != NULL) && (*s != '\0') ) + { + strcpy((char *)start, s); + start += strlen(s) + 1; + p->asset_tag_str = ++counter; + } + + s = xenstore_read(HVM_XS_BASEBOARD_LOCATION_IN_CHASSIS, NULL); + if ( (s != NULL) && (*s != '\0') ) + { + strcpy((char *)start, s); + start += strlen(s) + 1; + p->location_in_chassis_str = ++counter; + } + + *((uint8_t *)start) = 0; + return start + 1; + } + + /* Only present when passed in or overriden */ return start; } diff --git a/tools/firmware/hvmloader/smbios_types.h b/tools/firmware/hvmloader/smbios_types.h index acb63e2fe9..834ef38fba 100644 --- a/tools/firmware/hvmloader/smbios_types.h +++ b/tools/firmware/hvmloader/smbios_types.h @@ -90,6 +90,12 @@ struct smbios_type_2 { uint8_t product_name_str; uint8_t version_str; uint8_t serial_number_str; + uint8_t asset_tag_str; + uint8_t feature_flags; + uint8_t location_in_chassis_str; + uint16_t chassis_handle; + uint8_t board_type; + uint8_t contained_handle_count; } __attribute__ ((packed)); /* System Enclosure - Contained Elements */ diff --git a/xen/include/public/hvm/hvm_xs_strings.h b/xen/include/public/hvm/hvm_xs_strings.h index fea1dd4407..e3b328321d 100644 --- a/xen/include/public/hvm/hvm_xs_strings.h +++ b/xen/include/public/hvm/hvm_xs_strings.h @@ -62,18 +62,24 @@ /* The following xenstore values are used to override some of the default * string values in the SMBIOS table constructed in hvmloader. */ -#define HVM_XS_BIOS_STRINGS "bios-strings" -#define HVM_XS_BIOS_VENDOR "bios-strings/bios-vendor" -#define HVM_XS_BIOS_VERSION "bios-strings/bios-version" -#define HVM_XS_SYSTEM_MANUFACTURER "bios-strings/system-manufacturer" -#define HVM_XS_SYSTEM_PRODUCT_NAME "bios-strings/system-product-name" -#define HVM_XS_SYSTEM_VERSION "bios-strings/system-version" -#define HVM_XS_SYSTEM_SERIAL_NUMBER "bios-strings/system-serial-number" -#define HVM_XS_ENCLOSURE_MANUFACTURER "bios-strings/enclosure-manufacturer" -#define HVM_XS_ENCLOSURE_SERIAL_NUMBER "bios-strings/enclosure-serial-number" -#define HVM_XS_ENCLOSURE_ASSET_TAG "bios-strings/enclosure-asset-tag" -#define HVM_XS_BATTERY_MANUFACTURER "bios-strings/battery-manufacturer" -#define HVM_XS_BATTERY_DEVICE_NAME "bios-strings/battery-device-name" +#define HVM_XS_BIOS_STRINGS "bios-strings" +#define HVM_XS_BIOS_VENDOR "bios-strings/bios-vendor" +#define HVM_XS_BIOS_VERSION "bios-strings/bios-version" +#define HVM_XS_SYSTEM_MANUFACTURER "bios-strings/system-manufacturer" +#define HVM_XS_SYSTEM_PRODUCT_NAME "bios-strings/system-product-name" +#define HVM_XS_SYSTEM_VERSION "bios-strings/system-version" +#define HVM_XS_SYSTEM_SERIAL_NUMBER "bios-strings/system-serial-number" +#define HVM_XS_BASEBOARD_MANUFACTURER "bios-strings/baseboard-manufacturer" +#define HVM_XS_BASEBOARD_PRODUCT_NAME "bios-strings/baseboard-product-name" +#define HVM_XS_BASEBOARD_VERSION "bios-strings/baseboard-version" +#define HVM_XS_BASEBOARD_SERIAL_NUMBER "bios-strings/baseboard-serial-number" +#define HVM_XS_BASEBOARD_ASSET_TAG "bios-strings/baseboard-asset-tag" +#define HVM_XS_BASEBOARD_LOCATION_IN_CHASSIS "bios-strings/baseboard-location-in-chassis" +#define HVM_XS_ENCLOSURE_MANUFACTURER "bios-strings/enclosure-manufacturer" +#define HVM_XS_ENCLOSURE_SERIAL_NUMBER "bios-strings/enclosure-serial-number" +#define HVM_XS_ENCLOSURE_ASSET_TAG "bios-strings/enclosure-asset-tag" +#define HVM_XS_BATTERY_MANUFACTURER "bios-strings/battery-manufacturer" +#define HVM_XS_BATTERY_DEVICE_NAME "bios-strings/battery-device-name" /* 1 to 99 OEM strings can be set in xenstore using values of the form * below. These strings will be loaded into the SMBIOS type 11 structure.
Extend smbios type 2 struct to match specification, add support to override strings from toolstack. Signed-off-by: Xin Li <xin.li@citrix.com> --- CC: Igor Druzhinin <igor.druzhinin@citrix.com> CC: Sergey Dyasli <sergey.dyasli@citrix.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> --- tools/firmware/hvmloader/smbios.c | 64 ++++++++++++++++++++++++- tools/firmware/hvmloader/smbios_types.h | 6 +++ xen/include/public/hvm/hvm_xs_strings.h | 30 +++++++----- 3 files changed, 87 insertions(+), 13 deletions(-)