| Message ID | 20240805210444.497723-1-gregorhaas1997@gmail.com |
|---|---|
| Headers | show |
| Series | Add support for generating OpenSBI domains in the device tree | expand |
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 >
Hi Daniel, That's correct -- I believe this [1] patch on the OpenSBI mailing list addresses this issue. I am currently waiting for it to be reviewed. Thanks, Gregor [1] http://lists.infradead.org/pipermail/opensbi/2024-August/007240.html On Thu, Aug 22, 2024 at 2:49 PM Daniel Henrique Barboza < dbarboza@ventanamicro.com> wrote: > > > 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 > > >
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 > >
Hi Alistair! On Sun, Sep 8, 2024 at 8:27 PM Alistair Francis <alistair23@gmail.com> wrote: > 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 > No worries -- thanks for your review! > > > > > 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've just sent a v4 patch series which includes documentation! Please let me know what you think. > 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. > I've found this patch series really useful for programmatically generating test fixtures in an isolation system I'm working on, which is built on top of OpenSBI domains. In that sense, I've found generating the correct flags is easier rather than manually editing or generating several dozen device tree files for each test configuration. I take your point that these flags are hard to get right, and there may be more user-friendly ways to do this. FWIW, I that this will only rarely break if QEMU's DTB changes -- the only part that really depends on QEMU's DTB (rather than just adding new information to it) is the device-linking part for memregions, and it gives a loud, direct error if it cannot find the user-specified device. > 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 > I've included this in my v4 patch series as well! > 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 > > > > > Thanks, Gregor
On Wed, Sep 11, 2024 at 7:08 AM Gregor Haas <gregorhaas1997@gmail.com> wrote: > > Hi Alistair! > > On Sun, Sep 8, 2024 at 8:27 PM Alistair Francis <alistair23@gmail.com> wrote: >> >> 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 > > > No worries -- thanks for your review! > >> >> >> > >> > 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've just sent a v4 patch series which includes documentation! Please let > me know what you think. > >> >> 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. > > > I've found this patch series really useful for programmatically generating test > fixtures in an isolation system I'm working on, which is built on top of OpenSBI > domains. In that sense, I've found generating the correct flags is easier rather > than manually editing or generating several dozen device tree files for each > test configuration. That's fair > > I take your point that these flags are hard to get right, and there may be more > user-friendly ways to do this. FWIW, I that this will only rarely break if QEMU's > DTB changes -- the only part that really depends on QEMU's DTB (rather than > just adding new information to it) is the device-linking part for memregions, and > it gives a loud, direct error if it cannot find the user-specified device. Maybe some unit tests would help here as well, to make sure we catch the error when the change happens. Alistair
On Mon, Sep 09, 2024 at 01:27:05PM GMT, Alistair Francis wrote: > 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. > I also feel like this is just pushing the population of device tree nodes from an editor of a .dts file to the QEMU command line. If some generation is needed, then maybe we need a script, possibly one which has the same command line inputs as proposed here. afaik, we haven't typically taken patches which help overlay the generated devicetree with additional nodes. For example, see [1] for one such proposal and rejection. [1] https://lore.kernel.org/all/20210926183410.256484-1-sjg@chromium.org/ Thanks, drew
Hi Drew, On Tue, Sep 17, 2024 at 5:45 AM Andrew Jones <ajones@ventanamicro.com> wrote: > > On Mon, Sep 09, 2024 at 01:27:05PM GMT, Alistair Francis wrote: > > 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. > > > > I also feel like this is just pushing the population of device tree > nodes from an editor of a .dts file to the QEMU command line. If some > generation is needed, then maybe we need a script, possibly one which > has the same command line inputs as proposed here. afaik, we haven't > typically taken patches which help overlay the generated devicetree > with additional nodes. For example, see [1] for one such proposal > and rejection. > > [1] https://lore.kernel.org/all/20210926183410.256484-1-sjg@chromium.org/ > Thanks, > drew Thanks for the link! After reading through that chain, I can see that there is typically high resistance to guest-specific device tree patches. As such, I'll probably abandon this patch series rather than doing more work to clean it up. I do wonder why there is so much resistance to these types of changes when the need for them arises so often in various boot firmwares. Thanks, Gregor
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 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