[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 06/10] pc: acpi: isolate FADT specific data i
|
From: |
Igor Mammedov |
|
Subject: |
Re: [Qemu-devel] [PATCH v2 06/10] pc: acpi: isolate FADT specific data into AcpiFadtData structure |
|
Date: |
Thu, 1 Mar 2018 10:39:08 +0100 |
On Thu, 1 Mar 2018 09:43:35 +0100
Auger Eric <address@hidden> wrote:
> Hi Igor,
>
> On 28/02/18 15:23, Igor Mammedov wrote:
> > move FADT data initialization out of fadt_setup() into dedicated
> > init_fadt_data() that will set common for pc/q35 values in
> > AcpiFadtData structure and acpi_get_pm_info() will complement
> > it with pc/q35 specific values initialization.
> >
> > That will allow to get rid of fadt_setup() and generalize
> > build_fadt() so it could be easily extended for rev5 and
> > reused by ARM target.
> >
> > While at it also move facs/dsdt/xdsdt offsets from build_fadt()
> > arg list into AcpiFadtData, as they belong to the same dataset.
> >
> > Signed-off-by: Igor Mammedov <address@hidden>
>
> > ---
> > v2:
> > changes requested by Auger Eric <address@hidden>
> suggested ;-)
>
> > - s/pm1_/pm1a_/
> > - s/c2_latency/plvl2_lat/
> > - s/c3_latency/plvl3_lat/
> > - cleanup ACPI_PORT_SMI_CMD TODO, move it to hw/isa/apm.h
> > - move
> > if (f->facs_tbl_offset) / if (f->dsdt_tbl_offset)
> > bios_linker_loader_add_pointer(...)
> > into the patch that makes build_fadt() generic
> > ---
> > include/hw/acpi/acpi-defs.h | 28 +++++++
> > hw/i386/acpi-build.c | 190
> > ++++++++++++++++++++++++--------------------
> > 2 files changed, 130 insertions(+), 88 deletions(-)
> >
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index 9942bc5..3fb0ace 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -175,6 +175,34 @@ struct AcpiFadtDescriptorRev5_1 {
> >
> > typedef struct AcpiFadtDescriptorRev5_1 AcpiFadtDescriptorRev5_1;
> >
> > +typedef struct AcpiFadtData {
> > + struct AcpiGenericAddress pm1a_cnt; /* PM1a_CNT_BLK */
> > + struct AcpiGenericAddress pm1a_evt; /* PM1a_EVT_BLK */
> > + struct AcpiGenericAddress pm_tmr; /* PM_TMR_BLK */
> > + struct AcpiGenericAddress gpe0_blk; /* GPE0_BLK */
> > + struct AcpiGenericAddress reset_reg; /* RESET_REG */
> > + uint8_t reset_val; /* RESET_VALUE */
> > + uint8_t rev; /* Revision */
> > + uint32_t flags; /* Flags */
> > + uint32_t smi_cmd; /* SMI_CMD */
> > + uint16_t sci_int; /* SCI_INT */
> > + uint8_t int_model; /* INT_MODEL */
> > + uint8_t acpi_enable_cmd; /* ACPI_ENABLE */
> > + uint8_t acpi_disable_cmd; /* ACPI_DISABLE */
> > + uint8_t rtc_century; /* CENTURY */
> > + uint16_t plvl2_lat; /* P_LVL2_LAT */
> > + uint16_t plvl3_lat; /* P_LVL3_LAT */
> > +
> > + /*
> > + * respective tables offsets within ACPI_BUILD_TABLE_FILE,
> > + * NULL if table doesn't exist (in that case field's value
> > + * won't be patched by linker and will be kept set to 0)
> > + */
> > + unsigned *facs_tbl_offset; /* FACS offset in */
> > + unsigned *dsdt_tbl_offset;
> > + unsigned *xdsdt_tbl_offset;
> > +} AcpiFadtData;
> > +
> > #define ACPI_FADT_ARM_PSCI_COMPLIANT (1 << 0)
> > #define ACPI_FADT_ARM_PSCI_USE_HVC (1 << 1)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 699f3a0..1f88ed1 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -91,17 +91,11 @@ typedef struct AcpiMcfgInfo {
> > } AcpiMcfgInfo;
> >
> > typedef struct AcpiPmInfo {
> > - bool force_rev1_fadt;
> > bool s3_disabled;
> > bool s4_disabled;
> > bool pcihp_bridge_en;
> > uint8_t s4_val;
> > - uint16_t sci_int;
> > - uint8_t acpi_enable_cmd;
> > - uint8_t acpi_disable_cmd;
> > - uint32_t gpe0_blk;
> > - uint32_t gpe0_blk_len;
> > - uint32_t io_base;
> > + AcpiFadtData fadt;
> > uint16_t cpu_hp_io_base;
> > uint16_t pcihp_io_base;
> > uint16_t pcihp_io_len;
> > @@ -124,20 +118,59 @@ typedef struct AcpiBuildPciBusHotplugState {
> > bool pcihp_bridge_en;
> > } AcpiBuildPciBusHotplugState;
> >
> > +static void init_common_fadt_data(Object *o, AcpiFadtData *data)
> > +{
> > + uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE,
> > NULL);
> > + AmlAddressSpace as = AML_AS_SYSTEM_IO;
> > + AcpiFadtData fadt = {
> > + .rev = 3,
> > + .flags =
> > + (1 << ACPI_FADT_F_WBINVD) |
> > + (1 << ACPI_FADT_F_PROC_C1) |
> > + (1 << ACPI_FADT_F_SLP_BUTTON) |
> > + (1 << ACPI_FADT_F_RTC_S4) |
> > + (1 << ACPI_FADT_F_USE_PLATFORM_CLOCK) |
> > + /* APIC destination mode ("Flat Logical") has an upper limit
> > of 8
> > + * CPUs for more than 8 CPUs, "Clustered Logical" mode has to
> > be
> > + * used
> > + */
> > + ((max_cpus > 8) ? (1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL)
> > : 0),
> > + .int_model = 1 /* Multiple APIC */,
> > + .rtc_century = RTC_CENTURY,
> > + .plvl2_lat = 0xfff /* C2 state not supported */,
> > + .plvl3_lat = 0xfff /* C3 state not supported */,
> > + .smi_cmd = ACPI_PORT_SMI_CMD,
> > + .sci_int = object_property_get_uint(o, ACPI_PM_PROP_SCI_INT, NULL),
> > + .acpi_enable_cmd =
> > + object_property_get_uint(o, ACPI_PM_PROP_ACPI_ENABLE_CMD,
> > NULL),
> > + .acpi_disable_cmd =
> > + object_property_get_uint(o, ACPI_PM_PROP_ACPI_DISABLE_CMD,
> > NULL),
> > + .pm1a_evt = { .space_id = as, .bit_width = 4 * 8, .address = io },
> > + .pm1a_cnt = { .space_id = as, .bit_width = 2 * 8,
> > + .address = io + 0x04 },
> > + .pm_tmr = { .space_id = as, .bit_width = 4 * 8, .address = io +
> > 0x08 },
> > + .gpe0_blk = { .space_id = as, .bit_width =
> > + object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK_LEN, NULL) *
> > 8,
> > + .address = object_property_get_uint(o, ACPI_PM_PROP_GPE0_BLK,
> > NULL)
> > + },
> > + };
> > + *data = fadt;
> > +}
> > +
> > static void acpi_get_pm_info(AcpiPmInfo *pm)
> > {
> > Object *piix = piix4_pm_find();
> > Object *lpc = ich9_lpc_find();
> > Object *obj = piix ? piix : lpc;
> > QObject *o;
> > -
> > - pm->force_rev1_fadt = false;
> > pm->cpu_hp_io_base = 0;
> > pm->pcihp_io_base = 0;
> > pm->pcihp_io_len = 0;
> > +
> > + init_common_fadt_data(obj, &pm->fadt);
> > if (piix) {
> > /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
> > - pm->force_rev1_fadt = true;
> > + pm->fadt.rev = 1;
> > pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> > pm->pcihp_io_base =
> > object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> > @@ -145,10 +178,19 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> > object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> > }
> > if (lpc) {
> > + struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
> > + .bit_width = 8, .address = ICH9_RST_CNT_IOPORT };
> > + pm->fadt.reset_reg = r;
> > + pm->fadt.reset_val = 0xf;
> > + pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
> > pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
> > }
> > assert(obj);
> >
> > + /* The above need not be conditional on machine type because the reset
> > port
> > + * happens to be the same on PIIX (pc) and ICH9 (q35). */
> > + QEMU_BUILD_BUG_ON(ICH9_RST_CNT_IOPORT != RCR_IOPORT);
> > +
> > /* Fill in optional s3/s4 related properties */
> > o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
> > if (o) {
> > @@ -172,22 +214,6 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> > }
> > qobject_decref(o);
> >
> > - /* Fill in mandatory properties */
> > - pm->sci_int = object_property_get_uint(obj, ACPI_PM_PROP_SCI_INT,
> > NULL);
> > -
> > - pm->acpi_enable_cmd = object_property_get_uint(obj,
> > -
> > ACPI_PM_PROP_ACPI_ENABLE_CMD,
> > - NULL);
> > - pm->acpi_disable_cmd =
> > - object_property_get_uint(obj,
> > - ACPI_PM_PROP_ACPI_DISABLE_CMD,
> > - NULL);
> > - pm->io_base = object_property_get_uint(obj, ACPI_PM_PROP_PM_IO_BASE,
> > - NULL);
> > - pm->gpe0_blk = object_property_get_uint(obj, ACPI_PM_PROP_GPE0_BLK,
> > - NULL);
> > - pm->gpe0_blk_len = object_property_get_uint(obj,
> > ACPI_PM_PROP_GPE0_BLK_LEN,
> > - NULL);
> > pm->pcihp_bridge_en =
> > object_property_get_bool(obj,
> > "acpi-pci-hotplug-with-bridge-support",
> > NULL);
> > @@ -273,73 +299,53 @@ build_facs(GArray *table_data, BIOSLinker *linker)
> > }
> >
> > /* Load chipset information in FADT */
> > -static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiPmInfo *pm)
> > +static void fadt_setup(AcpiFadtDescriptorRev3 *fadt, AcpiFadtData f)
> > {
> > - fadt->model = 1;
> > + fadt->model = f.int_model;
> > fadt->reserved1 = 0;
> > - fadt->sci_int = cpu_to_le16(pm->sci_int);
> > - fadt->smi_cmd = cpu_to_le32(ACPI_PORT_SMI_CMD);
> > - fadt->acpi_enable = pm->acpi_enable_cmd;
> > - fadt->acpi_disable = pm->acpi_disable_cmd;
> > + fadt->sci_int = cpu_to_le16(f.sci_int);
> > + fadt->smi_cmd = cpu_to_le32(f.smi_cmd);
> > + fadt->acpi_enable = f.acpi_enable_cmd;
> > + fadt->acpi_disable = f.acpi_disable_cmd;
> > /* EVT, CNT, TMR offset matches hw/acpi/core.c */
> > - fadt->pm1a_evt_blk = cpu_to_le32(pm->io_base);
> > - fadt->pm1a_cnt_blk = cpu_to_le32(pm->io_base + 0x04);
> > - fadt->pm_tmr_blk = cpu_to_le32(pm->io_base + 0x08);
> > - fadt->gpe0_blk = cpu_to_le32(pm->gpe0_blk);
> > + fadt->pm1a_evt_blk = cpu_to_le32(f.pm1a_evt.address);
> > + fadt->pm1a_cnt_blk = cpu_to_le32(f.pm1a_cnt.address);
> > + fadt->pm_tmr_blk = cpu_to_le32(f.pm_tmr.address);
> > + fadt->gpe0_blk = cpu_to_le32(f.gpe0_blk.address);
> > /* EVT, CNT, TMR length matches hw/acpi/core.c */
> > - fadt->pm1_evt_len = 4;
> > - fadt->pm1_cnt_len = 2;
> > - fadt->pm_tmr_len = 4;
> > - fadt->gpe0_blk_len = pm->gpe0_blk_len;
> > - fadt->plvl2_lat = cpu_to_le16(0xfff); /* C2 state not supported */
> > - fadt->plvl3_lat = cpu_to_le16(0xfff); /* C3 state not supported */
> > - fadt->flags = cpu_to_le32((1 << ACPI_FADT_F_WBINVD) |
> > - (1 << ACPI_FADT_F_PROC_C1) |
> > - (1 << ACPI_FADT_F_SLP_BUTTON) |
> > - (1 << ACPI_FADT_F_RTC_S4));
> > - fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
> > - /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
> > - * For more than 8 CPUs, "Clustered Logical" mode has to be used
> > - */
> > - if (max_cpus > 8) {
> > - fadt->flags |= cpu_to_le32(1 <<
> > ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
> > - }
> > - fadt->century = RTC_CENTURY;
> > - if (pm->force_rev1_fadt) {
> > + fadt->pm1_evt_len = f.pm1a_evt.bit_width / 8;
> > + fadt->pm1_cnt_len = f.pm1a_cnt.bit_width / 8;
> > + fadt->pm_tmr_len = f.pm_tmr.bit_width / 8;
> > + fadt->gpe0_blk_len = f.gpe0_blk.bit_width / 8;
> > + fadt->plvl2_lat = cpu_to_le16(f.plvl2_lat);
> > + fadt->plvl3_lat = cpu_to_le16(f.plvl3_lat);
> > + fadt->flags = cpu_to_le32(f.flags);
> > + fadt->century = f.rtc_century;
> > + if (f.rev == 1) {
> > return;
> > }
> >
> > - fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
> > - fadt->reset_value = 0xf;
> > - fadt->reset_register.space_id = AML_SYSTEM_IO;
> > - fadt->reset_register.bit_width = 8;
> > - fadt->reset_register.address = cpu_to_le64(ICH9_RST_CNT_IOPORT);
> > - /* The above need not be conditional on machine type because the reset
> > port
> > - * happens to be the same on PIIX (pc) and ICH9 (q35). */
> > - QEMU_BUILD_BUG_ON(ICH9_RST_CNT_IOPORT != RCR_IOPORT);
> > + fadt->reset_value = f.reset_val;
> > + fadt->reset_register = f.reset_reg;
> > + fadt->reset_register.address = cpu_to_le64(f.reset_reg.address);
> >
> > - fadt->xpm1a_event_block.space_id = AML_SYSTEM_IO;
> > - fadt->xpm1a_event_block.bit_width = fadt->pm1_evt_len * 8;
> > - fadt->xpm1a_event_block.address = cpu_to_le64(pm->io_base);
> > + fadt->xpm1a_event_block = f.pm1a_evt;
> > + fadt->xpm1a_event_block.address = cpu_to_le64(f.pm1a_evt.address);
> >
> > - fadt->xpm1a_control_block.space_id = AML_SYSTEM_IO;
> > - fadt->xpm1a_control_block.bit_width = fadt->pm1_cnt_len * 8;
> > - fadt->xpm1a_control_block.address = cpu_to_le64(pm->io_base + 0x4);
> > + fadt->xpm1a_control_block = f.pm1a_cnt;
> > + fadt->xpm1a_control_block.address = cpu_to_le64(f.pm1a_cnt.address);
> >
> > - fadt->xpm_timer_block.space_id = AML_SYSTEM_IO;
> > - fadt->xpm_timer_block.bit_width = fadt->pm_tmr_len * 8;
> > - fadt->xpm_timer_block.address = cpu_to_le64(pm->io_base + 0x8);
> > + fadt->xpm_timer_block = f.pm_tmr;
> > + fadt->xpm_timer_block.address = cpu_to_le64(f.pm_tmr.address);
> >
> > - fadt->xgpe0_block.space_id = AML_SYSTEM_IO;
> > - fadt->xgpe0_block.bit_width = pm->gpe0_blk_len * 8;
> > - fadt->xgpe0_block.address = cpu_to_le64(pm->gpe0_blk);
> > + fadt->xgpe0_block = f.gpe0_blk;
> > + fadt->xgpe0_block.address = cpu_to_le64(f.gpe0_blk.address);
> > }
> >
> >
> > /* FADT */
> > static void
> > -build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
> > - unsigned facs_tbl_offset, unsigned dsdt_tbl_offset,
> > +build_fadt(GArray *table_data, BIOSLinker *linker, AcpiFadtData *f,
> > const char *oem_id, const char *oem_table_id)
> > {
> > AcpiFadtDescriptorRev3 *fadt = acpi_data_push(table_data,
> > sizeof(*fadt));
> > @@ -347,29 +353,29 @@ build_fadt(GArray *table_data, BIOSLinker *linker,
> > AcpiPmInfo *pm,
> > unsigned dsdt_entry_offset = (char *)&fadt->dsdt - table_data->data;
> > unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - table_data->data;
> > int fadt_size = sizeof(*fadt);
> > - int rev = 3;
> >
> > /* FACS address to be filled by Guest linker */
> > bios_linker_loader_add_pointer(linker,
> > ACPI_BUILD_TABLE_FILE, fw_ctrl_offset, sizeof(fadt->firmware_ctrl),
> > - ACPI_BUILD_TABLE_FILE, facs_tbl_offset);
> > + ACPI_BUILD_TABLE_FILE, *f->facs_tbl_offset);
> >
> > /* DSDT address to be filled by Guest linker */
> > - fadt_setup(fadt, pm);
> > + fadt_setup(fadt, *f);
> > bios_linker_loader_add_pointer(linker,
> > ACPI_BUILD_TABLE_FILE, dsdt_entry_offset, sizeof(fadt->dsdt),
> > - ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> > - if (pm->force_rev1_fadt) {
> > - rev = 1;
> > + ACPI_BUILD_TABLE_FILE, *f->dsdt_tbl_offset);
> > +
> > + if (f->rev == 1) {
> > fadt_size = offsetof(typeof(*fadt), reset_register);
> > - } else {
> > + } else if (f->xdsdt_tbl_offset) {
> I fail to understand why you added this check in this patch on not in
> [PATCH v2 08/10] but anyway
Yep, it's not related to this patch and should be in 8/10.
if I respin series, I'll move it there.
(condition is there is mostly for spec compliance (i.e. optional field)
but it is always true in current code, though if we ever make it NULL
in future it won't break and will still work as expected without
fixing build_fadt())
>
> Reviewed-by: Eric Auger <address@hidden>
Thanks!
>
> Thanks
>
> Eric
> > bios_linker_loader_add_pointer(linker,
> > ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset,
> > sizeof(fadt->x_dsdt),
> > - ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);
> > + ACPI_BUILD_TABLE_FILE, *f->xdsdt_tbl_offset);
> > }
> >
> > build_header(linker, table_data,
> > - (void *)fadt, "FACP", fadt_size, rev, oem_id,
> > oem_table_id);
> > + (void *)fadt, "FACP", fadt_size, f->rev,
> > + oem_id, oem_table_id);
> > }
> >
> > void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> > @@ -2049,7 +2055,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> > crs = aml_resource_template();
> > aml_append(crs,
> > - aml_io(AML_DECODE16, pm->gpe0_blk, pm->gpe0_blk, 1,
> > pm->gpe0_blk_len)
> > + aml_io(
> > + AML_DECODE16,
> > + pm->fadt.gpe0_blk.address,
> > + pm->fadt.gpe0_blk.address,
> > + 1,
> > + pm->fadt.gpe0_blk.bit_width / 8)
> > );
> > aml_append(dev, aml_name_decl("_CRS", crs));
> > aml_append(scope, dev);
> > @@ -2696,7 +2707,10 @@ void acpi_build(AcpiBuildTables *tables,
> > MachineState *machine)
> > /* ACPI tables pointed to by RSDT */
> > fadt = tables_blob->len;
> > acpi_add_table(table_offsets, tables_blob);
> > - build_fadt(tables_blob, tables->linker, &pm, facs, dsdt,
> > + pm.fadt.facs_tbl_offset = &facs;
> > + pm.fadt.dsdt_tbl_offset = &dsdt;
> > + pm.fadt.xdsdt_tbl_offset = &dsdt;
> > + build_fadt(tables_blob, tables->linker, &pm.fadt,
> > slic_oem.id, slic_oem.table_id);
> > aml_len += tables_blob->len - fadt;
> >
> >