[3/3] hw/arm/virt-acpi-build: Add second UART to ACPI tables

Message ID 1512745328-5109-4-git-send-email-peter.maydell@linaro.org
State New
Headers show
Series
  • hw/arm/virt: Add another UART
Related show

Commit Message

Peter Maydell Dec. 8, 2017, 3:02 p.m.
Add the second UART to the ACPI tables.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
Pure guesswork, as I don't have a UEFI setup to hand and
am not familiar with ACPI table formats either...
---
 hw/arm/virt-acpi-build.c | 5 +++++
 1 file changed, 5 insertions(+)

-- 
2.7.4

Comments

Shannon Zhao Dec. 12, 2017, 5:53 a.m. | #1
On 2017/12/8 23:02, Peter Maydell wrote:
> Add the second UART to the ACPI tables.

> 

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

> Pure guesswork, as I don't have a UEFI setup to hand and

> am not familiar with ACPI table formats either...

> ---

>  hw/arm/virt-acpi-build.c | 5 +++++

>  1 file changed, 5 insertions(+)

> 

> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c

> index 3d78ff6..a38287b 100644

> --- a/hw/arm/virt-acpi-build.c

> +++ b/hw/arm/virt-acpi-build.c

> @@ -689,6 +689,7 @@ static void build_fadt(GArray *table_data, BIOSLinker *linker,

>  static void

>  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)

>  {

> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);

>      Aml *scope, *dsdt;

>      const MemMapEntry *memmap = vms->memmap;

>      const int *irqmap = vms->irqmap;

> @@ -706,6 +707,10 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)

>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);

>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],

>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));

> +    if (!vmc->no_second_uart) {

> +        acpi_dsdt_add_uart(scope, &memmap[VIRT_UART_2],

> +                           (irqmap[VIRT_UART_2] + ARM_SPI_BASE));

> +    }

>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);

>      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);

>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],

> 

Reviewed-by: Shannon Zhao <zhaoshenglong@huawei.com>


-- 
Shannon
Laszlo Ersek Dec. 12, 2017, 11:06 a.m. | #2
On 12/12/17 06:53, Shannon Zhao wrote:
> 

> 

> On 2017/12/8 23:02, Peter Maydell wrote:

>> Add the second UART to the ACPI tables.

>>

>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

>> ---

>> Pure guesswork, as I don't have a UEFI setup to hand and

>> am not familiar with ACPI table formats either...

>> ---

>>  hw/arm/virt-acpi-build.c | 5 +++++

>>  1 file changed, 5 insertions(+)

>>

>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c

>> index 3d78ff6..a38287b 100644

>> --- a/hw/arm/virt-acpi-build.c

>> +++ b/hw/arm/virt-acpi-build.c

>> @@ -689,6 +689,7 @@ static void build_fadt(GArray *table_data, BIOSLinker *linker,

>>  static void

>>  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)

>>  {

>> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);

>>      Aml *scope, *dsdt;

>>      const MemMapEntry *memmap = vms->memmap;

>>      const int *irqmap = vms->irqmap;

>> @@ -706,6 +707,10 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)

>>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);

>>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],

>>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));

>> +    if (!vmc->no_second_uart) {

>> +        acpi_dsdt_add_uart(scope, &memmap[VIRT_UART_2],

>> +                           (irqmap[VIRT_UART_2] + ARM_SPI_BASE));

>> +    }

>>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);

>>      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);

>>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],

>>

> Reviewed-by: Shannon Zhao <zhaoshenglong@huawei.com>

> 


(Responding here so I don't have to manually update Shannon's email
address while replying to patch 3/3 directly.)

acpi_dsdt_add_uart() does:

    Aml *dev = aml_device("COM0");
    aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0011")));
    aml_append(dev, aml_name_decl("_UID", aml_int(0)));

The device name should be unique. Plus, within the same _HID, the _UID
should be unique as well. I suggest adding another unsigned parameter to
acpi_dsdt_add_uart(), and hooking it into COMx and _UID.


BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I
can see from the firmware code, the firmware will use the PL011 whose
description comes first in the DTB (and ignore the other PL011), in an
fdt_next_node() traversal. Is that OK for the intended use case?
(Perhaps I should have asked this under the second patch, which updates
the DTB generator.)

Thanks,
Laszlo
Peter Maydell Dec. 12, 2017, 11:11 a.m. | #3
On 12 December 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:
> BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I

> can see from the firmware code, the firmware will use the PL011 whose

> description comes first in the DTB (and ignore the other PL011), in an

> fdt_next_node() traversal. Is that OK for the intended use case?

> (Perhaps I should have asked this under the second patch, which updates

> the DTB generator.)


I haven't tested, since I don't have a working setup for that to hand.
(Do you have instructions somewhere for getting it working?)

The behaviour we would want would be for the firmware to keep using
the PL011 at 0x09000000. (In an ideal world the firmware would
prefer the UART marked in the 'stdout-path' in the DTB /chosen node,
as the kernel does, I guess.)

thanks
-- PMM
Laszlo Ersek Dec. 12, 2017, 11:33 a.m. | #4
On 12/12/17 12:11, Peter Maydell wrote:
> On 12 December 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:

>> BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I

>> can see from the firmware code, the firmware will use the PL011 whose

>> description comes first in the DTB (and ignore the other PL011), in an

>> fdt_next_node() traversal. Is that OK for the intended use case?

>> (Perhaps I should have asked this under the second patch, which updates

>> the DTB generator.)

> 

> I haven't tested, since I don't have a working setup for that to hand.

> (Do you have instructions somewhere for getting it working?)


The Wiki page I most frequently refer to (for a libvirt-less description
anyway) is Ard's:

  https://wiki.linaro.org/LEG/UEFIforQEMU

There's also:

  https://www.kraxel.org/repos/
  https://fedoraproject.org/wiki/Architectures/AArch64/Install_with_QEMU

> The behaviour we would want would be for the firmware to keep using

> the PL011 at 0x09000000.


With these QEMU patches, I reckon that's going to happen, yes.

> (In an ideal world the firmware would

> prefer the UART marked in the 'stdout-path' in the DTB /chosen node,

> as the kernel does, I guess.)


Hmmm, I recall that we used to have some code related to the /chosen
node... We have a helper function for locating that
(GetOrInsertChosenNode), but we no longer use it, it seems? The last
(only?) use was apparently removed in:

https://github.com/tianocore/edk2/commit/29589acf1010

I'll let Ard comment too.

Thanks
Laszlo
Ard Biesheuvel Dec. 12, 2017, 11:44 a.m. | #5
On 12 December 2017 at 11:33, Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/12/17 12:11, Peter Maydell wrote:

>> On 12 December 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:

>>> BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I

>>> can see from the firmware code, the firmware will use the PL011 whose

>>> description comes first in the DTB (and ignore the other PL011), in an

>>> fdt_next_node() traversal. Is that OK for the intended use case?

