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

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

Le 02/09/2025 à 16:10, Jan Beulich a écrit :
> 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).
>       */
>
> ?
>

Sounds good to me.

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

It's mostly regarding this snippet that I am not sure it is a good idea
to expose "unknown" counts.

     for ( cpu_num = 1; cpu_num <= vcpus; cpu_num++ )
         do_struct(smbios_type_4_init(p, cpu_num, cpu_manufacturer));

AFAIU, we have as much smbios type 4 tables as we have vCPUs, things
would be very confusing if the CPU count of each exposed "socket" (per
vcpu) is unknown.

To me, either we should have 1 smbios type 4 table (i.e one socket) with
unknown core count, or what we currently have, but with one core per
"socket".

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

Most softwares will expose it as-is as said in the SMBIOS specification;
i.e "Enhanced Virtualization". Especially since it's not bound to
hardware (here virtualized) capability.
But yes, it would make more sense to only expose it when we have
meaningful nested virtualization.

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

Not a lot is said, apart that there are also "Peer" devices (SMBIOS
3.2+) defined as (7.10.9 Peer Devices)

 > Because some slots can be partitioned into smaller electrical widths,
 > additional peer device Segment/Bus/Device/Function are defined. These
 > peer groups are defined in Table 52. The base device is the lowest
 > ordered Segment/Bus/Device/Function and is listed first (offsets
 > 0Dh-11h). Peer devices are listed in the peer grouping section.

With Table 52 having the same layout as the segment/bus/... values we
have for the "base" ones.

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

So overall

  uint16_t segment_group;
  uint8_t bus;
  uint8_t devfn;

?

segment_group looks a bit off compared with the rest.
We could use `seg` like we do in Xen PCI code, as it is about PCI
segment here.

>
> Jan

Teddy


--
Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech