Re: [PATCH v4] hw/smbios: support for type 7 (cache information)

[Igor Mammedov]

On Sun, 22 Dec 2024 10:40:05 +0000
Hal Martin <hal.martin@gmail.com> wrote:

> This patch adds support for SMBIOS type 7 (Cache Information) to qemu.
> 
> level: cache level (1-8)
> size: cache size
> 
> Example usage:
> -smbios type=7,level=1,size=1M
> 
> Note: this does not change the actual layout of the CPU cache(s),
> it simply allows the user to specify data in SMBIOS to be presented
> to the guest. This is useful for example if you have software that
> queries SMBIOS for the CPU cache levels/size.

when posting a new version, do not put it into the same thread lest it would be 
lost,
send  it as a new patch with vX prefix.

Also put after --- line
changelog and a reference to previous revision

PS:
use ./scripts/checkpatch.pl before sending the patch
 
> 
> Signed-off-by: Hal Martin <hal.martin@gmail.com>
> ---
>  hw/smbios/smbios.c           | 116 +++++++++++++++++++++++++++++++++++
>  include/hw/firmware/smbios.h |  29 +++++++++
>  qemu-options.hx              |   2 +
>  3 files changed, 147 insertions(+)
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 02a09eb9cd..9885e902b0 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -18,6 +18,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/units.h"
>  #include "qapi/error.h"
> +#include "qemu/error-report.h"
>  #include "qemu/config-file.h"
>  #include "qemu/module.h"
>  #include "qemu/option.h"
> @@ -83,6 +84,13 @@ static struct {
>      .processor_family = 0x01, /* Other */
>  };
>  
> +struct type7_instance {
> +    uint16_t level;
> +    uint64_t size;
> +    QTAILQ_ENTRY(type7_instance) next;
> +};
> +static QTAILQ_HEAD(, type7_instance) type7 = QTAILQ_HEAD_INITIALIZER(type7);
> +
>  struct type8_instance {
>      const char *internal_reference, *external_reference;
>      uint8_t connector_type, port_type;
> @@ -330,6 +338,23 @@ static const QemuOptDesc qemu_smbios_type4_opts[] = {
>      { /* end of list */ }
>  };
>  
> +static const QemuOptDesc qemu_smbios_type7_opts[] = {
> +    {
> +        .name = "type",
> +        .type = QEMU_OPT_NUMBER,
> +        .help = "SMBIOS element type",
> +    },{
> +        .name = "level",
> +        .type = QEMU_OPT_NUMBER,
> +        .help = "cache level",
> +    },{
> +        .name = "size",
> +        .type = QEMU_OPT_SIZE,
> +        .help = "cache size",
> +    },
> +    { /* end of list */ }
> +};
> +
>  static const QemuOptDesc qemu_smbios_type8_opts[] = {
>      {
>          .name = "type",
> @@ -733,6 +758,80 @@ static void smbios_build_type_4_table(MachineState *ms, 
> unsigned instance,
>      smbios_type4_count++;
>  }
>  
> +static void smbios_build_type_7_table(SmbiosEntryPointType ep_type)
> +{
> +    unsigned instance = 0;
> +    size_t tbl_len = SMBIOS_TYPE_7_LEN_V21;
> +    struct type7_instance *t7;
> +    char designation[50];
> +    uint16_t cache_size = 0;
> +    uint32_t cache_size2 = 0;
> +
> +    if (ep_type == SMBIOS_ENTRY_POINT_TYPE_64) {
> +        tbl_len = SMBIOS_TYPE_7_LEN_V31;
> +    }
> +
> +    QTAILQ_FOREACH(t7, &type7, next) {
> +        if (t7->size < 1024) {

> +            error_report("SMBIOS CPU cache size (%lu) is too small (>1k)",
> +                         t7->size);
> +            exit(1);

using error_fatal() would be better here, and avoid having to call exit manually

> +        }
> +        SMBIOS_BUILD_TABLE_PRE(7, T0_BASE + instance, true);
> +        sprintf(designation, "CPU Internal L%d", t7->level);

try to avoid sprintf(), use a safer alternatives from glib instead
()see: g_print() && co and also g_autofree

> +        SMBIOS_TABLE_SET_STR(7, socket_designation, designation);
> +        /* cache not socketed, enabled, write back */
> +        t->cache_configuration =  cpu_to_le16(0x180 | ((t7->level) - 1));
> +        if (tbl_len == SMBIOS_TYPE_7_LEN_V21) {
> +            if (t7->size > 1024*MiB) {
> +                error_report("SMBIOS 2.0 doesn't support CPU cache "
> +                            "sizes more than 1024 MiB, use "
> +                            "-machine smbios-entry-point-type=64 option to "
> +                            "enable SMBIOS 3.0 support");
> +                exit(1);

ditto

> +            }
> +        }
> +        /* size is defined in 1k granularity */
> +        cache_size = t7->size/1024;
here and below, that will silently round up input of not right granularity,
I'd rather have explicit granularity check and bail out in case value is
not matching expected one.
Maybe put it in the beginning  along with 'SMBIOS CPU cache size' check above.

Also here and elsewhere try to use KiB ... instead on plain numbers

> +        if (t7->size > INT16_MAX) {
> +            /* set granularity to 64KiB */
> +            cache_size = cpu_to_le16(t7->size/(64*1024) | (0x1 << 15));
> +        }
> +
> +        t->supported_sram_type = cpu_to_le16(0x10); /* pipeline burst */
> +        t->current_sram_type = cpu_to_le16(0x10); /* pipeline burst */
> +        t->cache_speed = 0x1; /* 1 ns */
> +        t->error_correction_type = 0x6; /* Multi-bit ECC */
> +        t->system_cache_type = 0x05; /* Unified */
> +        t->associativity = 0x6; /* Fully Associative */
> +
> +        if (tbl_len == SMBIOS_TYPE_7_LEN_V31) {
> +            if (t7->size > ((uint64_t)2 << 45)) {

instead of shift use numbers with KiB/MiB/... suffixes to make it more readable

> +                error_report("SMBIOS CPU cache size (%lu) is too large",
> +                             t7->size);

it would be nice to also tell user what expected range is

> +                exit(1);
> +            }
> +            cache_size2 = t7->size/1024;
> +            /* For Cache sizes greater than 2047 MB, the */
> +            /* Maximum Cache Size field is set to 0xFFFF */
> +            if (cache_size2 > (2 << 20)) {

ditto wrt KiB/...

> +                cache_size = 0xffff;
> +                /* set granularity to 64KiB */
> +                cache_size2 = cpu_to_le32(t7->size/(64*1024) | (0x1 << 31));

and here too

> +            }
> +
> +            t->maximum_cache_size2 = cache_size2;
> +            t->installed_cache_size2 = cache_size2;
> +
> +        }
> +        t->installed_size = cache_size;
> +        /* set max size to installed size */
> +        t->maximum_cache_size = cache_size;
> +        SMBIOS_BUILD_TABLE_POST;
> +        instance++;
> +    }
> +}
> +
>  static void smbios_build_type_8_table(void)
>  {
>      unsigned instance = 0;
> @@ -1120,6 +1219,7 @@ static bool smbios_get_tables_ep(MachineState *ms,
>          }
>      }
>  
> +    smbios_build_type_7_table(ep_type);
>      smbios_build_type_8_table();
>      smbios_build_type_9_table(errp);
>      smbios_build_type_11_table();
> @@ -1478,6 +1578,22 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
>                             UINT16_MAX);
>              }
>              return;
> +        case 7:
> +            if (!qemu_opts_validate(opts, qemu_smbios_type7_opts, errp)) {
> +                return;
> +            }
> +            struct type7_instance *t7_i;
> +            t7_i = g_new0(struct type7_instance, 1);
> +            t7_i->level = qemu_opt_get_number(opts, "level", 0);
> +            t7_i->size = qemu_opt_get_size(opts, "size", 0);
> +            /* Only cache levels 1-8 are permitted */
> +            if (t7_i->level > 8) {
> +                error_setg(errp, "SMBIOS CPU cache level %d is invalid 
> (1-8)",
                                                                  ^^^^^^^^^^^^^

it's confusing, how user would know that (1-8) is an acceptable range

> +                           t7_i->level);
> +                return;
> +            }
> +            QTAILQ_INSERT_TAIL(&type7, t7_i, next);
> +            return;
>          case 8:
>              if (!qemu_opts_validate(opts, qemu_smbios_type8_opts, errp)) {
>                  return;
> diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h
> index f066ab7262..996d36fead 100644
> --- a/include/hw/firmware/smbios.h
> +++ b/include/hw/firmware/smbios.h
> @@ -220,6 +220,35 @@ typedef enum smbios_type_4_len_ver {
>      SMBIOS_TYPE_4_LEN_V30 = offsetofend(struct smbios_type_4, thread_count2),
>  } smbios_type_4_len_ver;
>  
> +/* SMBIOS type 7 - Cache Information (v2.0+) */
> +struct smbios_type_7 {
> +    struct smbios_structure_header header;
> +    uint8_t socket_designation;
> +    uint16_t cache_configuration;
> +    uint16_t maximum_cache_size;
> +    uint16_t installed_size;
> +    uint16_t supported_sram_type;
> +    uint16_t current_sram_type;
> +    uint8_t cache_speed;
> +    uint8_t error_correction_type;
> +    uint8_t system_cache_type;
> +    uint8_t associativity;
> +    /* SMBIOS spec 3.1.0, Table 36  */
> +    uint32_t maximum_cache_size2;
> +    uint32_t installed_cache_size2;
> +    /* contained elements follow */
> +} QEMU_PACKED;
> +
> +typedef enum smbios_type_7_len_ver {
> +    SMBIOS_TYPE_7_LEN_V20 = offsetofend(struct smbios_type_7,
> +                                        current_sram_type),
> +    SMBIOS_TYPE_7_LEN_V21 = offsetofend(struct smbios_type_7,
> +                                        associativity),
> +    SMBIOS_TYPE_7_LEN_V31 = offsetofend(struct smbios_type_7,
> +                                        installed_cache_size2),
> +} smbios_type_7_len_ver;
> +
> +
>  /* SMBIOS type 8 - Port Connector Information */
>  struct smbios_type_8 {
>      struct smbios_structure_header header;
> diff --git a/qemu-options.hx b/qemu-options.hx
> index cc694d3b89..a85bfac647 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2644,6 +2644,8 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
>      "              
> [,asset=str][,part=str][,max-speed=%d][,current-speed=%d]\n"
>      "              [,processor-family=%d][,processor-id=%d]\n"
>      "                specify SMBIOS type 4 fields\n"
> +    "-smbios type=7[,level=%d][,size=%d]\n"
> +    "                specify SMBIOS type 7 fields\n"
>      "-smbios 
> type=8[,external_reference=str][,internal_reference=str][,connector_type=%d][,port_type=%d]\n"
>      "                specify SMBIOS type 8 fields\n"
>      "-smbios type=11[,value=str][,path=filename]\n"