On Fri, 26 Jun 2015 17:15:45 +0530
Bharata B Rao <bhar...@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 <bhar...@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 <bhar...@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.
>
