From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55937) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1erK3e-00031O-JN for qemu-devel@nongnu.org; Thu, 01 Mar 2018 03:59:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1erK3c-0004s4-SL for qemu-devel@nongnu.org; Thu, 01 Mar 2018 03:59:22 -0500 References: <1519827835-239519-1-git-send-email-imammedo@redhat.com> <1519827835-239519-11-git-send-email-imammedo@redhat.com> From: Auger Eric Message-ID: Date: Thu, 1 Mar 2018 09:59:07 +0100 MIME-Version: 1.0 In-Reply-To: <1519827835-239519-11-git-send-email-imammedo@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 10/10] tests: acpi: don't read all fields in test_acpi_fadt_table() 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 Igor, On 28/02/18 15:23, Igor Mammedov wrote: > there is no point to read fields here but not actually > checking them so drop it and read only header + dsdt/facs > addresses since it's needed later to fetch that tables. > > With this cleanup we can get rid of AcpiFadtDescriptorRev3/ > ACPI_FADT_COMMON_DEF which have no users left. > > Signed-off-by: Igor Mammedov My only comment is you could have removed include/hw/acpi/acpi-defs.h defs in a separate subsequent patch after updating the test stuff. Besides, Reviewed-by: Eric Auger Eric > --- > include/hw/acpi/acpi-defs.h | 81 -------------------------------------------- > tests/bios-tables-test.c | 82 ++++++++++----------------------------------- > 2 files changed, 18 insertions(+), 145 deletions(-) > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > index fe15e95..5955eb4 100644 > --- a/include/hw/acpi/acpi-defs.h > +++ b/include/hw/acpi/acpi-defs.h > @@ -75,82 +75,6 @@ struct AcpiTableHeader { > } QEMU_PACKED; > typedef struct AcpiTableHeader AcpiTableHeader; > > -/* > - * 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 */ \ > - /* 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 x_facs; /* 64-bit physical address of FACS */ \ > - uint64_t x_dsdt; /* 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; \ > - > struct AcpiGenericAddress { > uint8_t space_id; /* Address space where struct or register exists */ > uint8_t bit_width; /* Size in bits of given register */ > @@ -160,11 +84,6 @@ struct AcpiGenericAddress { > uint64_t address; /* 64-bit address of struct or register */ > } QEMU_PACKED; > > -struct AcpiFadtDescriptorRev3 { > - ACPI_FADT_COMMON_DEF > -} QEMU_PACKED; > -typedef struct AcpiFadtDescriptorRev3 AcpiFadtDescriptorRev3; > - > typedef struct AcpiFadtData { > struct AcpiGenericAddress pm1a_cnt; /* PM1a_CNT_BLK */ > struct AcpiGenericAddress pm1a_evt; /* PM1a_EVT_BLK */ > diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c > index 65b271a..cd753ff 100644 > --- a/tests/bios-tables-test.c > +++ b/tests/bios-tables-test.c > @@ -29,7 +29,8 @@ typedef struct { > uint32_t rsdp_addr; > AcpiRsdpDescriptor rsdp_table; > AcpiRsdtDescriptorRev1 rsdt_table; > - AcpiFadtDescriptorRev3 fadt_table; > + uint32_t dsdt_addr; > + uint32_t facs_addr; > AcpiFacsDescriptorRev1 facs_table; > uint32_t *rsdt_tables_addr; > int rsdt_tables_nr; > @@ -127,71 +128,18 @@ static void test_acpi_rsdt_table(test_data *data) > data->rsdt_tables_nr = tables_nr; > } > > -static void test_acpi_fadt_table(test_data *data) > +static void fadt_fetch_facs_and_dsdt_ptrs(test_data *data) > { > - AcpiFadtDescriptorRev3 *fadt_table = &data->fadt_table; > uint32_t addr; > + AcpiTableHeader hdr; > > /* FADT table comes first */ > addr = le32_to_cpu(data->rsdt_tables_addr[0]); > - ACPI_READ_TABLE_HEADER(fadt_table, addr); > - > - ACPI_READ_FIELD(fadt_table->firmware_ctrl, addr); > - ACPI_READ_FIELD(fadt_table->dsdt, addr); > - ACPI_READ_FIELD(fadt_table->model, addr); > - ACPI_READ_FIELD(fadt_table->reserved1, addr); > - ACPI_READ_FIELD(fadt_table->sci_int, addr); > - ACPI_READ_FIELD(fadt_table->smi_cmd, addr); > - ACPI_READ_FIELD(fadt_table->acpi_enable, addr); > - ACPI_READ_FIELD(fadt_table->acpi_disable, addr); > - ACPI_READ_FIELD(fadt_table->S4bios_req, addr); > - ACPI_READ_FIELD(fadt_table->reserved2, addr); > - ACPI_READ_FIELD(fadt_table->pm1a_evt_blk, addr); > - ACPI_READ_FIELD(fadt_table->pm1b_evt_blk, addr); > - ACPI_READ_FIELD(fadt_table->pm1a_cnt_blk, addr); > - ACPI_READ_FIELD(fadt_table->pm1b_cnt_blk, addr); > - ACPI_READ_FIELD(fadt_table->pm2_cnt_blk, addr); > - ACPI_READ_FIELD(fadt_table->pm_tmr_blk, addr); > - ACPI_READ_FIELD(fadt_table->gpe0_blk, addr); > - ACPI_READ_FIELD(fadt_table->gpe1_blk, addr); > - ACPI_READ_FIELD(fadt_table->pm1_evt_len, addr); > - ACPI_READ_FIELD(fadt_table->pm1_cnt_len, addr); > - ACPI_READ_FIELD(fadt_table->pm2_cnt_len, addr); > - ACPI_READ_FIELD(fadt_table->pm_tmr_len, addr); > - ACPI_READ_FIELD(fadt_table->gpe0_blk_len, addr); > - ACPI_READ_FIELD(fadt_table->gpe1_blk_len, addr); > - ACPI_READ_FIELD(fadt_table->gpe1_base, addr); > - ACPI_READ_FIELD(fadt_table->reserved3, addr); > - ACPI_READ_FIELD(fadt_table->plvl2_lat, addr); > - ACPI_READ_FIELD(fadt_table->plvl3_lat, addr); > - ACPI_READ_FIELD(fadt_table->flush_size, addr); > - ACPI_READ_FIELD(fadt_table->flush_stride, addr); > - ACPI_READ_FIELD(fadt_table->duty_offset, addr); > - ACPI_READ_FIELD(fadt_table->duty_width, addr); > - ACPI_READ_FIELD(fadt_table->day_alrm, addr); > - ACPI_READ_FIELD(fadt_table->mon_alrm, addr); > - ACPI_READ_FIELD(fadt_table->century, addr); > - ACPI_READ_FIELD(fadt_table->boot_flags, addr); > - ACPI_READ_FIELD(fadt_table->reserved, addr); > - ACPI_READ_FIELD(fadt_table->flags, addr); > - ACPI_READ_GENERIC_ADDRESS(fadt_table->reset_register, addr); > - ACPI_READ_FIELD(fadt_table->reset_value, addr); > - ACPI_READ_FIELD(fadt_table->arm_boot_flags, addr); > - ACPI_READ_FIELD(fadt_table->minor_revision, addr); > - ACPI_READ_FIELD(fadt_table->x_facs, addr); > - ACPI_READ_FIELD(fadt_table->x_dsdt, addr); > - ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1a_event_block, addr); > - ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1b_event_block, addr); > - ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1a_control_block, addr); > - ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm1b_control_block, addr); > - ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm2_control_block, addr); > - ACPI_READ_GENERIC_ADDRESS(fadt_table->xpm_timer_block, addr); > - ACPI_READ_GENERIC_ADDRESS(fadt_table->xgpe0_block, addr); > - ACPI_READ_GENERIC_ADDRESS(fadt_table->xgpe1_block, addr); > - > - ACPI_ASSERT_CMP(fadt_table->signature, "FACP"); > - g_assert(!acpi_calc_checksum((uint8_t *)fadt_table, > - le32_to_cpu(fadt_table->length))); > + ACPI_READ_TABLE_HEADER(&hdr, addr); > + ACPI_ASSERT_CMP(hdr.signature, "FACP"); > + > + ACPI_READ_FIELD(data->facs_addr, addr); > + ACPI_READ_FIELD(data->dsdt_addr, addr); > } > > static void sanitize_fadt_ptrs(test_data *data) > @@ -206,6 +154,12 @@ static void sanitize_fadt_ptrs(test_data *data) > continue; > } > > + /* check original FADT checksum before sanitizing table */ > + g_assert(!(uint8_t)( > + acpi_calc_checksum((uint8_t *)sdt, sizeof(AcpiTableHeader)) + > + acpi_calc_checksum((uint8_t *)sdt->aml, sdt->aml_len) > + )); > + > /* sdt->aml field offset := spec offset - header size */ > memset(sdt->aml + 0, 0, 4); /* sanitize FIRMWARE_CTRL(36) ptr */ > memset(sdt->aml + 4, 0, 4); /* sanitize DSDT(40) ptr */ > @@ -226,7 +180,7 @@ static void sanitize_fadt_ptrs(test_data *data) > static void test_acpi_facs_table(test_data *data) > { > AcpiFacsDescriptorRev1 *facs_table = &data->facs_table; > - uint32_t addr = le32_to_cpu(data->fadt_table.firmware_ctrl); > + uint32_t addr = le32_to_cpu(data->facs_addr); > > ACPI_READ_FIELD(facs_table->signature, addr); > ACPI_READ_FIELD(facs_table->length, addr); > @@ -265,7 +219,7 @@ static void fetch_table(AcpiSdtTable *sdt_table, uint32_t addr) > static void test_acpi_dsdt_table(test_data *data) > { > AcpiSdtTable dsdt_table; > - uint32_t addr = le32_to_cpu(data->fadt_table.dsdt); > + uint32_t addr = le32_to_cpu(data->dsdt_addr); > > fetch_table(&dsdt_table, addr); > ACPI_ASSERT_CMP(dsdt_table.header.signature, "DSDT"); > @@ -674,7 +628,7 @@ static void test_acpi_one(const char *params, test_data *data) > test_acpi_rsdp_address(data); > test_acpi_rsdp_table(data); > test_acpi_rsdt_table(data); > - test_acpi_fadt_table(data); > + fadt_fetch_facs_and_dsdt_ptrs(data); > test_acpi_facs_table(data); > test_acpi_dsdt_table(data); > fetch_rsdt_referenced_tables(data); >