Re: [Qemu-devel] [RESEND PATCH v3 5/8] pc-dimm: Add pc_dimm_unrealize() for memory hot unplug support.

[Igor Mammedov]

On Fri, 12 Sep 2014 13:30:36 +0800
zhanghailiang <address@hidden> wrote:

> On 2014/9/4 21:28, Igor Mammedov wrote:
> > On Wed, 27 Aug 2014 16:08:36 +0800
> > Tang Chen<address@hidden>  wrote:
> >
> >> From: Hu Tao<address@hidden>
> >>
> >> Implement unrealize function for pc-dimm device. It delete subregion from
> > s/delete/removes/
> >
> >> hotplug region, and delete ram address range from guest ram list.
> >>
> >> Signed-off-by: Hu Tao<address@hidden>
> >> Signed-off-by: Tang Chen<address@hidden>
> >> ---
> >>   hw/mem/pc-dimm.c | 10 ++++++++++
> >>   1 file changed, 10 insertions(+)
> >>
> >> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> >> index 20fe0dc..34109a2 100644
> >> --- a/hw/mem/pc-dimm.c
> >> +++ b/hw/mem/pc-dimm.c
> >> @@ -270,12 +270,22 @@ static MemoryRegion 
> >> *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
> >>       return host_memory_backend_get_memory(dimm->hostmem,&error_abort);
> >>   }
> >>
> >> +static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
> >> +{
> >> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> >> +    MemoryRegion *mr = pc_dimm_get_memory_region(dimm);
> >> +
> >> +    memory_region_del_subregion(mr->container, mr);
> > usually MemoryRegion is treated as opaque and it's fields
> > accessed via memory_region_foo() helpers.
> >
> >> +    vmstate_unregister_ram(mr, dev);
> > these 2 lines look like a job for PCMachine which did original 
> > mapping/vmstate registration
> >
> 
> Actually, this patch also fix the bug *when hotplug memory failing in
> the place where after pc_dimm_plug but before the end of device_set_realized,
> it does not clear the work done by pc_dimm_plug*.
> 
> For there is no callback like pc_dimm_plug_fail_cb for us to call when fail,
> Maybe pc_dimm_unrealize is the only place where we can do the clear work...
Looking at device_set_realized() and pc-dimm case in patrticular
there is no point where it could fail after hotplug_handler_plug() is called.

But even if there where, one should use pc_dimm_unplug() first to
reverse actions performed by successful pc_dimm_plug().

The problem here is that currently unplug callback is actually
doing only unplug request part asking guest to eject memory,
but we also have destroy device when guest tells via ACPI to
ejct memory. You are doing it implicitly by unparenting pc-dimm
from ACPI code and pulling in pc-dimm.unrealize() unrelated
stuff that should be done by PCMachine.

I'm suggesting that we extend hotplug-handler API to handle
this async/split unplug workflow. By converting current
hotplug_handler_unplug() and related code to
hotplug_handler_unplug_request() that would do the first part
of unplug sequence (see/review http://patchwork.ozlabs.org/patch/387018/)

And then on top of it add hotplug_handler_unplug() that would
handle actual device removal when ACPI asks for it.

I'm working now on doing above for PCIDevices since they have
the same workflow (expect to submit patches next week) and
it looks like we would need to use the same approach for CPU
unplug as well.


> 
> Thanks,
> zhanghailiang
> 
> >> +}
> >> +
> >>   static void pc_dimm_class_init(ObjectClass *oc, void *data)
> >>   {
> >>       DeviceClass *dc = DEVICE_CLASS(oc);
> >>       PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
> >>
> >>       dc->realize = pc_dimm_realize;
> >> +    dc->unrealize = pc_dimm_unrealize;
> >>       dc->props = pc_dimm_properties;
> >>
> >>       ddc->get_memory_region = pc_dimm_get_memory_region;
> >
> >
> >
> >
> 
>