| Message ID | 20220223090706.4888-1-damien.hedde@greensocs.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Initial support for machine creation via QMP | expand |
Hi Philippe, I suppose it is ok if I change your mail in the reviewed by ? Thanks, Damien On 2/23/22 10:07, Damien Hedde wrote: > It allows adding a subregion to a memory region with error handling. > Like memory_region_add_subregion_overlap(), it handles priority as > well. Apart from the error handling, the behavior is the same. It > can be used to do the simple memory_region_add_subregion() (with no > overlap) by setting the priority parameter to 0. > > This commit is a preparation to further use of this function in the > context of qapi command which needs error handling support. > > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Reviewed-by: David Hildenbrand <david@redhat.com> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > --- > include/exec/memory.h | 22 ++++++++++++++++++++++ > softmmu/memory.c | 23 +++++++++++++++-------- > 2 files changed, 37 insertions(+), 8 deletions(-) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 4d5997e6bb..070dcb5255 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -2215,6 +2215,28 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr, > MemoryRegion *subregion, > int priority); > > +/** > + * memory_region_try_add_subregion: Add a subregion to a container > + * with error handling. > + * > + * Behaves like memory_region_add_subregion_overlap(), but errors are > + * reported if the subregion cannot be added. > + * > + * @mr: the region to contain the new subregion; must be a container > + * initialized with memory_region_init(). > + * @offset: the offset relative to @mr where @subregion is added. > + * @subregion: the subregion to be added. > + * @priority: used for resolving overlaps; highest priority wins. > + * @errp: pointer to Error*, to store an error if it happens. > + * > + * Returns: True in case of success, false otherwise. > + */ > +bool memory_region_try_add_subregion(MemoryRegion *mr, > + hwaddr offset, > + MemoryRegion *subregion, > + int priority, > + Error **errp); > + > /** > * memory_region_get_ram_addr: Get the ram address associated with a memory > * region > diff --git a/softmmu/memory.c b/softmmu/memory.c > index 678dc62f06..6bc76bf6da 100644 > --- a/softmmu/memory.c > +++ b/softmmu/memory.c > @@ -2541,27 +2541,34 @@ done: > memory_region_transaction_commit(); > } > > -static void memory_region_add_subregion_common(MemoryRegion *mr, > - hwaddr offset, > - MemoryRegion *subregion) > +bool memory_region_try_add_subregion(MemoryRegion *mr, > + hwaddr offset, > + MemoryRegion *subregion, > + int priority, > + Error **errp) > { > MemoryRegion *alias; > > - assert(!subregion->container); > + if (subregion->container) { > + error_setg(errp, "The memory region is already in another region"); > + return false; > + } > + > + subregion->priority = priority; > subregion->container = mr; > for (alias = subregion->alias; alias; alias = alias->alias) { > alias->mapped_via_alias++; > } > subregion->addr = offset; > memory_region_update_container_subregions(subregion); > + return true; > } > > void memory_region_add_subregion(MemoryRegion *mr, > hwaddr offset, > MemoryRegion *subregion) > { > - subregion->priority = 0; > - memory_region_add_subregion_common(mr, offset, subregion); > + memory_region_try_add_subregion(mr, offset, subregion, 0, &error_abort); > } > > void memory_region_add_subregion_overlap(MemoryRegion *mr, > @@ -2569,8 +2576,8 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr, > MemoryRegion *subregion, > int priority) > { > - subregion->priority = priority; > - memory_region_add_subregion_common(mr, offset, subregion); > + memory_region_try_add_subregion(mr, offset, subregion, priority, > + &error_abort); > } > > void memory_region_del_subregion(MemoryRegion *mr,
Ping ! It would be good to have some feedback on 1st and 2nd part. Thanks, Damien On 2/23/22 10:06, Damien Hedde wrote: > Hi, > > This series adds initial support to build a machine using QMP/QAPI > commands. With this series, one can start from the 'none' machine, > create cpus, sysbus devices, memory map them and wire interrupts. > > Sorry for the huge cc list on this cover-letter. Apart from people > who attended the kvm call about this topic, I've cc'ed you only > according to MAINTAINERS file. > > The series is divided in 4 parts which are independent of each other, > but we need the 4 parts to be able to use this mechanism: > + Patches 1 to 6 allow to use the qapi command device_add to cold > plug devices (like CLI -device do) > + Patches 7 to 10 modify the 'none' machine which serves as base > machine. > + Patches 11 to 13 handle memory mapping and memory creation > + Patches 14 allows dynamic cold plug of opentitan/sifive_e machine > to build some example. This last patch is based on a cleanup > series: it probably works without it, but some config errors are > not handled (see based-on below). > > Only patch 11 is reviewed-by. > > v4: > + cold plugging approach changed in order not to conflict with > startup. I do not add additional command to handle this so that > we can change everything easily. > + device_add in cold plug context is also now equivalent to -device > CLI regarding -fw_cfg. I also added patches to modify the 'none' > machine. > + reworked most of the none machine part > + updated the sybus-mmio-map command patch > > Note that there are still lot of limitations (for example if you try > to create more cpus than the _max_cpus_, tcg will abort()). > Basically all tasks done by machine init reading some parameters are > really tricky: for example, loading complex firmware. But we have to > start by something and all this is not accessible unless the user > asked for none machine and -preconfig. > > I can maintain the code introduced here. I'm not sure what's the > process. Is there something else to do than propose a patch to > MAINTAINERS ? > If there is a global agreement on moving on with these feature, it > would be great to have a login on qemu wiki so I can document > limitations and the work being done to solve them. > > A simple test can be done with the following scenario which build > a machine subset of the opentitan. > > $ cat commands.qmp > // RAM 0x10000000 > device_add driver=sysbus-memory id=ram size=0x4000 readonly=false > sysbus-mmio-map device=ram addr=268435456 > // CPUS > device_add driver=riscv.hart_array id=cpus cpu-type=lowrisc-ibex-riscv-cpu num-harts=1 resetvec=0x8080 > // ROM 0x00008000 > device_add driver=sysbus-memory id=rom size=0x4000 readonly=true > sysbus-mmio-map device=rom addr=32768 > // PLIC 0x48000000 > device_add driver=riscv.sifive.plic id=plic hart-config=M hartid-base=0 num-sources=180 num-priorities=3 priority-base=0x0 pending-base=0x1000 enable-base=0x2000 enable-stride=32 context-base=0x200000 context-stride=8 aperture-size=0x4005000 > sysbus-mmio-map device=plic addr=1207959552 > qom-set path=plic property=unnamed-gpio-out[1] value=cpus/harts[0]/unnamed-gpio-in[11] > // UART 0x40000000 > device_add driver=ibex-uart id=uart chardev=serial0 > sysbus-mmio-map device=uart addr=1073741824 > qom-set path=uart property=sysbus-irq[1] value=plic/unnamed-gpio-in[2] > // FIRMWARE > device_add driver=loader cpu-num=0 file=/path/to/firmware.elf > x-exit-preconfig > > $ qemu-system-riscv32 -display none -M none -preconfig -serial stdio -qmp unix:/tmp/qmp-sock,server > > In another terminal, you'll need to send the commands with, for example: > $ grep -v '^//' commands.qmp | qmp-shell /tmp/qmp-sock -v > > It is the same as running > $ qemu-system-riscv32 -display none -M opentitan -serial stdio -kernel path/to/firmware.elf > > If you need a firmware, you can pick this one > https://github.com/GreenSocs/qemu-qmp-machines/blob/master/opentitan-echo.elf > This firmware is just a small interrupt-based bare-metal program > echoing back whatever is sent in the uart. > > This repo contains also sifive_e machine example. > > Based-on: <20220218164646.132112-1-damien.hedde@greensocs.com> > "RiscV cleanups for user-related life cycles" > > Thanks for your comments, > -- > Damien > > Damien Hedde (13): > machine: add phase_get() and document phase_check()/advance() > machine&vl: introduce phase_until() to handle phase transitions > vl: support machine-initialized target in phase_until() > qapi/device_add: compute is_hotplug flag > qapi/device_add: handle the rom_order_override when cold-plugging > none-machine: add the NoneMachineState structure > none-machine: add 'ram-addr' property > none-machine: allow cold plugging sysbus devices > none-machine: allow several cpus > softmmu/memory: add memory_region_try_add_subregion function > add sysbus-mmio-map qapi command > hw/mem/system-memory: add a memory sysbus device > hw: set user_creatable on opentitan/sifive_e devices > > Mirela Grujic (1): > qapi/device_add: Allow execution in machine initialized phase > > qapi/qdev.json | 34 +++++++++++- > include/exec/memory.h | 22 ++++++++ > include/hw/mem/sysbus-memory.h | 28 ++++++++++ > include/hw/qdev-core.h | 33 ++++++++++++ > hw/char/ibex_uart.c | 1 + > hw/char/sifive_uart.c | 1 + > hw/core/null-machine.c | 68 ++++++++++++++++++++++-- > hw/core/qdev.c | 5 ++ > hw/core/sysbus.c | 49 ++++++++++++++++++ > hw/gpio/sifive_gpio.c | 1 + > hw/intc/riscv_aclint.c | 2 + > hw/intc/sifive_plic.c | 1 + > hw/mem/sysbus-memory.c | 80 +++++++++++++++++++++++++++++ > hw/misc/sifive_e_prci.c | 8 +++ > hw/misc/unimp.c | 1 + > hw/riscv/riscv_hart.c | 1 + > hw/timer/ibex_timer.c | 1 + > monitor/misc.c | 2 +- > softmmu/memory.c | 23 ++++++--- > softmmu/qdev-monitor.c | 20 +++++++- > softmmu/vl.c | 94 ++++++++++++++++++++++++++-------- > hmp-commands.hx | 1 + > hw/mem/meson.build | 2 + > 23 files changed, 439 insertions(+), 39 deletions(-) > create mode 100644 include/hw/mem/sysbus-memory.h > create mode 100644 hw/mem/sysbus-memory.c >
On 23/2/22 10:12, Damien Hedde wrote: > Hi Philippe, > > I suppose it is ok if I change your mail in the reviewed by ? No, the email is fine (git tools should take care of using the correct email via the .mailmap entry, see commit 90f285fd83). > Thanks, > Damien > > On 2/23/22 10:07, Damien Hedde wrote: >> It allows adding a subregion to a memory region with error handling. >> Like memory_region_add_subregion_overlap(), it handles priority as >> well. Apart from the error handling, the behavior is the same. It >> can be used to do the simple memory_region_add_subregion() (with no >> overlap) by setting the priority parameter to 0. >> >> This commit is a preparation to further use of this function in the >> context of qapi command which needs error handling support. >> >> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> Reviewed-by: David Hildenbrand <david@redhat.com> >> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >> --- >> include/exec/memory.h | 22 ++++++++++++++++++++++ >> softmmu/memory.c | 23 +++++++++++++++-------- >> 2 files changed, 37 insertions(+), 8 deletions(-)
On 3/3/22 14:32, Philippe Mathieu-Daudé wrote: > On 23/2/22 10:12, Damien Hedde wrote: >> Hi Philippe, >> >> I suppose it is ok if I change your mail in the reviewed by ? > > No, the email is fine (git tools should take care of using the > correct email via the .mailmap entry, see commit 90f285fd83). > >> Thanks, >> Damien >> ok. Looks like git keeps as-is the "*-by:" entries untouched when cc-ing them. -- Damien
Hi all, Thanks for the work! I'm from SiFive and we are very interested in this feature. QMP/QAPI configurable QEMU machine is a useful feature in our use case. With this feature, we can both model our versatile FPGA-based platforms more easily and model a new platform without modification of source code. It is helpful for early software development of SoC prototyping. We think this feature is also helpful to the QEMU community. Also, I have tested this patchset (v4) and newer v5 patchset [1] with Damien's firmware [2] and it works correctly. p.s. QMP option "-qmp socket,path=./qmpsocket,server" in v5 patchset instruction may not work? I use the option "-qmp unix:./qmpsocket,server" instead. [1] [PATCH v5 0/6] QAPI support for device cold-plug https://lore.kernel.org/qemu-devel/20220519153402.41540-1-damien.hedde@greensocs.com/ [2] Test firmware for patchset v5: https://github.com/GreenSocs/qemu-qmp-machines/tree/master/arm-virt v4: https://github.com/GreenSocs/qemu-qmp-machines/tree/eba16dab8b587e624d65c5c302aeef424bece3a0 On Thu, Mar 3, 2022 at 7:02 PM Damien Hedde <damien.hedde@greensocs.com> wrote: > Ping ! > > It would be good to have some feedback on 1st and 2nd part. > > Thanks, > Damien > > On 2/23/22 10:06, Damien Hedde wrote: > > Hi, > > > > This series adds initial support to build a machine using QMP/QAPI > > commands. With this series, one can start from the 'none' machine, > > create cpus, sysbus devices, memory map them and wire interrupts. > > > > Sorry for the huge cc list on this cover-letter. Apart from people > > who attended the kvm call about this topic, I've cc'ed you only > > according to MAINTAINERS file. > > > > The series is divided in 4 parts which are independent of each other, > > but we need the 4 parts to be able to use this mechanism: > > + Patches 1 to 6 allow to use the qapi command device_add to cold > > plug devices (like CLI -device do) > > + Patches 7 to 10 modify the 'none' machine which serves as base > > machine. > > + Patches 11 to 13 handle memory mapping and memory creation > > + Patches 14 allows dynamic cold plug of opentitan/sifive_e machine > > to build some example. This last patch is based on a cleanup > > series: it probably works without it, but some config errors are > > not handled (see based-on below). > > > > Only patch 11 is reviewed-by. > > > > v4: > > + cold plugging approach changed in order not to conflict with > > startup. I do not add additional command to handle this so that > > we can change everything easily. > > + device_add in cold plug context is also now equivalent to -device > > CLI regarding -fw_cfg. I also added patches to modify the 'none' > > machine. > > + reworked most of the none machine part > > + updated the sybus-mmio-map command patch > > > > Note that there are still lot of limitations (for example if you try > > to create more cpus than the _max_cpus_, tcg will abort()). > > Basically all tasks done by machine init reading some parameters are > > really tricky: for example, loading complex firmware. But we have to > > start by something and all this is not accessible unless the user > > asked for none machine and -preconfig. > > > > I can maintain the code introduced here. I'm not sure what's the > > process. Is there something else to do than propose a patch to > > MAINTAINERS ? > > If there is a global agreement on moving on with these feature, it > > would be great to have a login on qemu wiki so I can document > > limitations and the work being done to solve them. > > > > A simple test can be done with the following scenario which build > > a machine subset of the opentitan. > > > > $ cat commands.qmp > > // RAM 0x10000000 > > device_add driver=sysbus-memory id=ram size=0x4000 readonly=false > > sysbus-mmio-map device=ram addr=268435456 > > // CPUS > > device_add driver=riscv.hart_array id=cpus > cpu-type=lowrisc-ibex-riscv-cpu num-harts=1 resetvec=0x8080 > > // ROM 0x00008000 > > device_add driver=sysbus-memory id=rom size=0x4000 readonly=true > > sysbus-mmio-map device=rom addr=32768 > > // PLIC 0x48000000 > > device_add driver=riscv.sifive.plic id=plic hart-config=M hartid-base=0 > num-sources=180 num-priorities=3 priority-base=0x0 pending-base=0x1000 > enable-base=0x2000 enable-stride=32 context-base=0x200000 context-stride=8 > aperture-size=0x4005000 > > sysbus-mmio-map device=plic addr=1207959552 > > qom-set path=plic property=unnamed-gpio-out[1] > value=cpus/harts[0]/unnamed-gpio-in[11] > > // UART 0x40000000 > > device_add driver=ibex-uart id=uart chardev=serial0 > > sysbus-mmio-map device=uart addr=1073741824 > > qom-set path=uart property=sysbus-irq[1] value=plic/unnamed-gpio-in[2] > > // FIRMWARE > > device_add driver=loader cpu-num=0 file=/path/to/firmware.elf > > x-exit-preconfig > > > > $ qemu-system-riscv32 -display none -M none -preconfig -serial stdio > -qmp unix:/tmp/qmp-sock,server > > > > In another terminal, you'll need to send the commands with, for example: > > $ grep -v '^//' commands.qmp | qmp-shell /tmp/qmp-sock -v > > > > It is the same as running > > $ qemu-system-riscv32 -display none -M opentitan -serial stdio -kernel > path/to/firmware.elf > > > > If you need a firmware, you can pick this one > > > https://github.com/GreenSocs/qemu-qmp-machines/blob/master/opentitan-echo.elf > > This firmware is just a small interrupt-based bare-metal program > > echoing back whatever is sent in the uart. > > > > This repo contains also sifive_e machine example. > > > > Based-on: <20220218164646.132112-1-damien.hedde@greensocs.com> > > "RiscV cleanups for user-related life cycles" > > > > Thanks for your comments, > > -- > > Damien > > > > Damien Hedde (13): > > machine: add phase_get() and document phase_check()/advance() > > machine&vl: introduce phase_until() to handle phase transitions > > vl: support machine-initialized target in phase_until() > > qapi/device_add: compute is_hotplug flag > > qapi/device_add: handle the rom_order_override when cold-plugging > > none-machine: add the NoneMachineState structure > > none-machine: add 'ram-addr' property > > none-machine: allow cold plugging sysbus devices > > none-machine: allow several cpus > > softmmu/memory: add memory_region_try_add_subregion function > > add sysbus-mmio-map qapi command > > hw/mem/system-memory: add a memory sysbus device > > hw: set user_creatable on opentitan/sifive_e devices > > > > Mirela Grujic (1): > > qapi/device_add: Allow execution in machine initialized phase > > > > qapi/qdev.json | 34 +++++++++++- > > include/exec/memory.h | 22 ++++++++ > > include/hw/mem/sysbus-memory.h | 28 ++++++++++ > > include/hw/qdev-core.h | 33 ++++++++++++ > > hw/char/ibex_uart.c | 1 + > > hw/char/sifive_uart.c | 1 + > > hw/core/null-machine.c | 68 ++++++++++++++++++++++-- > > hw/core/qdev.c | 5 ++ > > hw/core/sysbus.c | 49 ++++++++++++++++++ > > hw/gpio/sifive_gpio.c | 1 + > > hw/intc/riscv_aclint.c | 2 + > > hw/intc/sifive_plic.c | 1 + > > hw/mem/sysbus-memory.c | 80 +++++++++++++++++++++++++++++ > > hw/misc/sifive_e_prci.c | 8 +++ > > hw/misc/unimp.c | 1 + > > hw/riscv/riscv_hart.c | 1 + > > hw/timer/ibex_timer.c | 1 + > > monitor/misc.c | 2 +- > > softmmu/memory.c | 23 ++++++--- > > softmmu/qdev-monitor.c | 20 +++++++- > > softmmu/vl.c | 94 ++++++++++++++++++++++++++-------- > > hmp-commands.hx | 1 + > > hw/mem/meson.build | 2 + > > 23 files changed, 439 insertions(+), 39 deletions(-) > > create mode 100644 include/hw/mem/sysbus-memory.h > > create mode 100644 hw/mem/sysbus-memory.c > > > >
Tested-by: Jim Shu <jim.shu@sifive.com> On Fri, Mar 4, 2022 at 7:00 PM Damien Hedde <damien.hedde@greensocs.com> wrote: > > > > On 3/3/22 14:32, Philippe Mathieu-Daudé wrote: > > On 23/2/22 10:12, Damien Hedde wrote: > >> Hi Philippe, > >> > >> I suppose it is ok if I change your mail in the reviewed by ? > > > > No, the email is fine (git tools should take care of using the > > correct email via the .mailmap entry, see commit 90f285fd83). > > > >> Thanks, > >> Damien > >> > > ok. > > Looks like git keeps as-is the "*-by:" entries untouched when cc-ing them. > > -- > Damien > >