diff mbox

[v3,2/5] pc, pc-dimm: Factor out reusable parts in pc_dimm_plug to a separate routine

Message ID 1435291564-14014-3-git-send-email-bharata@linux.vnet.ibm.com
State New
Headers show

Commit Message

Bharata B Rao June 26, 2015, 4:06 a.m. UTC 
pc_dimm_plug() has code that will be needed for memory plug handlers
in other archs too. Extract code from pc_dimm_plug() into a generic
routine pc_dimm_memory_plug() that resides in pc-dimm.c. Also
correspondingly refactor re-usable unplug code into pc_dimm_memory_unplug().

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/i386/pc.c             | 66 ++++-----------------------------------
 hw/mem/pc-dimm.c         | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/mem/pc-dimm.h |  4 +++
 3 files changed, 90 insertions(+), 60 deletions(-)

Comments

David Gibson June 26, 2015, 5:07 a.m. UTC | #1 
On Fri, Jun 26, 2015 at 09:36:01AM +0530, Bharata B Rao wrote:
> pc_dimm_plug() has code that will be needed for memory plug handlers
> in other archs too. Extract code from pc_dimm_plug() into a generic
> routine pc_dimm_memory_plug() that resides in pc-dimm.c. Also
> correspondingly refactor re-usable unplug code into pc_dimm_memory_unplug().
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Igor Mammedov June 26, 2015, 8:21 a.m. UTC | #2 
On Fri, 26 Jun 2015 09:36:01 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> pc_dimm_plug() has code that will be needed for memory plug handlers
> in other archs too. Extract code from pc_dimm_plug() into a generic
> routine pc_dimm_memory_plug() that resides in pc-dimm.c. Also
> correspondingly refactor re-usable unplug code into pc_dimm_memory_unplug().
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
thanks for splitting, much easier to review.


> ---
>  hw/i386/pc.c             | 66 ++++-----------------------------------
>  hw/mem/pc-dimm.c         | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/mem/pc-dimm.h |  4 +++
>  3 files changed, 90 insertions(+), 60 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e51c3be..cb57612 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -64,7 +64,6 @@
>  #include "hw/pci/pci_host.h"
>  #include "acpi-build.h"
>  #include "hw/mem/pc-dimm.h"
> -#include "trace.h"
>  #include "qapi/visitor.h"
>  #include "qapi-visit.h"
>  
> @@ -1554,88 +1553,35 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
>  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>                           DeviceState *dev, Error **errp)
>  {
> -    int slot;
>      HotplugHandlerClass *hhc;
>      Error *local_err = NULL;
>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> -    MachineState *machine = MACHINE(hotplug_dev);
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *mr = ddc->get_memory_region(dimm);
> -    uint64_t existing_dimms_capacity = 0;
>      uint64_t align = TARGET_PAGE_SIZE;
> -    uint64_t addr;
> -
> -    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
>  
>      if (memory_region_get_alignment(mr) && pcms->enforce_aligned_dimm) {
>          align = memory_region_get_alignment(mr);
>      }
>  
> -    addr = pc_dimm_get_free_addr(pcms->hotplug_memory.base,
> -                                 memory_region_size(&pcms->hotplug_memory.mr),
> -                                 !addr ? NULL : &addr, align,
> -                                 memory_region_size(mr), &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    if (existing_dimms_capacity + memory_region_size(mr) >
> -        machine->maxram_size - machine->ram_size) {
> -        error_setg(&local_err, "not enough space, currently 0x%" PRIx64
> -                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
> -                   existing_dimms_capacity,
> -                   machine->maxram_size - machine->ram_size);
> -        goto out;
> -    }
> -
> -    object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -    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);
> -
>      if (!pcms->acpi_dev) {
>          error_setg(&local_err,
>                     "memory hotplug is not enabled: missing acpi device");
>          goto out;
>      }
>  
> -    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> -        error_setg(&local_err, "hypervisor has no free memory slots left");
> +    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align, &local_err);
> +    if (local_err) {
>          goto out;
>      }
>  
> -    memory_region_add_subregion(&pcms->hotplug_memory.mr,
> -                                addr - pcms->hotplug_memory.base, mr);
> -    vmstate_register_ram(mr, dev);
> -
>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
> +    if (local_err) {
> +        pc_dimm_memory_unplug(dev, &pcms->hotplug_memory, mr);
> +    }
You are adding something new while patch claims to be just moving code.
I'm not sure it's save to call unplug here if hhc->plug() failed
since it might have triggered guest visible side effects.

I'd just drop this hunk and leave original code,
on top of that extra patch for hhc->plug(s/local_err/error_abort/)
might make a sense since hhc->plug() should never fail and if it's
then we should fix error in code instead of trying to recover.

>  out:
>      error_propagate(errp, local_err);
>  }
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index e70633d..98971b7 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -23,12 +23,92 @@
>  #include "qapi/visitor.h"
>  #include "qemu/range.h"
>  #include "sysemu/numa.h"
> +#include "sysemu/kvm.h"
> +#include "trace.h"
>  
>  typedef struct pc_dimms_capacity {
>       uint64_t size;
>       Error    **errp;
>  } pc_dimms_capacity;
>  
> +void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> +                         MemoryRegion *mr, uint64_t align, Error **errp)
> +{
> +    int slot;
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +    Error *local_err = NULL;
> +    uint64_t existing_dimms_capacity = 0;
> +    uint64_t addr;
> +
> +    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    addr = pc_dimm_get_free_addr(hpms->base,
> +                                 memory_region_size(&hpms->mr),
> +                                 !addr ? NULL : &addr, align,
> +                                 memory_region_size(mr), &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    if (existing_dimms_capacity + memory_region_size(mr) >
> +        machine->maxram_size - machine->ram_size) {
> +        error_setg(&local_err, "not enough space, currently 0x%" PRIx64
> +                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
> +                   existing_dimms_capacity,
> +                   machine->maxram_size - machine->ram_size);
> +        goto out;
> +    }
> +
> +    object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +    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);
> +
> +    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> +        error_setg(&local_err, "hypervisor has no free memory slots left");
> +        goto out;
> +    }
> +
> +    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
> +    vmstate_register_ram(mr, dev);
> +
> +out:
> +    error_propagate(errp, local_err);
> +}
> +
> +void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> +                           MemoryRegion *mr)
> +{
> +    memory_region_del_subregion(&hpms->mr, mr);
> +    vmstate_unregister_ram(mr, dev);
> +}
if you adding this here then you probably should be removing/replacing
related hunks from pc.c, I don't see it in this patch.

> +
>  static int pc_existing_dimms_capacity_internal(Object *obj, void *opaque)
>  {
>      pc_dimms_capacity *cap = opaque;
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index d528a3f..3eabef4 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -90,4 +90,8 @@ int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
>  
>  int qmp_pc_dimm_device_list(Object *obj, void *opaque);
>  uint64_t pc_existing_dimms_capacity(Error **errp);
> +void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> +                         MemoryRegion *mr, uint64_t align, Error **errp);
> +void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> +                           MemoryRegion *mr);
>  #endif
Bharata B Rao June 26, 2015, 11:45 a.m. UTC | #3 
On Fri, Jun 26, 2015 at 10:21:54AM +0200, Igor Mammedov wrote:
> On Fri, 26 Jun 2015 09:36:01 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > pc_dimm_plug() has code that will be needed for memory plug handlers
> > in other archs too. Extract code from pc_dimm_plug() into a generic
> > routine pc_dimm_memory_plug() that resides in pc-dimm.c. Also
> > correspondingly refactor re-usable unplug code into pc_dimm_memory_unplug().
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> thanks for splitting, much easier to review.

Yes it is much easier to see the changes and thanks for the review.