>>> (Perhaps I should have asked this under the second patch, which updates

>>> the DTB generator.)

>>

>> I haven't tested, since I don't have a working setup for that to hand.

>> (Do you have instructions somewhere for getting it working?)

>

> The Wiki page I most frequently refer to (for a libvirt-less description

> anyway) is Ard's:

>

>   https://wiki.linaro.org/LEG/UEFIforQEMU

>

> There's also:

>

>   https://www.kraxel.org/repos/

>   https://fedoraproject.org/wiki/Architectures/AArch64/Install_with_QEMU

>

>> The behaviour we would want would be for the firmware to keep using

>> the PL011 at 0x09000000.

>

> With these QEMU patches, I reckon that's going to happen, yes.

>


IIRC the DT nodes appear in the opposite order as they are added, so
adding the second UART after adding the original one is probably going
to cause trouble.

We do check the 'status' though, so if backward compatibility with
older firmwares is a concern, we could set status=disabled, and let
newer firmware builds enable it on the fly.

>> (In an ideal world the firmware would

>> prefer the UART marked in the 'stdout-path' in the DTB /chosen node,

>> as the kernel does, I guess.)

>

> Hmmm, I recall that we used to have some code related to the /chosen

> node... We have a helper function for locating that

> (GetOrInsertChosenNode), but we no longer use it, it seems? The last

> (only?) use was apparently removed in:

>

> https://github.com/tianocore/edk2/commit/29589acf1010

>

> I'll let Ard comment too.

>


That would require us to implement some non-trivial parsing and alias
resolution, given that stdout-path is simply a string that may refer
to a UART node via an alias.
Laszlo Ersek Dec. 12, 2017, 11:59 a.m. | #6
On 12/12/17 12:44, Ard Biesheuvel wrote:
> On 12 December 2017 at 11:33, Laszlo Ersek <lersek@redhat.com> wrote:

>> On 12/12/17 12:11, Peter Maydell wrote:

>>> On 12 December 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:

>>>> BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I

>>>> can see from the firmware code, the firmware will use the PL011 whose

>>>> description comes first in the DTB (and ignore the other PL011), in an

>>>> fdt_next_node() traversal. Is that OK for the intended use case?

>>>> (Perhaps I should have asked this under the second patch, which updates

>>>> the DTB generator.)

>>>

>>> I haven't tested, since I don't have a working setup for that to hand.

>>> (Do you have instructions somewhere for getting it working?)

>>

>> The Wiki page I most frequently refer to (for a libvirt-less description

>> anyway) is Ard's:

>>

>>   https://wiki.linaro.org/LEG/UEFIforQEMU

>>

>> There's also:

>>

>>   https://www.kraxel.org/repos/

>>   https://fedoraproject.org/wiki/Architectures/AArch64/Install_with_QEMU

>>

>>> The behaviour we would want would be for the firmware to keep using

>>> the PL011 at 0x09000000.

>>

>> With these QEMU patches, I reckon that's going to happen, yes.

>>

> 

> IIRC the DT nodes appear in the opposite order as they are added,


Oh! Thanks for catching that!

> so

> adding the second UART after adding the original one is probably going

> to cause trouble.

> 

> We do check the 'status' though, so if backward compatibility with

> older firmwares is a concern, we could set status=disabled, and let

> newer firmware builds enable it on the fly.


Peter, what is the intended use of the second UART?

(In the firmware, a second PL011 could be useful: it might make it
possible to separate the debug output (debug messages) from interactive
serial console IO. (On x86, in the default OVMF build, debug messages
are written to the QEMU debug IO port, and the serial port is used only
for interactive console IO.) I don't remember how important we deemed
this separation for aarch64, last time it surfaced.)

>>> (In an ideal world the firmware would

>>> prefer the UART marked in the 'stdout-path' in the DTB /chosen node,

>>> as the kernel does, I guess.)

>>

>> Hmmm, I recall that we used to have some code related to the /chosen

>> node... We have a helper function for locating that

>> (GetOrInsertChosenNode), but we no longer use it, it seems? The last

>> (only?) use was apparently removed in:

>>

>> https://github.com/tianocore/edk2/commit/29589acf1010

>>

>> I'll let Ard comment too.

>>

> 

> That would require us to implement some non-trivial parsing and alias

> resolution, given that stdout-path is simply a string that may refer

> to a UART node via an alias.


Let's not do that please...

Thanks
Laszlo
Peter Maydell Dec. 12, 2017, 12:07 p.m. | #7
On 12 December 2017 at 11:59, Laszlo Ersek <lersek@redhat.com> wrote:
> Peter, what is the intended use of the second UART?


Per the commit message:
# there are some
# use cases where having a second UART would be useful (like
# bare-metal testing where you don't really want to have to
# probe and set up a PCI device just to have a second comms
# channel).

From QEMU's point of view this is just "some people would like
more than one UART, x86 provides more than one UART, it's
easy to provide an extra UART and it's up to the guest what
it would like to do with it". If there's utility for the
OVMF firmware in having 2 UARTs that's doubly good reason
to have it.

thanks
-- PMM
Laszlo Ersek Dec. 12, 2017, 12:41 p.m. | #8
On 12/12/17 13:07, Peter Maydell wrote:
> On 12 December 2017 at 11:59, Laszlo Ersek <lersek@redhat.com> wrote:

>> Peter, what is the intended use of the second UART?

> 

> Per the commit message:

> # there are some

> # use cases where having a second UART would be useful (like

> # bare-metal testing where you don't really want to have to

> # probe and set up a PCI device just to have a second comms

> # channel).

> 

> From QEMU's point of view this is just "some people would like

> more than one UART, x86 provides more than one UART, it's

> easy to provide an extra UART and it's up to the guest what

> it would like to do with it". If there's utility for the

> OVMF firmware in having 2 UARTs that's doubly good reason

> to have it.


I agree. There's one user-visible complication: while with one UART,
it's not possible to mix things up, with two UARTs, users will
inevitably want to assign inverse roles to them, relative to what QEMU /
the firmware assigns originally.

E.g. if one PL011 is redirected to a regular file (debug messages) and
the other one to stdio (console), there will be complaints that "I
wanted it the other way around". The redirection happens on the backend
(chardev) level, but the firmware won't see the difference on the
frontend (PL011) level.

Is it possible to add a new property to the UART nodes in the DTB, like
"purpose"?

Or can we make a virt-arm policy that "first -serial is always ...,
second -serial is always ..."?

(
Technically the latter could work, because:

- QEMU_OPTION_serial / add_device_config() keeps the command line order,
- the traversal of DEV_SERIAL with serial_parse() also keeps that order,
- machvirt_init() uses the same order via serial_hds[]
- create_uart() always prepends the new node to the DTB, via
  qemu_fdt_add_subnode() (if I understood Ard right).

I'm not sure though whether libvirt would like the ordering of -serial
options to carry any meaning.
)

