| Message ID | 1512745328-5109-4-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/arm/virt: Add another UART | expand |
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
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
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
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
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.
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
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
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
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
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
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.
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
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
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?
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
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?
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
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
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.
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
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
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
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
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
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.
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
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],
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