| Message ID | 20180611121655.19616-8-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | pc-dimm: next bunch of cleanups | expand |
On Mon, Jun 11, 2018 at 02:16:51PM +0200, David Hildenbrand wrote: > We already verify when realizing that the memdev property has been > set. We have no more accesses to get_memory_region() before the device > is realized. > > So this function will never fail. Remove the stale check and the > error variable. Add a comment to the functions stating that they should > never be called on uninitialized devices. > > Signed-off-by: David Hildenbrand <david@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> and ppc parts Acked-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/i386/pc.c | 7 +------ > hw/mem/nvdimm.c | 2 +- > hw/mem/pc-dimm.c | 21 ++++++--------------- > hw/ppc/spapr.c | 14 +++----------- > include/hw/mem/pc-dimm.h | 4 +++- > 5 files changed, 14 insertions(+), 34 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 85c040482e..017396fe84 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1706,15 +1706,10 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev, > PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > - MemoryRegion *mr; > + MemoryRegion *mr = ddc->get_memory_region(dimm); > uint64_t align = TARGET_PAGE_SIZE; > bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM); > > - mr = ddc->get_memory_region(dimm, &local_err); > - if (local_err) { > - goto out; > - } > - > if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) { > align = memory_region_get_alignment(mr); > } > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c > index df9716231f..b2dc2bbb50 100644 > --- a/hw/mem/nvdimm.c > +++ b/hw/mem/nvdimm.c > @@ -96,7 +96,7 @@ static void nvdimm_init(Object *obj) > nvdimm_get_unarmed, nvdimm_set_unarmed, NULL); > } > > -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp) > +static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm) > { > NVDIMMDevice *nvdimm = NVDIMM(dimm); > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > index 5294734529..7bb6ce509c 100644 > --- a/hw/mem/pc-dimm.c > +++ b/hw/mem/pc-dimm.c > @@ -35,14 +35,9 @@ void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm); > Error *local_err = NULL; > - MemoryRegion *mr; > + MemoryRegion *mr = ddc->get_memory_region(dimm); > uint64_t addr; > > - mr = ddc->get_memory_region(dimm, &local_err); > - if (local_err) { > - goto out; > - } > - > addr = object_property_get_uint(OBJECT(dimm), > PC_DIMM_ADDR_PROP, &local_err); > if (local_err) { > @@ -89,7 +84,7 @@ void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine) > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm); > - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort); > + MemoryRegion *mr = ddc->get_memory_region(dimm); > > memory_device_unplug_region(machine, mr); > vmstate_unregister_ram(vmstate_mr, dev); > @@ -172,7 +167,7 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name, > return; > } > > - mr = ddc->get_memory_region(dimm, errp); > + mr = ddc->get_memory_region(dimm); > if (!mr) { > return; > } > @@ -223,13 +218,9 @@ static void pc_dimm_unrealize(DeviceState *dev, Error **errp) > host_memory_backend_set_mapped(dimm->hostmem, false); > } > > -static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp) > +static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm) > { > - if (!dimm->hostmem) { > - error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set"); > - return NULL; > - } > - > + g_assert(dimm->hostmem); > return host_memory_backend_get_memory(dimm->hostmem); > } > > @@ -252,7 +243,7 @@ static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md) > const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md); > MemoryRegion *mr; > > - mr = ddc->get_memory_region(dimm, &error_abort); > + mr = ddc->get_memory_region(dimm); > if (!mr) { > return 0; > } > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index a5f1bbd58a..214286fd2f 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3142,14 +3142,10 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev); > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > - MemoryRegion *mr; > + MemoryRegion *mr = ddc->get_memory_region(dimm); > uint64_t align, size, addr; > uint32_t node; > > - mr = ddc->get_memory_region(dimm, &local_err); > - if (local_err) { > - goto out; > - } > align = memory_region_get_alignment(mr); > size = memory_region_size(mr); > > @@ -3263,7 +3259,7 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms, > { > sPAPRDRConnector *drc; > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort); > + MemoryRegion *mr = ddc->get_memory_region(dimm); > uint64_t size = memory_region_size(mr); > uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE; > uint32_t avail_lmbs = 0; > @@ -3331,16 +3327,12 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev, > Error *local_err = NULL; > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > - MemoryRegion *mr; > + MemoryRegion *mr = ddc->get_memory_region(dimm); > uint32_t nr_lmbs; > uint64_t size, addr_start, addr; > int i; > sPAPRDRConnector *drc; > > - mr = ddc->get_memory_region(dimm, &local_err); > - if (local_err) { > - goto out; > - } > size = memory_region_size(mr); > nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE; > > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > index 627c8601d9..f0e6867803 100644 > --- a/include/hw/mem/pc-dimm.h > +++ b/include/hw/mem/pc-dimm.h > @@ -72,7 +72,9 @@ typedef struct PCDIMMDeviceClass { > > /* public */ > void (*realize)(PCDIMMDevice *dimm, Error **errp); > - MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp); > + > + /* functions should not be called before the device was realized */ > + MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm); > MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm); > } PCDIMMDeviceClass; >
On Mon, 11 Jun 2018 14:16:51 +0200 David Hildenbrand <david@redhat.com> wrote: > We already verify when realizing that the memdev property has been > set. We have no more accesses to get_memory_region() before the device > is realized. this stems from assumption that get_memory_region shouldn't be called before devices realize is executed, which I don't agree to begin with. However wrt error handling, we should probably leave device internal error up to devices and make check for error only in pre_plug handler (since pre_plug was successful, the rest machine helpers would have access to the same region until device is removed. > So this function will never fail. Remove the stale check and the > error variable. Add a comment to the functions stating that they should > never be called on uninitialized devices. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/i386/pc.c | 7 +------ > hw/mem/nvdimm.c | 2 +- > hw/mem/pc-dimm.c | 21 ++++++--------------- > hw/ppc/spapr.c | 14 +++----------- > include/hw/mem/pc-dimm.h | 4 +++- > 5 files changed, 14 insertions(+), 34 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 85c040482e..017396fe84 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1706,15 +1706,10 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev, > PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > - MemoryRegion *mr; > + MemoryRegion *mr = ddc->get_memory_region(dimm); > uint64_t align = TARGET_PAGE_SIZE; > bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM); > > - mr = ddc->get_memory_region(dimm, &local_err); > - if (local_err) { > - goto out; > - } > - > if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) { > align = memory_region_get_alignment(mr); > } > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c > index df9716231f..b2dc2bbb50 100644 > --- a/hw/mem/nvdimm.c > +++ b/hw/mem/nvdimm.c > @@ -96,7 +96,7 @@ static void nvdimm_init(Object *obj) > nvdimm_get_unarmed, nvdimm_set_unarmed, NULL); > } > > -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp) > +static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm) > { > NVDIMMDevice *nvdimm = NVDIMM(dimm); > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > index 5294734529..7bb6ce509c 100644 > --- a/hw/mem/pc-dimm.c > +++ b/hw/mem/pc-dimm.c > @@ -35,14 +35,9 @@ void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm); > Error *local_err = NULL; > - MemoryRegion *mr; > + MemoryRegion *mr = ddc->get_memory_region(dimm); > uint64_t addr; > > - mr = ddc->get_memory_region(dimm, &local_err); > - if (local_err) { > - goto out; > - } > - > addr = object_property_get_uint(OBJECT(dimm), > PC_DIMM_ADDR_PROP, &local_err); > if (local_err) { > @@ -89,7 +84,7 @@ void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine) > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm); > - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort); > + MemoryRegion *mr = ddc->get_memory_region(dimm); > > memory_device_unplug_region(machine, mr); > vmstate_unregister_ram(vmstate_mr, dev); > @@ -172,7 +167,7 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name, > return; > } > > - mr = ddc->get_memory_region(dimm, errp); > + mr = ddc->get_memory_region(dimm); > if (!mr) { > return; > } > @@ -223,13 +218,9 @@ static void pc_dimm_unrealize(DeviceState *dev, Error **errp) > host_memory_backend_set_mapped(dimm->hostmem, false); > } > > -static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp) > +static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm) > { > - if (!dimm->hostmem) { > - error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set"); > - return NULL; > - } > - > + g_assert(dimm->hostmem); > return host_memory_backend_get_memory(dimm->hostmem); > } > > @@ -252,7 +243,7 @@ static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md) > const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md); > MemoryRegion *mr; > > - mr = ddc->get_memory_region(dimm, &error_abort); > + mr = ddc->get_memory_region(dimm); > if (!mr) { > return 0; > } > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index a5f1bbd58a..214286fd2f 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3142,14 +3142,10 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev); > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > - MemoryRegion *mr; > + MemoryRegion *mr = ddc->get_memory_region(dimm); > uint64_t align, size, addr; > uint32_t node; > > - mr = ddc->get_memory_region(dimm, &local_err); > - if (local_err) { > - goto out; > - } > align = memory_region_get_alignment(mr); > size = memory_region_size(mr); > > @@ -3263,7 +3259,7 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms, > { > sPAPRDRConnector *drc; > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort); > + MemoryRegion *mr = ddc->get_memory_region(dimm); > uint64_t size = memory_region_size(mr); > uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE; > uint32_t avail_lmbs = 0; > @@ -3331,16 +3327,12 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev, > Error *local_err = NULL; > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > - MemoryRegion *mr; > + MemoryRegion *mr = ddc->get_memory_region(dimm); > uint32_t nr_lmbs; > uint64_t size, addr_start, addr; > int i; > sPAPRDRConnector *drc; > > - mr = ddc->get_memory_region(dimm, &local_err); > - if (local_err) { > - goto out; > - } > size = memory_region_size(mr); > nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE; > > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > index 627c8601d9..f0e6867803 100644 > --- a/include/hw/mem/pc-dimm.h > +++ b/include/hw/mem/pc-dimm.h > @@ -72,7 +72,9 @@ typedef struct PCDIMMDeviceClass { > > /* public */ > void (*realize)(PCDIMMDevice *dimm, Error **errp); > - MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp); > + > + /* functions should not be called before the device was realized */ > + MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm); > MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm); > } PCDIMMDeviceClass; >
On 13.06.2018 15:03, Igor Mammedov wrote: > On Mon, 11 Jun 2018 14:16:51 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> We already verify when realizing that the memdev property has been >> set. We have no more accesses to get_memory_region() before the device >> is realized. > this stems from assumption that get_memory_region shouldn't be called > before devices realize is executed, which I don't agree to begin with. > > However wrt error handling, we should probably leave device internal error > up to devices and make check for error only in pre_plug handler > (since pre_plug was successful, the rest machine helpers would have > access to the same region until device is removed. > Something like a generic Device "validate()"/"pre_realize()" function, that can be called before realize() and validates all properties (+initializes derived properties) would be something I could agree to. Then we could drop all error handling from access functions (as they have been validated early during pre_plug()) Moving all checks out of realize into pre_plug() looks ugly, because we have implementation details split across c-files. > >> So this function will never fail. Remove the stale check and the >> error variable. Add a comment to the functions stating that they should >> never be called on uninitialized devices. >>
On 13.06.2018 16:07, David Hildenbrand wrote: > On 13.06.2018 15:03, Igor Mammedov wrote: >> On Mon, 11 Jun 2018 14:16:51 +0200 >> David Hildenbrand <david@redhat.com> wrote: >> >>> We already verify when realizing that the memdev property has been >>> set. We have no more accesses to get_memory_region() before the device >>> is realized. >> this stems from assumption that get_memory_region shouldn't be called >> before devices realize is executed, which I don't agree to begin with. >> >> However wrt error handling, we should probably leave device internal error >> up to devices and make check for error only in pre_plug handler >> (since pre_plug was successful, the rest machine helpers would have >> access to the same region until device is removed. >> > > Something like a generic Device "validate()"/"pre_realize()" function, > that can be called before realize() and validates all properties > (+initializes derived properties) would be something I could agree to. > > Then we could drop all error handling from access functions (as they > have been validated early during pre_plug()) > > Moving all checks out of realize into pre_plug() looks ugly, because we > have implementation details split across c-files. > I am thinking about something like this From ee8da4f5774ff5fa190466674e36ee99020033c4 Mon Sep 17 00:00:00 2001 From: David Hildenbrand <david@redhat.com> Date: Wed, 13 Jun 2018 16:41:12 +0200 Subject: [PATCH RFC] device: add "prepare()" function The realize() function usually does several things. It validates properties, inititalizes state based on properties and performs e.g. registrations in the system that require "unrealize()" to be called when removing the device. In a pre_plug hotplug handler, we usually want to access certain properties or derived properties, e.g. to perform advanced checks (for resource asignment). Now we have the problem, that realize() was not called yet once we reach pre_plug(). So we theoretically have to duplicate checks and add error paths for cases that can theoretically never happen. Let's add the "prepare()" callback, which can be used to validate properties and inititalize some state based on these. It will be called once all static properties have been inititalized, but before the pre_plug hotplug handler is activated. The pre_plug handler can than rely on the fact that basic checks already were performed. In contrast to "realize()", "prepare()" should not perform any actions that would have to be rolled back in case e.g. pre_plug fails. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/core/qdev.c | 7 +++++++ include/hw/qdev-core.h | 14 ++++++++++---- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/hw/core/qdev.c b/hw/core/qdev.c index ffec461791..3bfc7e0d54 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -814,6 +814,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp) g_free(name); } + if (dc->prepare) { + dc->prepare(dev, &local_err); + if (local_err != NULL) { + goto fail; + } + } + hotplug_ctrl = qdev_get_hotplug_handler(dev); if (hotplug_ctrl) { hotplug_handler_pre_plug(hotplug_ctrl, dev, &local_err); diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index f1fd0f8736..63520c1a55 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -29,6 +29,7 @@ typedef enum DeviceCategory { DEVICE_CATEGORY_MAX } DeviceCategory; +typedef void (*DevicePrepare)(DeviceState *dev, Error **errp); typedef void (*DeviceRealize)(DeviceState *dev, Error **errp); typedef void (*DeviceUnrealize)(DeviceState *dev, Error **errp); typedef void (*DeviceReset)(DeviceState *dev); @@ -40,8 +41,11 @@ struct VMStateDescription; /** * DeviceClass: * @props: Properties accessing state fields. + * @prepare: Callback function invoked when the #DeviceState:realized + * property is changed to %true, before invoking the pre_plug hotplug handler. * @realize: Callback function invoked when the #DeviceState:realized - * property is changed to %true. + * property is changed to %true, after invoking the pre_plug hotplug handler + * but before invoking the plug handler. * @unrealize: Callback function invoked when the #DeviceState:realized * property is changed to %false. * @hotpluggable: indicates if #DeviceClass is hotpluggable, available @@ -54,7 +58,8 @@ struct VMStateDescription; * The former may not fail (it might assert or exit), the latter may return * error information to the caller and must be re-entrant. * Trivial field initializations should go into #TypeInfo.instance_init. - * Operations depending on @props static properties should go into @realize. + * Operations depending on @props static properties should go into @prepare + * or @realize. * After successful realization, setting static properties will fail. * * As an interim step, the #DeviceState:realized property can also be @@ -73,8 +78,8 @@ struct VMStateDescription; * * <note> * <para> - * Since TYPE_DEVICE doesn't implement @realize and @unrealize, types - * derived directly from it need not call their parent's @realize and + * Since TYPE_DEVICE doesn't implement @prepare, @realize and @unrealize, types + * derived directly from it need not call their parent's @prepare, @realize and * @unrealize. * For other types consult the documentation and implementation of the * respective parent types. @@ -107,6 +112,7 @@ typedef struct DeviceClass { /* callbacks */ DeviceReset reset; + DevicePrepare prepare; DeviceRealize realize; DeviceUnrealize unrealize;
On Wed, 13 Jun 2018 16:50:54 +0200 David Hildenbrand <david@redhat.com> wrote: > On 13.06.2018 16:07, David Hildenbrand wrote: > > On 13.06.2018 15:03, Igor Mammedov wrote: > >> On Mon, 11 Jun 2018 14:16:51 +0200 > >> David Hildenbrand <david@redhat.com> wrote: > >> > >>> We already verify when realizing that the memdev property has been > >>> set. We have no more accesses to get_memory_region() before the device > >>> is realized. > >> this stems from assumption that get_memory_region shouldn't be called > >> before devices realize is executed, which I don't agree to begin with. > >> > >> However wrt error handling, we should probably leave device internal error > >> up to devices and make check for error only in pre_plug handler > >> (since pre_plug was successful, the rest machine helpers would have > >> access to the same region until device is removed. > >> > > > > Something like a generic Device "validate()"/"pre_realize()" function, > > that can be called before realize() and validates all properties > > (+initializes derived properties) would be something I could agree to. > > > > Then we could drop all error handling from access functions (as they > > have been validated early during pre_plug()) > > > > Moving all checks out of realize into pre_plug() looks ugly, because we > > have implementation details split across c-files. > > > > I am thinking about something like this > > From ee8da4f5774ff5fa190466674e36ee99020033c4 Mon Sep 17 00:00:00 2001 > From: David Hildenbrand <david@redhat.com> > Date: Wed, 13 Jun 2018 16:41:12 +0200 > Subject: [PATCH RFC] device: add "prepare()" function > > The realize() function usually does several things. It validates > properties, inititalizes state based on properties and performs > e.g. registrations in the system that require "unrealize()" to be called > when removing the device. > > In a pre_plug hotplug handler, we usually want to access certain > properties or derived properties, e.g. to perform advanced checks > (for resource asignment). Now we have the problem, that realize() was > not called yet once we reach pre_plug(). So we theoretically have to > duplicate checks and add error paths for cases that can > theoretically never happen. I care less about duplicate checks in completely different parts of code, and I'd even insist on device:realize checks being self contained and not rely on any other external checks performed by users of device. And vice verse layer that uses device should do it's own checks when necessary and not rely on device's verification. That way loosely coupled code wouldn't fall apart whenever we drop or change checks in one of the parts. The only case where I'd make concession is minimizing error handling on hot path for performance reasons and this is not the case here. > Let's add the "prepare()" callback, which can be used to validate > properties and inititalize some state based on these. It will be called > once all static properties have been inititalized, but before the > pre_plug hotplug handler is activated. The pre_plug handler can than > rely on the fact that basic checks already were performed. pre_plug isn't part of device, it's a separate part that might vary depending on machine and which might modify device properties along the way and then exaggerating we would need 'prepare2()' and after that 'pre_plug2()' and ... Hence I dislike idea of adding more callbacks. I'd rather have a property setter do the job of initializing state of device when necessary instead of adding more callbacks. Which is in nvdimm case a bit more code compared to doing it in realize() but should be possible to implement. > In contrast to "realize()", "prepare()" should not perform any actions > that would have to be rolled back in case e.g. pre_plug fails. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/core/qdev.c | 7 +++++++ > include/hw/qdev-core.h | 14 ++++++++++---- > 2 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index ffec461791..3bfc7e0d54 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -814,6 +814,13 @@ static void device_set_realized(Object *obj, bool value, Error **errp) > g_free(name); > } > > + if (dc->prepare) { > + dc->prepare(dev, &local_err); > + if (local_err != NULL) { > + goto fail; > + } > + } > + > hotplug_ctrl = qdev_get_hotplug_handler(dev); > if (hotplug_ctrl) { > hotplug_handler_pre_plug(hotplug_ctrl, dev, &local_err); > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index f1fd0f8736..63520c1a55 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -29,6 +29,7 @@ typedef enum DeviceCategory { > DEVICE_CATEGORY_MAX > } DeviceCategory; > > +typedef void (*DevicePrepare)(DeviceState *dev, Error **errp); > typedef void (*DeviceRealize)(DeviceState *dev, Error **errp); > typedef void (*DeviceUnrealize)(DeviceState *dev, Error **errp); > typedef void (*DeviceReset)(DeviceState *dev); > @@ -40,8 +41,11 @@ struct VMStateDescription; > /** > * DeviceClass: > * @props: Properties accessing state fields. > + * @prepare: Callback function invoked when the #DeviceState:realized > + * property is changed to %true, before invoking the pre_plug hotplug handler. > * @realize: Callback function invoked when the #DeviceState:realized > - * property is changed to %true. > + * property is changed to %true, after invoking the pre_plug hotplug handler > + * but before invoking the plug handler. > * @unrealize: Callback function invoked when the #DeviceState:realized > * property is changed to %false. > * @hotpluggable: indicates if #DeviceClass is hotpluggable, available > @@ -54,7 +58,8 @@ struct VMStateDescription; > * The former may not fail (it might assert or exit), the latter may return > * error information to the caller and must be re-entrant. > * Trivial field initializations should go into #TypeInfo.instance_init. > - * Operations depending on @props static properties should go into @realize. > + * Operations depending on @props static properties should go into @prepare > + * or @realize. > * After successful realization, setting static properties will fail. > * > * As an interim step, the #DeviceState:realized property can also be > @@ -73,8 +78,8 @@ struct VMStateDescription; > * > * <note> > * <para> > - * Since TYPE_DEVICE doesn't implement @realize and @unrealize, types > - * derived directly from it need not call their parent's @realize and > + * Since TYPE_DEVICE doesn't implement @prepare, @realize and @unrealize, types > + * derived directly from it need not call their parent's @prepare, @realize and > * @unrealize. > * For other types consult the documentation and implementation of the > * respective parent types. > @@ -107,6 +112,7 @@ typedef struct DeviceClass { > > /* callbacks */ > DeviceReset reset; > + DevicePrepare prepare; > DeviceRealize realize; > DeviceUnrealize unrealize; >
On 14.06.2018 17:00, Igor Mammedov wrote: > On Wed, 13 Jun 2018 16:50:54 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> On 13.06.2018 16:07, David Hildenbrand wrote: >>> On 13.06.2018 15:03, Igor Mammedov wrote: >>>> On Mon, 11 Jun 2018 14:16:51 +0200 >>>> David Hildenbrand <david@redhat.com> wrote: >>>> >>>>> We already verify when realizing that the memdev property has been >>>>> set. We have no more accesses to get_memory_region() before the device >>>>> is realized. >>>> this stems from assumption that get_memory_region shouldn't be called >>>> before devices realize is executed, which I don't agree to begin with. >>>> >>>> However wrt error handling, we should probably leave device internal error >>>> up to devices and make check for error only in pre_plug handler >>>> (since pre_plug was successful, the rest machine helpers would have >>>> access to the same region until device is removed. >>>> >>> >>> Something like a generic Device "validate()"/"pre_realize()" function, >>> that can be called before realize() and validates all properties >>> (+initializes derived properties) would be something I could agree to. >>> >>> Then we could drop all error handling from access functions (as they >>> have been validated early during pre_plug()) >>> >>> Moving all checks out of realize into pre_plug() looks ugly, because we >>> have implementation details split across c-files. >>> >> >> I am thinking about something like this >> >> From ee8da4f5774ff5fa190466674e36ee99020033c4 Mon Sep 17 00:00:00 2001 >> From: David Hildenbrand <david@redhat.com> >> Date: Wed, 13 Jun 2018 16:41:12 +0200 >> Subject: [PATCH RFC] device: add "prepare()" function >> >> The realize() function usually does several things. It validates >> properties, inititalizes state based on properties and performs >> e.g. registrations in the system that require "unrealize()" to be called >> when removing the device. >> >> In a pre_plug hotplug handler, we usually want to access certain >> properties or derived properties, e.g. to perform advanced checks >> (for resource asignment). Now we have the problem, that realize() was >> not called yet once we reach pre_plug(). So we theoretically have to >> duplicate checks and add error paths for cases that can >> theoretically never happen. > I care less about duplicate checks in completely different parts of code, > and I'd even insist on device:realize checks being self contained and not > rely on any other external checks performed by users of device. And vice > verse layer that uses device should do it's own checks when necessary > and not rely on device's verification. That way loosely coupled code > wouldn't fall apart whenever we drop or change checks in one of the parts. > > The only case where I'd make concession is minimizing error handling > on hot path for performance reasons and this is not the case here. > >> Let's add the "prepare()" callback, which can be used to validate >> properties and inititalize some state based on these. It will be called >> once all static properties have been inititalized, but before the >> pre_plug hotplug handler is activated. The pre_plug handler can than >> rely on the fact that basic checks already were performed. > > pre_plug isn't part of device, it's a separate part that might vary > depending on machine and which might modify device properties along > the way and then exaggerating we would need 'prepare2()' and after > that 'pre_plug2()' and ... That's how two parties (device vs hotplug handler) work together to get results done ... Just like inititalize() -> realized() vs. pre_plug -> plug(). There has to be some hand shaking. > > Hence I dislike idea of adding more callbacks. I'd rather have a property > setter do the job of initializing state of device when necessary instead > of adding more callbacks. Which is in nvdimm case a bit more code compared > to doing it in realize() but should be possible to implement. Although I strongly disagree (especially about the fact of calling class functions *anytime* after a device has been created but not initialized) I will implement it like that for now (otherwise I won't be making any progress here but instead be creating more and more problems to solve). nvdimm will only have static properties. When realizing or when calling get_memory_region(), properties will be checked and the nvdimm_mr inititalized, if not already done.
On Thu, 14 Jun 2018 17:11:38 +0200 David Hildenbrand <david@redhat.com> wrote: > On 14.06.2018 17:00, Igor Mammedov wrote: > > On Wed, 13 Jun 2018 16:50:54 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> On 13.06.2018 16:07, David Hildenbrand wrote: > >>> On 13.06.2018 15:03, Igor Mammedov wrote: > >>>> On Mon, 11 Jun 2018 14:16:51 +0200 > >>>> David Hildenbrand <david@redhat.com> wrote: > >>>> > >>>>> We already verify when realizing that the memdev property has been > >>>>> set. We have no more accesses to get_memory_region() before the device > >>>>> is realized. > >>>> this stems from assumption that get_memory_region shouldn't be called > >>>> before devices realize is executed, which I don't agree to begin with. > >>>> > >>>> However wrt error handling, we should probably leave device internal error > >>>> up to devices and make check for error only in pre_plug handler > >>>> (since pre_plug was successful, the rest machine helpers would have > >>>> access to the same region until device is removed. > >>>> > >>> > >>> Something like a generic Device "validate()"/"pre_realize()" function, > >>> that can be called before realize() and validates all properties > >>> (+initializes derived properties) would be something I could agree to. > >>> > >>> Then we could drop all error handling from access functions (as they > >>> have been validated early during pre_plug()) > >>> > >>> Moving all checks out of realize into pre_plug() looks ugly, because we > >>> have implementation details split across c-files. > >>> > >> > >> I am thinking about something like this > >> > >> From ee8da4f5774ff5fa190466674e36ee99020033c4 Mon Sep 17 00:00:00 2001 > >> From: David Hildenbrand <david@redhat.com> > >> Date: Wed, 13 Jun 2018 16:41:12 +0200 > >> Subject: [PATCH RFC] device: add "prepare()" function > >> > >> The realize() function usually does several things. It validates > >> properties, inititalizes state based on properties and performs > >> e.g. registrations in the system that require "unrealize()" to be called > >> when removing the device. > >> > >> In a pre_plug hotplug handler, we usually want to access certain > >> properties or derived properties, e.g. to perform advanced checks > >> (for resource asignment). Now we have the problem, that realize() was > >> not called yet once we reach pre_plug(). So we theoretically have to > >> duplicate checks and add error paths for cases that can > >> theoretically never happen. > > I care less about duplicate checks in completely different parts of code, > > and I'd even insist on device:realize checks being self contained and not > > rely on any other external checks performed by users of device. And vice > > verse layer that uses device should do it's own checks when necessary > > and not rely on device's verification. That way loosely coupled code > > wouldn't fall apart whenever we drop or change checks in one of the parts. > > > > The only case where I'd make concession is minimizing error handling > > on hot path for performance reasons and this is not the case here. > > > >> Let's add the "prepare()" callback, which can be used to validate > >> properties and inititalize some state based on these. It will be called > >> once all static properties have been inititalized, but before the > >> pre_plug hotplug handler is activated. The pre_plug handler can than > >> rely on the fact that basic checks already were performed. > > > > pre_plug isn't part of device, it's a separate part that might vary > > depending on machine and which might modify device properties along > > the way and then exaggerating we would need 'prepare2()' and after > > that 'pre_plug2()' and ... > > That's how two parties (device vs hotplug handler) work together to get > results done ... Just like inititalize() -> realized() vs. pre_plug -> > plug(). There has to be some hand shaking. In general hotplug handler is optional, for example a board might create initial memory wit fixed layout as: dev = object_new(dimm) set memdev + set addr qdev_init_no_fail() generalized pc-dimm helpers for hotplug handler is just impl. detail, a board might have other ideas how to use pc-dimm and choose to implement wiring differently. So device model should do sanity checks independently from whomever uses it. > > > > Hence I dislike idea of adding more callbacks. I'd rather have a property > > setter do the job of initializing state of device when necessary instead > > of adding more callbacks. Which is in nvdimm case a bit more code compared > > to doing it in realize() but should be possible to implement. > > Although I strongly disagree (especially about the fact of calling class > functions *anytime* after a device has been created but not initialized) > I will implement it like that for now (otherwise I won't be making any > progress here but instead be creating more and more problems to solve). > > > nvdimm will only have static properties. When realizing or when calling > get_memory_region(), properties will be checked and the nvdimm_mr > inititalized, if not already done. I'm not a fun of using class callbacks (due undefined semantic), but since we are already using it here, that might just work in this case.
>>> pre_plug isn't part of device, it's a separate part that might vary >>> depending on machine and which might modify device properties along >>> the way and then exaggerating we would need 'prepare2()' and after >>> that 'pre_plug2()' and ... >> >> That's how two parties (device vs hotplug handler) work together to get >> results done ... Just like inititalize() -> realized() vs. pre_plug -> >> plug(). There has to be some hand shaking. > In general hotplug handler is optional, for example a board might > create initial memory wit fixed layout as: > dev = object_new(dimm) > set memdev + set addr > qdev_init_no_fail() > > generalized pc-dimm helpers for hotplug handler is just impl. detail, > a board might have other ideas how to use pc-dimm and choose to > implement wiring differently. So device model should do sanity checks > independently from whomever uses it. In general, I am not a fan of implementing things the "what if someday ..." way. It overcomplicates code and you can be sure that usually something is missed either way (as there is no concrete use case, not even in somebodys mind). But I can see that for pc-dimms that can actually make sense (in contrast to other, more specific devices).
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 85c040482e..017396fe84 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1706,15 +1706,10 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev, PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); PCDIMMDevice *dimm = PC_DIMM(dev); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); - MemoryRegion *mr; + MemoryRegion *mr = ddc->get_memory_region(dimm); uint64_t align = TARGET_PAGE_SIZE; bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM); - mr = ddc->get_memory_region(dimm, &local_err); - if (local_err) { - goto out; - } - if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) { align = memory_region_get_alignment(mr); } diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c index df9716231f..b2dc2bbb50 100644 --- a/hw/mem/nvdimm.c +++ b/hw/mem/nvdimm.c @@ -96,7 +96,7 @@ static void nvdimm_init(Object *obj) nvdimm_get_unarmed, nvdimm_set_unarmed, NULL); } -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp) +static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm) { NVDIMMDevice *nvdimm = NVDIMM(dimm); diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index 5294734529..7bb6ce509c 100644 --- a/hw/mem/pc-dimm.c +++ b/hw/mem/pc-dimm.c @@ -35,14 +35,9 @@ void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm); Error *local_err = NULL; - MemoryRegion *mr; + MemoryRegion *mr = ddc->get_memory_region(dimm); uint64_t addr; - mr = ddc->get_memory_region(dimm, &local_err); - if (local_err) { - goto out; - } - addr = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err); if (local_err) { @@ -89,7 +84,7 @@ void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine) PCDIMMDevice *dimm = PC_DIMM(dev); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm); - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort); + MemoryRegion *mr = ddc->get_memory_region(dimm); memory_device_unplug_region(machine, mr); vmstate_unregister_ram(vmstate_mr, dev); @@ -172,7 +167,7 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name, return; } - mr = ddc->get_memory_region(dimm, errp); + mr = ddc->get_memory_region(dimm); if (!mr) { return; } @@ -223,13 +218,9 @@ static void pc_dimm_unrealize(DeviceState *dev, Error **errp) host_memory_backend_set_mapped(dimm->hostmem, false); } -static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp) +static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm) { - if (!dimm->hostmem) { - error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set"); - return NULL; - } - + g_assert(dimm->hostmem); return host_memory_backend_get_memory(dimm->hostmem); } @@ -252,7 +243,7 @@ static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md) const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md); MemoryRegion *mr; - mr = ddc->get_memory_region(dimm, &error_abort); + mr = ddc->get_memory_region(dimm); if (!mr) { return 0; } diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index a5f1bbd58a..214286fd2f 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3142,14 +3142,10 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev); PCDIMMDevice *dimm = PC_DIMM(dev); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); - MemoryRegion *mr; + MemoryRegion *mr = ddc->get_memory_region(dimm); uint64_t align, size, addr; uint32_t node; - mr = ddc->get_memory_region(dimm, &local_err); - if (local_err) { - goto out; - } align = memory_region_get_alignment(mr); size = memory_region_size(mr); @@ -3263,7 +3259,7 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms, { sPAPRDRConnector *drc; PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort); + MemoryRegion *mr = ddc->get_memory_region(dimm); uint64_t size = memory_region_size(mr); uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE; uint32_t avail_lmbs = 0; @@ -3331,16 +3327,12 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev, Error *local_err = NULL; PCDIMMDevice *dimm = PC_DIMM(dev); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); - MemoryRegion *mr; + MemoryRegion *mr = ddc->get_memory_region(dimm); uint32_t nr_lmbs; uint64_t size, addr_start, addr; int i; sPAPRDRConnector *drc; - mr = ddc->get_memory_region(dimm, &local_err); - if (local_err) { - goto out; - } size = memory_region_size(mr); nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE; diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h index 627c8601d9..f0e6867803 100644 --- a/include/hw/mem/pc-dimm.h +++ b/include/hw/mem/pc-dimm.h @@ -72,7 +72,9 @@ typedef struct PCDIMMDeviceClass { /* public */ void (*realize)(PCDIMMDevice *dimm, Error **errp); - MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp); + + /* functions should not be called before the device was realized */ + MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm); MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm); } PCDIMMDeviceClass;
We already verify when realizing that the memdev property has been set. We have no more accesses to get_memory_region() before the device is realized. So this function will never fail. Remove the stale check and the error variable. Add a comment to the functions stating that they should never be called on uninitialized devices. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/i386/pc.c | 7 +------ hw/mem/nvdimm.c | 2 +- hw/mem/pc-dimm.c | 21 ++++++--------------- hw/ppc/spapr.c | 14 +++----------- include/hw/mem/pc-dimm.h | 4 +++- 5 files changed, 14 insertions(+), 34 deletions(-)