Sorry if these questions don't belong on the QEMU level.

Thanks
Laszlo
Peter Maydell Dec. 12, 2017, 12:47 p.m. | #9
On 12 December 2017 at 12:41, Laszlo Ersek <lersek@redhat.com> wrote:
> I agree. There's one user-visible complication: while with one UART,

> it's not possible to mix things up, with two UARTs, users will

> inevitably want to assign inverse roles to them, relative to what QEMU /

> the firmware assigns originally.

>

> E.g. if one PL011 is redirected to a regular file (debug messages) and

> the other one to stdio (console), there will be complaints that "I

> wanted it the other way around". The redirection happens on the backend

> (chardev) level, but the firmware won't see the difference on the

> frontend (PL011) level.

>

> Is it possible to add a new property to the UART nodes in the DTB, like

> "purpose"?


This is what the "chosen" stdout-path node in the DTB is for,
but you said you didn't want to look at that :-) (it means
"device to be used for boot console output".)

> Or can we make a virt-arm policy that "first -serial is always ...,

> second -serial is always ..."?


Well, the first -serial will always be the 0x09000000 one
that we have today, and the second -serial will be the other
one (unless you have -machine secure=yes, in which case
second -serial is the secure-only UART and third -serial is
the second NS UART).

Is this any different to the x86 case, where there are two
UARTs at different IO port/IRQ locations?

I think the only thing users really expect is that if you're
using just the one serial port for anything then it's the
first one.

thanks
-- PMM
Laszlo Ersek Dec. 12, 2017, 1:28 p.m. | #10
On 12/12/17 13:47, Peter Maydell wrote:
> On 12 December 2017 at 12:41, Laszlo Ersek <lersek@redhat.com> wrote:

>> I agree. There's one user-visible complication: while with one UART,

>> it's not possible to mix things up, with two UARTs, users will

>> inevitably want to assign inverse roles to them, relative to what QEMU /

>> the firmware assigns originally.

>>

>> E.g. if one PL011 is redirected to a regular file (debug messages) and

>> the other one to stdio (console), there will be complaints that "I

>> wanted it the other way around". The redirection happens on the backend

>> (chardev) level, but the firmware won't see the difference on the

>> frontend (PL011) level.

>>

>> Is it possible to add a new property to the UART nodes in the DTB, like

>> "purpose"?

> 

> This is what the "chosen" stdout-path node in the DTB is for,

> but you said you didn't want to look at that :-) (it means

> "device to be used for boot console output".)


:(

Ard is right that we really shouldn't do that kind of parsing magic in
very early UEFI stuff.


>> Or can we make a virt-arm policy that "first -serial is always ...,

>> second -serial is always ..."?

> 

> Well, the first -serial will always be the 0x09000000 one

> that we have today, and the second -serial will be the other

> one (unless you have -machine secure=yes, in which case

> second -serial is the secure-only UART and third -serial is

> the second NS UART).

> 

> Is this any different to the x86 case, where there are two

> UARTs at different IO port/IRQ locations?


OVMF (x86) uses two distinct devices for the two purposes discussed.

* It uses the "debug console device" for debug message output, at
hard-coded IO port 0x402; so if you'd like to capture those messages,
then you have to add:

  -chardev file,id=debugfile,path=debug.log \
  -device isa-debugcon,iobase=0x402,chardev=debugfile \

(or the more traditional

  -debugcon file:debug.log \
  -global isa-debugcon.iobase=0x402 \
)

* For console I/O, it uses the first serial port. (I think I have once
investigated what it takes to use other serial ports for console I/O --
I'm no longer sure if "it happens to be impossible in OVMF", or just
"nobody ever does that".)


The important distinction (on the UEFI level anyway) is that the debug
messages need to go to a super dumb device, while console I/O can use a
much higher-level serial IO abstraction ("protocol").


> I think the only thing users really expect is that if you're

> using just the one serial port for anything then it's the

> first one.


You are right -- we've never had two (non-secure) UARTs on virt-arm, so
we can invent whatever assignment, as long as:

- the single UART case doesn't break (keeps receiving both debug output
and console IO),

- whatever we invent for the two UARTs case now, it remains consistent
over time. I think this implies we can rely on the FDT node ordering in
the firmware (if we want to use the 2nd UART at all, that is), and then
QEMU shouldn't change the serial_hds[] <-> FDT node mapping in the
future, in machvirt_init() and the callees thereof.

Thanks!
Laszlo
Ard Biesheuvel Dec. 12, 2017, 1:56 p.m. | #11
On 12 December 2017 at 13:28, Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/12/17 13:47, Peter Maydell wrote:

>> On 12 December 2017 at 12:41, Laszlo Ersek <lersek@redhat.com> wrote:

>>> I agree. There's one user-visible complication: while with one UART,

>>> it's not possible to mix things up, with two UARTs, users will

>>> inevitably want to assign inverse roles to them, relative to what QEMU /

>>> the firmware assigns originally.

>>>

>>> E.g. if one PL011 is redirected to a regular file (debug messages) and

>>> the other one to stdio (console), there will be complaints that "I

>>> wanted it the other way around". The redirection happens on the backend

>>> (chardev) level, but the firmware won't see the difference on the

>>> frontend (PL011) level.

>>>

>>> Is it possible to add a new property to the UART nodes in the DTB, like

>>> "purpose"?

>>

>> This is what the "chosen" stdout-path node in the DTB is for,

>> but you said you didn't want to look at that :-) (it means

>> "device to be used for boot console output".)

>

> :(

>

> Ard is right that we really shouldn't do that kind of parsing magic in

> very early UEFI stuff.

>

>

>>> Or can we make a virt-arm policy that "first -serial is always ...,

>>> second -serial is always ..."?

>>

>> Well, the first -serial will always be the 0x09000000 one

>> that we have today, and the second -serial will be the other

>> one (unless you have -machine secure=yes, in which case

>> second -serial is the secure-only UART and third -serial is

>> the second NS UART).

>>

>> Is this any different to the x86 case, where there are two

>> UARTs at different IO port/IRQ locations?

>

> OVMF (x86) uses two distinct devices for the two purposes discussed.

>

> * It uses the "debug console device" for debug message output, at

> hard-coded IO port 0x402; so if you'd like to capture those messages,

> then you have to add:

>

>   -chardev file,id=debugfile,path=debug.log \

>   -device isa-debugcon,iobase=0x402,chardev=debugfile \

>

> (or the more traditional

>

>   -debugcon file:debug.log \

>   -global isa-debugcon.iobase=0x402 \

> )

>

> * For console I/O, it uses the first serial port. (I think I have once

> investigated what it takes to use other serial ports for console I/O --

> I'm no longer sure if "it happens to be impossible in OVMF", or just

> "nobody ever does that".)

>

>

> The important distinction (on the UEFI level anyway) is that the debug

> messages need to go to a super dumb device, while console I/O can use a

> much higher-level serial IO abstraction ("protocol").

>

>

>> I think the only thing users really expect is that if you're

>> using just the one serial port for anything then it's the

>> first one.

>

> You are right -- we've never had two (non-secure) UARTs on virt-arm, so

> we can invent whatever assignment, as long as:

>

> - the single UART case doesn't break (keeps receiving both debug output

> and console IO),

>

> - whatever we invent for the two UARTs case now, it remains consistent

> over time. I think this implies we can rely on the FDT node ordering in

> the firmware (if we want to use the 2nd UART at all, that is), and then

> QEMU shouldn't change the serial_hds[] <-> FDT node mapping in the

> future, in machvirt_init() and the callees thereof.

>


Given that the DEBUG output is only produced on DEBUG builds, couldn't
we simply stipulate that DEBUG output appears on the PL011 with the
lowest physical address? That keeps the current status quo, and is
probably sufficient for 99% of the use cases of people using the DEBUG
builds.

Then, we can introduce some code to decode stdout-path in FdtClientDxe
(a higher level DT parsing driver) so that the actual serial console
gets installed onto whichever UART is described as the system console.
Also, given that ArmVirtQemu is tightly coupled to QEMU/mach-virt, we
could decide to only use node names in stdout-path (as we do
currently) to simplify the parsing logic.
Andrew Jones Dec. 12, 2017, 2:10 p.m. | #12
On Tue, Dec 12, 2017 at 02:28:51PM +0100, Laszlo Ersek wrote:
> On 12/12/17 13:47, Peter Maydell wrote:

> > On 12 December 2017 at 12:41, Laszlo Ersek <lersek@redhat.com> wrote:

> >> I agree. There's one user-visible complication: while with one UART,

> >> it's not possible to mix things up, with two UARTs, users will

> >> inevitably want to assign inverse roles to them, relative to what QEMU /

> >> the firmware assigns originally.

> >>

> >> E.g. if one PL011 is redirected to a regular file (debug messages) and

> >> the other one to stdio (console), there will be complaints that "I

> >> wanted it the other way around". The redirection happens on the backend

> >> (chardev) level, but the firmware won't see the difference on the

> >> frontend (PL011) level.

> >>

> >> Is it possible to add a new property to the UART nodes in the DTB, like

> >> "purpose"?

> > 

> > This is what the "chosen" stdout-path node in the DTB is for,

> > but you said you didn't want to look at that :-) (it means

> > "device to be used for boot console output".)

> 

> :(

> 

> Ard is right that we really shouldn't do that kind of parsing magic in

> very early UEFI stuff.

> 

> 

> >> Or can we make a virt-arm policy that "first -serial is always ...,

> >> second -serial is always ..."?

> > 

> > Well, the first -serial will always be the 0x09000000 one

> > that we have today, and the second -serial will be the other

> > one (unless you have -machine secure=yes, in which case

> > second -serial is the secure-only UART and third -serial is

> > the second NS UART).

> > 

> > Is this any different to the x86 case, where there are two

> > UARTs at different IO port/IRQ locations?

> 

> OVMF (x86) uses two distinct devices for the two purposes discussed.

> 

> * It uses the "debug console device" for debug message output, at

> hard-coded IO port 0x402; so if you'd like to capture those messages,

> then you have to add:

> 

>   -chardev file,id=debugfile,path=debug.log \

>   -device isa-debugcon,iobase=0x402,chardev=debugfile \

> 

> (or the more traditional

> 

>   -debugcon file:debug.log \

>   -global isa-debugcon.iobase=0x402 \

> )

> 

> * For console I/O, it uses the first serial port. (I think I have once


If "first" here means the first '-serial' argument on the command line,
then mach-virt has the same requirement. The console must be the first
'-serial' argument. The secure UART, provided when the 'secure' property
is set, gets the next one. So

 -chardev file,path=secure-uart.log,id=s -serial stdio -serial chardev:s

works as expected, but

 -chardev file,path=secure-uart.log,id=s -serial chardev:s -serial stdio

does not.

This series keeps that requirement and adds that the second NS UART's
'-serial' argument must come after the secure UART, when the machine
has the 'secure' property set.

Command line ordering requirements aren't awesome, so it might be nice
to add a serial option 'console', allowing

 -chardev file,path=secure-uart.log,id=s -serial chardev:s -serial stdio,console

to work too.

> investigated what it takes to use other serial ports for console I/O --

> I'm no longer sure if "it happens to be impossible in OVMF", or just

> "nobody ever does that".)

> 

> 

> The important distinction (on the UEFI level anyway) is that the debug

> messages need to go to a super dumb device, while console I/O can use a

> much higher-level serial IO abstraction ("protocol").

> 

> 

> > I think the only thing users really expect is that if you're

> > using just the one serial port for anything then it's the

> > first one.

> 

> You are right -- we've never had two (non-secure) UARTs on virt-arm, so

> we can invent whatever assignment, as long as:

> 

> - the single UART case doesn't break (keeps receiving both debug output

> and console IO),

> 

> - whatever we invent for the two UARTs case now, it remains consistent

> over time. I think this implies we can rely on the FDT node ordering in

> the firmware (if we want to use the 2nd UART at all, that is), and then

> QEMU shouldn't change the serial_hds[] <-> FDT node mapping in the

> future, in machvirt_init() and the callees thereof.

> 

> Thanks!

> Laszlo

> 


Personally I think AAVMF should learn how to parse stdout-path, but as
an interim solution it could just hard code the console address
(0x9000000), like it does the base of memory (which I think it should
eventually learn to get from the DT, pointed to by register x0, too :-)

Always generating the second UART's FDT node with status = "disabled",
as Ard suggests, even when the user has provided a '-serial' command
line option for it, i.e. serial_hds[i] != NULL, i > 0, doesn't seem like
what we'd want to do. Wouldn't Linux assume (when boot directly without
AAVMF) that the UART should not be used in that case?

Thanks,
drew
Peter Maydell Dec. 12, 2017, 2:10 p.m. | #13
On 12 December 2017 at 13:56, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Given that the DEBUG output is only produced on DEBUG builds, couldn't

> we simply stipulate that DEBUG output appears on the PL011 with the

> lowest physical address? That keeps the current status quo, and is

> probably sufficient for 99% of the use cases of people using the DEBUG

> builds.

>

> Then, we can introduce some code to decode stdout-path in FdtClientDxe

> (a higher level DT parsing driver) so that the actual serial console

> gets installed onto whichever UART is described as the system console.

> Also, given that ArmVirtQemu is tightly coupled to QEMU/mach-virt, we

> could decide to only use node names in stdout-path (as we do

> currently) to simplify the parsing logic.


That doesn't actually usefully separate debug output from the
console, though, because stdout-path is also going to point
to the UART with the lowest physical address...

