Re: [PATCH v3 0/1] Add support for generating OpenSBI domains in the device tree

[Alistair Francis]

On Tue, Aug 6, 2024 at 7:05 AM Gregor Haas <gregorhaas1997@gmail.com> wrote:
>
> This patch series adds support for specifying OpenSBI domains on the QEMU
> command line. A simple example of what this looks like is below, including
> mapping the board's UART into the secondary domain:

Thanks for the patch, sorry it took me so long to look into this

>
> qemu-system-riscv64 -machine virt -bios fw_jump.bin -cpu max -smp 2 -m 4G 
> -nographic \
>         -device opensbi-memregion,id=mem,base=0xBC000000,order=26,mmio=false \
>         -device 
> opensbi-memregion,id=uart,base=0x10000000,order=12,mmio=true,device0="/soc/serial@10000000"
>  \
>         -device 
> opensbi-domain,id=domain,possible-harts=0-1,boot-hart=0x0,next-addr=0xBC000000,next-mode=1,region0=mem,perms0=0x3f,region1=uart,perms1=0x3f

This will need documentation added under docs (probably under
docs/system/riscv) of how this should be used.

I'm not convinced this is something we want though. A user can dump
the QEMU DTB and edit it to support OpenSBI domains if they want.

My worry is that the command line method is complex for users to get
right and will be fragile and prone to breakage as parts of QEMU's DTB
changes.

Also, how are users supposed to know what options to use? Maybe some
documentation will help clear up how this should be used by users

>
> As a result of the above configuration, QEMU will add the following subnodes 
> to
> the device tree:
>
> chosen {
>         opensbi-domains {
>                 compatible = "opensbi,domain,config";
>
>                 domain {
>                         next-mode = <0x01>;
>                         next-addr = <0x00 0xbc000000>;
>                         boot-hart = <0x03>;
>                         regions = <0x8000 0x3f 0x8002 0x3f>;
>                         possible-harts = <0x03 0x01>;
>                         phandle = <0x8003>;
>                         compatible = "opensbi,domain,instance";
>                 };
>
>                 uart {
>                         phandle = <0x8002>;
>                         devices = <0x1800000>;
>                         mmio;
>                         order = <0x0c>;
>                         base = <0x00 0x10000000>;
>                         compatible = "opensbi,domain,memregion";
>                 };
>
>                 mem {
>                         phandle = <0x8000>;
>                         order = <0x1a>;
>                         base = <0x00 0xbc000000>;
>                         compatible = "opensbi,domain,memregion";
>                 };
>         };
> };
>
> This results in OpenSBI output as below, where regions 01-03 are inherited 
> from
> the root domain and regions 00 and 04 correspond to the user specified ones:
>
> Domain1 Name              : domain
> Domain1 Boot HART         : 0
> Domain1 HARTs             : 0,1
> Domain1 Region00          : 0x0000000010000000-0x0000000010000fff M: 
> (I,R,W,X) S/U: (R,W,X)
> Domain1 Region01          : 0x0000000002000000-0x000000000200ffff M: (I,R,W) 
> S/U: ()
> Domain1 Region02          : 0x0000000080080000-0x000000008009ffff M: (R,W) 
> S/U: ()
> Domain1 Region03          : 0x0000000080000000-0x000000008007ffff M: (R,X) 
> S/U: ()
> Domain1 Region04          : 0x00000000bc000000-0x00000000bfffffff M: (R,W,X) 
> S/U: (R,W,X)
> Domain1 Next Address      : 0x00000000bc000000
> Domain1 Next Arg1         : 0x0000000000000000
> Domain1 Next Mode         : S-mode
> Domain1 SysReset          : no
> Domain1 SysSuspend        : no
>
> v3:
> - Addressed review comments from v2 by adding default values to new 
> properties.
>   This results in concrete errors at QEMU configuration time if a mandatory
>   property (as mandated by the OpenSBI spec) is not provided.
> - Changed command line encoding for the possible-harts field from a CPU 
> bitmask
>   (e.g. where bit X is set if CPU X is a possible hart) to a range format 
> (e.g.
>   the possible harts should be CPUs X-Y, where Y >= X). This does constrain 
> the
>   hart assignment to consecutive ranges of harts, but this constraint is also
>   present for other QEMU subsystems (such as NUMA).
> - Added create_fdt_one_device(), which is invoked when scanning the device 
> tree
>   for a memregion's devices. This function allocates a phandle for a region's
>   device if one does not yet exist.
>
> v2:
> - Addressed review comments from v1. Specifically, renamed domain.{c,h} ->
>   opensbi_domain.{c,h} to increase clarity of what these files do. Also, more
>   consistently use g_autofree for dynamically allocated variables
> - Added an "assign" flag to OpenSBIDomainState, which indicates whether to
>   assign the domain's boot hart to it at domain parsing time.
>
> Gregor Haas (1):
>   Add support for generating OpenSBI domains in the device tree
>
>  MAINTAINERS                       |   7 +
>  hw/riscv/Kconfig                  |   4 +
>  hw/riscv/meson.build              |   1 +
>  hw/riscv/opensbi_domain.c         | 542 ++++++++++++++++++++++++++++++

There should be a license comment at the start of new files. Have a
look at other QEMU files for examples

Alistair

>  hw/riscv/virt.c                   |   3 +
>  include/hw/riscv/opensbi_domain.h |  50 +++
>  6 files changed, 607 insertions(+)
>  create mode 100644 hw/riscv/opensbi_domain.c
>  create mode 100644 include/hw/riscv/opensbi_domain.h
>
> --
> 2.45.2
>
>