[PATCH v3 0/3] virt: Report UART correctly in ACPI DBG2/SPCR

[PATCH v3 0/3] virt: Report UART correctly in ACPI DBG2/SPCR

[Peter Maydell]

This patchseries is Udo's patch, plus the necessary extra patches
that update the golden-reference files for bios-tables-test so that
'make check' continues to pass.

Changes v2->v3:
 * report the UART as requiring 32-bit accesses, not 16-bit;
   it turns out that Linux has a bug where it fails to enable
   the console if the ACPI table reports 16-bit access width
 * ACPI table binaries regenerated to match that

thanks
-- PMM

Peter Maydell (2):
  tests/qtest/bios-tables-test: Allow changes to virt SPCR and DBG2
  tests/qtest/bios-tables-test: Update virt SPCR and DBG2 golden
    references

Udo Steinberg (1):
  hw/arm/virt: Report correct register sizes in ACPI DBG2/SPCR tables.

 hw/arm/virt-acpi-build.c  |   4 ++--
 tests/data/acpi/virt/DBG2 | Bin 87 -> 87 bytes
 tests/data/acpi/virt/SPCR | Bin 80 -> 80 bytes
 3 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v3 1/3] tests/qtest/bios-tables-test: Allow changes to virt SPCR and DBG2

[Peter Maydell]

Allow changes to the virt board SPCR and DBG2 -- we are going to fix
an error in the UART descriptions there.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/bios-tables-test-allowed-diff.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8bf..6673e2c4c13 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,3 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/virt/SPCR",
+"tests/data/acpi/virt/DBG2",
-- 
2.34.1

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

[Peter Maydell]

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(-)

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[] */
-- 
2.34.1

[PATCH v3 3/3] tests/qtest/bios-tables-test: Update virt SPCR and DBG2 golden references

[Peter Maydell]

Update the virt SPCR and DBG2 golden reference files to have the
fix for the description of the UART.

Diffs from iasl:

@@ -1,57 +1,57 @@
 /*
  * Intel ACPI Component Architecture
  * AML/ASL+ Disassembler version 20200925 (64-bit version)
  * Copyright (c) 2000 - 2020 Intel Corporation
  *
- * Disassembly of tests/data/acpi/virt/SPCR, Fri Nov  3 14:12:06 2023
+ * Disassembly of /tmp/aml-E6YUD2, Fri Nov  3 14:12:06 2023
  *
  * ACPI Data Table [SPCR]
  *
  * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue
  */

 [000h 0000   4]                    Signature : "SPCR"    [Serial Port Console Redirection table]
 [004h 0004   4]                 Table Length : 00000050
 [008h 0008   1]                     Revision : 02
-[009h 0009   1]                     Checksum : CB
+[009h 0009   1]                     Checksum : B1
 [00Ah 0010   6]                       Oem ID : "BOCHS "
 [010h 0016   8]                 Oem Table ID : "BXPC    "
 [018h 0024   4]                 Oem Revision : 00000001
 [01Ch 0028   4]              Asl Compiler ID : "BXPC"
 [020h 0032   4]        Asl Compiler Revision : 00000001

 [024h 0036   1]               Interface Type : 03
 [025h 0037   3]                     Reserved : 000000

 [028h 0040  12]         Serial Port Register : [Generic Address Structure]
 [028h 0040   1]                     Space ID : 00 [SystemMemory]
-[029h 0041   1]                    Bit Width : 08
+[029h 0041   1]                    Bit Width : 20
 [02Ah 0042   1]                   Bit Offset : 00
-[02Bh 0043   1]         Encoded Access Width : 01 [Byte Access:8]
+[02Bh 0043   1]         Encoded Access Width : 03 [DWord Access:32]
 [02Ch 0044   8]                      Address : 0000000009000000

 [034h 0052   1]               Interrupt Type : 08
 [035h 0053   1]          PCAT-compatible IRQ : 00
 [036h 0054   4]                    Interrupt : 00000021
 [03Ah 0058   1]                    Baud Rate : 03
 [03Bh 0059   1]                       Parity : 00
 [03Ch 0060   1]                    Stop Bits : 01
 [03Dh 0061   1]                 Flow Control : 02
 [03Eh 0062   1]                Terminal Type : 00
 [04Ch 0076   1]                     Reserved : 00
 [040h 0064   2]                PCI Device ID : FFFF
 [042h 0066   2]                PCI Vendor ID : FFFF
 [044h 0068   1]                      PCI Bus : 00
 [045h 0069   1]                   PCI Device : 00
 [046h 0070   1]                 PCI Function : 00
 [047h 0071   4]                    PCI Flags : 00000000
 [04Bh 0075   1]                  PCI Segment : 00
 [04Ch 0076   4]                     Reserved : 00000000

 Raw Table Data: Length 80 (0x50)

-    0000: 53 50 43 52 50 00 00 00 02 CB 42 4F 43 48 53 20  // SPCRP.....BOCHS
+    0000: 53 50 43 52 50 00 00 00 02 B1 42 4F 43 48 53 20  // SPCRP.....BOCHS
     0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43  // BXPC    ....BXPC
-    0020: 01 00 00 00 03 00 00 00 00 08 00 01 00 00 00 09  // ................
+    0020: 01 00 00 00 03 00 00 00 00 20 00 03 00 00 00 09  // ......... ......
     0030: 00 00 00 00 08 00 21 00 00 00 03 00 01 02 00 00  // ......!.........
     0040: FF FF FF FF 00 00 00 00 00 00 00 00 00 00 00 00  // ................

