Re: [Xen-devel] [PATCH v1 1/1] hvmloader: allow overriding SMBIOS type 2 info

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.03.19 at 07:45, <talons.lee@xxxxxxxxx> wrote:
> Extend smbios type 2 struct to match specification, add support to
> override strings from toolstack.
> 
> Signed-off-by: Xin Li <xin.li@xxxxxxxxxx>
> 
> ---
> CC: Igor Druzhinin <igor.druzhinin@xxxxxxxxxx>
> CC: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel