| Message ID | 160309734178.2739814.3488437759887793902.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: Error handling fixes and cleanups (round 3) | expand |
On 10/19/20 10:49 AM, Greg Kurz wrote: > As recommended in "qapi/error.h", add a bool return value to > spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead > of local_err in spapr_memory_plug(). > > This allows to get rid of the error propagation overhead. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/ppc/spapr.c | 23 ++++++++++------------- > hw/ppc/spapr_nvdimm.c | 5 +++-- > include/hw/ppc/spapr_nvdimm.h | 2 +- > 3 files changed, 14 insertions(+), 16 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 62f217a6b914..0cc19b5863a4 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3382,7 +3382,7 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr, > return 0; > } > > -static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, > +static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, > bool dedicated_hp_event_source, Error **errp) > { > SpaprDrc *drc; > @@ -3403,7 +3403,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, > addr / SPAPR_MEMORY_BLOCK_SIZE); > spapr_drc_detach(drc); > } > - return; > + return false; > } > if (!hotplugged) { > spapr_drc_reset(drc); > @@ -3425,12 +3425,12 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, > nr_lmbs); > } > } > + return true; > } > > static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > Error **errp) > { > - Error *local_err = NULL; > SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev); > PCDIMMDevice *dimm = PC_DIMM(dev); > uint64_t size, addr; > @@ -3444,27 +3444,24 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > if (!is_nvdimm) { > addr = object_property_get_uint(OBJECT(dimm), > PC_DIMM_ADDR_PROP, &error_abort); > - spapr_add_lmbs(dev, addr, size, > - spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), > - &local_err); > + if (!spapr_add_lmbs(dev, addr, size, > + spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) { > + goto out_unplug; > + } > } else { > slot = object_property_get_int(OBJECT(dimm), > PC_DIMM_SLOT_PROP, &error_abort); > /* We should have valid slot number at this point */ > g_assert(slot >= 0); > - spapr_add_nvdimm(dev, slot, &local_err); > - } > - > - if (local_err) { > - goto out_unplug; > + if (!spapr_add_nvdimm(dev, slot, errp)) { > + goto out_unplug; > + } > } > > return; > > out_unplug: > pc_dimm_unplug(dimm, MACHINE(ms)); > -out: > - error_propagate(errp, local_err); > } > > static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c > index 9e3d94071fe1..a833a63b5ed3 100644 > --- a/hw/ppc/spapr_nvdimm.c > +++ b/hw/ppc/spapr_nvdimm.c > @@ -89,7 +89,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm, > } > > > -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) > +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) > { > SpaprDrc *drc; > bool hotplugged = spapr_drc_hotplugged(dev); > @@ -98,12 +98,13 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) > g_assert(drc); > > if (!spapr_drc_attach(drc, dev, errp)) { > - return; > + return false; > } > > if (hotplugged) { > spapr_hotplug_req_add_by_index(drc); > } > + return true; > } > > static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt, > diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h > index 490b19a009f4..344582d2f5f7 100644 > --- a/include/hw/ppc/spapr_nvdimm.h > +++ b/include/hw/ppc/spapr_nvdimm.h > @@ -30,6 +30,6 @@ int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr, > void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt); > bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm, > uint64_t size, Error **errp); > -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp); > +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp); Maybe document the returned value? Otherwise: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > #endif > > >
On Mon, 19 Oct 2020 10:49:01 +0200 Greg Kurz <groug@kaod.org> wrote: > As recommended in "qapi/error.h", add a bool return value to > spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead > of local_err in spapr_memory_plug(). > > This allows to get rid of the error propagation overhead. > > Signed-off-by: Greg Kurz <groug@kaod.org> you won't need this patch if check from spapr_drc_attach() were moved to _pre_plug() time. > --- > hw/ppc/spapr.c | 23 ++++++++++------------- > hw/ppc/spapr_nvdimm.c | 5 +++-- > include/hw/ppc/spapr_nvdimm.h | 2 +- > 3 files changed, 14 insertions(+), 16 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 62f217a6b914..0cc19b5863a4 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3382,7 +3382,7 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr, > return 0; > } > > -static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, > +static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, > bool dedicated_hp_event_source, Error **errp) > { > SpaprDrc *drc; > @@ -3403,7 +3403,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, > addr / SPAPR_MEMORY_BLOCK_SIZE); > spapr_drc_detach(drc); > } > - return; > + return false; > } > if (!hotplugged) { > spapr_drc_reset(drc); > @@ -3425,12 +3425,12 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, > nr_lmbs); > } > } > + return true; > } > > static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > Error **errp) > { > - Error *local_err = NULL; > SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev); > PCDIMMDevice *dimm = PC_DIMM(dev); > uint64_t size, addr; > @@ -3444,27 +3444,24 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > if (!is_nvdimm) { > addr = object_property_get_uint(OBJECT(dimm), > PC_DIMM_ADDR_PROP, &error_abort); > - spapr_add_lmbs(dev, addr, size, > - spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), > - &local_err); > + if (!spapr_add_lmbs(dev, addr, size, > + spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) { > + goto out_unplug; > + } > } else { > slot = object_property_get_int(OBJECT(dimm), > PC_DIMM_SLOT_PROP, &error_abort); > /* We should have valid slot number at this point */ > g_assert(slot >= 0); > - spapr_add_nvdimm(dev, slot, &local_err); > - } > - > - if (local_err) { > - goto out_unplug; > + if (!spapr_add_nvdimm(dev, slot, errp)) { > + goto out_unplug; > + } > } > > return; > > out_unplug: > pc_dimm_unplug(dimm, MACHINE(ms)); > -out: > - error_propagate(errp, local_err); > } > > static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c > index 9e3d94071fe1..a833a63b5ed3 100644 > --- a/hw/ppc/spapr_nvdimm.c > +++ b/hw/ppc/spapr_nvdimm.c > @@ -89,7 +89,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm, > } > > > -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) > +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) > { > SpaprDrc *drc; > bool hotplugged = spapr_drc_hotplugged(dev); > @@ -98,12 +98,13 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) > g_assert(drc); > > if (!spapr_drc_attach(drc, dev, errp)) { > - return; > + return false; > } > > if (hotplugged) { > spapr_hotplug_req_add_by_index(drc); > } > + return true; > } > > static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt, > diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h > index 490b19a009f4..344582d2f5f7 100644 > --- a/include/hw/ppc/spapr_nvdimm.h > +++ b/include/hw/ppc/spapr_nvdimm.h > @@ -30,6 +30,6 @@ int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr, > void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt); > bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm, > uint64_t size, Error **errp); > -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp); > +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp); > > #endif > >
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 62f217a6b914..0cc19b5863a4 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3382,7 +3382,7 @@ int spapr_lmb_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr, return 0; } -static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, +static bool spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, bool dedicated_hp_event_source, Error **errp) { SpaprDrc *drc; @@ -3403,7 +3403,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, addr / SPAPR_MEMORY_BLOCK_SIZE); spapr_drc_detach(drc); } - return; + return false; } if (!hotplugged) { spapr_drc_reset(drc); @@ -3425,12 +3425,12 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, nr_lmbs); } } + return true; } static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { - Error *local_err = NULL; SpaprMachineState *ms = SPAPR_MACHINE(hotplug_dev); PCDIMMDevice *dimm = PC_DIMM(dev); uint64_t size, addr; @@ -3444,27 +3444,24 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, if (!is_nvdimm) { addr = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP, &error_abort); - spapr_add_lmbs(dev, addr, size, - spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), - &local_err); + if (!spapr_add_lmbs(dev, addr, size, + spapr_ovec_test(ms->ov5_cas, OV5_HP_EVT), errp)) { + goto out_unplug; + } } else { slot = object_property_get_int(OBJECT(dimm), PC_DIMM_SLOT_PROP, &error_abort); /* We should have valid slot number at this point */ g_assert(slot >= 0); - spapr_add_nvdimm(dev, slot, &local_err); - } - - if (local_err) { - goto out_unplug; + if (!spapr_add_nvdimm(dev, slot, errp)) { + goto out_unplug; + } } return; out_unplug: pc_dimm_unplug(dimm, MACHINE(ms)); -out: - error_propagate(errp, local_err); } static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c index 9e3d94071fe1..a833a63b5ed3 100644 --- a/hw/ppc/spapr_nvdimm.c +++ b/hw/ppc/spapr_nvdimm.c @@ -89,7 +89,7 @@ bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm, } -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) { SpaprDrc *drc; bool hotplugged = spapr_drc_hotplugged(dev); @@ -98,12 +98,13 @@ void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp) g_assert(drc); if (!spapr_drc_attach(drc, dev, errp)) { - return; + return false; } if (hotplugged) { spapr_hotplug_req_add_by_index(drc); } + return true; } static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt, diff --git a/include/hw/ppc/spapr_nvdimm.h b/include/hw/ppc/spapr_nvdimm.h index 490b19a009f4..344582d2f5f7 100644 --- a/include/hw/ppc/spapr_nvdimm.h +++ b/include/hw/ppc/spapr_nvdimm.h @@ -30,6 +30,6 @@ int spapr_pmem_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr, void spapr_dt_persistent_memory(SpaprMachineState *spapr, void *fdt); bool spapr_nvdimm_validate(HotplugHandler *hotplug_dev, NVDIMMDevice *nvdimm, uint64_t size, Error **errp); -void spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp); +bool spapr_add_nvdimm(DeviceState *dev, uint64_t slot, Error **errp); #endif
As recommended in "qapi/error.h", add a bool return value to spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead of local_err in spapr_memory_plug(). This allows to get rid of the error propagation overhead. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr.c | 23 ++++++++++------------- hw/ppc/spapr_nvdimm.c | 5 +++-- include/hw/ppc/spapr_nvdimm.h | 2 +- 3 files changed, 14 insertions(+), 16 deletions(-)