-- PMM
Ard Biesheuvel Dec. 12, 2017, 2:12 p.m. | #14
On 12 December 2017 at 14:10, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 December 2017 at 13:56, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>> Given that the DEBUG output is only produced on DEBUG builds, couldn't

>> we simply stipulate that DEBUG output appears on the PL011 with the

>> lowest physical address? That keeps the current status quo, and is

>> probably sufficient for 99% of the use cases of people using the DEBUG

>> builds.

>>

>> Then, we can introduce some code to decode stdout-path in FdtClientDxe

>> (a higher level DT parsing driver) so that the actual serial console

>> gets installed onto whichever UART is described as the system console.

>> Also, given that ArmVirtQemu is tightly coupled to QEMU/mach-virt, we

>> could decide to only use node names in stdout-path (as we do

>> currently) to simplify the parsing logic.

>

> That doesn't actually usefully separate debug output from the

> console, though, because stdout-path is also going to point

> to the UART with the lowest physical address...

>


By default, yes, just like is currently the case. But I would assume
this new serial port could be appointed 'console' and put into
stdout-path by QEMU, based on the command line options?
Peter Maydell Dec. 12, 2017, 2:13 p.m. | #15
On 12 December 2017 at 14:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 12 December 2017 at 14:10, Peter Maydell <peter.maydell@linaro.org> wrote:

>> That doesn't actually usefully separate debug output from the

>> console, though, because stdout-path is also going to point

>> to the UART with the lowest physical address...

>>

>

> By default, yes, just like is currently the case. But I would assume

> this new serial port could be appointed 'console' and put into

> stdout-path by QEMU, based on the command line options?


We don't have any command line options for doing that, and I'm
generally reluctant to introduce new command line UI, especially
new command line UI that's specific to Arm and not also needed
for x86.

thanks
-- PMM
Ard Biesheuvel Dec. 12, 2017, 2:16 p.m. | #16
On 12 December 2017 at 14:13, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 December 2017 at 14:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>> On 12 December 2017 at 14:10, Peter Maydell <peter.maydell@linaro.org> wrote:

>>> That doesn't actually usefully separate debug output from the

>>> console, though, because stdout-path is also going to point

>>> to the UART with the lowest physical address...

>>>

>>

>> By default, yes, just like is currently the case. But I would assume

>> this new serial port could be appointed 'console' and put into

>> stdout-path by QEMU, based on the command line options?

>

> We don't have any command line options for doing that, and I'm

> generally reluctant to introduce new command line UI, especially

> new command line UI that's specific to Arm and not also needed

> for x86.

>


If stdout-path is always going to point to pl011@0x9000000, why would
we need to parse it?
Peter Maydell Dec. 12, 2017, 2:17 p.m. | #17
On 12 December 2017 at 14:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 12 December 2017 at 14:13, Peter Maydell <peter.maydell@linaro.org> wrote:

>> On 12 December 2017 at 14:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>>> On 12 December 2017 at 14:10, Peter Maydell <peter.maydell@linaro.org> wrote:

>>>> That doesn't actually usefully separate debug output from the

>>>> console, though, because stdout-path is also going to point

>>>> to the UART with the lowest physical address...

>>>>

>>>

>>> By default, yes, just like is currently the case. But I would assume

>>> this new serial port could be appointed 'console' and put into

>>> stdout-path by QEMU, based on the command line options?

>>

>> We don't have any command line options for doing that, and I'm

>> generally reluctant to introduce new command line UI, especially

>> new command line UI that's specific to Arm and not also needed

>> for x86.

>>

>

> If stdout-path is always going to point to pl011@0x9000000, why would

> we need to parse it?


If you had always parsed stdout-path, we wouldn't need to worry
about the order in which the UART nodes appear in the dtb...

thanks
-- PMM
Andrew Jones Dec. 12, 2017, 2:18 p.m. | #18
On Tue, Dec 12, 2017 at 01:56:44PM +0000, Ard Biesheuvel wrote:
> On 12 December 2017 at 13:28, Laszlo Ersek <lersek@redhat.com> wrote:

> > On 12/12/17 13:47, Peter Maydell wrote:

> >> On 12 December 2017 at 12:41, Laszlo Ersek <lersek@redhat.com> wrote:

> >>> I agree. There's one user-visible complication: while with one UART,

> >>> it's not possible to mix things up, with two UARTs, users will

> >>> inevitably want to assign inverse roles to them, relative to what QEMU /

> >>> the firmware assigns originally.

> >>>

> >>> E.g. if one PL011 is redirected to a regular file (debug messages) and

> >>> the other one to stdio (console), there will be complaints that "I

> >>> wanted it the other way around". The redirection happens on the backend

> >>> (chardev) level, but the firmware won't see the difference on the

> >>> frontend (PL011) level.

> >>>

> >>> Is it possible to add a new property to the UART nodes in the DTB, like

> >>> "purpose"?

> >>

> >> This is what the "chosen" stdout-path node in the DTB is for,

> >> but you said you didn't want to look at that :-) (it means

> >> "device to be used for boot console output".)

> >

> > :(

> >

> > Ard is right that we really shouldn't do that kind of parsing magic in

> > very early UEFI stuff.

> >

> >

> >>> Or can we make a virt-arm policy that "first -serial is always ...,

> >>> second -serial is always ..."?

> >>

> >> Well, the first -serial will always be the 0x09000000 one

> >> that we have today, and the second -serial will be the other

> >> one (unless you have -machine secure=yes, in which case

> >> second -serial is the secure-only UART and third -serial is

> >> the second NS UART).

> >>

> >> Is this any different to the x86 case, where there are two

> >> UARTs at different IO port/IRQ locations?

> >

> > OVMF (x86) uses two distinct devices for the two purposes discussed.

> >

> > * It uses the "debug console device" for debug message output, at

> > hard-coded IO port 0x402; so if you'd like to capture those messages,

> > then you have to add:

> >

> >   -chardev file,id=debugfile,path=debug.log \

> >   -device isa-debugcon,iobase=0x402,chardev=debugfile \

> >

> > (or the more traditional

> >

> >   -debugcon file:debug.log \

> >   -global isa-debugcon.iobase=0x402 \

> > )

> >

> > * For console I/O, it uses the first serial port. (I think I have once

> > investigated what it takes to use other serial ports for console I/O --

> > I'm no longer sure if "it happens to be impossible in OVMF", or just

> > "nobody ever does that".)

> >

> >

> > The important distinction (on the UEFI level anyway) is that the debug

> > messages need to go to a super dumb device, while console I/O can use a

> > much higher-level serial IO abstraction ("protocol").

> >

> >

> >> I think the only thing users really expect is that if you're

> >> using just the one serial port for anything then it's the

> >> first one.

> >

> > You are right -- we've never had two (non-secure) UARTs on virt-arm, so

> > we can invent whatever assignment, as long as:

> >

> > - the single UART case doesn't break (keeps receiving both debug output

> > and console IO),

