qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [ARM SMBIOS V4 PATCH 1/2] smbios: add smbios 3.0 suppor


From: Leif Lindholm
Subject: Re: [Qemu-devel] [ARM SMBIOS V4 PATCH 1/2] smbios: add smbios 3.0 support
Date: Wed, 26 Aug 2015 18:04:09 +0100
User-agent: Mutt/1.5.21 (2010-09-15)

Hi Wei,

On Wed, Aug 26, 2015 at 11:41:26AM -0500, Wei Huang wrote:
> While fixing up the patches, I started to lean towards keeping -21 in
> smbios-entry-point naming. Here are my points:
> 
> 1) It is very easy to get confused the "union SmbiosEntryPoint" with
> "struct smbios_entry_point" if we remove _21 from the original "struct
> smbios_21_entry_point". A symmetric naming of "struct
> smbios_21_entry_point" & "struct smbios_30_entry_point" actually is
> easier to read;
> 2) SMBIOS 3.0 specification clearly separate 2.1 and 3.0 definitions. I
> understand that people who started from old spec might be a bit confused
> to read the code; But for those who pick up SMBIOS 3.0 (and beyond)
> fresh, they will know the difference while reading the code.
> 3) The issues Peter found is x86 specific and we know that QEMU only
> generates SMBIOS 2.1 tables for PC model. So we can fix the test code to
> only use _21 struct. In the future, when we started to create test code
> for ARM, it can be further merged.
> 
> Anyway, I attached the patch for your review. It addresses some of your
> concerns regarding SMBIOS 2.1 usage cases by detailed comments (see
> smbios.h).

In general, I care a lot more about getting this functionality than
about the finer details of the inner workings of the code. The
comments are an improvement, and using -21 aligns the code closer with
current spec (but further from Linux).

Thanks,

Leif