> 
> 
> > ---
> >  hw/i386/pc.c             | 66 ++++-----------------------------------
> >  hw/mem/pc-dimm.c         | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/mem/pc-dimm.h |  4 +++
> >  3 files changed, 90 insertions(+), 60 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index e51c3be..cb57612 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -64,7 +64,6 @@
> >  #include "hw/pci/pci_host.h"
> >  #include "acpi-build.h"
> >  #include "hw/mem/pc-dimm.h"
> > -#include "trace.h"
> >  #include "qapi/visitor.h"
> >  #include "qapi-visit.h"
> >  
> > @@ -1554,88 +1553,35 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
> >  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> >                           DeviceState *dev, Error **errp)
> >  {
> > -    int slot;
> >      HotplugHandlerClass *hhc;
> >      Error *local_err = NULL;
> >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> > -    MachineState *machine = MACHINE(hotplug_dev);
> >      PCDIMMDevice *dimm = PC_DIMM(dev);
> >      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> >      MemoryRegion *mr = ddc->get_memory_region(dimm);
> > -    uint64_t existing_dimms_capacity = 0;
> >      uint64_t align = TARGET_PAGE_SIZE;
> > -    uint64_t addr;
> > -
> > -    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
> > -    if (local_err) {
> > -        goto out;
> > -    }
> >  
> >      if (memory_region_get_alignment(mr) && pcms->enforce_aligned_dimm) {
> >          align = memory_region_get_alignment(mr);
> >      }
> >  
> > -    addr = pc_dimm_get_free_addr(pcms->hotplug_memory.base,
> > -                                 memory_region_size(&pcms->hotplug_memory.mr),
> > -                                 !addr ? NULL : &addr, align,
> > -                                 memory_region_size(mr), &local_err);
> > -    if (local_err) {
> > -        goto out;
> > -    }
> > -
> > -    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
> > -    if (local_err) {
> > -        goto out;
> > -    }
> > -
> > -    if (existing_dimms_capacity + memory_region_size(mr) >
> > -        machine->maxram_size - machine->ram_size) {
> > -        error_setg(&local_err, "not enough space, currently 0x%" PRIx64
> > -                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
> > -                   existing_dimms_capacity,
> > -                   machine->maxram_size - machine->ram_size);
> > -        goto out;
> > -    }
> > -
> > -    object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
> > -    if (local_err) {
> > -        goto out;
> > -    }
> > -    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);
> > -
> >      if (!pcms->acpi_dev) {
> >          error_setg(&local_err,
> >                     "memory hotplug is not enabled: missing acpi device");
> >          goto out;
> >      }
> >  
> > -    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> > -        error_setg(&local_err, "hypervisor has no free memory slots left");
> > +    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align, &local_err);
> > +    if (local_err) {
> >          goto out;
> >      }
> >  
> > -    memory_region_add_subregion(&pcms->hotplug_memory.mr,
> > -                                addr - pcms->hotplug_memory.base, mr);
> > -    vmstate_register_ram(mr, dev);
> > -
> >      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> >      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
> > +    if (local_err) {
> > +        pc_dimm_memory_unplug(dev, &pcms->hotplug_memory, mr);
> > +    }
> You are adding something new while patch claims to be just moving code.
> I'm not sure it's save to call unplug here if hhc->plug() failed
> since it might have triggered guest visible side effects.

In sPAPR, the routine equivalent to hhc->plug() could fail and
I needed pc_dimm_memory_unplug() to recover. But now I have rearranged
the code there to ensure that it never fails like in x86. However
I need pc_dimm_memory_unplug() nevertheless in sPAPR.

> 
> I'd just drop this hunk and leave original code,
> on top of that extra patch for hhc->plug(s/local_err/error_abort/)
> might make a sense since hhc->plug() should never fail and if it's
> then we should fix error in code instead of trying to recover.

I have added a small patch for this in my series.