> >

> > - whatever we invent for the two UARTs case now, it remains consistent

> > over time. I think this implies we can rely on the FDT node ordering in

> > the firmware (if we want to use the 2nd UART at all, that is), and then

> > QEMU shouldn't change the serial_hds[] <-> FDT node mapping in the

> > future, in machvirt_init() and the callees thereof.

> >

> 

> Given that the DEBUG output is only produced on DEBUG builds, couldn't

> we simply stipulate that DEBUG output appears on the PL011 with the

> lowest physical address? That keeps the current status quo, and is

> probably sufficient for 99% of the use cases of people using the DEBUG

> builds.


I was thinking that if AAVMF learned to use a second UART for debug
output, that the debug builds could go away, as they have for x86 (IIUC).
If a user wires up a second UART, then AAVMF could unconditionally
output debug messages to it. Not wiring it up would lose the messages,
which is no different than the non-verbose build we have today. It
would be nice if could tell AAVMF not to use a second UART for debug
messages too, though, as the debug messages make AAVMF quite slow, and
the user may want to provide the guest a second UART.

> 

> Then, we can introduce some code to decode stdout-path in FdtClientDxe

> (a higher level DT parsing driver) so that the actual serial console

> gets installed onto whichever UART is described as the system console.

> Also, given that ArmVirtQemu is tightly coupled to QEMU/mach-virt, we

> could decide to only use node names in stdout-path (as we do

> currently) to simplify the parsing logic.

>


That sounds reasonable and would only require adding a comment above
the setting of stdout-path in the QEMU code to formalize it.

Thanks,
drew
Ard Biesheuvel Dec. 12, 2017, 2:31 p.m. | #19
On 12 December 2017 at 14:17, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 December 2017 at 14:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>> On 12 December 2017 at 14:13, Peter Maydell <peter.maydell@linaro.org> wrote:

>>> On 12 December 2017 at 14:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>>>> On 12 December 2017 at 14:10, Peter Maydell <peter.maydell@linaro.org> wrote:

>>>>> That doesn't actually usefully separate debug output from the

>>>>> console, though, because stdout-path is also going to point

>>>>> to the UART with the lowest physical address...

>>>>>

>>>>

>>>> By default, yes, just like is currently the case. But I would assume

>>>> this new serial port could be appointed 'console' and put into

>>>> stdout-path by QEMU, based on the command line options?

>>>

>>> We don't have any command line options for doing that, and I'm

>>> generally reluctant to introduce new command line UI, especially

>>> new command line UI that's specific to Arm and not also needed

>>> for x86.

>>>

>>

>> If stdout-path is always going to point to pl011@0x9000000, why would

>> we need to parse it?

>

> If you had always parsed stdout-path, we wouldn't need to worry

> about the order in which the UART nodes appear in the dtb...

>


That's a fair point. But it still does not justify introducing added
complexity in the firmware, if the conclusion is always going to be
that the console will be on pl011@9000000.

To Drew's point re DEBUG builds, I don't think we should generally
enable DEBUG and send the output to nowhereland if the user does not
wire it up. That's a MMIO trap and two world switches for each
character written, if I am not mistaken, and the DEBUG builds are very
noisy.

But still, given that stdout-path is essentially written in stone
anyway, could we also decide that it will always refer to
pl011@9000000 by its nodename? That way, we can easily implement the
early DEBUG logic by using any non-disabled non-secure PL011 that is
not referenced in stdout-path, and falling back to the one that is in
stdout-path, which will essentially work as it does today. And for the
more elaborate DXE driver, we can find the nodename from stdout-path.
Andrew Jones Dec. 12, 2017, 2:51 p.m. | #20
On Tue, Dec 12, 2017 at 02:31:05PM +0000, Ard Biesheuvel wrote:
> On 12 December 2017 at 14:17, Peter Maydell <peter.maydell@linaro.org> wrote:

> > On 12 December 2017 at 14:16, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> >> On 12 December 2017 at 14:13, Peter Maydell <peter.maydell@linaro.org> wrote:

> >>> On 12 December 2017 at 14:12, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> >>>> On 12 December 2017 at 14:10, Peter Maydell <peter.maydell@linaro.org> wrote:

> >>>>> That doesn't actually usefully separate debug output from the

> >>>>> console, though, because stdout-path is also going to point

> >>>>> to the UART with the lowest physical address...

> >>>>>

> >>>>

> >>>> By default, yes, just like is currently the case. But I would assume

> >>>> this new serial port could be appointed 'console' and put into

> >>>> stdout-path by QEMU, based on the command line options?

> >>>

> >>> We don't have any command line options for doing that, and I'm

> >>> generally reluctant to introduce new command line UI, especially

> >>> new command line UI that's specific to Arm and not also needed

> >>> for x86.

> >>>

> >>

> >> If stdout-path is always going to point to pl011@0x9000000, why would

> >> we need to parse it?

> >

> > If you had always parsed stdout-path, we wouldn't need to worry

> > about the order in which the UART nodes appear in the dtb...

> >

> 

> That's a fair point. But it still does not justify introducing added

> complexity in the firmware, if the conclusion is always going to be

> that the console will be on pl011@9000000.

> 

> To Drew's point re DEBUG builds, I don't think we should generally

> enable DEBUG and send the output to nowhereland if the user does not

> wire it up. That's a MMIO trap and two world switches for each

> character written, if I am not mistaken, and the DEBUG builds are very

> noisy.


100% agree, which is why I said "If a user wires up a second UART...",
but I guess my "Not wiring it up would lose the messages..." sentence
was ambiguous. I didn't mean they'd output to no where, I meant those
messages wouldn't be output. I.e. some sort of debug_printf() function
would be written that only actually does a write when there's a debug
UART on the other end to write to.

I just commented on patch 2/3 of this series asking if we shouldn't
just not allocate the second UART's MMIO region and IRQ, when the user
doesn't provide a '-serial' option for it. If we didn't, then AAVMF
would just not output debug messages when only one UART (the console)
is found in the DT.

Thanks,
drew
Andrew Jones Dec. 12, 2017, 3:09 p.m. | #21
On Tue, Dec 12, 2017 at 12:06:39PM +0100, Laszlo Ersek wrote:
> On 12/12/17 06:53, Shannon Zhao wrote:

> > 

> > 

> > On 2017/12/8 23:02, Peter Maydell wrote:

> >> Add the second UART to the ACPI tables.

> >>

> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> >> ---

> >> Pure guesswork, as I don't have a UEFI setup to hand and

> >> am not familiar with ACPI table formats either...

> >> ---

> >>  hw/arm/virt-acpi-build.c | 5 +++++

> >>  1 file changed, 5 insertions(+)

> >>

> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c

> >> index 3d78ff6..a38287b 100644

> >> --- a/hw/arm/virt-acpi-build.c

> >> +++ b/hw/arm/virt-acpi-build.c

> >> @@ -689,6 +689,7 @@ static void build_fadt(GArray *table_data, BIOSLinker *linker,

> >>  static void

> >>  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)

> >>  {

> >> +    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);

> >>      Aml *scope, *dsdt;

> >>      const MemMapEntry *memmap = vms->memmap;

> >>      const int *irqmap = vms->irqmap;

> >> @@ -706,6 +707,10 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)

> >>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);

> >>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],

> >>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));

> >> +    if (!vmc->no_second_uart) {

> >> +        acpi_dsdt_add_uart(scope, &memmap[VIRT_UART_2],

> >> +                           (irqmap[VIRT_UART_2] + ARM_SPI_BASE));

> >> +    }

> >>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);

> >>      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);

> >>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],

> >>

> > Reviewed-by: Shannon Zhao <zhaoshenglong@huawei.com>

> > 

> 

> (Responding here so I don't have to manually update Shannon's email

> address while replying to patch 3/3 directly.)

> 

> acpi_dsdt_add_uart() does:

> 

>     Aml *dev = aml_device("COM0");

>     aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0011")));

>     aml_append(dev, aml_name_decl("_UID", aml_int(0)));

> 

> The device name should be unique. Plus, within the same _HID, the _UID

> should be unique as well. I suggest adding another unsigned parameter to

> acpi_dsdt_add_uart(), and hooking it into COMx and _UID.


Yes, I agree with Laszlo here (and confirmed with Igor). Also, if memory
serves me, the _ADR part of acpi_dsdt_add_uart() is only there to deal
with early Linux ACPI support not finding the console UART correctly.
I believe it does now, meaning the _ADR could even be removed for COM0,
and we certainly don't want it for COM1 because it's point is to find
COM0, so the new unsigned parameter should also be used as condition for
it. A later patch could remove it altogether.

Thanks,
drew
Laszlo Ersek Dec. 12, 2017, 4:38 p.m. | #22
On 12/12/17 15:51, Andrew Jones wrote:
> On Tue, Dec 12, 2017 at 02:31:05PM +0000, Ard Biesheuvel wrote:

>> On 12 December 2017 at 14:17, Peter Maydell

>> <peter.maydell@linaro.org> wrote:

>>> On 12 December 2017 at 14:16, Ard Biesheuvel

>>> <ard.biesheuvel@linaro.org> wrote:

>>>> On 12 December 2017 at 14:13, Peter Maydell

>>>> <peter.maydell@linaro.org> wrote:

>>>>> On 12 December 2017 at 14:12, Ard Biesheuvel

>>>>> <ard.biesheuvel@linaro.org> wrote:

>>>>>> On 12 December 2017 at 14:10, Peter Maydell

>>>>>> <peter.maydell@linaro.org> wrote:

>>>>>>> That doesn't actually usefully separate debug output from the

>>>>>>> console, though, because stdout-path is also going to point

>>>>>>> to the UART with the lowest physical address...

>>>>>>>

>>>>>>

>>>>>> By default, yes, just like is currently the case. But I would

>>>>>> assume this new serial port could be appointed 'console' and put

>>>>>> into stdout-path by QEMU, based on the command line options?

>>>>>

>>>>> We don't have any command line options for doing that, and I'm

>>>>> generally reluctant to introduce new command line UI, especially

>>>>> new command line UI that's specific to Arm and not also needed

>>>>> for x86.

>>>>>

>>>>

>>>> If stdout-path is always going to point to pl011@0x9000000, why

>>>> would we need to parse it?

>>>

>>> If you had always parsed stdout-path, we wouldn't need to worry

>>> about the order in which the UART nodes appear in the dtb...

>>>

>>

>> That's a fair point. But it still does not justify introducing added

>> complexity in the firmware, if the conclusion is always going to be

>> that the console will be on pl011@9000000.

>>

>> To Drew's point re DEBUG builds, I don't think we should generally

>> enable DEBUG and send the output to nowhereland if the user does not

>> wire it up. That's a MMIO trap and two world switches for each

>> character written, if I am not mistaken, and the DEBUG builds are

>> very noisy.

>

> 100% agree, which is why I said "If a user wires up a second UART...",

> but I guess my "Not wiring it up would lose the messages..." sentence

> was ambiguous. I didn't mean they'd output to no where, I meant those

> messages wouldn't be output. I.e. some sort of debug_printf() function

> would be written that only actually does a write when there's a debug

> UART on the other end to write to.


Right; this can be implemented based on the DTB. Please see below.


> I just commented on patch 2/3 of this series asking if we shouldn't

> just not allocate the second UART's MMIO region and IRQ, when the user

> doesn't provide a '-serial' option for it. If we didn't, then AAVMF

> would just not output debug messages when only one UART (the console)

> is found in the DT.


This is what we ultimately want down-stream, yes; but up-stream, it
would be an incompatible change -- DEBUG builds used with one UART only
would suddenly not produce debug messages.


I don't think I can continue discussing this without going into
ArmVirtQemu specifics. We have the following layers of drivers and
library classes / instances (more abstract higher up on the diagram).

