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

From: Hu Tao <hutao@cn.fujitsu.com>

Implement unrealize function for pc-dimm device. It delete subregion from
hotplug region, and delete ram address range from guest ram list.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 hw/mem/pc-dimm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

On Wed, 27 Aug 2014 16:08:36 +0800
Tang Chen <tangchen@cn.fujitsu.com> wrote:

> From: Hu Tao <hutao@cn.fujitsu.com>
> 
> 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 <hutao@cn.fujitsu.com>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  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

> +}
> +
>  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;

On 2014/9/4 21:28, Igor Mammedov wrote:
> On Wed, 27 Aug 2014 16:08:36 +0800
> Tang Chen<tangchen@cn.fujitsu.com>  wrote:
>
>> From: Hu Tao<hutao@cn.fujitsu.com>
>>
>> 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<hutao@cn.fujitsu.com>
>> Signed-off-by: Tang Chen<tangchen@cn.fujitsu.com>
>> ---
>>   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...

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;
>
>
>
>

On Fri, 12 Sep 2014 13:30:36 +0800
zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:

> On 2014/9/4 21:28, Igor Mammedov wrote:
> > On Wed, 27 Aug 2014 16:08:36 +0800
> > Tang Chen<tangchen@cn.fujitsu.com>  wrote:
> >
> >> From: Hu Tao<hutao@cn.fujitsu.com>
> >>
> >> 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<hutao@cn.fujitsu.com>
> >> Signed-off-by: Tang Chen<tangchen@cn.fujitsu.com>
> >> ---
> >>   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;
> >
> >
> >
> >
> 
>

Hi Igor, Zhang,

On 09/12/2014 09:17 PM, Igor Mammedov wrote:
> ......
> 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/)

I've read the above patch.

1. I think you proposal would help to resolve the following problem:

     If guest OS failed to handle hoplug sci, QEmu does not know it, and 
could
     release resources incorrectly.

     Would you please have a look at the following patch.
https://www.mail-archive.com/qemu-devel%40nongnu.org/msg253025.html

     I added a pthread wait condition to synchronize :
     sci request -> guest OS handling -> _OST -> QEmu handling
     (Of course, according to the previous discussing, _OST is optional.)

     And IIUC, your proposal may be :
     hotplug request -> guest OS handling -> real hotplug handling

     right ?

     I think it is the same. How do you think synchronize it with a wait 
condition ?
     Or you have any better idea ?
     Since no one has replied the patch, I'm not sure if it is OK.


2. Let's finish memory and cpu hotplug job based on the current 
framework, shall we ?
     In the above patch, it renames a lot of functions that are being 
used in memory and
     cpu hotplug patches. I think we can push memory and cpu hotplug 
jobs, and in the
     next phase, let's improve the framework.

     And of course, the problem I mentioned above should also be put in 
the next phase.

     So I want to submit the next version memory hotplug patches based 
on the original
     framework, and help to improve it in the next phase.

How do you think ?

>
> 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.
>
>
>