Re: [PATCH 3/3] hvmloader: add new SMBIOS tables (7,8,9,26,27,28)

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.07.2025 01:45, Petr Beneš wrote:
> From: Petr Beneš <w1benny@xxxxxxxxx>

This isn't in line with the first S-o-b, nor with the fact that in the cover
letter you say this was previously submitted (and hence authored?) by Anton.

> SMBIOS tables like 7,8,9,26,27,28 are neccessary to prevent sandbox detection
> by malware using WMI-queries. New tables can be mapped to memory from binary
> file specified in "smbios_firmware" parameter of domain configuration.

Who or what says these are "necessary"? Also, to have at least a basic
understanding without consulting the spec (and without reading the patch in
its entirety), mentioning what the numbers actually stand for would be helpful,
I think. This way the connection to "necessary" or maybe merely "desirable"
would likely also be easier to make.

> @@ -700,6 +724,66 @@ smbios_type_4_init(
>      return start+1;
>  }
>  
> +/* Type 7 -- Cache Information */
> +static void *
> +smbios_type_7_init(void *start)
> +{
> +    struct smbios_type_7 *p = start;
> +

Nit: What use is this blank line?

> +    void *pts;
> +    uint32_t length;
> +
> +    pts = get_smbios_pt_struct(7, &length);
> +    if ( pts != NULL && length >= sizeof(struct smbios_type_7) )
> +    {
> +        memcpy(start, pts, length);
> +        p->header.handle = SMBIOS_HANDLE_TYPE7;
> +        return start + length;
> +    }
> +
> +    return start;
> +}
> +
> +/* Type 8 -- Port Connector Information */
> +static void *
> +smbios_type_8_init(void *start)
> +{
> +    struct smbios_type_8 *p = start;
> +
> +    void *pts;
> +    uint32_t length;
> +
> +    pts = get_smbios_pt_struct(8, &length);
> +    if ( pts != NULL && length >= sizeof(struct smbios_type_8) )
> +    {
> +        memcpy(start, pts, length);
> +        p->header.handle = SMBIOS_HANDLE_TYPE8;
> +        return start + length;
> +    }
> +
> +    return start;
> +}
> +
> +/* Type 9 -- System Slots */
> +static void *
> +smbios_type_9_init(void *start)
> +{
> +    struct smbios_type_9 *p = start;
> +
> +    void *pts;
> +    uint32_t length;
> +
> +    pts = get_smbios_pt_struct(9, &length);
> +    if ( pts != NULL && length >= sizeof(struct smbios_type_9) )
> +    {
> +        memcpy(start, pts, length);
> +        p->header.handle = SMBIOS_HANDLE_TYPE9;
> +        return start + length;
> +    }
> +
> +    return start;
> +}

These all look largely identical with one another, and they also look to
match smbios_type_39_init(). Surely we could do with less redundancy by
having some common helper dealing with "Only present when passed in" cases,
as smbios_type_39_init() states it in a comment.

> @@ -780,7 +864,7 @@ smbios_type_17_init(void *start, uint32_t memory_size_mb, 
> int instance)
>  {
>      char buf[16];
>      struct smbios_type_17 *p = start;
> -    
> +
>      memset(p, 0, sizeof(*p));
>  
>      p->header.type = 17;
> @@ -815,7 +899,7 @@ static void *
>  smbios_type_19_init(void *start, uint32_t memory_size_mb, int instance)
>  {
>      struct smbios_type_19 *p = start;
> -    
> +
>      memset(p, 0, sizeof(*p));
>  
>      p->header.type = 19;

These two hunks look like they belong in patch 1. They're entirely unrelated
here (i.e. not even adjacent to code being touched).

> @@ -232,6 +270,45 @@ struct smbios_type_22 {
>      uint32_t oem_specific;
>  } __attribute__ ((packed));
>  
> +/* SMBIOS type 26 - Voltage Probe */
> +struct smbios_type_26 {
> +    struct smbios_structure_header header;
> +    uint8_t description_str;
> +    uint8_t location_and_status;
> +    uint16_t maximum_value;
> +    uint16_t minimum_value;
> +    uint16_t resolution;
> +    uint16_t tolerance;
> +    uint16_t accuracy;
> +    uint32_t oem_defined;
> +    uint16_t nominal_value;     // Optional

Nit: We don't use C++ style comments in Xen.

> @@ -252,9 +329,9 @@ struct smbios_type_39 {
>      uint8_t revision_level_str;
>      uint16_t max_capacity;
>      uint16_t characteristics;
> -    uint16_t input_voltage_probe_handle;
> -    uint16_t cooling_device_handle;
> -    uint16_t input_current_probe_handle;
> +    uint16_t input_voltage_probe_handle;    // Optional
> +    uint16_t cooling_device_handle;         // Optional
> +    uint16_t input_current_probe_handle;    // Optional
>  } __attribute__ ((packed));

This again looks like an unrelated change. I don't mind it living here, but
such wants mentioning in the description. Then again this may better be done
in patch 2, to accompany the size checks (where it matters from what offset
onwards fields are optional).

Jan