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
