From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54280) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1erJvz-0004bJ-Af for qemu-devel@nongnu.org; Thu, 01 Mar 2018 03:51:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1erJvy-0001EC-6c for qemu-devel@nongnu.org; Thu, 01 Mar 2018 03:51:27 -0500 References: <1519827835-239519-1-git-send-email-imammedo@redhat.com> <1519827835-239519-10-git-send-email-imammedo@redhat.com> From: Auger Eric Message-ID: <986e15d2-47cd-be73-da0f-39874b38a325@redhat.com> Date: Thu, 1 Mar 2018 09:51:14 +0100 MIME-Version: 1.0 In-Reply-To: <1519827835-239519-10-git-send-email-imammedo@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 09/10] virt_arm: acpi: reuse common build_fadt() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov , qemu-devel@nongnu.org Cc: "Michael S. Tsirkin" , Shannon Zhao , qemu-arm@nongnu.org Hi, On 28/02/18 15:23, Igor Mammedov wrote: > Extend generic build_fadt() to support rev5.1 FADT > and reuse it for 'virt' board, it would allow to > phase out usage of AcpiFadtDescriptorRev5_1 and > later ACPI_FADT_COMMON_DEF. > > Signed-off-by: Igor Mammedov > --- > v2: > - update comment to mention that build_fadt() supports 5.1 revision > --- > include/hw/acpi/acpi-defs.h | 12 ++---------- > hw/acpi/aml-build.c | 23 +++++++++++++++++++++-- > hw/arm/virt-acpi-build.c | 33 ++++++++++++--------------------- > 3 files changed, 35 insertions(+), 33 deletions(-) > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > index 3fb0ace..fe15e95 100644 > --- a/include/hw/acpi/acpi-defs.h > +++ b/include/hw/acpi/acpi-defs.h > @@ -165,16 +165,6 @@ struct AcpiFadtDescriptorRev3 { > } QEMU_PACKED; > typedef struct AcpiFadtDescriptorRev3 AcpiFadtDescriptorRev3; > > -struct AcpiFadtDescriptorRev5_1 { > - ACPI_FADT_COMMON_DEF > - /* 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; > - > typedef struct AcpiFadtData { > struct AcpiGenericAddress pm1a_cnt; /* PM1a_CNT_BLK */ > struct AcpiGenericAddress pm1a_evt; /* PM1a_EVT_BLK */ > @@ -192,6 +182,8 @@ typedef struct AcpiFadtData { > uint8_t rtc_century; /* CENTURY */ > uint16_t plvl2_lat; /* P_LVL2_LAT */ > uint16_t plvl3_lat; /* P_LVL3_LAT */ > + uint16_t arm_boot_arch; /* ARM_BOOT_ARCH */ > + uint8_t minor_ver; /* FADT Minor Version */ > > /* > * respective tables offsets within ACPI_BUILD_TABLE_FILE, > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > index 8f45298..3fa557c 100644 > --- a/hw/acpi/aml-build.c > +++ b/hw/acpi/aml-build.c > @@ -1679,7 +1679,7 @@ void build_slit(GArray *table_data, BIOSLinker *linker) > table_data->len - slit_start, 1, NULL, NULL); > } > > -/* build rev1/rev3 FADT */ > +/* build rev1/rev3/rev5.1 FADT */ > void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f, > const char *oem_id, const char *oem_table_id) > { > @@ -1755,7 +1755,14 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f, > > build_append_gas_from_struct(tbl, &f->reset_reg); /* RESET_REG */ > build_append_int_noprefix(tbl, f->reset_val, 1); /* RESET_VALUE */ > - build_append_int_noprefix(tbl, 0, 3); /* Reserved, ACPI 3.0 */ > + /* Since ACPI 5.1 */ > + if ((f->rev >= 6) || ((f->rev == 5) && f->minor_ver > 0)) { > + build_append_int_noprefix(tbl, f->arm_boot_arch, 2); /* ARM_BOOT_ARCH */ > + /* FADT Minor Version */ > + build_append_int_noprefix(tbl, f->minor_ver, 1); > + } else { > + build_append_int_noprefix(tbl, 0, 3); /* Reserved upto ACPI 5.0 */ > + } > build_append_int_noprefix(tbl, 0, 8); /* X_FIRMWARE_CTRL */ > > /* XDSDT address to be filled by Guest linker at runtime */ > @@ -1779,6 +1786,18 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f, > build_append_gas_from_struct(tbl, &f->gpe0_blk); /* X_GPE0_BLK */ > build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0); /* X_GPE1_BLK */ > > + if (f->rev <= 4) { > + goto build_hdr; > + } > + > + /* SLEEP_CONTROL_REG */ > + build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0); > + /* SLEEP_STATUS_REG */ > + build_append_gas(tbl, AML_AS_SYSTEM_MEMORY, 0 , 0, 0, 0); > + > + /* TODO: extra fields need to be added to support revisions above rev5 */ > + assert(f->rev == 5); My previous comment about asserting here if f->rev != 5 and previous (f->rev >= 6) check was skipped but that's not a big deal. Reviewed-by: Eric Auger Thanks Eric > + > build_hdr: > build_header(linker, tbl, (void *)(tbl->data + fadt_start), > "FACP", tbl->len - fadt_start, f->rev, oem_id, oem_table_id); > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index b644da9..c7c6a57 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -654,39 +654,30 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker, > VirtMachineState *vms, unsigned dsdt_tbl_offset) > { > - int fadt_start = table_data->len; > - AcpiFadtDescriptorRev5_1 *fadt = acpi_data_push(table_data, sizeof(*fadt)); > - unsigned xdsdt_entry_offset = (char *)&fadt->x_dsdt - table_data->data; > - uint16_t bootflags; > + /* ACPI v5.1 */ > + AcpiFadtData fadt = { > + .rev = 5, > + .minor_ver = 1, > + .flags = 1 << ACPI_FADT_F_HW_REDUCED_ACPI, > + .xdsdt_tbl_offset = &dsdt_tbl_offset, > + }; > > switch (vms->psci_conduit) { > case QEMU_PSCI_CONDUIT_DISABLED: > - bootflags = 0; > + fadt.arm_boot_arch = 0; > break; > case QEMU_PSCI_CONDUIT_HVC: > - bootflags = ACPI_FADT_ARM_PSCI_COMPLIANT | ACPI_FADT_ARM_PSCI_USE_HVC; > + fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT | > + ACPI_FADT_ARM_PSCI_USE_HVC; > break; > case QEMU_PSCI_CONDUIT_SMC: > - bootflags = ACPI_FADT_ARM_PSCI_COMPLIANT; > + fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT; > break; > default: > g_assert_not_reached(); > } > > - /* Hardware Reduced = 1 and use PSCI 0.2+ */ > - fadt->flags = cpu_to_le32(1 << ACPI_FADT_F_HW_REDUCED_ACPI); > - fadt->arm_boot_flags = cpu_to_le16(bootflags); > - > - /* ACPI v5.1 (fadt->revision.fadt->minor_revision) */ > - fadt->minor_revision = 0x1; > - > - /* DSDT address to be filled by Guest linker */ > - bios_linker_loader_add_pointer(linker, > - ACPI_BUILD_TABLE_FILE, xdsdt_entry_offset, sizeof(fadt->x_dsdt), > - ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset); > - > - build_header(linker, table_data, (void *)(table_data->data + fadt_start), > - "FACP", table_data->len - fadt_start, 5, NULL, NULL); > + build_fadt(table_data, linker, &fadt, NULL, NULL); > } > > /* DSDT */ >