On Mon, May 8, 2023 at 11:48 AM Li, Yong <yong...@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.mayd...@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...@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...@intel.com> > >> Reviewed-by: LIU Zhiwei <zhiwei_...@linux.alibaba.com> > >> Reviewed-by: Philippe Mathieu-Daudé <phi...@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 > >> > >>
