1. Patchew
  2. QEMU
  3. View series

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

Yong Li posted 1 patch 1 year ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230425073509.3618388-1-yong.li@intel.com
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Weiwei Li <liweiwei@iscas.ac.cn>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
hw/riscv/virt.c         | 4 ++++
include/hw/riscv/virt.h | 2 ++
2 files changed, 6 insertions(+)
[PATCH v3] hw/riscv/virt: Add a second UART for secure world
Posted by Yong Li 1 year ago 
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>
---
 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
Re: [PATCH v3] hw/riscv/virt: Add a second UART for secure world
Posted by Alistair Francis 1 year ago 
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
>
>
Re: [PATCH v3] hw/riscv/virt: Add a second UART for secure world
Posted by Li, Yong 1 year ago 
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
>>
>>
Re: [PATCH v3] hw/riscv/virt: Add a second UART for secure world
Posted by Alistair Francis 1 year ago 
On Mon, May 8, 2023 at 11:48 AM Li, Yong <yong.li@intel.com> wrote:
>
> 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

That is something worth including in the commit message, to help
explain what your patch is trying to do.

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

The ARM implementation isn't the same as this patch though. The ARM
virt machine added a secure memory region and guarded the entire
change behind a flag.

You can see the below commit for more details on the ARM implementation

commit 3df708eb48180fcf11956b81fd6a036cd13ed5f1
Author: Peter Maydell <peter.maydell@linaro.org>
Date:   Thu Jan 21 14:15:07 2016 +0000

   hw/arm/virt: add secure memory region and UART

   Add a secure memory region to the virt board, which is the
   same as the nonsecure memory region except that it also has
   a secure-only UART in it. This is only created if the
   board is started with the '-machine secure=on' property.

>
> 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.

Why can't the secure world not use the existing UART and the
non-secure world use a PCIe UART?

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

The patch does add a second UART for use by anyone though. It's not
only available to secure world

>
> 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.

It would also be worth pointing to documentation or a spec that
describes why having a second UART is important for secure world.

It's probably worth sending a v4 with a detailed commit message
describing why this patch is required. That should include details
about why a second UART is important for secure world. That helps the
patch get accepted in the first place, but also include useful
information for future users.

Alistair

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.