diff mbox series

[v3,2/3] hw/arm/virt: Report correct register sizes in ACPI DBG2/SPCR tables.

Message ID 20231103152120.829962-3-peter.maydell@linaro.org (mailing list archive)
State New, archived
Headers show
Series virt: Report UART correctly in ACPI DBG2/SPCR | expand

Commit Message

Peter Maydell Nov. 3, 2023, 3:21 p.m. UTC 
From: Udo Steinberg <udo@hypervisor.org>

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 in some cases (see individual registers), so access
  size must be at least 2.

Linux doesn't seem to care about this error in the table, but it does
affect at least the NOVA microhypervisor.

In theory we therefore have a choice between reporting the access
size as 2 (16 bit accesses) or 3 (32-bit accesses).  In practice,
Linux does not correctly handle the case where the table reports the
access size as 2: as of kernel commit 750b95887e5678, the code in
acpi_parse_spcr() tries to tell the serial driver to use 16 bit
accesses by passing "mmio16" in the option string, but the PL011
driver code in pl011_console_match() only recognizes "mmio" or
"mmio32". The result is that unless the user has enabled 'earlycon'

We therefore choose to report the access size as 32 bits; this works
for NOVA and also for Linux.  It is also what the UEFI firmware on a
Raspberry Pi 4 reports, so we're in line with existing real-world
practice.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1938
Signed-off-by: Udo Steinberg <udo@hypervisor.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
[PMM: minor commit message tweaks; use 32 bit accesses]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt-acpi-build.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Peter Maydell Nov. 3, 2023, 3:26 p.m. UTC | #1 
On Fri, 3 Nov 2023 at 15:21, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> From: Udo Steinberg <udo@hypervisor.org>
>
> 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 in some cases (see individual registers), so access
>   size must be at least 2.
>
> Linux doesn't seem to care about this error in the table, but it does
> affect at least the NOVA microhypervisor.
>
> In theory we therefore have a choice between reporting the access
> size as 2 (16 bit accesses) or 3 (32-bit accesses).  In practice,
> Linux does not correctly handle the case where the table reports the
> access size as 2: as of kernel commit 750b95887e5678, the code in
> acpi_parse_spcr() tries to tell the serial driver to use 16 bit
> accesses by passing "mmio16" in the option string, but the PL011
> driver code in pl011_console_match() only recognizes "mmio" or
> "mmio32". The result is that unless the user has enabled 'earlycon'
>
Oops, a line seems to have got deleted here -- should continue

"there is no console output from the guest kernel."

> We therefore choose to report the access size as 32 bits; this works
> for NOVA and also for Linux.  It is also what the UEFI firmware on a
> Raspberry Pi 4 reports, so we're in line with existing real-world
> practice.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1938
> Signed-off-by: Udo Steinberg <udo@hypervisor.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> [PMM: minor commit message tweaks; use 32 bit accesses]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> --

thanks
-- PMM
Igor Mammedov Nov. 6, 2023, 2:05 p.m. UTC | #2 
On Fri, 3 Nov 2023 15:26:22 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Fri, 3 Nov 2023 at 15:21, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > From: Udo Steinberg <udo@hypervisor.org>
> >
> > 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 in some cases (see individual registers), so access
> >   size must be at least 2.

it might be worth mentioning that QEMU impl. uses 32 bit registers and
can correctly handle 32 bit access only, while 16 (or any other) bit access
to 32 bit registers won't actually work.  

ex:
pl011_write()
   ...
   switch (offset >> 2)

essentially only 1st byte will be accessed correctly,
the rest will be misplaced as read/write handlers do not account
for split access possibility.

So it's not about what linux or NOVA do, but rather fixing
ACPI description to match what the device model is capable of.

> >
> > Linux doesn't seem to care about this error in the table, but it does
> > affect at least the NOVA microhypervisor.
> >
> > In theory we therefore have a choice between reporting the access
> > size as 2 (16 bit accesses) or 3 (32-bit accesses).  In practice,
> > Linux does not correctly handle the case where the table reports the
> > access size as 2: as of kernel commit 750b95887e5678, the code in
> > acpi_parse_spcr() tries to tell the serial driver to use 16 bit
> > accesses by passing "mmio16" in the option string, but the PL011
> > driver code in pl011_console_match() only recognizes "mmio" or
> > "mmio32". The result is that unless the user has enabled 'earlycon'
> >  
> Oops, a line seems to have got deleted here -- should continue
> 
> "there is no console output from the guest kernel."
> 
> > We therefore choose to report the access size as 32 bits; this works
> > for NOVA and also for Linux.  It is also what the UEFI firmware on a
> > Raspberry Pi 4 reports, so we're in line with existing real-world
> > practice.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1938
> > Signed-off-by: Udo Steinberg <udo@hypervisor.org>
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > [PMM: minor commit message tweaks; use 32 bit accesses]
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > --  
> 
> thanks
> -- PMM
>
Peter Maydell Nov. 6, 2023, 2:38 p.m. UTC | #3 
On Mon, 6 Nov 2023 at 14:05, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Fri, 3 Nov 2023 15:26:22 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
> > On Fri, 3 Nov 2023 at 15:21, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > From: Udo Steinberg <udo@hypervisor.org>
> > >
> > > 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 in some cases (see individual registers), so access
> > >   size must be at least 2.
>
> it might be worth mentioning that QEMU impl. uses 32 bit registers and
> can correctly handle 32 bit access only, while 16 (or any other) bit access
> to 32 bit registers won't actually work.
>
> ex:
> pl011_write()
>    ...
>    switch (offset >> 2)
>
> essentially only 1st byte will be accessed correctly,
> the rest will be misplaced as read/write handlers do not account
> for split access possibility.

No, 16 bit accesses should work OK -- we set our .impl.min_access_size
and .impl.max_access_size to 4, so the memory subsystem should
implement 8 and 16 bit accesses for us by converting them to
32 bit accesses.

thanks
-- PMM
Udo Steinberg Nov. 6, 2023, 2:42 p.m. UTC | #4 
On Mon, 6 Nov 2023 15:05:33 +0100 Igor Mammedov (IM) wrote:

> it might be worth mentioning that QEMU impl. uses 32 bit registers and
> can correctly handle 32 bit access only, while 16 (or any other) bit access
> to 32 bit registers won't actually work.  
> 
> ex:
> pl011_write()
>    ...
>    switch (offset >> 2)
> 
> essentially only 1st byte will be accessed correctly,
> the rest will be misplaced as read/write handlers do not account
> for split access possibility.
> 
> So it's not about what linux or NOVA do, but rather fixing
> ACPI description to match what the device model is capable of.

The latest version of Peter's patch series advertises 32-bit access width,
so that should not derail the current PL011 device model anymore.

Independent of that, the PL011 device model should be fixed to also support
8-bit and 16-bit accesses, because the PL011 data sheet says 16-bit and the
SBSA says 8-, 16- and 32-bit accesses are valid, so if the model cannot
handle 8 and 16, it is incomplete/buggy.

Cheers,
Udo
diff mbox series

Patch

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9ce136cd88c..8bc35a483c9 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, 3,
                      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, 3,
                      vms->memmap[VIRT_UART].base);
 
     /* AddressSize[] */