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

[Daniel Henrique Barboza]

On 8/5/24 6:04 PM, Gregor Haas 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:
>
> 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
>
> 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

I believe we need OpenSBI patches for this output, don't we? If I try this 
example using stock
OpenSBI 1.5.1 from QEMU this happens:


OpenSBI v1.5.1
   ____                    _____ ____ _____
  / __ \                  / ____|  _ \_   _|
 | |  | |_ __   ___ _ __ | (___ | |_) || |
 | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
 | |__| | |_) |  __/ | | |____) | |_) || |_
  \____/| .__/ \___|_| |_|_____/|____/_____|
        | |
        |_|

sbi_domain_finalize: platform domains_init() failed (error -3)
init_coldboot: domain finalize failed (error -3)
(--- hangs ---)

It's not a big deal or a blocker to have this merged in QEMU regardless, but it 
would be nice
to have this documented somewhere (perhaps a new doc file). This would prevent 
users from trying
to use the device without the proper support.

This can be done after this patch is queued though. Thanks,


Daniel


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