| Message ID | 20210708154617.1538485-32-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acpi: refactor error prone build_header() and packed structures usage in ACPI tables | expand |
Hi Igor, On 7/8/21 5:46 PM, Igor Mammedov wrote: > it replaces error-prone pointer arithmetic for build_header() API, > with 2 calls to start and finish table creation, > which hides offsets magic from API user. > > while at it, replace packed structure with endian agnostic > build_append_FOO() API. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > CC: drjones@redhat.com > CC: peter.maydell@linaro.org > CC: shannon.zhaosl@gmail.com > CC: qemu-arm@nongnu.org > --- > include/hw/acpi/acpi-defs.h | 32 ----------------- > hw/arm/virt-acpi-build.c | 68 ++++++++++++++++++++++--------------- > 2 files changed, 41 insertions(+), 59 deletions(-) > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > index 6f2f08a9de..012c4ffb3a 100644 > --- a/include/hw/acpi/acpi-defs.h > +++ b/include/hw/acpi/acpi-defs.h > @@ -117,38 +117,6 @@ typedef struct AcpiFadtData { > #define ACPI_FADT_ARM_PSCI_COMPLIANT (1 << 0) > #define ACPI_FADT_ARM_PSCI_USE_HVC (1 << 1) > > -/* > - * Serial Port Console Redirection Table (SPCR), Rev. 1.02 > - * > - * For .interface_type see Debug Port Table 2 (DBG2) serial port > - * subtypes in Table 3, Rev. May 22, 2012 > - */ > -struct AcpiSerialPortConsoleRedirection { > - ACPI_TABLE_HEADER_DEF > - uint8_t interface_type; > - uint8_t reserved1[3]; > - struct AcpiGenericAddress base_address; > - uint8_t interrupt_types; > - uint8_t irq; > - uint32_t gsi; > - uint8_t baud; > - uint8_t parity; > - uint8_t stopbits; > - uint8_t flowctrl; > - uint8_t term_type; > - uint8_t reserved2; > - uint16_t pci_device_id; > - uint16_t pci_vendor_id; > - uint8_t pci_bus; > - uint8_t pci_slot; > - uint8_t pci_func; > - uint32_t pci_flags; > - uint8_t pci_seg; > - uint32_t reserved3; > -} QEMU_PACKED; > -typedef struct AcpiSerialPortConsoleRedirection > - AcpiSerialPortConsoleRedirection; > - > /* > * ACPI 1.0 Firmware ACPI Control Structure (FACS) > */ > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index a07540affb..e8553dcae5 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -349,39 +349,53 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > acpi_table_composed(linker, &table); > } > > +/* > + * Serial Port Console Redirection Table (SPCR) > + * Rev: 1.07 > + */ was /* * Serial Port Console Redirection Table (SPCR), Rev. 1.02 */ By the way https://uefi.org/acpi/specs does not list 1.02 nor 1.07 https://docs.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table ? > static void > build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > { > - AcpiSerialPortConsoleRedirection *spcr; > - const MemMapEntry *uart_memmap = &vms->memmap[VIRT_UART]; > - int irq = vms->irqmap[VIRT_UART] + ARM_SPI_BASE; > - int spcr_start = table_data->len; > - > - spcr = acpi_data_push(table_data, sizeof(*spcr)); > - > - spcr->interface_type = 0x3; /* ARM PL011 UART */ > - > - spcr->base_address.space_id = AML_AS_SYSTEM_MEMORY; > - spcr->base_address.bit_width = 8; > - spcr->base_address.bit_offset = 0; > - spcr->base_address.access_width = 1; > - spcr->base_address.address = cpu_to_le64(uart_memmap->base); > - > - spcr->interrupt_types = (1 << 3); /* Bit[3] ARMH GIC interrupt */ > - spcr->gsi = cpu_to_le32(irq); /* Global System Interrupt */ > + AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = vms->oem_id, > + .oem_table_id = vms->oem_table_id }; > > - spcr->baud = 3; /* Baud Rate: 3 = 9600 */ > - spcr->parity = 0; /* No Parity */ > - spcr->stopbits = 1; /* 1 Stop bit */ > - spcr->flowctrl = (1 << 1); /* Bit[1] = RTS/CTS hardware flow control */ > - spcr->term_type = 0; /* Terminal Type: 0 = VT100 */ > + acpi_init_table(&table, table_data); > > - spcr->pci_device_id = 0xffff; /* PCI Device ID: not a PCI device */ > - spcr->pci_vendor_id = 0xffff; /* PCI Vendor ID: not a PCI device */ > + /* Interface Type */ > + build_append_int_noprefix(table_data, 3, 1); /* ARM PL011 UART */ > + build_append_int_noprefix(table_data, 0, 3); /* Reserved */ > + /* Base Address */ > + build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 8, 0, 1, > + vms->memmap[VIRT_UART].base); > + /* Interrupt Type */ > + build_append_int_noprefix(table_data, > + (1 << 3) /* Bit[3] ARMH GIC interrupt */, 1); > + build_append_int_noprefix(table_data, 0, 1); /* IRQ */ > + /* Global System Interrupt */ > + build_append_int_noprefix(table_data, > + vms->irqmap[VIRT_UART] + ARM_SPI_BASE, 4); > + build_append_int_noprefix(table_data, 3 /* 9600 */, 1); /* Baud Rate */ > + build_append_int_noprefix(table_data, 0 /* No Parity */, 1); /* Parity */ > + /* Stop Bits */ > + build_append_int_noprefix(table_data, 1 /* 1 Stop bit */, 1); > + /* Flow Control */ > + build_append_int_noprefix(table_data, > + (1 << 1) /* RTS/CTS hardware flow control */, 1); > + /* Terminal Type */ > + build_append_int_noprefix(table_data, 0 /* VT100 */, 1); > + build_append_int_noprefix(table_data, 0, 1); /* Language */ reserved2 -> language in 1.07? but that's fine maybe just mention it in the commit msg? > + /* PCI Device ID */ > + build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2); > + /* PCI Vendor ID */ > + build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2); > + build_append_int_noprefix(table_data, 0, 1); /* PCI Bus Number */ > + build_append_int_noprefix(table_data, 0, 1); /* PCI Device Number */ > + build_append_int_noprefix(table_data, 0, 1); /* PCI Function Number */ > + build_append_int_noprefix(table_data, 0, 4); /* PCI Flags */ > + build_append_int_noprefix(table_data, 0, 1); /* PCI Segment */ > + build_append_int_noprefix(table_data, 0, 4); /* Reserved */ > > - build_header(linker, table_data, (void *)(table_data->data + spcr_start), > - "SPCR", table_data->len - spcr_start, 2, vms->oem_id, > - vms->oem_table_id); > + acpi_table_composed(linker, &table); > } > > /* > Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric
On Thu, 2 Sep 2021 17:49:55 +0200 Eric Auger <eauger@redhat.com> wrote: > Hi Igor, > > On 7/8/21 5:46 PM, Igor Mammedov wrote: > > it replaces error-prone pointer arithmetic for build_header() API, > > with 2 calls to start and finish table creation, > > which hides offsets magic from API user. > > > > while at it, replace packed structure with endian agnostic > > build_append_FOO() API. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > CC: drjones@redhat.com > > CC: peter.maydell@linaro.org > > CC: shannon.zhaosl@gmail.com > > CC: qemu-arm@nongnu.org > > --- > > include/hw/acpi/acpi-defs.h | 32 ----------------- > > hw/arm/virt-acpi-build.c | 68 ++++++++++++++++++++++--------------- > > 2 files changed, 41 insertions(+), 59 deletions(-) > > > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > > index 6f2f08a9de..012c4ffb3a 100644 > > --- a/include/hw/acpi/acpi-defs.h > > +++ b/include/hw/acpi/acpi-defs.h > > @@ -117,38 +117,6 @@ typedef struct AcpiFadtData { > > #define ACPI_FADT_ARM_PSCI_COMPLIANT (1 << 0) > > #define ACPI_FADT_ARM_PSCI_USE_HVC (1 << 1) > > > > -/* > > - * Serial Port Console Redirection Table (SPCR), Rev. 1.02 > > - * > > - * For .interface_type see Debug Port Table 2 (DBG2) serial port > > - * subtypes in Table 3, Rev. May 22, 2012 > > - */ > > -struct AcpiSerialPortConsoleRedirection { > > - ACPI_TABLE_HEADER_DEF > > - uint8_t interface_type; > > - uint8_t reserved1[3]; > > - struct AcpiGenericAddress base_address; > > - uint8_t interrupt_types; > > - uint8_t irq; > > - uint32_t gsi; > > - uint8_t baud; > > - uint8_t parity; > > - uint8_t stopbits; > > - uint8_t flowctrl; > > - uint8_t term_type; > > - uint8_t reserved2; > > - uint16_t pci_device_id; > > - uint16_t pci_vendor_id; > > - uint8_t pci_bus; > > - uint8_t pci_slot; > > - uint8_t pci_func; > > - uint32_t pci_flags; > > - uint8_t pci_seg; > > - uint32_t reserved3; > > -} QEMU_PACKED; > > -typedef struct AcpiSerialPortConsoleRedirection > > - AcpiSerialPortConsoleRedirection; > > - > > /* > > * ACPI 1.0 Firmware ACPI Control Structure (FACS) > > */ > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > > index a07540affb..e8553dcae5 100644 > > --- a/hw/arm/virt-acpi-build.c > > +++ b/hw/arm/virt-acpi-build.c > > @@ -349,39 +349,53 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > > acpi_table_composed(linker, &table); > > } > > > > +/* > > + * Serial Port Console Redirection Table (SPCR) > > + * Rev: 1.07 > > + */ > was > /* > * Serial Port Console Redirection Table (SPCR), Rev. 1.02 > */ > > By the way > https://uefi.org/acpi/specs does not list 1.02 nor 1.07 > https://docs.microsoft.com/en-us/windows-hardware/drivers/serports/serial-port-console-redirection-table > ? That's problem with using MS 'specifications', which is a web page pointing to the current revision only. (I wasn't able to find 1.02 revision to use doc comments). So I had to use currently available, which is 1.07 currently, which seems to be binary compatible with our original 1.02 (that's why it passes bios-tables-test). > > > static void > > build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > > { > > - AcpiSerialPortConsoleRedirection *spcr; > > - const MemMapEntry *uart_memmap = &vms->memmap[VIRT_UART]; > > - int irq = vms->irqmap[VIRT_UART] + ARM_SPI_BASE; > > - int spcr_start = table_data->len; > > - > > - spcr = acpi_data_push(table_data, sizeof(*spcr)); > > - > > - spcr->interface_type = 0x3; /* ARM PL011 UART */ > > - > > - spcr->base_address.space_id = AML_AS_SYSTEM_MEMORY; > > - spcr->base_address.bit_width = 8; > > - spcr->base_address.bit_offset = 0; > > - spcr->base_address.access_width = 1; > > - spcr->base_address.address = cpu_to_le64(uart_memmap->base); > > - > > - spcr->interrupt_types = (1 << 3); /* Bit[3] ARMH GIC interrupt */ > > - spcr->gsi = cpu_to_le32(irq); /* Global System Interrupt */ > > + AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = vms->oem_id, > > + .oem_table_id = vms->oem_table_id }; > > > > - spcr->baud = 3; /* Baud Rate: 3 = 9600 */ > > - spcr->parity = 0; /* No Parity */ > > - spcr->stopbits = 1; /* 1 Stop bit */ > > - spcr->flowctrl = (1 << 1); /* Bit[1] = RTS/CTS hardware flow control */ > > - spcr->term_type = 0; /* Terminal Type: 0 = VT100 */ > > + acpi_init_table(&table, table_data); > > > > - spcr->pci_device_id = 0xffff; /* PCI Device ID: not a PCI device */ > > - spcr->pci_vendor_id = 0xffff; /* PCI Vendor ID: not a PCI device */ > > + /* Interface Type */ > > + build_append_int_noprefix(table_data, 3, 1); /* ARM PL011 UART */ > > + build_append_int_noprefix(table_data, 0, 3); /* Reserved */ > > + /* Base Address */ > > + build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 8, 0, 1, > > + vms->memmap[VIRT_UART].base); > > + /* Interrupt Type */ > > + build_append_int_noprefix(table_data, > > + (1 << 3) /* Bit[3] ARMH GIC interrupt */, 1); > > + build_append_int_noprefix(table_data, 0, 1); /* IRQ */ > > + /* Global System Interrupt */ > > + build_append_int_noprefix(table_data, > > + vms->irqmap[VIRT_UART] + ARM_SPI_BASE, 4); > > + build_append_int_noprefix(table_data, 3 /* 9600 */, 1); /* Baud Rate */ > > + build_append_int_noprefix(table_data, 0 /* No Parity */, 1); /* Parity */ > > + /* Stop Bits */ > > + build_append_int_noprefix(table_data, 1 /* 1 Stop bit */, 1); > > + /* Flow Control */ > > + build_append_int_noprefix(table_data, > > + (1 << 1) /* RTS/CTS hardware flow control */, 1); > > + /* Terminal Type */ > > + build_append_int_noprefix(table_data, 0 /* VT100 */, 1); > > + build_append_int_noprefix(table_data, 0, 1); /* Language */ > reserved2 -> language in 1.07? but that's fine > maybe just mention it in the commit msg? sure, I'll add that into commit message. > > + /* PCI Device ID */ > > + build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2); > > + /* PCI Vendor ID */ > > + build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2); > > + build_append_int_noprefix(table_data, 0, 1); /* PCI Bus Number */ > > + build_append_int_noprefix(table_data, 0, 1); /* PCI Device Number */ > > + build_append_int_noprefix(table_data, 0, 1); /* PCI Function Number */ > > + build_append_int_noprefix(table_data, 0, 4); /* PCI Flags */ > > + build_append_int_noprefix(table_data, 0, 1); /* PCI Segment */ > > + build_append_int_noprefix(table_data, 0, 4); /* Reserved */ > > > > - build_header(linker, table_data, (void *)(table_data->data + spcr_start), > > - "SPCR", table_data->len - spcr_start, 2, vms->oem_id, > > - vms->oem_table_id); > > + acpi_table_composed(linker, &table); > > } > > > > /* > > > Reviewed-by: Eric Auger <eric.auger@redhat.com> > > Eric >
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h index 6f2f08a9de..012c4ffb3a 100644 --- a/include/hw/acpi/acpi-defs.h +++ b/include/hw/acpi/acpi-defs.h @@ -117,38 +117,6 @@ typedef struct AcpiFadtData { #define ACPI_FADT_ARM_PSCI_COMPLIANT (1 << 0) #define ACPI_FADT_ARM_PSCI_USE_HVC (1 << 1) -/* - * Serial Port Console Redirection Table (SPCR), Rev. 1.02 - * - * For .interface_type see Debug Port Table 2 (DBG2) serial port - * subtypes in Table 3, Rev. May 22, 2012 - */ -struct AcpiSerialPortConsoleRedirection { - ACPI_TABLE_HEADER_DEF - uint8_t interface_type; - uint8_t reserved1[3]; - struct AcpiGenericAddress base_address; - uint8_t interrupt_types; - uint8_t irq; - uint32_t gsi; - uint8_t baud; - uint8_t parity; - uint8_t stopbits; - uint8_t flowctrl; - uint8_t term_type; - uint8_t reserved2; - uint16_t pci_device_id; - uint16_t pci_vendor_id; - uint8_t pci_bus; - uint8_t pci_slot; - uint8_t pci_func; - uint32_t pci_flags; - uint8_t pci_seg; - uint32_t reserved3; -} QEMU_PACKED; -typedef struct AcpiSerialPortConsoleRedirection - AcpiSerialPortConsoleRedirection; - /* * ACPI 1.0 Firmware ACPI Control Structure (FACS) */ diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index a07540affb..e8553dcae5 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -349,39 +349,53 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) acpi_table_composed(linker, &table); } +/* + * Serial Port Console Redirection Table (SPCR) + * Rev: 1.07 + */ static void build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) { - AcpiSerialPortConsoleRedirection *spcr; - const MemMapEntry *uart_memmap = &vms->memmap[VIRT_UART]; - int irq = vms->irqmap[VIRT_UART] + ARM_SPI_BASE; - int spcr_start = table_data->len; - - spcr = acpi_data_push(table_data, sizeof(*spcr)); - - spcr->interface_type = 0x3; /* ARM PL011 UART */ - - spcr->base_address.space_id = AML_AS_SYSTEM_MEMORY; - spcr->base_address.bit_width = 8; - spcr->base_address.bit_offset = 0; - spcr->base_address.access_width = 1; - spcr->base_address.address = cpu_to_le64(uart_memmap->base); - - spcr->interrupt_types = (1 << 3); /* Bit[3] ARMH GIC interrupt */ - spcr->gsi = cpu_to_le32(irq); /* Global System Interrupt */ + AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = vms->oem_id, + .oem_table_id = vms->oem_table_id }; - spcr->baud = 3; /* Baud Rate: 3 = 9600 */ - spcr->parity = 0; /* No Parity */ - spcr->stopbits = 1; /* 1 Stop bit */ - spcr->flowctrl = (1 << 1); /* Bit[1] = RTS/CTS hardware flow control */ - spcr->term_type = 0; /* Terminal Type: 0 = VT100 */ + acpi_init_table(&table, table_data); - spcr->pci_device_id = 0xffff; /* PCI Device ID: not a PCI device */ - spcr->pci_vendor_id = 0xffff; /* PCI Vendor ID: not a PCI device */ + /* Interface Type */ + build_append_int_noprefix(table_data, 3, 1); /* ARM PL011 UART */ + build_append_int_noprefix(table_data, 0, 3); /* Reserved */ + /* Base Address */ + build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 8, 0, 1, + vms->memmap[VIRT_UART].base); + /* Interrupt Type */ + build_append_int_noprefix(table_data, + (1 << 3) /* Bit[3] ARMH GIC interrupt */, 1); + build_append_int_noprefix(table_data, 0, 1); /* IRQ */ + /* Global System Interrupt */ + build_append_int_noprefix(table_data, + vms->irqmap[VIRT_UART] + ARM_SPI_BASE, 4); + build_append_int_noprefix(table_data, 3 /* 9600 */, 1); /* Baud Rate */ + build_append_int_noprefix(table_data, 0 /* No Parity */, 1); /* Parity */ + /* Stop Bits */ + build_append_int_noprefix(table_data, 1 /* 1 Stop bit */, 1); + /* Flow Control */ + build_append_int_noprefix(table_data, + (1 << 1) /* RTS/CTS hardware flow control */, 1); + /* Terminal Type */ + build_append_int_noprefix(table_data, 0 /* VT100 */, 1); + build_append_int_noprefix(table_data, 0, 1); /* Language */ + /* PCI Device ID */ + build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2); + /* PCI Vendor ID */ + build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2); + build_append_int_noprefix(table_data, 0, 1); /* PCI Bus Number */ + build_append_int_noprefix(table_data, 0, 1); /* PCI Device Number */ + build_append_int_noprefix(table_data, 0, 1); /* PCI Function Number */ + build_append_int_noprefix(table_data, 0, 4); /* PCI Flags */ + build_append_int_noprefix(table_data, 0, 1); /* PCI Segment */ + build_append_int_noprefix(table_data, 0, 4); /* Reserved */ - build_header(linker, table_data, (void *)(table_data->data + spcr_start), - "SPCR", table_data->len - spcr_start, 2, vms->oem_id, - vms->oem_table_id); + acpi_table_composed(linker, &table); } /*
it replaces error-prone pointer arithmetic for build_header() API, with 2 calls to start and finish table creation, which hides offsets magic from API user. while at it, replace packed structure with endian agnostic build_append_FOO() API. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- CC: drjones@redhat.com CC: peter.maydell@linaro.org CC: shannon.zhaosl@gmail.com CC: qemu-arm@nongnu.org --- include/hw/acpi/acpi-defs.h | 32 ----------------- hw/arm/virt-acpi-build.c | 68 ++++++++++++++++++++++--------------- 2 files changed, 41 insertions(+), 59 deletions(-)