* [PATCH v3] hw/riscv/virt: Add a second UART for secure world @ 2023-04-25 7:35 Yong Li 2023-05-07 23:05 ` Alistair Francis 0 siblings, 1 reply; 4+ messages in thread From: Yong Li @ 2023-04-25 7:35 UTC (permalink / raw) To: qemu-devel Cc: Yong Li, LIU Zhiwei, Philippe Mathieu-Daudé, Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, open list:RISC-V TCG CPUs 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] hw/riscv/virt: Add a second UART for secure world 2023-04-25 7:35 [PATCH v3] hw/riscv/virt: Add a second UART for secure world Yong Li @ 2023-05-07 23:05 ` Alistair Francis 2023-05-08 1:47 ` Li, Yong 0 siblings, 1 reply; 4+ messages in thread From: Alistair Francis @ 2023-05-07 23:05 UTC (permalink / raw) To: Yong Li Cc: qemu-devel, LIU Zhiwei, Philippe Mathieu-Daudé, Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, open list:RISC-V TCG CPUs 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 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] hw/riscv/virt: Add a second UART for secure world 2023-05-07 23:05 ` Alistair Francis @ 2023-05-08 1:47 ` Li, Yong 2023-05-15 5:14 ` Alistair Francis 0 siblings, 1 reply; 4+ messages in thread From: Li, Yong @ 2023-05-08 1:47 UTC (permalink / raw) To: Alistair Francis Cc: qemu-devel, LIU Zhiwei, Philippe Mathieu-Daudé, Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, open list:RISC-V TCG CPUs 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 >> >> ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] hw/riscv/virt: Add a second UART for secure world 2023-05-08 1:47 ` Li, Yong @ 2023-05-15 5:14 ` Alistair Francis 0 siblings, 0 replies; 4+ messages in thread From: Alistair Francis @ 2023-05-15 5:14 UTC (permalink / raw) To: Li, Yong Cc: qemu-devel, LIU Zhiwei, Philippe Mathieu-Daudé, Palmer Dabbelt, Alistair Francis, Bin Meng, Weiwei Li, Daniel Henrique Barboza, open list:RISC-V TCG CPUs 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 > >> > >> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-05-15 5:16 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-04-25 7:35 [PATCH v3] hw/riscv/virt: Add a second UART for secure world Yong Li 2023-05-07 23:05 ` Alistair Francis 2023-05-08 1:47 ` Li, Yong 2023-05-15 5:14 ` Alistair Francis
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).