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

["Teddy Astie" <teddy.astie@xxxxxxxxxx>]

Le 02/09/2025 à 14:38, Jan Beulich a écrit :
> 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.
>

Yes (this is also true for SMBIOS 3.9.0 spec). Not sure how to express
that aside saying that UUID encoding differs between SMBIOS
specification and Xen representation.

> 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.
>

It's more to insist that the byte ordering differs with the first parts.

>> @@ -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.
>

yes

>> @@ -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)?
>

Making it Unknown would feel a bit weird, unless we only keep only one
(instead of the number of vCPUs) of these table with core_count,
core_enabled, thread_count as 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.
>

I am not sure how to interpret the SMBIOS spec on this

 > Enhanced Virtualization indicates that the processor can execute
 > enhanced virtualization instructions. This bit does not indicate the
 > present state of this feature

I see it as: Virtualization is available or can be enabled (with nested
virtualization).
Or as : We have virtualization related instructions.

>> @@ -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.
>

My version of the specification (3.9.0) says

0Dh 2.6+ Segment Group Number (Base)
0Fh 2.6+ Bus Number (Base)
10h 2.6+ Device/Function Number (Base)

Regarding sgn, maybe we can use `segment` instead ?

> Jan
>

Teddy


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech