Re: [PATCH qemu.git 1/1] hw/arm/virt: make second UART available

[Philippe Mathieu-Daudé]

Hi Axel,

On 14/11/22 13:06, ~axelheider wrote:
> From: Axel Heider <axel.heider@hensoldt.net>
>
> The first UART always always exists. If the security extensions are
> enabled, the second UART also always exists. Otherwise, it only exists
> if a backend is configured explicitly via '-serial <backend>', where
> 'null' creates a dummy backend. This allows enabling the second UART
> explicitly on demand and does not interfere with any existing setup
> that just expect one (or two if security extensions are enabled)
> UARTs.
>
> Signed-off-by: Axel Heider <axel.heider@hensoldt.net>
> ---
>   hw/arm/virt-acpi-build.c | 12 ++++-----
>   hw/arm/virt.c            | 55 ++++++++++++++++++++++++++++++----------
>   include/hw/arm/virt.h    |  4 +--
>   3 files changed, 49 insertions(+), 22 deletions(-)


> @@ -843,6 +843,27 @@ static void create_uart(const VirtMachineState *vms, int 
> uart,
>                           MemoryRegion *mem, Chardev *chr)
>   {
>       char *nodename;
> +    /*
> +     * The first UART always always exists. If the security extensions are
> +     * enabled, the second UART also always exists. Otherwise, it only exists
> +     * if a backend is configured explicitly via '-serial <backend>', where
> +     * 'null' creates a dummy backend. This allows enabling the second UART
> +     * explicitly on demand and does not interfere with any existing setup that
> +     * just expect one (or two if security extensions are enabled) UARTs.
> +     */
> +    switch(uart) {
> +    case VIRT_UART0:
> +        break;
> +    case VIRT_UART1:

Maybe pass a 'is_secure' boolean?

> +        if (!vms->secure && !chr) {
> +            return;
> +        }
> +        break;
> +    default:
> +        error_report("unsupported UART ID %d", uart);
> +        exit(1);
> +    }


> @@ -2222,11 +2248,12 @@ static void machvirt_init(MachineState *machine)
>   
>       fdt_add_pmu_nodes(vms);
>   
> -    create_uart(vms, VIRT_UART, sysmem, serial_hd(0));
> +    create_uart(vms, VIRT_UART0, sysmem, serial_hd(0));
> +    create_uart(vms, VIRT_UART1, vms->secure ? secure_sysmem : sysmem,
> +                serial_hd(1));
>   
>       if (vms->secure) {
>           create_secure_ram(vms, secure_sysmem, secure_tag_sysmem);
> -        create_uart(vms, VIRT_SECURE_UART, secure_sysmem, serial_hd(1));

Correct.

>       }
>   
>       if (tag_sysmem) {
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 6ec479ca2b..90563c132b 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -69,7 +69,7 @@ enum {
>       VIRT_GIC_ITS,
>       VIRT_GIC_REDIST,
>       VIRT_SMMU,
> -    VIRT_UART,
> +    VIRT_UART0,
>       VIRT_MMIO,
>       VIRT_RTC,
>       VIRT_FW_CFG,
> @@ -79,7 +79,7 @@ enum {
>       VIRT_PCIE_ECAM,
>       VIRT_PLATFORM_BUS,
>       VIRT_GPIO,
> -    VIRT_SECURE_UART,
> +    VIRT_UART1, /* secure UART if vms->secure */
(I'm not sure changing the name is worth the churn).

Regards,

Phil.