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
