[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 31/35] acpi: arm/virt: build_spcr: use acpi_init_table()/a
|
From: |
Igor Mammedov |
|
Subject: |
Re: [PATCH v2 31/35] acpi: arm/virt: build_spcr: use acpi_init_table()/acpi_table_composed() instead of build_header() |
|
Date: |
Fri, 3 Sep 2021 09:52:52 +0200 |
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
>