> >> I can take this suggestion, with clear comment in header file so nobody
> >> will get confused. Peter, please let me know if you object.
> > 
> > I don't object (though the opinion of the qemu smbios/acpi
> > folk is probably more important than mine).
> > 
> > Please make sure you test the x86 platform has not been broken by
> > this change (preferably more thoroughly than just running
> > 'make check'...).
> 
> (I have tested the code with the following methods:
> 
> * dmidecode insider an ARM guest VM
> * "make check" on both ARM and x86 hosts
> * dmidecode insider a x86-64 Windows 7 VM
> )
> 
> 
> > 
> > thanks
> > -- PMM
> > 
> 
> commit 4abc639a34a43acccd81725f2176760058bb9849
> Author: Wei Huang <address@hidden>
> Date:   Tue Aug 25 16:34:47 2015 -0400
> 
>     smbios: add smbios 3.0 support
>     
>     This patch adds support for SMBIOS 3.0 entry point. When caller invokes
>     smbios_set_defaults(), it can specify entry point as 2.1 or 3.0. Then
>     smbios_get_tables() will return the entry point table in right format.
>     
>     Acked-by: Gabriel Somlo <address@hidden>
>     Tested-by: Gabriel Somlo <address@hidden>
>     Tested-by: Leif Lindholm <address@hidden>
>     Signed-off-by: Wei Huang <address@hidden>
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 9558467..b82921d 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -173,7 +173,8 @@ static void pc_init1(MachineState *machine)
>          MachineClass *mc = MACHINE_GET_CLASS(machine);
>          /* These values are guest ABI, do not change */
>          smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
> -                            mc->name, smbios_legacy_mode, 
> smbios_uuid_encoded);
> +                            mc->name, smbios_legacy_mode, 
> smbios_uuid_encoded,
> +                            SMBIOS_ENTRY_POINT_21);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index c07d65b..7217cbf 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -165,7 +165,8 @@ static void pc_q35_init(MachineState *machine)
>      if (smbios_defaults) {
>          /* These values are guest ABI, do not change */
>          smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
> -                            mc->name, smbios_legacy_mode, 
> smbios_uuid_encoded);
> +                            mc->name, smbios_legacy_mode, 
> smbios_uuid_encoded,
> +                            SMBIOS_ENTRY_POINT_21);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index efdbb5d..b81a1d3 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -55,7 +55,9 @@ static uint8_t *smbios_tables;
>  static size_t smbios_tables_len;
>  static unsigned smbios_table_max;
>  static unsigned smbios_table_cnt;
> -static struct smbios_entry_point ep;
> +static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_21;
> +
> +static SmbiosEntryPoint ep;
>  
>  static int smbios_type4_count = 0;
>  static bool smbios_immutable;
> @@ -771,11 +773,12 @@ void smbios_set_cpuid(uint32_t version, uint32_t 
> features)
>  
>  void smbios_set_defaults(const char *manufacturer, const char *product,
>                           const char *version, bool legacy_mode,
> -                         bool uuid_encoded)
> +                         bool uuid_encoded, SmbiosEntryPointType ep_type)
>  {
>      smbios_have_defaults = true;
>      smbios_legacy = legacy_mode;
>      smbios_uuid_encoded = uuid_encoded;
> +    smbios_ep_type = ep_type;
>  
>      /* drop unwanted version of command-line file blob(s) */
>      if (smbios_legacy) {
> @@ -808,26 +811,53 @@ void smbios_set_defaults(const char *manufacturer, 
> const char *product,
>  
>  static void smbios_entry_point_setup(void)
>  {
> -    memcpy(ep.anchor_string, "_SM_", 4);
> -    memcpy(ep.intermediate_anchor_string, "_DMI_", 5);
> -    ep.length = sizeof(struct smbios_entry_point);
> -    ep.entry_point_revision = 0; /* formatted_area reserved, per spec v2.1+ 
> */
> -    memset(ep.formatted_area, 0, 5);
> -
> -    /* compliant with smbios spec v2.8 */
> -    ep.smbios_major_version = 2;
> -    ep.smbios_minor_version = 8;
> -    ep.smbios_bcd_revision = 0x28;
> -
> -    /* set during table construction, but BIOS may override: */
> -    ep.structure_table_length = cpu_to_le16(smbios_tables_len);
> -    ep.max_structure_size = cpu_to_le16(smbios_table_max);
> -    ep.number_of_structures = cpu_to_le16(smbios_table_cnt);
> -
> -    /* BIOS must recalculate: */
> -    ep.checksum = 0;
> -    ep.intermediate_checksum = 0;
> -    ep.structure_table_address = cpu_to_le32(0);
> +    switch (smbios_ep_type) {
> +    case SMBIOS_ENTRY_POINT_21:
> +        memcpy(ep.ep21.anchor_string, "_SM_", 4);
> +        memcpy(ep.ep21.intermediate_anchor_string, "_DMI_", 5);
> +        ep.ep21.length = sizeof(struct smbios_21_entry_point);
> +        ep.ep21.entry_point_revision = 0; /* formatted_area reserved */
> +        memset(ep.ep21.formatted_area, 0, 5);
> +
> +        /* compliant with smbios spec v2.8 */
> +        ep.ep21.smbios_major_version = 2;
> +        ep.ep21.smbios_minor_version = 8;
> +        ep.ep21.smbios_bcd_revision = 0x28;
> +
> +        /* set during table construction, but BIOS may override: */
> +        ep.ep21.structure_table_length = cpu_to_le16(smbios_tables_len);
> +        ep.ep21.max_structure_size = cpu_to_le16(smbios_table_max);
> +        ep.ep21.number_of_structures = cpu_to_le16(smbios_table_cnt);
> +
> +        /* BIOS must recalculate */
> +        ep.ep21.checksum = 0;
> +        ep.ep21.intermediate_checksum = 0;
> +        ep.ep21.structure_table_address = cpu_to_le32(0);
> +
> +        break;
> +    case SMBIOS_ENTRY_POINT_30:
> +        memcpy(ep.ep30.anchor_string, "_SM3_", 5);
> +        ep.ep30.length = sizeof(struct smbios_30_entry_point);
> +        ep.ep30.entry_point_revision = 1;
> +        ep.ep30.reserved = 0;
> +
> +        /* compliant with smbios spec 3.0 */
> +        ep.ep30.smbios_major_version = 3;
> +        ep.ep30.smbios_minor_version = 0;
> +        ep.ep30.smbios_doc_rev = 0;
> +
> +        /* set during table construct, but BIOS might override */
> +        ep.ep30.structure_table_max_size = cpu_to_le32(smbios_tables_len);
> +
> +        /* BIOS must recalculate */
> +        ep.ep30.checksum = 0;
> +        ep.ep30.structure_table_address = cpu_to_le64(0);
> +
> +        break;
> +    default:
> +        abort();
> +        break;
> +    }
>  }
>  
>  void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
> @@ -885,7 +915,15 @@ void smbios_get_tables(const struct smbios_phys_mem_area 
> *mem_array,
>      *tables = smbios_tables;
>      *tables_len = smbios_tables_len;
>      *anchor = (uint8_t *)&ep;
> -    *anchor_len = sizeof(struct smbios_entry_point);
> +
> +    /* calculate length based on anchor string */
> +    if (!strncmp((char *)&ep, "_SM_", 4)) {
> +        *anchor_len = sizeof(struct smbios_21_entry_point);
> +    } else if (!strncmp((char *)&ep, "_SM3_", 5)) {
> +        *anchor_len = sizeof(struct smbios_30_entry_point);
> +    } else {
> +        abort();
> +    }
>  }
>  
>  static void save_opt(const char **dest, QemuOpts *opts, const char *name)
> diff --git a/include/hw/smbios/smbios.h b/include/hw/smbios/smbios.h
> index 4269aab..76ccf70 100644
> --- a/include/hw/smbios/smbios.h
> +++ b/include/hw/smbios/smbios.h
> @@ -23,25 +23,27 @@ struct smbios_phys_mem_area {
>      uint64_t length;
>  };
>  
> -void smbios_entry_add(QemuOpts *opts);
> -void smbios_set_cpuid(uint32_t version, uint32_t features);
> -void smbios_set_defaults(const char *manufacturer, const char *product,
> -                         const char *version, bool legacy_mode,
> -                         bool uuid_encoded);
> -uint8_t *smbios_get_table_legacy(size_t *length);
> -void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
> -                       const unsigned int mem_array_size,
> -                       uint8_t **tables, size_t *tables_len,
> -                       uint8_t **anchor, size_t *anchor_len);
> -
>  /*
>   * SMBIOS spec defined tables
>   */
> +typedef enum SmbiosEntryPointType {
> +    SMBIOS_ENTRY_POINT_21,
> +    SMBIOS_ENTRY_POINT_30,
> +} SmbiosEntryPointType;
> +
> +/* SMBIOS Entry Point
> + * There are two types of entry points defined in the SMBIOS specification
> + * (see below). BIOS must place the entry point(s) at a 16-bit-aligned
> + * address between 0xf0000 and 0xfffff. Note that either entry point type
> + * can be used in a 64-bit target system, except that SMBIOS 2.1 entry point
> + * only allows the SMBIOS struct table to reside below 4GB address space.
> + */
>  
> -/* SMBIOS entry point (anchor).
> - * BIOS must place this at a 16-bit-aligned address between 0xf0000 and 
> 0xfffff.
> +/* SMBIOS 2.1 (32-bit) Entry Point
> + *  - introduced since SMBIOS 2.1
> + *  - supports structure table below 4GB only
>   */
> -struct smbios_entry_point {
> +struct smbios_21_entry_point {
>      uint8_t anchor_string[4];
>      uint8_t checksum;
>      uint8_t length;
> @@ -58,6 +60,28 @@ struct smbios_entry_point {
>      uint8_t smbios_bcd_revision;
>  } QEMU_PACKED;
>  
> +/* SMBIOS 3.0 (64-bit) Entry Point
> + *  - introduced since SMBIOS 3.0
> + *  - supports structure table at 64-bit address space
> + */
> +struct smbios_30_entry_point {
> +    uint8_t anchor_string[5];
> +    uint8_t checksum;
> +    uint8_t length;
> +    uint8_t smbios_major_version;
> +    uint8_t smbios_minor_version;
> +    uint8_t smbios_doc_rev;
> +    uint8_t entry_point_revision;
> +    uint8_t reserved;
> +    uint32_t structure_table_max_size;
> +    uint64_t structure_table_address;
> +} QEMU_PACKED;
> +
> +typedef union {
> +    struct smbios_21_entry_point ep21;
> +    struct smbios_30_entry_point ep30;
> +} QEMU_PACKED SmbiosEntryPoint;
> +
>  /* This goes at the beginning of every SMBIOS structure. */
>  struct smbios_structure_header {
>      uint8_t type;
> @@ -232,4 +256,14 @@ struct smbios_type_127 {
>      struct smbios_structure_header header;
>  } QEMU_PACKED;
>  
> +void smbios_entry_add(QemuOpts *opts);
> +void smbios_set_cpuid(uint32_t version, uint32_t features);
> +void smbios_set_defaults(const char *manufacturer, const char *product,
> +                         const char *version, bool legacy_mode,
> +                         bool uuid_encoded, SmbiosEntryPointType ep_type);
> +uint8_t *smbios_get_table_legacy(size_t *length);
> +void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
> +                       const unsigned int mem_array_size,
> +                       uint8_t **tables, size_t *tables_len,
> +                       uint8_t **anchor, size_t *anchor_len);
>  #endif /*QEMU_SMBIOS_H */
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 613867a..9686328 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -50,7 +50,7 @@ typedef struct {
>      int rsdt_tables_nr;
>      GArray *tables;
>      uint32_t smbios_ep_addr;
> -    struct smbios_entry_point smbios_ep_table;
> +    struct smbios_21_entry_point smbios_ep_table;
>  } test_data;
>  
>  #define LOW(x) ((x) & 0xff)
> @@ -601,7 +601,7 @@ static void test_acpi_asl(test_data *data)
>  
>  static bool smbios_ep_table_ok(test_data *data)
>  {
> -    struct smbios_entry_point *ep_table = &data->smbios_ep_table;
> +    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
>      uint32_t addr = data->smbios_ep_addr;
>  
>      ACPI_READ_ARRAY(ep_table->anchor_string, addr);
> @@ -681,7 +681,7 @@ static inline bool smbios_single_instance(uint8_t type)
>  static void test_smbios_structs(test_data *data)
>  {
>      DECLARE_BITMAP(struct_bitmap, SMBIOS_MAX_TYPE+1) = { 0 };
> -    struct smbios_entry_point *ep_table = &data->smbios_ep_table;
> +    struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
>      uint32_t addr = ep_table->structure_table_address;
>      int i, len, max_len = 0;
>      uint8_t type, prv, crt;




reply via email to

[Prev in Thread] Current Thread [Next in Thread]