> 
> >  out:
> >      error_propagate(errp, local_err);
> >  }
> > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > index e70633d..98971b7 100644
> > --- a/hw/mem/pc-dimm.c
> > +++ b/hw/mem/pc-dimm.c
> > @@ -23,12 +23,92 @@
> >  #include "qapi/visitor.h"
> >  #include "qemu/range.h"
> >  #include "sysemu/numa.h"
> > +#include "sysemu/kvm.h"
> > +#include "trace.h"
> >  
> >  typedef struct pc_dimms_capacity {
> >       uint64_t size;
> >       Error    **errp;
> >  } pc_dimms_capacity;
> >  
> > +void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> > +                         MemoryRegion *mr, uint64_t align, Error **errp)
> > +{
> > +    int slot;
> > +    MachineState *machine = MACHINE(qdev_get_machine());
> > +    PCDIMMDevice *dimm = PC_DIMM(dev);
> > +    Error *local_err = NULL;
> > +    uint64_t existing_dimms_capacity = 0;
> > +    uint64_t addr;
> > +
> > +    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    addr = pc_dimm_get_free_addr(hpms->base,
> > +                                 memory_region_size(&hpms->mr),
> > +                                 !addr ? NULL : &addr, align,
> > +                                 memory_region_size(mr), &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    if (existing_dimms_capacity + memory_region_size(mr) >
> > +        machine->maxram_size - machine->ram_size) {
> > +        error_setg(&local_err, "not enough space, currently 0x%" PRIx64
> > +                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
> > +                   existing_dimms_capacity,
> > +                   machine->maxram_size - machine->ram_size);
> > +        goto out;
> > +    }
> > +
> > +    object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +    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);
> > +
> > +    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> > +        error_setg(&local_err, "hypervisor has no free memory slots left");
> > +        goto out;
> > +    }
> > +
> > +    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
> > +    vmstate_register_ram(mr, dev);
> > +
> > +out:
> > +    error_propagate(errp, local_err);
> > +}
> > +
> > +void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
> > +                           MemoryRegion *mr)
> > +{
> > +    memory_region_del_subregion(&hpms->mr, mr);
> > +    vmstate_unregister_ram(mr, dev);
> > +}
> if you adding this here then you probably should be removing/replacing
> related hunks from pc.c, I don't see it in this patch.

In v2, I was calling this routine from pc_dimm_unplug, but somehow missed
this hunk during rebasing in v3.

I have addressed all you comments for this version and ready with next
version. However will wait till monday before respinning to see if I
am required to do more changes.

