| Message ID | 20231021143308.04f1a6fe@darkstar.speedport.ip (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PATCH: #1938: [ARM/PL011] Wrong UART register spacing reported in DBG2/SPCR | expand |
On Sat, 21 Oct 2023 at 13:33, Udo Steinberg <udo@hypervisor.org> wrote: > > > From: Udo Steinberg <udo@hypervisor.org> > Date: Sat, 21 Oct 2023 14:10:30 +0200 > Subject: [PATCH] hw/arm/virt: Report correct register sizes in ACPI DBG2/SPCR > tables. > > Documentation for using the GAS in ACPI tables to report debug UART addresses at > https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/acpi-debug-port-table > states the following: > > - The Register Bit Width field contains the register stride and must be a > power of 2 that is at least as large as the access size. On 32-bit > platforms this value cannot exceed 32. On 64-bit platforms this value > cannot exceed 64. > - The Access Size field is used to determine whether byte, WORD, DWORD, or > QWORD accesses are to be used. QWORD accesses are only valid on 64-bit > architectures. > > Documentation for the ARM PL011 at > https://developer.arm.com/documentation/ddi0183/latest/ > states that the registers are: > > - spaced 4 bytes apart (see Table 3-2), so register stride must be 32. > - 16 bits in size (see individual registers), so access size must be 2. > > The PL011 documentation does not mention whether 8-bit accesses or 32-bit > accesses to the registers are also allowed. Because a standard PL011 (not > the SBSA version) is connected via a 16-bit bus using PWDATA[15:0] and > PRDATA[15:0] (see Figure 2-1), using 16-bit access is the safest choice. For the hardware, Linux kernel commit 84c3e03bdd1146 does suggest that 16-bit is the traditional access size (the 32-bit access support added in that commit is for a not-very-pl011-like UART which has the registers in a different order, incidentally). As far as the PL011 itself is concerned, it doesn't implement the APB bus PSTRB signal, so it reads/writes all bits of its data bus every time. The interconnect/ bus fabric gets a say in what happens but it would be pretty unlikely for it to go out of its way to deny 32 bit accesses I think (it would have to hardcode that this particular APB peripheral had a 16 bit width and then deny accesses based on access width on its upstream bus). Anyway, 16-bit access should be OK. (In practice I see the Linux driver doing both 8-bit and 32-bit accesses, depending on the register: our impl handles all sizes, converting anything that's not 32-bit to 32-bit in the memory subsystem code.) > For SBSA-compatible UARTs the DBG2/SPCR table should report a different > subtype (0xd or 0xe) instead of 0x3. There is an annoying dance that has to be done with the test suite when we do ACPI table updates, because otherwise "make check" fails because the new ACPI table doesn't match its golden reference file. I had to figure that out recently so I'll do that for you (will involve me resending this patch with some extras before/after that update the test suite). > Fixes #1938. We generally note this with the syntax: Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1938 but I'll fix that up when I take this patch upstream (and add a note about what guest software ran into the bug). > Signed-off-by: Udo Steinberg <udo@hypervisor.org> > --- > hw/arm/virt-acpi-build.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 9ce136cd88..91ed7fc94a 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -482,7 +482,7 @@ build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > 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, > + build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 32, 0, 2, > vms->memmap[VIRT_UART].base); > /* Interrupt Type */ > build_append_int_noprefix(table_data, > @@ -673,7 +673,7 @@ build_dbg2(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > build_append_int_noprefix(table_data, 34, 2); > > /* BaseAddressRegister[] */ > - build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 8, 0, 1, > + build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 32, 0, 2, > vms->memmap[VIRT_UART].base); > > /* AddressSize[] */ Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 9ce136cd88..91ed7fc94a 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -482,7 +482,7 @@ build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) 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, + build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 32, 0, 2, vms->memmap[VIRT_UART].base); /* Interrupt Type */ build_append_int_noprefix(table_data, @@ -673,7 +673,7 @@ build_dbg2(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) build_append_int_noprefix(table_data, 34, 2); /* BaseAddressRegister[] */ - build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 8, 0, 1, + build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 32, 0, 2, vms->memmap[VIRT_UART].base); /* AddressSize[] */