Re: [Qemu-devel] [PATCH v9 08/24] hw/arm/virt-acpi-build: Generate FADT table and update ACPI headers

[Alex Bennée]

Shannon Zhao <address@hidden> writes:

> From: Shannon Zhao <address@hidden>
>
> In the case of mach virt, it is used to set the Hardware Reduced bit
> and enable PSCI SMP booting through HVC. So ignore FACS and FADT
> points to DSDT.
>
> Update the header definitions for FADT taking into account the new
> additions of ACPI v5.1 in `include/hw/acpi/acpi-defs.h`
>
> Signed-off-by: Shannon Zhao <address@hidden>
> Signed-off-by: Shannon Zhao <address@hidden>
> ---
>  hw/arm/virt-acpi-build.c    |  31 ++++++++++
>  include/hw/acpi/acpi-defs.h | 135 
> ++++++++++++++++++++++++++++++++------------
>  2 files changed, 129 insertions(+), 37 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 2cf2cc5..0791501 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -137,6 +137,31 @@ static void acpi_dsdt_add_virtio(Aml *scope,
>      }
>  }
>  
> +/* FADT */
> +static void
> +build_fadt(GArray *table_data, GArray *linker, unsigned dsdt)
> +{
> +    AcpiFadtDescriptorRev5_1 *fadt = acpi_data_push(table_data, 
> sizeof(*fadt));
> +
> +    /* Hardware Reduced = 1 and use PSCI 0.2+ and with HVC */
> +    fadt->flags = cpu_to_le32(1 << ACPI_FADT_F_HW_REDUCED_ACPI);
> +    fadt->arm_boot_flags = cpu_to_le16((1 << ACPI_FADT_ARM_USE_PSCI_G_0_2) |
> +                                       (1 <<
> ACPI_FADT_ARM_PSCI_USE_HVC));

So the ACPI_FADT_* symbols are actually bit positions. Perhaps naming
them *_SHIFT will make that clearer else where.

> +
> +    /* ACPI v5.1 (fadt->revision.fadt->minor_revision) */
> +    fadt->minor_revision = 0x1;
> +
> +    fadt->dsdt = cpu_to_le32(dsdt);
> +    /* DSDT address to be filled by Guest linker */
> +    bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> +                                   ACPI_BUILD_TABLE_FILE,
> +                                   table_data, &fadt->dsdt,
> +                                   sizeof fadt->dsdt);
> +
> +    build_header(linker, table_data,
> +                 (void *)fadt, "FACP", sizeof(*fadt), 5);
> +}
> +
>  /* DSDT */
>  static void
>  build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> @@ -183,6 +208,7 @@ static
>  void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
>  {
>      GArray *table_offsets;
> +    unsigned dsdt;
>      GArray *tables_blob = tables->table_data;
>  
>      table_offsets = g_array_new(false, true /* clear */,
> @@ -202,8 +228,13 @@ void virt_acpi_build(VirtGuestInfo *guest_info, 
> AcpiBuildTables *tables)
>       */
>  
>      /* DSDT is pointed to by FADT */
> +    dsdt = tables_blob->len;
>      build_dsdt(tables_blob, tables->linker, guest_info);
>  
> +    /* FADT MADT GTDT pointed to by RSDT */
> +    acpi_add_table(table_offsets, tables_blob);
> +    build_fadt(tables_blob, tables->linker, dsdt);
> +
>      /* Cleanup memory that's no longer used. */
>      g_array_free(table_offsets, true);
>  }
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index c4468f8..fadcf84 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -88,46 +88,54 @@ struct AcpiTableHeader         /* ACPI common table 
> header */
>  typedef struct AcpiTableHeader AcpiTableHeader;
>  
>  /*
> - * ACPI 1.0 Fixed ACPI Description Table (FADT)
> + * ACPI Fixed ACPI Description Table (FADT)
>   */
> +#define ACPI_FADT_COMMON_DEF /* FADT common definition */ \
> +    ACPI_TABLE_HEADER_DEF    /* ACPI common table header */ \
> +    uint32_t firmware_ctrl;  /* Physical address of FACS */ \
> +    uint32_t dsdt;         /* Physical address of DSDT */ \
> +    uint8_t  model;        /* System Interrupt Model */ \
> +    uint8_t  reserved1;    /* Reserved */ \
> +    uint16_t sci_int;      /* System vector of SCI interrupt */ \
> +    uint32_t smi_cmd;      /* Port address of SMI command port */ \
> +    uint8_t  acpi_enable;  /* Value to write to smi_cmd to enable ACPI */ \
> +    uint8_t  acpi_disable; /* Value to write to smi_cmd to disable ACPI */ \
> +    /* Value to write to SMI CMD to enter S4BIOS state */ \
> +    uint8_t  S4bios_req; \
> +    uint8_t  reserved2;    /* Reserved - must be zero */ \
> +    /* Port address of Power Mgt 1a acpi_event Reg Blk */ \
> +    uint32_t pm1a_evt_blk; \
> +    /* Port address of Power Mgt 1b acpi_event Reg Blk */ \
> +    uint32_t pm1b_evt_blk; \
> +    uint32_t pm1a_cnt_blk; /* Port address of Power Mgt 1a Control Reg Blk 
> */ \
> +    uint32_t pm1b_cnt_blk; /* Port address of Power Mgt 1b Control Reg Blk 
> */ \
> +    uint32_t pm2_cnt_blk;  /* Port address of Power Mgt 2 Control Reg Blk */ 
> \
> +    uint32_t pm_tmr_blk;   /* Port address of Power Mgt Timer Ctrl Reg Blk 
> */ \
> +    /* Port addr of General Purpose acpi_event 0 Reg Blk */ \
> +    uint32_t gpe0_blk; \
> +    /* Port addr of General Purpose acpi_event 1 Reg Blk */ \
> +    uint32_t gpe1_blk; \
> +    uint8_t  pm1_evt_len;  /* Byte length of ports at pm1_x_evt_blk */ \
> +    uint8_t  pm1_cnt_len;  /* Byte length of ports at pm1_x_cnt_blk */ \
> +    uint8_t  pm2_cnt_len;  /* Byte Length of ports at pm2_cnt_blk */ \
> +    uint8_t  pm_tmr_len;   /* Byte Length of ports at pm_tm_blk */ \
> +    uint8_t  gpe0_blk_len; /* Byte Length of ports at gpe0_blk */ \
> +    uint8_t  gpe1_blk_len; /* Byte Length of ports at gpe1_blk */ \
> +    uint8_t  gpe1_base;    /* Offset in gpe model where gpe1 events start */ 
> \
> +    uint8_t  reserved3;    /* Reserved */ \
> +    uint16_t plvl2_lat;    /* Worst case HW latency to enter/exit C2 state 
> */ \
> +    uint16_t plvl3_lat;    /* Worst case HW latency to enter/exit C3 state 
> */ \
> +    uint16_t flush_size;   /* Size of area read to flush caches */ \
> +    uint16_t flush_stride; /* Stride used in flushing caches */ \
> +    uint8_t  duty_offset;  /* Bit location of duty cycle field in p_cnt reg 
> */ \
> +    uint8_t  duty_width;   /* Bit width of duty cycle field in p_cnt reg */ \
> +    uint8_t  day_alrm;     /* Index to day-of-month alarm in RTC CMOS RAM */ 
> \
> +    uint8_t  mon_alrm;     /* Index to month-of-year alarm in RTC CMOS RAM 
> */ \
> +    uint8_t  century;      /* Index to century in RTC CMOS RAM */
> +
>  struct AcpiFadtDescriptorRev1
>  {
> -    ACPI_TABLE_HEADER_DEF     /* ACPI common table header */
> -    uint32_t firmware_ctrl;          /* Physical address of FACS */
> -    uint32_t dsdt;                   /* Physical address of DSDT */
> -    uint8_t  model;                  /* System Interrupt Model */
> -    uint8_t  reserved1;              /* Reserved */
> -    uint16_t sci_int;                /* System vector of SCI interrupt */
> -    uint32_t smi_cmd;                /* Port address of SMI command port */
> -    uint8_t  acpi_enable;            /* Value to write to smi_cmd to enable 
> ACPI */
> -    uint8_t  acpi_disable;           /* Value to write to smi_cmd to disable 
> ACPI */
> -    uint8_t  S4bios_req;             /* Value to write to SMI CMD to enter 
> S4BIOS state */
> -    uint8_t  reserved2;              /* Reserved - must be zero */
> -    uint32_t pm1a_evt_blk;           /* Port address of Power Mgt 1a 
> acpi_event Reg Blk */
> -    uint32_t pm1b_evt_blk;           /* Port address of Power Mgt 1b 
> acpi_event Reg Blk */
> -    uint32_t pm1a_cnt_blk;           /* Port address of Power Mgt 1a Control 
> Reg Blk */
> -    uint32_t pm1b_cnt_blk;           /* Port address of Power Mgt 1b Control 
> Reg Blk */
> -    uint32_t pm2_cnt_blk;            /* Port address of Power Mgt 2 Control 
> Reg Blk */
> -    uint32_t pm_tmr_blk;             /* Port address of Power Mgt Timer Ctrl 
> Reg Blk */
> -    uint32_t gpe0_blk;               /* Port addr of General Purpose 
> acpi_event 0 Reg Blk */
> -    uint32_t gpe1_blk;               /* Port addr of General Purpose 
> acpi_event 1 Reg Blk */
> -    uint8_t  pm1_evt_len;            /* Byte length of ports at 
> pm1_x_evt_blk */
> -    uint8_t  pm1_cnt_len;            /* Byte length of ports at 
> pm1_x_cnt_blk */
> -    uint8_t  pm2_cnt_len;            /* Byte Length of ports at pm2_cnt_blk 
> */
> -    uint8_t  pm_tmr_len;             /* Byte Length of ports at pm_tm_blk */
> -    uint8_t  gpe0_blk_len;           /* Byte Length of ports at gpe0_blk */
> -    uint8_t  gpe1_blk_len;           /* Byte Length of ports at gpe1_blk */
> -    uint8_t  gpe1_base;              /* Offset in gpe model where gpe1 
> events start */
> -    uint8_t  reserved3;              /* Reserved */
> -    uint16_t plvl2_lat;              /* Worst case HW latency to enter/exit 
> C2 state */
> -    uint16_t plvl3_lat;              /* Worst case HW latency to enter/exit 
> C3 state */
> -    uint16_t flush_size;             /* Size of area read to flush caches */
> -    uint16_t flush_stride;           /* Stride used in flushing caches */
> -    uint8_t  duty_offset;            /* Bit location of duty cycle field in 
> p_cnt reg */
> -    uint8_t  duty_width;             /* Bit width of duty cycle field in 
> p_cnt reg */
> -    uint8_t  day_alrm;               /* Index to day-of-month alarm in RTC 
> CMOS RAM */
> -    uint8_t  mon_alrm;               /* Index to month-of-year alarm in RTC 
> CMOS RAM */
> -    uint8_t  century;                /* Index to century in RTC CMOS RAM */
> +    ACPI_FADT_COMMON_DEF
>      uint8_t  reserved4;              /* Reserved */
>      uint8_t  reserved4a;             /* Reserved */
>      uint8_t  reserved4b;             /* Reserved */
> @@ -135,6 +143,59 @@ struct AcpiFadtDescriptorRev1
>  } QEMU_PACKED;
>  typedef struct AcpiFadtDescriptorRev1 AcpiFadtDescriptorRev1;
>  
> +struct AcpiGenericAddress {
> +    uint8_t space_id;        /* Address space where struct or register 
> exists */
> +    uint8_t bit_width;       /* Size in bits of given register */
> +    uint8_t bit_offset;      /* Bit offset within the register */
> +    uint8_t access_width;    /* Minimum Access size (ACPI 3.0) */
> +    uint64_t address;        /* 64-bit address of struct or register */
> +} QEMU_PACKED;
> +
> +struct AcpiFadtDescriptorRev5_1 {
> +    ACPI_FADT_COMMON_DEF
> +    /* IA-PC Boot Architecture Flags (see below for individual flags) */
> +    uint16_t boot_flags;
> +    uint8_t reserved;    /* Reserved, must be zero */
> +    /* Miscellaneous flag bits (see below for individual flags) */
> +    uint32_t flags;
> +    /* 64-bit address of the Reset register */
> +    struct AcpiGenericAddress reset_register;
> +    /* Value to write to the reset_register port to reset the system */
> +    uint8_t reset_value;
> +    /* ARM-Specific Boot Flags (see below for individual flags) (ACPI 5.1) */
> +    uint16_t arm_boot_flags;
> +    uint8_t minor_revision;  /* FADT Minor Revision (ACPI 5.1) */
> +    uint64_t Xfacs;          /* 64-bit physical address of FACS */
> +    uint64_t Xdsdt;          /* 64-bit physical address of DSDT */
> +    /* 64-bit Extended Power Mgt 1a Event Reg Blk address */
> +    struct AcpiGenericAddress xpm1a_event_block;
> +    /* 64-bit Extended Power Mgt 1b Event Reg Blk address */
> +    struct AcpiGenericAddress xpm1b_event_block;
> +    /* 64-bit Extended Power Mgt 1a Control Reg Blk address */
> +    struct AcpiGenericAddress xpm1a_control_block;
> +    /* 64-bit Extended Power Mgt 1b Control Reg Blk address */
> +    struct AcpiGenericAddress xpm1b_control_block;
> +    /* 64-bit Extended Power Mgt 2 Control Reg Blk address */
> +    struct AcpiGenericAddress xpm2_control_block;
> +    /* 64-bit Extended Power Mgt Timer Ctrl Reg Blk address */
> +    struct AcpiGenericAddress xpm_timer_block;
> +    /* 64-bit Extended General Purpose Event 0 Reg Blk address */
> +    struct AcpiGenericAddress xgpe0_block;
> +    /* 64-bit Extended General Purpose Event 1 Reg Blk address */
> +    struct AcpiGenericAddress xgpe1_block;
> +    /* 64-bit Sleep Control register (ACPI 5.0) */
> +    struct AcpiGenericAddress sleep_control;
> +    /* 64-bit Sleep Status register (ACPI 5.0) */
> +    struct AcpiGenericAddress sleep_status;
> +} QEMU_PACKED;
> +
> +typedef struct AcpiFadtDescriptorRev5_1 AcpiFadtDescriptorRev5_1;
> +
> +enum {
> +    ACPI_FADT_ARM_USE_PSCI_G_0_2 = 0,
> +    ACPI_FADT_ARM_PSCI_USE_HVC = 1,
> +};

Where do these names come from? Are they really ARM specific?

I'm not sure what the benefit of the enum is here over a #define,
especially if the values mean something to the outside world. If it was
being passed around and we were doing type checking it would make more
sense.

> +
>  /*
>   * ACPI 1.0 Root System Description Table (RSDT)
>   */

-- 
Alex Bennée