Regards,
Bharata.
Igor Mammedov June 26, 2015, 12:02 p.m. UTC | #4 
On Fri, 26 Jun 2015 17:15:45 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Fri, Jun 26, 2015 at 10:21:54AM +0200, Igor Mammedov wrote:
> > On Fri, 26 Jun 2015 09:36:01 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > pc_dimm_plug() has code that will be needed for memory plug
> > > handlers in other archs too. Extract code from pc_dimm_plug()
> > > into a generic routine pc_dimm_memory_plug() that resides in
> > > pc-dimm.c. Also correspondingly refactor re-usable unplug code
> > > into pc_dimm_memory_unplug().
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > thanks for splitting, much easier to review.
> 
> Yes it is much easier to see the changes and thanks for the review.
> 
> > 
> > 
> > > ---
> > >  hw/i386/pc.c             | 66
> > > ++++----------------------------------- hw/mem/pc-dimm.c
> > > | 80 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > include/hw/mem/pc-dimm.h |  4 +++ 3 files changed, 90
> > > insertions(+), 60 deletions(-)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index e51c3be..cb57612 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -64,7 +64,6 @@
> > >  #include "hw/pci/pci_host.h"
> > >  #include "acpi-build.h"
> > >  #include "hw/mem/pc-dimm.h"
> > > -#include "trace.h"
> > >  #include "qapi/visitor.h"
> > >  #include "qapi-visit.h"
> > >  
> > > @@ -1554,88 +1553,35 @@ void ioapic_init_gsi(GSIState *gsi_state,
> > > const char *parent_name) static void pc_dimm_plug(HotplugHandler
> > > *hotplug_dev, DeviceState *dev, Error **errp)
> > >  {
> > > -    int slot;
> > >      HotplugHandlerClass *hhc;
> > >      Error *local_err = NULL;
> > >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> > > -    MachineState *machine = MACHINE(hotplug_dev);
> > >      PCDIMMDevice *dimm = PC_DIMM(dev);
> > >      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> > >      MemoryRegion *mr = ddc->get_memory_region(dimm);
> > > -    uint64_t existing_dimms_capacity = 0;
> > >      uint64_t align = TARGET_PAGE_SIZE;
> > > -    uint64_t addr;
> > > -
> > > -    addr = object_property_get_int(OBJECT(dimm),
> > > PC_DIMM_ADDR_PROP, &local_err);
> > > -    if (local_err) {
> > > -        goto out;
> > > -    }
> > >  
> > >      if (memory_region_get_alignment(mr) &&
> > > pcms->enforce_aligned_dimm) { align =
> > > memory_region_get_alignment(mr); }
> > >  
> > > -    addr = pc_dimm_get_free_addr(pcms->hotplug_memory.base,
> > > -
> > > memory_region_size(&pcms->hotplug_memory.mr),
> > > -                                 !addr ? NULL : &addr, align,
> > > -                                 memory_region_size(mr),
> > > &local_err);
> > > -    if (local_err) {
> > > -        goto out;
> > > -    }
> > > -
> > > -    existing_dimms_capacity =
> > > pc_existing_dimms_capacity(&local_err);
> > > -    if (local_err) {
> > > -        goto out;
> > > -    }
> > > -
> > > -    if (existing_dimms_capacity + memory_region_size(mr) >
> > > -        machine->maxram_size - machine->ram_size) {
> > > -        error_setg(&local_err, "not enough space, currently 0x%"
> > > PRIx64
> > > -                   " in use of total hot pluggable 0x"
> > > RAM_ADDR_FMT,
> > > -                   existing_dimms_capacity,
> > > -                   machine->maxram_size - machine->ram_size);
> > > -        goto out;
> > > -    }
> > > -
> > > -    object_property_set_int(OBJECT(dev), addr,
> > > PC_DIMM_ADDR_PROP, &local_err);
> > > -    if (local_err) {
> > > -        goto out;
> > > -    }
> > > -    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);
> > > -
> > >      if (!pcms->acpi_dev) {
> > >          error_setg(&local_err,
> > >                     "memory hotplug is not enabled: missing acpi
> > > device"); goto out;
> > >      }
> > >  
> > > -    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> > > -        error_setg(&local_err, "hypervisor has no free memory
> > > slots left");
> > > +    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align,
> > > &local_err);
> > > +    if (local_err) {
> > >          goto out;
> > >      }
> > >  
> > > -    memory_region_add_subregion(&pcms->hotplug_memory.mr,
> > > -                                addr -
> > > pcms->hotplug_memory.base, mr);
> > > -    vmstate_register_ram(mr, dev);
> > > -
> > >      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
> > >      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
> > > +    if (local_err) {
> > > +        pc_dimm_memory_unplug(dev, &pcms->hotplug_memory, mr);
> > > +    }
> > You are adding something new while patch claims to be just moving
> > code. I'm not sure it's save to call unplug here if hhc->plug()
> > failed since it might have triggered guest visible side effects.
> 
> In sPAPR, the routine equivalent to hhc->plug() could fail and
> I needed pc_dimm_memory_unplug() to recover. But now I have rearranged
> the code there to ensure that it never fails like in x86. However
> I need pc_dimm_memory_unplug() nevertheless in sPAPR.
> 
> > 
> > I'd just drop this hunk and leave original code,
> > on top of that extra patch for hhc->plug(s/local_err/error_abort/)
> > might make a sense since hhc->plug() should never fail and if it's
> > then we should fix error in code instead of trying to recover.
> 
> I have added a small patch for this in my series.
> 
> > 
> > >  out:
> > >      error_propagate(errp, local_err);
> > >  }
> > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > > index e70633d..98971b7 100644
> > > --- a/hw/mem/pc-dimm.c
> > > +++ b/hw/mem/pc-dimm.c
> > > @@ -23,12 +23,92 @@
> > >  #include "qapi/visitor.h"
> > >  #include "qemu/range.h"
> > >  #include "sysemu/numa.h"
> > > +#include "sysemu/kvm.h"
> > > +#include "trace.h"
> > >  
> > >  typedef struct pc_dimms_capacity {
> > >       uint64_t size;
> > >       Error    **errp;
> > >  } pc_dimms_capacity;
> > >  
> > > +void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState
> > > *hpms,
> > > +                         MemoryRegion *mr, uint64_t align, Error
> > > **errp) +{
> > > +    int slot;
> > > +    MachineState *machine = MACHINE(qdev_get_machine());
> > > +    PCDIMMDevice *dimm = PC_DIMM(dev);
> > > +    Error *local_err = NULL;
> > > +    uint64_t existing_dimms_capacity = 0;
> > > +    uint64_t addr;
> > > +
> > > +    addr = object_property_get_int(OBJECT(dimm),
> > > PC_DIMM_ADDR_PROP, &local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    addr = pc_dimm_get_free_addr(hpms->base,
> > > +                                 memory_region_size(&hpms->mr),
> > > +                                 !addr ? NULL : &addr, align,
> > > +                                 memory_region_size(mr),
> > > &local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    existing_dimms_capacity =
> > > pc_existing_dimms_capacity(&local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +
> > > +    if (existing_dimms_capacity + memory_region_size(mr) >
> > > +        machine->maxram_size - machine->ram_size) {
> > > +        error_setg(&local_err, "not enough space, currently 0x%"
> > > PRIx64
> > > +                   " in use of total hot pluggable 0x"
> > > RAM_ADDR_FMT,
> > > +                   existing_dimms_capacity,
> > > +                   machine->maxram_size - machine->ram_size);
> > > +        goto out;
> > > +    }
> > > +
> > > +    object_property_set_int(OBJECT(dev), addr,
> > > PC_DIMM_ADDR_PROP, &local_err);
> > > +    if (local_err) {
> > > +        goto out;
> > > +    }
> > > +    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);
> > > +
> > > +    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
> > > +        error_setg(&local_err, "hypervisor has no free memory
> > > slots left");
> > > +        goto out;
> > > +    }
> > > +
> > > +    memory_region_add_subregion(&hpms->mr, addr - hpms->base,
> > > mr);
> > > +    vmstate_register_ram(mr, dev);
> > > +
> > > +out:
> > > +    error_propagate(errp, local_err);
> > > +}
> > > +
> > > +void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState
> > > *hpms,
> > > +                           MemoryRegion *mr)
> > > +{
> > > +    memory_region_del_subregion(&hpms->mr, mr);
> > > +    vmstate_unregister_ram(mr, dev);
> > > +}
> > if you adding this here then you probably should be
> > removing/replacing related hunks from pc.c, I don't see it in this
> > patch.
> 
> In v2, I was calling this routine from pc_dimm_unplug, but somehow
> missed this hunk during rebasing in v3.
> 
> I have addressed all you comments for this version and ready with next
> version. However will wait till monday before respinning to see if I
> am required to do more changes.
I've did line by line review of all patches so there shouldn't  be
more comments from my side.


> 
> Regards,
> Bharata.
>
diff mbox

Patch

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e51c3be..cb57612 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -64,7 +64,6 @@ 
 #include "hw/pci/pci_host.h"
 #include "acpi-build.h"
 #include "hw/mem/pc-dimm.h"
-#include "trace.h"
 #include "qapi/visitor.h"
 #include "qapi-visit.h"
 
@@ -1554,88 +1553,35 @@  void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
 static void pc_dimm_plug(HotplugHandler *hotplug_dev,
                          DeviceState *dev, Error **errp)
 {
-    int slot;
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
-    MachineState *machine = MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr = ddc->get_memory_region(dimm);
-    uint64_t existing_dimms_capacity = 0;
     uint64_t align = TARGET_PAGE_SIZE;
-    uint64_t addr;
-
-    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
-    if (local_err) {
-        goto out;
-    }
 
     if (memory_region_get_alignment(mr) && pcms->enforce_aligned_dimm) {
         align = memory_region_get_alignment(mr);
     }
 
-    addr = pc_dimm_get_free_addr(pcms->hotplug_memory.base,
-                                 memory_region_size(&pcms->hotplug_memory.mr),
-                                 !addr ? NULL : &addr, align,
-                                 memory_region_size(mr), &local_err);
-    if (local_err) {
-        goto out;
-    }
-
-    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
-    if (local_err) {
-        goto out;
-    }
-
-    if (existing_dimms_capacity + memory_region_size(mr) >
-        machine->maxram_size - machine->ram_size) {
-        error_setg(&local_err, "not enough space, currently 0x%" PRIx64
-                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
-                   existing_dimms_capacity,
-                   machine->maxram_size - machine->ram_size);
-        goto out;
-    }
-
-    object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
-    if (local_err) {
-        goto out;
-    }
-    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);
-
     if (!pcms->acpi_dev) {
         error_setg(&local_err,
                    "memory hotplug is not enabled: missing acpi device");
         goto out;
     }
 
-    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
-        error_setg(&local_err, "hypervisor has no free memory slots left");
+    pc_dimm_memory_plug(dev, &pcms->hotplug_memory, mr, align, &local_err);
+    if (local_err) {
         goto out;
     }
 
-    memory_region_add_subregion(&pcms->hotplug_memory.mr,
-                                addr - pcms->hotplug_memory.base, mr);
-    vmstate_register_ram(mr, dev);
-
     hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
     hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
+    if (local_err) {
+        pc_dimm_memory_unplug(dev, &pcms->hotplug_memory, mr);
+    }
+
 out:
     error_propagate(errp, local_err);
 }
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index e70633d..98971b7 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -23,12 +23,92 @@ 
 #include "qapi/visitor.h"
 #include "qemu/range.h"
 #include "sysemu/numa.h"