The top left node stands for debug messages written by any modules, and
the top right node stands for the abstraction (EFI_SERIAL_IO_PROTOCOL)
through which interactive console / terminal IO occurs ultimately (it
requires a few more layers on top but those don't matter for now).


[any driver that writes debug messages]    SerialDxe
  |  |                                     (DXE driver that produces
  |  |                                     EFI_SERIAL_IO_PROTOCOL;
  |  DebugLib:BaseDebugLibNull  +-----+----uses DebugLib for one
  |  (used for RELEASE builds; /     /     ASSERT_EFI_ERROR();
  |  DEBUG messages are dropped     /      uses SerialPortLib for
  |  indiscriminately)             /       producing
  |                               /        EFI_SERIAL_IO_PROTOCOL)
  |                              /           |
  DebugLib:BaseDebugLibSerialPort            |
  (for DEBUG / NOOPT builds;                 |
  debug messages are written to              |
  the serial port)                           |
    |  |                                     |
    |  |                                     |
    |  SerialPortLib: FdtPL011SerialPortLib  |
    |  (used by DXE and later modules, the UART
    |  base need not be parsed repeatedly from the
    |  DTB)                                       \
    |                                              \
    |                                               \
    SerialPortLib: EarlyFdtPL011SerialPortLib        \
    (used for early (pre-DXE) modules; the   \        +
    DTB is parsed on every serial port        \       |
    action for the UART base address)          \      |
                                                \     |
                                                 \    |
                                                  PL011UartLib
                                                  (actual hw access)

This is super messy, because in a DEBUG build, debug messages (such as
an assertion failure report) in SerialDxe itself would have to be logged
to "one" serial port, but the exact same driver should use the "other"
serial port for implementing EFI_SERIAL_IO_PROTOCOL. Currently both
"paths" go through FdtPL011SerialPortLib, which -- i.e., the
SerialPortLib *class* itself -- cannot handle more than one serial port
(in the same UEFI executable).

So, for upstream,

(1) we need two new DebugLib instances (pre-DXE, and DXE-or-later), for
    the ArmVirtQemu platform, such that they use PL011UartLib directly,
    without going through the SerialPortLib class.

    These DebugLib instances will have to parse the DTB for the UART
    base address (on all calls, or initially). If there is only one
    UART, use that, otherwise, use the one that is found second in the
    DTB.

(2) In RELEASE builds, BaseDebugLibNull should remain OK (DEBUG messages
    are dropped statically).

(3) The current SerialPortLib instances need not be changed; they can
    continue using the first UART found in the DTB.


For downstream, the DebugLib instances from (1) can be customized so
that debug messages are dropped if only one UART is found in the DTB.
(I.e., never write a DEBUG message unless you can write it separate from
whatever port is used by FdtPL011SerialPortLib.)

Thanks,
Laszlo
Peter Maydell Dec. 13, 2017, 1:56 p.m. | #23
On 12 December 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:
> BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I

> can see from the firmware code, the firmware will use the PL011 whose

> description comes first in the DTB (and ignore the other PL011), in an

> fdt_next_node() traversal. Is that OK for the intended use case?


I have now tested this, and annoyingly UEFI and the kernel seem
to disagree about enumeration order. That is, if QEMU creates
them in the code in the order 0x09050000 (uart 2), 0x09000000 (uart 1),
then they appear in the dtb with uart 1 first, and the kernel enumerates
them as ttyAMA0 being uart 1 and ttyAMA1 being uart 2, but UEFI
outputs to uart 2...

thanks
-- PMM
Laszlo Ersek Dec. 13, 2017, 4:01 p.m. | #24
On 12/13/17 14:56, Peter Maydell wrote:
> On 12 December 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:

>> BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I

>> can see from the firmware code, the firmware will use the PL011 whose

>> description comes first in the DTB (and ignore the other PL011), in an

>> fdt_next_node() traversal. Is that OK for the intended use case?

> 

> I have now tested this,


Thank you!

> and annoyingly UEFI and the kernel seem

> to disagree about enumeration order. That is, if QEMU creates

> them in the code in the order 0x09050000 (uart 2), 0x09000000 (uart 1),

> then they appear in the dtb with uart 1 first, and the kernel enumerates

> them as ttyAMA0 being uart 1 and ttyAMA1 being uart 2, but UEFI

> outputs to uart 2...


Ouch. This reminds me (remotely) of QEMU commit 587078f0ed63
("hw/arm/virt: explain device-to-transport mapping in
create_virtio_devices()", 2015-02-05).

I'd still like to avoid the "sophisticated" /chosen lookup (the lookup
itself is not too complex, but evaluating whatever we find there against
each of the scanned UART nodes appears difficult, if I'm to understand
Ard's earlier point correctly). I hope that we can match the kernel's
logic with simple modifications to our scanning loops, e.g. we could
simply pick the last UART rather than the first, or else do a maximum
(or minimum) search for the UART base, and stick with the maximum (or
minimum) found.

However, for that, we first have to understand what the kernel does. Can
someone explain that? (I tried taking a look, but it's turtles all the
way down.)

Thanks,
Laszlo
Ard Biesheuvel Dec. 13, 2017, 4:46 p.m. | #25
On 13 December 2017 at 16:01, Laszlo Ersek <lersek@redhat.com> wrote:
> On 12/13/17 14:56, Peter Maydell wrote:

>> On 12 December 2017 at 11:06, Laszlo Ersek <lersek@redhat.com> wrote:

>>> BTW, has anyone tested this with the ArmVirtQemu firmware? As far as I

>>> can see from the firmware code, the firmware will use the PL011 whose

>>> description comes first in the DTB (and ignore the other PL011), in an

>>> fdt_next_node() traversal. Is that OK for the intended use case?

>>

>> I have now tested this,

>

> Thank you!

>

>> and annoyingly UEFI and the kernel seem

>> to disagree about enumeration order. That is, if QEMU creates

>> them in the code in the order 0x09050000 (uart 2), 0x09000000 (uart 1),

>> then they appear in the dtb with uart 1 first, and the kernel enumerates

>> them as ttyAMA0 being uart 1 and ttyAMA1 being uart 2, but UEFI

>> outputs to uart 2...

>

> Ouch. This reminds me (remotely) of QEMU commit 587078f0ed63

> ("hw/arm/virt: explain device-to-transport mapping in

> create_virtio_devices()", 2015-02-05).

>

> I'd still like to avoid the "sophisticated" /chosen lookup (the lookup

> itself is not too complex, but evaluating whatever we find there against

> each of the scanned UART nodes appears difficult, if I'm to understand

> Ard's earlier point correctly).


This may be a lot simpler than I originally thought. stdout-path
contains a string of the from

<nodename-or-alias>[:<baudrate[<parity>[<data-bits>[<flow-control>]]]]

where nodename-or-alias can be resolved into a path via fdt_get_alias
(), or used directly as a path otherwise.

> I hope that we can match the kernel's

> logic with simple modifications to our scanning loops, e.g. we could

> simply pick the last UART rather than the first, or else do a maximum

> (or minimum) search for the UART base, and stick with the maximum (or

> minimum) found.

>

> However, for that, we first have to understand what the kernel does. Can

> someone explain that? (I tried taking a look, but it's turtles all the

> way down.)

>


The kernel uses the contents of the SPCR table or /chosen/stdout-path
as the console unless overridden on the kernel command line. This is
independent of enumeration order.
Peter Maydell Dec. 13, 2017, 4:48 p.m. | #26
On 13 December 2017 at 16:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> The kernel uses the contents of the SPCR table or /chosen/stdout-path

> as the console unless overridden on the kernel command line. This is

> independent of enumeration order.


True, but we don't want to assign ttyAMA0 and ttyAMA1 to the
UARTs arbitrarily -- ttyAMA0 must continue to be the one
at address 0x09000000, or we break commandlines.

thanks
-- PMM

Patch

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 3d78ff6..a38287b 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -689,6 +689,7 @@  static void build_fadt(GArray *table_data, BIOSLinker *linker,
 static void
 build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {
+    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
     Aml *scope, *dsdt;
     const MemMapEntry *memmap = vms->memmap;
     const int *irqmap = vms->irqmap;
@@ -706,6 +707,10 @@  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     acpi_dsdt_add_cpus(scope, vms->smp_cpus);
     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
+    if (!vmc->no_second_uart) {
+        acpi_dsdt_add_uart(scope, &memmap[VIRT_UART_2],
+                           (irqmap[VIRT_UART_2] + ARM_SPI_BASE));
+    }
     acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
     acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
     acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],