Re: [PATCH v3] hw/riscv/virt: Add a second UART for secure world

["Li, Yong" <yong.li@intel.com>]

Hi Alistair,

Thanks for the information, what I'm doing is to implement the 
StandaloneMm and secure boot feature for RISC-V by following the ARM's way

https://trustedfirmware-a.readthedocs.io/en/latest/components/secure-partition-manager-mm.html

So here what I need from virt is actually the VIRT_SECURE_UART which 
will be delicately and isolated/used for secure world like it is in arm 
virt

(the isolation could be controlled by riscv worldguard feature if qemu 
will support)

Similar definition in ARM virt is 
https://github.com/qemu/qemu/blob/38441756b70eec5807b5f60dad11a93a91199866/hw/arm/virt.c#L142

I guess the secure uart should not be pass-through from the pcie, it 
would be more reasonable to make it a dedicated one in virt.c compared 
to the UART0 in normal world.


So sorry, I did not know the background and did not make it clear in the 
patch (it is not a second uart for normal world usage for vm, 
application and etc),

It is an UART for secure world. I guess I can re-do the patch and change 
the VIRT_UART1 to VIRT_SECURE_UART  to make it clear.

Please let me know if further comments. Thanks so much!


On 2023/5/8 7:05, Alistair Francis wrote:
> On Tue, Apr 25, 2023 at 5:36 PM Yong Li <yong.li@intel.com> wrote:
>> The virt machine can have two UARTs and the second UART
>> can be used by the secure payload, firmware or OS residing
>> in secure world. Will include the UART device to FDT in a
>> seperated patch.
>>
>> Signed-off-by: Yong Li <yong.li@intel.com>
>> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> This has come up before (see
> https://gitlab.com/qemu-project/qemu/-/issues/955) and we decided that
> we don't want to add a second UART. If you would like a second one you
> can attach it via PCIe.
>
> I think we need a really compelling reason to add another UART. There
> was a push recently to move more towards a "PCIe board" where
> everything is attached via PCIe, and this is going in the opposite
> direction.
>
> Alistair
>
>> ---
>>   hw/riscv/virt.c         | 4 ++++
>>   include/hw/riscv/virt.h | 2 ++
>>   2 files changed, 6 insertions(+)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index 4e3efbee16..8e11c4b9b3 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -88,6 +88,7 @@ static const MemMapEntry virt_memmap[] = {
>>       [VIRT_APLIC_S] =      {  0xd000000, APLIC_SIZE(VIRT_CPUS_MAX) },
>>       [VIRT_UART0] =        { 0x10000000,         0x100 },
>>       [VIRT_VIRTIO] =       { 0x10001000,        0x1000 },
>> +    [VIRT_UART1] =        { 0x10002000,         0x100 },
>>       [VIRT_FW_CFG] =       { 0x10100000,          0x18 },
>>       [VIRT_FLASH] =        { 0x20000000,     0x4000000 },
>>       [VIRT_IMSIC_M] =      { 0x24000000, VIRT_IMSIC_MAX_SIZE },
>> @@ -1506,6 +1507,9 @@ static void virt_machine_init(MachineState *machine)
>>       serial_mm_init(system_memory, memmap[VIRT_UART0].base,
>>           0, qdev_get_gpio_in(DEVICE(mmio_irqchip), UART0_IRQ), 399193,
>>           serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> +    serial_mm_init(system_memory, memmap[VIRT_UART1].base,
>> +        0, qdev_get_gpio_in(DEVICE(mmio_irqchip), UART1_IRQ), 399193,
>> +        serial_hd(1), DEVICE_LITTLE_ENDIAN);
>>
>>       sysbus_create_simple("goldfish_rtc", memmap[VIRT_RTC].base,
>>           qdev_get_gpio_in(DEVICE(mmio_irqchip), RTC_IRQ));
>> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
>> index e5c474b26e..8d2f8f225d 100644
>> --- a/include/hw/riscv/virt.h
>> +++ b/include/hw/riscv/virt.h
>> @@ -74,6 +74,7 @@ enum {
>>       VIRT_APLIC_S,
>>       VIRT_UART0,
>>       VIRT_VIRTIO,
>> +    VIRT_UART1,
>>       VIRT_FW_CFG,
>>       VIRT_IMSIC_M,
>>       VIRT_IMSIC_S,
>> @@ -88,6 +89,7 @@ enum {
>>   enum {
>>       UART0_IRQ = 10,
>>       RTC_IRQ = 11,
>> +    UART1_IRQ = 12,
>>       VIRTIO_IRQ = 1, /* 1 to 8 */
>>       VIRTIO_COUNT = 8,
>>       PCIE_IRQ = 0x20, /* 32 to 35 */
>> --
>> 2.25.1
>>
>>