@@ -1,57 +1,57 @@
 /*
  * Intel ACPI Component Architecture
  * AML/ASL+ Disassembler version 20200925 (64-bit version)
  * Copyright (c) 2000 - 2020 Intel Corporation
  *
- * Disassembly of tests/data/acpi/virt/DBG2, Fri Nov  3 14:12:06 2023
+ * Disassembly of /tmp/aml-V1YUD2, Fri Nov  3 14:12:06 2023
  *
  * ACPI Data Table [DBG2]
  *
  * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue
  */

 [000h 0000   4]                    Signature : "DBG2"    [Debug Port table type 2]
 [004h 0004   4]                 Table Length : 00000057
 [008h 0008   1]                     Revision : 00
-[009h 0009   1]                     Checksum : CF
+[009h 0009   1]                     Checksum : B5
 [00Ah 0010   6]                       Oem ID : "BOCHS "
 [010h 0016   8]                 Oem Table ID : "BXPC    "
 [018h 0024   4]                 Oem Revision : 00000001
 [01Ch 0028   4]              Asl Compiler ID : "BXPC"
 [020h 0032   4]        Asl Compiler Revision : 00000001

 [024h 0036   4]                  Info Offset : 0000002C
 [028h 0040   4]                   Info Count : 00000001

 [02Ch 0044   1]                     Revision : 00
 [02Dh 0045   2]                       Length : 002B
 [02Fh 0047   1]               Register Count : 01
 [030h 0048   2]              Namepath Length : 0005
 [032h 0050   2]              Namepath Offset : 0026
 [034h 0052   2]              OEM Data Length : 0000 [Optional field not present]
 [036h 0054   2]              OEM Data Offset : 0000 [Optional field not present]
 [038h 0056   2]                    Port Type : 8000
 [03Ah 0058   2]                 Port Subtype : 0003
 [03Ch 0060   2]                     Reserved : 0000
 [03Eh 0062   2]          Base Address Offset : 0016
 [040h 0064   2]          Address Size Offset : 0022

 [042h 0066  12]        Base Address Register : [Generic Address Structure]
 [042h 0066   1]                     Space ID : 00 [SystemMemory]
-[043h 0067   1]                    Bit Width : 08
+[043h 0067   1]                    Bit Width : 20
 [044h 0068   1]                   Bit Offset : 00
-[045h 0069   1]         Encoded Access Width : 01 [Byte Access:8]
+[045h 0069   1]         Encoded Access Width : 03 [DWord Access:32]
 [046h 0070   8]                      Address : 0000000009000000

 [04Eh 0078   4]                 Address Size : 00001000

 [052h 0082   5]                     Namepath : "COM0"

 Raw Table Data: Length 87 (0x57)

-    0000: 44 42 47 32 57 00 00 00 00 CF 42 4F 43 48 53 20  // DBG2W.....BOCHS
+    0000: 44 42 47 32 57 00 00 00 00 B5 42 4F 43 48 53 20  // DBG2W.....BOCHS
     0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43  // BXPC    ....BXPC
     0020: 01 00 00 00 2C 00 00 00 01 00 00 00 00 2B 00 01  // ....,........+..
     0030: 05 00 26 00 00 00 00 00 00 80 03 00 00 00 16 00  // ..&.............
-    0040: 22 00 00 08 00 01 00 00 00 09 00 00 00 00 00 10  // "...............
+    0040: 22 00 00 20 00 03 00 00 00 09 00 00 00 00 00 10  // ".. ............
     0050: 00 00 43 4F 4D 30 00                             // ..COM0.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/bios-tables-test-allowed-diff.h |   2 --
 tests/data/acpi/virt/DBG2                   | Bin 87 -> 87 bytes
 tests/data/acpi/virt/SPCR                   | Bin 80 -> 80 bytes
 3 files changed, 2 deletions(-)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 6673e2c4c13..dfb8523c8bf 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,3 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/virt/SPCR",
-"tests/data/acpi/virt/DBG2",
diff --git a/tests/data/acpi/virt/DBG2 b/tests/data/acpi/virt/DBG2
index 86e6314f7b0235ef8ed3e0221e09f996c41f5e98..0a05e1a47f9c303c6a6c9ca8414c62ec4ac90f98 100644
GIT binary patch
delta 37
ncmWF!=W=m!HwtF}f~^y|EJYL;n1M`A5T8MSfx+3|*MI>4b2kL{

delta 37
ncmWF!=W=m!HwtF}g7Xu(EJZjN7=cVq5T8MSfx+3|*MI>4bG-!j

diff --git a/tests/data/acpi/virt/SPCR b/tests/data/acpi/virt/SPCR
index 24e0a579e7d73f432a614380e29aa95113344186..cf0f2b75226515097c08d2e2016a83a4f08812ba 100644
GIT binary patch
delta 23
ecmWFt;0g|K4hmpkU|`xfkxQOgfq{9VjtT%gOa!L@

delta 23
ecmWFt;0g|K4hmpkU|>2ukxQPLgMo3PjtT%g(gddf

-- 
2.34.1

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

[Peter Maydell]

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

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

[Igor Mammedov]

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
> 

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

[Peter Maydell]

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

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

[Udo Steinberg]

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