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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.03.19 at 11:54, <xin.li@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Tuesday, March 26, 2019 9:15 PM
>> 
>> >>> On 26.03.19 at 07:45, <talons.lee@xxxxxxxxx> 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


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