Currently, hvmloader uses SMBIOS 2.4, however, when using OVMF, the
SMBIOS is patched to 2.8, which has clarified the UUID format (as GUID).

In Linux, if the SMBIOS version is >= 2.6, the GUID format is used, else
(undefined as per SMBIOS spec), big endian is used (used by Xen). Therefore,
you have a endian mismatch causing the UUIDs to mismatch in the guest.

$ cat /sys/hypervisor/uuid
e865e63f-3d30-4f0b-83e0-8fdfc1e30eb7
$ cat /sys/devices/virtual/dmi/id/product_uuid
3fe665e8-303d-0b4f-83e0-8fdfc1e30eb7
$ cat /sys/devices/virtual/dmi/id/product_serial
e865e63f-3d30-4f0b-83e0-8fdfc1e30eb7

This patch updates the SMBIOS version from 2.4 to 2.6 and does the appropriate
modifications of the table. This effectively fix this endianness mismatch with
OVMF; while the UUID displayed by Linux is still the same for SeaBIOS.

Fixes: c683914ef913 ("Add code to generate SMBIOS tables to hvmloader.")
(SMBIOS versions before 2.6 has a ill-defined UUID definition)
Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
v2:
 - rebase onto staging
 - introduce missing SMBIOS 2.5-2.6 fields
 - check for new SMBIOS 2.6 table lengths
 - update UUID conversion comment
 - add Fixes: note
 ---
 tools/firmware/hvmloader/smbios.c       | 50 ++++++++++++++++++++-----
 tools/firmware/hvmloader/smbios_types.h |  9 +++++
 2 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/tools/firmware/hvmloader/smbios.c b/tools/firmware/hvmloader/smbios.c
index 76c7090d16..e445475d78 100644
--- a/tools/firmware/hvmloader/smbios.c
+++ b/tools/firmware/hvmloader/smbios.c
@@ -396,7 +396,7 @@ smbios_entry_point_init(void *start,
     memcpy(ep->anchor_string, "_SM_", 4);
     ep->length = 0x1f;
     ep->smbios_major_version = 2;
-    ep->smbios_minor_version = 4;
+    ep->smbios_minor_version = 6;
     ep->max_structure_size = max_structure_size;
     ep->entry_point_revision = 0;
     memcpy(ep->intermediate_anchor_string, "_DMI_", 5);
@@ -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.
+     */
+    /* First component */
+    for ( unsigned int i = 0; i < 4; i++ )
+        p->uuid[i] = uuid[4 - i - 1];
+    /* Second component */
+    p->uuid[4] = uuid[5];
+    p->uuid[5] = uuid[4];
+    /* Third component */
+    p->uuid[6] = uuid[7];
+    p->uuid[7] = uuid[6];
+    /* Rest */
+    memcpy(p->uuid + 8, uuid + 8, 8);
 
     p->wake_up_type = 0x06; /* power switch */
     p->sku_str = 0;
@@ -705,8 +720,8 @@ smbios_type_4_init(
     struct smbios_type_4 *p = start;
     uint32_t eax, ebx, ecx, edx;
 
-    /* Specification says Type 4 table has length of 23h for v2.3+. */
-    BUILD_BUG_ON(sizeof(*p) != 35);
+    /* Specification says Type 4 table has length of 2Ah for v2.6. */
+    BUILD_BUG_ON(sizeof(*p) != 42);
 
     memset(p, 0, sizeof(*p));
 
@@ -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 */
     p->manufacturer_str = 2;
 
     cpuid(1, &eax, &ebx, &ecx, &edx);
@@ -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;
+
+    /*
+     * 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;
+
     start += sizeof(*p);
 
     strncpy(buf, "CPU ", sizeof(buf));
@@ -780,8 +811,8 @@ smbios_type_8_init(void *start)
 static void *
 smbios_type_9_init(void *start)
 {
-    /* Specification says Type 9 table has length of 0Dh for v2.1-2.5. */
-    BUILD_BUG_ON(sizeof(struct smbios_type_9) != 13);
+    /* Specification says Type 9 table has length of 11h for v2.6+. */
+    BUILD_BUG_ON(sizeof(struct smbios_type_9) != 17);
 
     /* Only present when passed in. */
     return smbios_pt_copy(start, 9, SMBIOS_HANDLE_TYPE9,
@@ -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));
 
@@ -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;
 
     start += sizeof(*p);
     strcpy(start, "DIMM ");
diff --git a/tools/firmware/hvmloader/smbios_types.h b/tools/firmware/hvmloader/smbios_types.h
index c04b435d31..860617d86a 100644
--- 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;
 } __attribute__ ((packed));
 
 /* SMBIOS type 11 - OEM Strings */
@@ -227,6 +235,7 @@ struct smbios_type_17 {
     uint8_t serial_number_str;
     uint8_t asset_tag_str;
     uint8_t part_number_str;
+    uint8_t attributes;
 } __attribute__ ((packed));
 
 /* SMBIOS type 19 - Memory Array Mapped Address */
-- 
2.50.1



Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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.

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.

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

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

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

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

Jan

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

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

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

On 03.09.2025 16:02, Teddy Astie wrote:
> Le 02/09/2025 à 16:10, Jan Beulich a écrit :
>> On 02.09.2025 15:24, Teddy Astie wrote:
>>> 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.

I wouldn't mind that, yet I wonder why the spec says "group". If there's a
(good) reason, carrying this through into our naming may be advisable.

Jan