+#include "sysemu/kvm.h"
+#include "trace.h"
 
 typedef struct pc_dimms_capacity {
      uint64_t size;
      Error    **errp;
 } pc_dimms_capacity;
 
+void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
+                         MemoryRegion *mr, uint64_t align, Error **errp)
+{
+    int slot;
+    MachineState *machine = MACHINE(qdev_get_machine());
+    PCDIMMDevice *dimm = PC_DIMM(dev);
+    Error *local_err = NULL;
+    uint64_t existing_dimms_capacity = 0;
+    uint64_t addr;
+
+    addr = object_property_get_int(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    addr = pc_dimm_get_free_addr(hpms->base,
+                                 memory_region_size(&hpms->mr),
+                                 !addr ? NULL : &addr, align,
+                                 memory_region_size(mr), &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    existing_dimms_capacity = pc_existing_dimms_capacity(&local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    if (existing_dimms_capacity + memory_region_size(mr) >
+        machine->maxram_size - machine->ram_size) {
+        error_setg(&local_err, "not enough space, currently 0x%" PRIx64
+                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
+                   existing_dimms_capacity,
+                   machine->maxram_size - machine->ram_size);
+        goto out;
+    }
+
+    object_property_set_int(OBJECT(dev), addr, PC_DIMM_ADDR_PROP, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    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);
+
+    if (kvm_enabled() && !kvm_has_free_slot(machine)) {
+        error_setg(&local_err, "hypervisor has no free memory slots left");
+        goto out;
+    }
+
+    memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
+    vmstate_register_ram(mr, dev);
+
+out:
+    error_propagate(errp, local_err);
+}
+
+void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
+                           MemoryRegion *mr)
+{
+    memory_region_del_subregion(&hpms->mr, mr);
+    vmstate_unregister_ram(mr, dev);
+}
+
 static int pc_existing_dimms_capacity_internal(Object *obj, void *opaque)
 {
     pc_dimms_capacity *cap = opaque;
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index d528a3f..3eabef4 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -90,4 +90,8 @@  int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
 
 int qmp_pc_dimm_device_list(Object *obj, void *opaque);
 uint64_t pc_existing_dimms_capacity(Error **errp);
+void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
+                         MemoryRegion *mr, uint64_t align, Error **errp);
+void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
+                           MemoryRegion *mr);
 #endif