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

[Michael S. Tsirkin]

On Sun, Aug 11, 2024 at 10:45:38AM +0000, Hal Martin wrote:
> This patch adds support for SMBIOS type 7 (Cache Information) to qemu.
> 
> level: cache level (1-8)
> size: cache size in bytes
> 
> Example usage:
> -smbios type=7,level=1,size=0x8000
> 
> Signed-off-by: Hal Martin <hal.martin@gmail.com>

A bunch of style issues here:

> ---
>  hw/smbios/smbios.c           | 63 ++++++++++++++++++++++++++++++++++++
>  include/hw/firmware/smbios.h | 18 +++++++++++
>  qemu-options.hx              |  2 ++
>  3 files changed, 83 insertions(+)
> 
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index a394514264..65942f2354 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -83,6 +83,12 @@ static struct {
>      .processor_family = 0x01, /* Other */
>  };
>  
> +struct type7_instance {
> +    uint16_t level, 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 +336,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_NUMBER,
> +        .help = "cache size",
> +    },
> +    { /* end of list */ }
> +};
> +
>  static const QemuOptDesc qemu_smbios_type8_opts[] = {
>      {
>          .name = "type",
> @@ -733,6 +756,32 @@ static void smbios_build_type_4_table(MachineState *ms, 
> unsigned instance,
>      smbios_type4_count++;
>  }
>  
> +static void smbios_build_type_7_table(void)
> +{
> +    unsigned instance = 0;
> +    struct type7_instance *t7;
> +    char designation[20];
> +
> +    QTAILQ_FOREACH(t7, &type7, next) {
> +        SMBIOS_BUILD_TABLE_PRE(7, T0_BASE + instance, true);
> +        sprintf(designation, "CPU Internal L%d", t7->level);
> +        SMBIOS_TABLE_SET_STR(7, socket_designation, designation);
> +        t->cache_configuration =  0x180 | (t7->level-1); /* not socketed, 
> enabled, write back*/

bad comment style, line too long, bad math style

> +        t->installed_size =  t7->size;
> +        t->maximum_cache_size =  t7->size; /* set max to installed */
> +        t->supported_sram_type = 0x10; /* pipeline burst */
> +        t->current_sram_type = 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 */
> +        t->maximum_cache_size2 = t7->size;
> +        t->installed_cache_size2 = t7->size;
> +        SMBIOS_BUILD_TABLE_POST;
> +        instance++;
> +    }
> +}
> +
>  static void smbios_build_type_8_table(void)
>  {
>      unsigned instance = 0;
> @@ -1120,6 +1169,7 @@ static bool smbios_get_tables_ep(MachineState *ms,
>          }
>      }
>  
> +    smbios_build_type_7_table();
>      smbios_build_type_8_table();
>      smbios_build_type_9_table(errp);
>      smbios_build_type_11_table();
> @@ -1478,6 +1528,19 @@ 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", 0x0);

bad comma style

> +            t7_i->size = qemu_opt_get_number(opts, "size", 0x0200);
> +            /* Only cache levels 1-8 are permitted */
> +            if (t7_i->level > 0 && t7_i->level < 9) {
> +                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..1ea1506b46 100644
> --- a/include/hw/firmware/smbios.h
> +++ b/include/hw/firmware/smbios.h
> @@ -220,6 +220,24 @@ 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;
> +    uint32_t maximum_cache_size2;
> +    uint32_t installed_cache_size2;
> +    /* contained elements follow */
> +} QEMU_PACKED;
> +
>  /* 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 cee0da2014..3b49813fcc 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2706,6 +2706,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"
> -- 
> 2.42.0