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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.09.2025 15:24, Teddy Astie wrote:
> 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.

Maybe

    /*
     * Xen toolstack uses big endian UUIDs, whereas the UUIDs used by SMBIOS,
     * also known as GUIDs, have the first 3 components appearing in little
     * endian (with this requirement clarified by SMBIOS >= 2.6).
     */

?

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

I don't see how unknown or not is related to how many structure instances
we surface. Your suggestion of using 1 in all three fields isn't quite
what we'd like to present to guests. Once we sort virtual topology in Xen,
we may want to expose sane values here. Yet if we expose 1-s now, that
adjustment would need to happen in lock-step with the hypervisor changes.
Whereas when we expose "unknown" that can be done at a convenient later
time.

>>> +    /*
>>> +     * 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.

You want to view what we expose to the guest from the guest's perspective
on its own little world, I think.

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

Without any clarification what "(Base)" means, afaics.

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

Why not segment_group or seg_group (seeing how devfn also is an abbreviation)?
And if we omit _number there and on devfn, it's hard to see why it shouldn't
be just "bus" then as well.

Jan