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 Fri, Aug 29, 2025 at 09:58:16AM +0000, Teddy Astie wrote:
> 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.

Just curious, why change hvmloader to generate an SMBIOS v2.6 table
when OVMF later say it's v2.8 ? Why do we change hvmloader when the
problem is OVMF making change to the table before giving it to the OS?

As far as I can tell, OVMF doesn't take into account the version of the
SMBIOS table provided by hvmloader and just use the default set at build
time. This can be changed with the PCD
gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion, it's currently set to
v2.8. The main used of the version number by OVMF seems to be to check
the max sizes.

Before making any changes in hvmloader, we should first fix OvmfXen to
take into account the version of the SMBIOS table that is received.
Otherwise, we just add more tech dept. We might one day want to use a
newer version of SMBIOS, OVMF should be ready by then.


Now, for the change of SMBIOS version. I think that starting to provide
a v2.6 is a good move, but we should list the correct reason for doing
so. OVMF can mostly stay out of the picture here, and be fix separately.
One main issue we can state is that Linux and Windows interpret the UUID
in the SMBIOS differently, when the version is v2.4. I've booted Windows
with SeaBIOS and read the UUID from the SMBIOS table with

    wmic csproduct get uuid

and the UUID return is read according to the new definition propose in
SMBIOS v2.6, even if the table is said to be v2.4, so the UUID is
different from the one set by the toolstack. Moving to v2.6 would indeed
fix this discrepancy.


Next, we are going to want a way to fallback to v2.4 when a guest needs
to observe no changes across reboot. `xl` already have
`smbios_firmware=` which is passthrough `hvmloader` via xenstore entry
`hvmloader/smbios/{address,length}`, but that interface would allow to
only replace a structure (like replacing the type 1 structure) and
doesn't allow to change the version. (And that would duplicate SMBIOS
table generation between hvmloader and the toolstack, which isn't ideal.)
So, will need some changes to `xl`, `libxl`, and `hvmloader`.


> Fixes: c683914ef913 ("Add code to generate SMBIOS tables to hvmloader.")
> (SMBIOS versions before 2.6 has a ill-defined UUID definition)

This space in a commit description is usually reserved for tags and
sometime comment of change made to a patch while
committing/cherry-picking. Comment about a previous commit can be before
these tags, like stating that "commit a1b2c3 made some unhelpful
changes and still have the Fixes:a1b2c3 (...) tag.

> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>

Thanks,

-- 

--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Hello,

Le 17/12/2025 à 16:26, Anthony PERARD a écrit :
> On Fri, Aug 29, 2025 at 09:58:16AM +0000, Teddy Astie wrote:
>> 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.
> 
> Just curious, why change hvmloader to generate an SMBIOS v2.6 table
> when OVMF later say it's v2.8 ? Why do we change hvmloader when the
> problem is OVMF making change to the table before giving it to the OS?
> 
> As far as I can tell, OVMF doesn't take into account the version of the
> SMBIOS table provided by hvmloader and just use the default set at build
> time. This can be changed with the PCD
> gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion, it's currently set to
> v2.8. The main used of the version number by OVMF seems to be to check
> the max sizes.
> 
> Before making any changes in hvmloader, we should first fix OvmfXen to
> take into account the version of the SMBIOS table that is received.
> Otherwise, we just add more tech dept. We might one day want to use a
> newer version of SMBIOS, OVMF should be ready by then.
> 

That would be better, but with a small issue. SMBIOS 2.4 doesn't define 
EFI as a way to query SMBIOS location. That probably doesn't really 
matter, as the table content of 2.4 still has a meaning in the next 
SMBIOS specifications; but is still worth noting.

> 
> Now, for the change of SMBIOS version. I think that starting to provide
> a v2.6 is a good move, but we should list the correct reason for doing
> so. OVMF can mostly stay out of the picture here, and be fix separately.
> One main issue we can state is that Linux and Windows interpret the UUID
> in the SMBIOS differently, when the version is v2.4. I've booted Windows
> with SeaBIOS and read the UUID from the SMBIOS table with
> 
>      wmic csproduct get uuid
> 
> and the UUID return is read according to the new definition propose in
> SMBIOS v2.6, even if the table is said to be v2.4, so the UUID is
> different from the one set by the toolstack. Moving to v2.6 would indeed
> fix this discrepancy.
> 
On Windows, only doing the endianness change (expected for v2.6) would 
fix what Windows reads, as AFAIU it always considers GUID encoding.

> 
> Next, we are going to want a way to fallback to v2.4 when a guest needs
> to observe no changes across reboot. `xl` already have
> `smbios_firmware=` which is passthrough `hvmloader` via xenstore entry
> `hvmloader/smbios/{address,length}`, but that interface would allow to
> only replace a structure (like replacing the type 1 structure) and
> doesn't allow to change the version. (And that would duplicate SMBIOS
> table generation between hvmloader and the toolstack, which isn't ideal.)
> So, will need some changes to `xl`, `libxl`, and `hvmloader`.
> 

I had in mind the idea of moving the entire SMBIOS table creation to be 
fully done in the toolstack (to give it full control on this); so 
hvmloader will no longer have to generate it. The caveats is that it 
implies moving various things around.

> 
>> Fixes: c683914ef913 ("Add code to generate SMBIOS tables to hvmloader.")
>> (SMBIOS versions before 2.6 has a ill-defined UUID definition)
> 
> This space in a commit description is usually reserved for tags and
> sometime comment of change made to a patch while
> committing/cherry-picking. Comment about a previous commit can be before
> these tags, like stating that "commit a1b2c3 made some unhelpful
> changes and still have the Fixes:a1b2c3 (...) tag.
> 
>> Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
> 
> Thanks,
> 

Teddy


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