Re: [RESEND PATCH v2 2/3] hvmloader: Update to SMBIOS 2.6

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.08.2025 11:58, Teddy Astie wrote:
> @@ -505,7 +505,22 @@ smbios_type_1_init(void *start, const char *xen_version,
>      p->version_str = 3;
>      p->serial_number_str = 4;
>  
> -    memcpy(p->uuid, uuid, 16);
> +    /*
> +     * Xen toolstack uses big endian UUIDs, however GUIDs (which requirement
> +     * is clarified by SMBIOS >= 2.6) has the first 3 components appearing as
> +     * being little endian and the rest as still being big endian.
> +     */

The SMBIOS spec I'm looking at (2.7.1) doesn't mention the term GUID at all
(except of course when discussing the EFI System Table entry). It's all UUID
there. Here and in the description I think this needs expressing better, to
not raise extra questions.

As to endian-ness: Since everything from byte 8 onwards are merely bytes, I
don't think it makes much sense to talk of endian-ness for that latter half.

> @@ -716,7 +731,7 @@ smbios_type_4_init(
>  
>      p->socket_designation_str = 1;
>      p->processor_type = 0x03; /* CPU */
> -    p->processor_family = 0x01; /* other */
> +    p->processor_family = p->processor_family_2 = 0x01; /* other */

In the hypervisor we need to avoid such double assignments for Misra's
sake. I think we're better off avoiding them in hvmloader as well.

> @@ -736,6 +751,22 @@ smbios_type_4_init(
>      p->l2_cache_handle = 0xffff; /* No cache information structure provided. 
> */
>      p->l3_cache_handle = 0xffff; /* No cache information structure provided. 
> */
>  
> +    /*
> +     * We have a smbios type 4 table per vCPU (which is per socket),
> +     * which means here that we have 1 socket per vCPU.
> +     */
> +    p->core_count = p->core_enabled = p->thread_count = 1;

Might we be better off keeping them all at 0 (unknown)?

> +    /*
> +     * We set 64-bits, execute protection and enhanced virtualization.
> +     * We don't set Multi-Core (bit 3) because this individual processor
> +     * (as being a single vCPU) is only having one core.
> +     *
> +     * SMBIOS specification says that these bits don't state anything
> +     * regarding the actual availability of such features.
> +     */
> +    p->processor_characteristics = 0x64;

Unless nested virt is enabled for the guest, I think we'd better avoid
setting the Enhanced Virtualization bit.

> @@ -870,8 +901,8 @@ smbios_type_17_init(void *start, uint32_t memory_size_mb, 
> int instance)
>      char buf[16];
>      struct smbios_type_17 *p = start;
>  
> -    /* Specification says Type 17 table has length of 1Bh for v2.3-2.6. */
> -    BUILD_BUG_ON(sizeof(*p) != 27);
> +    /* Specification says Type 17 table has length of 1Ch for v2.6. */
> +    BUILD_BUG_ON(sizeof(*p) != 28);
>  
>      memset(p, 0, sizeof(*p));

With this, ...

> @@ -890,6 +921,7 @@ smbios_type_17_init(void *start, uint32_t memory_size_mb, 
> int instance)
>      p->bank_locator_str = 0;
>      p->memory_type = 0x07; /* RAM */
>      p->type_detail = 0;
> +    p->attributes = 0;

... I don't think we really need this. In fact I was considering to make
a patch to strip all the unnecessary assignments of zero.

> --- a/tools/firmware/hvmloader/smbios_types.h
> +++ b/tools/firmware/hvmloader/smbios_types.h
> @@ -147,6 +147,11 @@ struct smbios_type_4 {
>      uint8_t serial_number_str;
>      uint8_t asset_tag_str;
>      uint8_t part_number_str;
> +    uint8_t core_count;
> +    uint8_t core_enabled;
> +    uint8_t thread_count;
> +    uint16_t processor_characteristics;
> +    uint16_t processor_family_2;
>  } __attribute__ ((packed));
>  
>  /* SMBIOS type 7 - Cache Information */
> @@ -185,6 +190,9 @@ struct smbios_type_9 {
>      uint16_t slot_id;
>      uint8_t slot_characteristics_1;
>      uint8_t slot_characteristics_2;
> +    uint16_t sgn_base;
> +    uint8_t bus_number_base;
> +    uint8_t devfn_base;

Where do the _base suffixes come from? Nothing like that is said in the
spec I'm looking at. Also "sgn" is imo too much of an abbreviation.

Jan