Message ID | 20180702093755.7384-2-david@redhat.com |
---|---|
State | New |
Headers | show |
Series | pc-dimm: pre_plug "slot" and "addr" assignment | expand |
Hi David, On 07/02/2018 11:37 AM, David Hildenbrand wrote: > We can assign and verify the slot before realizing and trying to plug. > reading/writing the slot property should never fail, so let's reduce > error handling a bit by using &error_abort. > > To do this during pre_plug, add and use (x86, ppc) pc_dimm_pre_plug(). > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/i386/pc.c | 2 ++ > hw/mem/pc-dimm.c | 35 ++++++++++++++++++----------------- > hw/ppc/spapr.c | 9 ++++++++- > include/hw/mem/pc-dimm.h | 1 + > 4 files changed, 29 insertions(+), 18 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index f310040351..bf986baf91 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1695,6 +1695,8 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'"); > return; > } > + > + pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp); > } > > static void pc_memory_plug(HotplugHandler *hotplug_dev, > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > index 65843bc52a..e56c4daef2 100644 > --- a/hw/mem/pc-dimm.c > +++ b/hw/mem/pc-dimm.c > @@ -29,10 +29,27 @@ > > static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp); > > +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp) > +{ > + Error *local_err = NULL; > + int slot; > + > + slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, > + &error_abort); > + slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot, > + machine->ram_slots, &local_err); > + if (local_err) { > + goto out; > + } > + object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &error_abort); > + trace_mhp_pc_dimm_assigned_slot(slot); > +out: > + error_propagate(errp, local_err); > +} > + > void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align, > Error **errp) > { > - int slot; > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm, > @@ -59,22 +76,6 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align, > } > trace_mhp_pc_dimm_assigned_address(addr); > > - slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err); > - if (local_err) { > - goto out; > - } > - > - slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot, > - machine->ram_slots, &local_err); > - if (local_err) { > - goto out; > - } > - object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &local_err); > - if (local_err) { > - goto out; > - } > - trace_mhp_pc_dimm_assigned_slot(slot); > - > memory_device_plug_region(machine, mr, addr); > vmstate_register_ram(vmstate_mr, dev); > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index b32b971a14..bf012235b6 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3191,6 +3191,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev); > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > + Error *local_err = NULL; > MemoryRegion *mr; > uint64_t size; > Object *memdev; > @@ -3216,7 +3217,13 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > memdev = object_property_get_link(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, > &error_abort); > pagesize = host_memory_backend_pagesize(MEMORY_BACKEND(memdev)); > - spapr_check_pagesize(spapr, pagesize, errp); > + spapr_check_pagesize(spapr, pagesize, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + > + pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp); > } > > struct sPAPRDIMMState { > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > index 26ebb7d5e9..7b120416d1 100644 > --- a/include/hw/mem/pc-dimm.h > +++ b/include/hw/mem/pc-dimm.h > @@ -79,6 +79,7 @@ typedef struct PCDIMMDeviceClass { > Error **errp); > } PCDIMMDeviceClass; > > +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp); > void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align, > Error **errp); > void pc_dimm_unplug(DeviceState *dev, MachineState *machine); > Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index f310040351..bf986baf91 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1695,6 +1695,8 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'"); return; } + + pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp); } static void pc_memory_plug(HotplugHandler *hotplug_dev, diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index 65843bc52a..e56c4daef2 100644 --- a/hw/mem/pc-dimm.c +++ b/hw/mem/pc-dimm.c @@ -29,10 +29,27 @@ static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp); +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp) +{ + Error *local_err = NULL; + int slot; + + slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, + &error_abort); + slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot, + machine->ram_slots, &local_err); + if (local_err) { + goto out; + } + object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &error_abort); + trace_mhp_pc_dimm_assigned_slot(slot); +out: + error_propagate(errp, local_err); +} + void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align, Error **errp) { - int slot; PCDIMMDevice *dimm = PC_DIMM(dev); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm, @@ -59,22 +76,6 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align, } trace_mhp_pc_dimm_assigned_address(addr); - slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err); - if (local_err) { - goto out; - } - - slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot, - machine->ram_slots, &local_err); - if (local_err) { - goto out; - } - object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &local_err); - if (local_err) { - goto out; - } - trace_mhp_pc_dimm_assigned_slot(slot); - memory_device_plug_region(machine, mr, addr); vmstate_register_ram(vmstate_mr, dev); diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index b32b971a14..bf012235b6 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3191,6 +3191,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev); PCDIMMDevice *dimm = PC_DIMM(dev); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); + Error *local_err = NULL; MemoryRegion *mr; uint64_t size; Object *memdev; @@ -3216,7 +3217,13 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, memdev = object_property_get_link(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, &error_abort); pagesize = host_memory_backend_pagesize(MEMORY_BACKEND(memdev)); - spapr_check_pagesize(spapr, pagesize, errp); + spapr_check_pagesize(spapr, pagesize, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + + pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp); } struct sPAPRDIMMState { diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h index 26ebb7d5e9..7b120416d1 100644 --- a/include/hw/mem/pc-dimm.h +++ b/include/hw/mem/pc-dimm.h @@ -79,6 +79,7 @@ typedef struct PCDIMMDeviceClass { Error **errp); } PCDIMMDeviceClass; +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp); void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align, Error **errp); void pc_dimm_unplug(DeviceState *dev, MachineState *machine);