| Message ID | 20210424162229.3312116-2-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw: Fix reset of bus-less devices | expand |
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é <f4bug@amsat.org> > --- > 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, &vmstate_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), &vmstate_spapr_tce_table, tcet); > > QLIST_REMOVE(tcet, list);
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...
On Tue, 27 Apr 2021 11:20:07 +0200 Philippe Mathieu-Daudé <f4bug@amsat.org> 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 0x0000000100629a98 in spapr_vio_busdev_reset (qdev=0x10165c400) at /home/greg/Work/qemu/qemu-virtiofs/include/hw/ppc/spapr_vio.h:31 #1 0x00000001009fd32c in device_transitional_reset (obj=0x10165c400) at /home/greg/Work/qemu/qemu-virtiofs/include/hw/qdev-core.h:17 #2 0x0000000100a00e24 in resettable_phase_hold (obj=0x10165c400, opaque=<optimized out>, type=<optimized out>) at ../../hw/core/resettable.c:182 #3 0x00000001009f9108 in bus_reset_child_foreach (obj=<optimized out>, cb=0x100a00cc0 <resettable_phase_hold>, opaque=0x0, type=<optimized out>) at ../../hw/core/bus.c:97 #4 0x0000000100a00db8 in resettable_child_foreach (rc=0x1014f5400, type=RESET_TYPE_COLD, opaque=0x0, cb=0x100a00cc0 <resettable_phase_hold>, obj=0x10156e600) at ../../hw/core/resettable.c:96 #5 0x0000000100a00db8 in resettable_phase_hold (obj=0x10156e600, opaque=<optimized out>, type=<optimized out>) at ../../hw/core/resettable.c:173 #6 0x00000001009fcaa8 in device_reset_child_foreach (obj=<optimized out>, cb=0x100a00cc0 <resettable_phase_hold>, opaque=0x0, type=<optimized out>) at ../../hw/core/qdev.c:366 #7 0x0000000100a00db8 in resettable_child_foreach (rc=0x1013eef90, type=RESET_TYPE_COLD, opaque=0x0, cb=0x100a00cc0 <resettable_phase_hold>, obj=0x10164a0e0) at ../../hw/core/resettable.c:96 #8 0x0000000100a00db8 in resettable_phase_hold (obj=0x10164a0e0, opaque=<optimized out>, type=<optimized out>) at ../../hw/core/resettable.c:173 #9 0x00000001009f9108 in bus_reset_child_foreach (obj=<optimized out>, cb=0x100a00cc0 <resettable_phase_hold>, opaque=0x0, type=<optimized out>) at ../../hw/core/bus.c:97 #10 0x0000000100a00db8 in resettable_child_foreach (rc=0x1012b1a00, type=RESET_TYPE_COLD, opaque=0x0, cb=0x100a00cc0 <resettable_phase_hold>, obj=0x10154d4b0) at ../../hw/core/resettable.c:96 #11 0x0000000100a00db8 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 0x0000000100a01794 in resettable_assert_reset (obj=0x10154d4b0, type=<optimized out>) at ../../hw/core/resettable.c:60 #13 0x0000000100a01c60 in resettable_reset (obj=0x10154d4b0, type=<optimized out>) at ../../hw/core/resettable.c:45 #14 0x0000000100a020ec in resettable_cold_reset_fn (opaque=<optimized out>) at ../../hw/core/resettable.c:269 #15 0x0000000100a00718 in qemu_devices_reset () at ../../hw/core/reset.c:69 #16 0x0000000100624024 in spapr_machine_reset (machine=0x101545480) at ../../hw/ppc/spapr.c:1587 #17 0x00000001007b8128 in qemu_system_reset (reason=<optimized out>) at ../../softmmu/runstate.c:442 #18 0x00000001007b8fa8 in main_loop_should_exit () at ../../softmmu/runstate.c:687 #19 0x00000001007b8fa8 in qemu_main_loop () at ../../softmmu/runstate.c:721 #20 0x00000001002f5150 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at ../../softmmu/main.c:50 And it seems rtas_quiesce() could just do bus_cold_reset(&bus->bus) rather than open-coding the walk of vio_bus children.
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... >
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, &vmstate_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), &vmstate_spapr_tce_table, tcet); QLIST_REMOVE(tcet, list);
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. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/ppc/spapr_iommu.c | 10 ++++++++++ 1 file changed, 10 insertions(+)