Re: [PATCH 1/5] hw/ppc/spapr_iommu: Register machine reset handler
On Tue, Apr 27, 2021 at 11:20:07AM +0200, Philippe Mathieu-Daudé wrote: > On 4/27/21 3:45 AM, David Gibson wrote: > > On Sat, Apr 24, 2021 at 06:22:25PM +0200, Philippe Mathieu-Daudé wrote: > >> The TYPE_SPAPR_TCE_TABLE device is bus-less, thus isn't reset > >> automatically. Register a reset handler to get reset with the > >> machine. > >> > >> It doesn't seem to be an issue because it is that way since the > >> device QDev'ifycation 8 years ago, in commit a83000f5e3f > >> ("spapr-tce: make sPAPRTCETable a proper device"). > >> Still, correct to have a proper API usage. > > > > So, the reason this works now is that we explicitly call > > device_reset() on the TCE table from the TCE tables "owner", either a > > PHB (spapr_phb_reset()) or a VIO device (spapr_vio_quiesce_one()). > > > > I think we want either that, or the register_reset(), not both. > > rtas_quiesce() seems to call a DeviceClass::reset() on the > children of TYPE_SPAPR_VIO_BUS: > > Abstract TYPE_VIO_SPAPR_DEVICE has the TYPE_SPAPR_VIO_BUS bus_type, > and registers the spapr_vio_busdev_reset() handler, which calls > spapr_vio_quiesce_one()... > > So either we already have 2 resets, or the bus is never reset? There are 2 resets, and this is intentional. We reset once at machine reset time, via the bus. Once a booting OS is done with the firmware it calls "quiesce" to put all the devices back into a safe state. The easiest way to do that is just to invoke their reset callbacks, so that's what we do. > The bus is created in spapr_machine_init(): > > /* Set up VIO bus */ > spapr->vio_bus = spapr_vio_bus_init(); > > TYPE_SPAPR_MACHINE class registers spapr_machine_reset(), which > manually calls qemu_devices_reset() and spapr_drc_reset_all(), > but I can't understand if a callee resets vio_bus... > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH 1/5] hw/ppc/spapr_iommu: Register machine reset handler
On Tue, 27 Apr 2021 11:20:07 +0200 Philippe Mathieu-Daudé wrote: > On 4/27/21 3:45 AM, David Gibson wrote: > > On Sat, Apr 24, 2021 at 06:22:25PM +0200, Philippe Mathieu-Daudé wrote: > >> The TYPE_SPAPR_TCE_TABLE device is bus-less, thus isn't reset > >> automatically. Register a reset handler to get reset with the > >> machine. > >> > >> It doesn't seem to be an issue because it is that way since the > >> device QDev'ifycation 8 years ago, in commit a83000f5e3f > >> ("spapr-tce: make sPAPRTCETable a proper device"). > >> Still, correct to have a proper API usage. > > > > So, the reason this works now is that we explicitly call > > device_reset() on the TCE table from the TCE tables "owner", either a > > PHB (spapr_phb_reset()) or a VIO device (spapr_vio_quiesce_one()). > > > > I think we want either that, or the register_reset(), not both. > > rtas_quiesce() seems to call a DeviceClass::reset() on the > children of TYPE_SPAPR_VIO_BUS: > > Abstract TYPE_VIO_SPAPR_DEVICE has the TYPE_SPAPR_VIO_BUS bus_type, > and registers the spapr_vio_busdev_reset() handler, which calls > spapr_vio_quiesce_one()... > > So either we already have 2 resets, or the bus is never reset? > rtas_quiesce() is called when the guests definitively transition from the SLOF FW to the OS. It isn't a true reset path actually, even if it needs to reset a few devices. On the other hand, your patch would _really_ cause the TCE table device to be reset twice at machine reset AFAICT. > The bus is created in spapr_machine_init(): > > /* Set up VIO bus */ > spapr->vio_bus = spapr_vio_bus_init(); > > TYPE_SPAPR_MACHINE class registers spapr_machine_reset(), which > manually calls qemu_devices_reset() and spapr_drc_reset_all(), > but I can't understand if a callee resets vio_bus... The vio_bus *is* reset: #0 0x000100629a98 in spapr_vio_busdev_reset (qdev=0x10165c400) at /home/greg/Work/qemu/qemu-virtiofs/include/hw/ppc/spapr_vio.h:31 #1 0x0001009fd32c in device_transitional_reset (obj=0x10165c400) at /home/greg/Work/qemu/qemu-virtiofs/include/hw/qdev-core.h:17 #2 0x000100a00e24 in resettable_phase_hold (obj=0x10165c400, opaque=, type=) at ../../hw/core/resettable.c:182 #3 0x0001009f9108 in bus_reset_child_foreach (obj=, cb=0x100a00cc0 , opaque=0x0, type=) at ../../hw/core/bus.c:97 #4 0x000100a00db8 in resettable_child_foreach (rc=0x1014f5400, type=RESET_TYPE_COLD, opaque=0x0, cb=0x100a00cc0 , obj=0x10156e600) at ../../hw/core/resettable.c:96 #5 0x000100a00db8 in resettable_phase_hold (obj=0x10156e600, opaque=, type=) at ../../hw/core/resettable.c:173 #6 0x0001009fcaa8 in device_reset_child_foreach (obj=, cb=0x100a00cc0 , opaque=0x0, type=) at ../../hw/core/qdev.c:366 #7 0x000100a00db8 in resettable_child_foreach (rc=0x1013eef90, type=RESET_TYPE_COLD, opaque=0x0, cb=0x100a00cc0 , obj=0x10164a0e0) at ../../hw/core/resettable.c:96 #8 0x000100a00db8 in resettable_phase_hold (obj=0x10164a0e0, opaque=, type=) at ../../hw/core/resettable.c:173 #9 0x0001009f9108 in bus_reset_child_foreach (obj=, cb=0x100a00cc0 , opaque=0x0, type=) at ../../hw/core/bus.c:97 #10 0x000100a00db8 in resettable_child_foreach (rc=0x1012b1a00, type=RESET_TYPE_COLD, opaque=0x0, cb=0x100a00cc0 , obj=0x10154d4b0) at ../../hw/core/resettable.c:96 #11 0x000100a00db8 in resettable_phase_hold (obj=obj@entry=0x10154d4b0, opaque=opaque@entry=0x0, type=type@entry=RESET_TYPE_COLD) at ../../hw/core/resettable.c:173 #12 0x000100a01794 in resettable_assert_reset (obj=0x10154d4b0, type=) at ../../hw/core/resettable.c:60 #13 0x000100a01c60 in resettable_reset (obj=0x10154d4b0, type=) at ../../hw/core/resettable.c:45 #14 0x000100a020ec in resettable_cold_reset_fn (opaque=) at ../../hw/core/resettable.c:269 #15 0x000100a00718 in qemu_devices_reset () at ../../hw/core/reset.c:69 #16 0x000100624024 in spapr_machine_reset (machine=0x101545480) at ../../hw/ppc/spapr.c:1587 #17 0x0001007b8128 in qemu_system_reset (reason=) at ../../softmmu/runstate.c:442 #18 0x0001007b8fa8 in main_loop_should_exit () at ../../softmmu/runstate.c:687 #19 0x0001007b8fa8 in qemu_main_loop () at ../../softmmu/runstate.c:721 #20 0x0001002f5150 in main (argc=, argv=, envp=) at ../../softmmu/main.c:50 And it seems rtas_quiesce() could just do bus_cold_reset(>bus) rather than open-coding the walk of vio_bus children.
Re: [PATCH 1/5] hw/ppc/spapr_iommu: Register machine reset handler
On 4/27/21 3:45 AM, David Gibson wrote: > On Sat, Apr 24, 2021 at 06:22:25PM +0200, Philippe Mathieu-Daudé wrote: >> The TYPE_SPAPR_TCE_TABLE device is bus-less, thus isn't reset >> automatically. Register a reset handler to get reset with the >> machine. >> >> It doesn't seem to be an issue because it is that way since the >> device QDev'ifycation 8 years ago, in commit a83000f5e3f >> ("spapr-tce: make sPAPRTCETable a proper device"). >> Still, correct to have a proper API usage. > > So, the reason this works now is that we explicitly call > device_reset() on the TCE table from the TCE tables "owner", either a > PHB (spapr_phb_reset()) or a VIO device (spapr_vio_quiesce_one()). > > I think we want either that, or the register_reset(), not both. rtas_quiesce() seems to call a DeviceClass::reset() on the children of TYPE_SPAPR_VIO_BUS: Abstract TYPE_VIO_SPAPR_DEVICE has the TYPE_SPAPR_VIO_BUS bus_type, and registers the spapr_vio_busdev_reset() handler, which calls spapr_vio_quiesce_one()... So either we already have 2 resets, or the bus is never reset? The bus is created in spapr_machine_init(): /* Set up VIO bus */ spapr->vio_bus = spapr_vio_bus_init(); TYPE_SPAPR_MACHINE class registers spapr_machine_reset(), which manually calls qemu_devices_reset() and spapr_drc_reset_all(), but I can't understand if a callee resets vio_bus...
Re: [PATCH 1/5] hw/ppc/spapr_iommu: Register machine reset handler
On Sat, Apr 24, 2021 at 06:22:25PM +0200, Philippe Mathieu-Daudé wrote: > The TYPE_SPAPR_TCE_TABLE device is bus-less, thus isn't reset > automatically. Register a reset handler to get reset with the > machine. > > It doesn't seem to be an issue because it is that way since the > device QDev'ifycation 8 years ago, in commit a83000f5e3f > ("spapr-tce: make sPAPRTCETable a proper device"). > Still, correct to have a proper API usage. So, the reason this works now is that we explicitly call device_reset() on the TCE table from the TCE tables "owner", either a PHB (spapr_phb_reset()) or a VIO device (spapr_vio_quiesce_one()). I think we want either that, or the register_reset(), not both. > > Signed-off-by: Philippe Mathieu-Daudé > --- > hw/ppc/spapr_iommu.c | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > index 24537ffcbd3..f7dad1dc0fe 100644 > --- a/hw/ppc/spapr_iommu.c > +++ b/hw/ppc/spapr_iommu.c > @@ -24,6 +24,7 @@ > #include "sysemu/kvm.h" > #include "kvm_ppc.h" > #include "migration/vmstate.h" > +#include "sysemu/reset.h" > #include "sysemu/dma.h" > #include "exec/address-spaces.h" > #include "trace.h" > @@ -302,6 +303,11 @@ static const VMStateDescription vmstate_spapr_tce_table > = { > } > }; > > +static void spapr_tce_reset_handler(void *dev) > +{ > +device_legacy_reset(DEVICE(dev)); > +} > + > static void spapr_tce_table_realize(DeviceState *dev, Error **errp) > { > SpaprTceTable *tcet = SPAPR_TCE_TABLE(dev); > @@ -324,6 +330,8 @@ static void spapr_tce_table_realize(DeviceState *dev, > Error **errp) > > vmstate_register(VMSTATE_IF(tcet), tcet->liobn, _spapr_tce_table, > tcet); > + > +qemu_register_reset(spapr_tce_reset_handler, dev); > } > > void spapr_tce_set_need_vfio(SpaprTceTable *tcet, bool need_vfio) > @@ -425,6 +433,8 @@ static void spapr_tce_table_unrealize(DeviceState *dev) > { > SpaprTceTable *tcet = SPAPR_TCE_TABLE(dev); > > +qemu_unregister_reset(spapr_tce_reset_handler, dev); > + > vmstate_unregister(VMSTATE_IF(tcet), _spapr_tce_table, tcet); > > QLIST_REMOVE(tcet, list); -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature