Re: [Qemu-devel] [PATCH v3 13/22] memory-device: complete factoring out unplug handling

[David Hildenbrand]

On 24/09/2018 16:18, Igor Mammedov wrote:
> On Thu, 20 Sep 2018 12:32:34 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> With the new memory device functions in place, we can factor out
>> unplugging of memory devices completely.
>>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  hw/mem/memory-device.c         | 8 ++++++--
>>  hw/mem/pc-dimm.c               | 4 +---
>>  include/hw/mem/memory-device.h | 2 +-
>>  3 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> index 34e92148ec..3e5a3a7951 100644
>> --- a/hw/mem/memory-device.c
>> +++ b/hw/mem/memory-device.c
>> @@ -289,9 +289,13 @@ void memory_device_plug(MemoryDeviceState *md, 
>> MachineState *ms)
>>                                  addr - ms->device_memory->base, mr);
>>  }
>>  
>> -void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr)
>> +void memory_device_unplug(MemoryDeviceState *md, MachineState *ms)
> I'd keep machine argument 1st since it's machine helper after all,
> but that's a matter of taste.

I'll stick to this for now (this is also how pc_dimm handles it, and
probably not worth the trouble).

> 
>>  {
>> -    /* we expect a previous call to memory_device_get_free_addr() */
>> +    const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
>> +    MemoryRegion *mr;
>> +
>> +    /* we expect a previous call to memory_device_pre_plug() */
> ... 'succeeded and can't fail at this point */
> 
>> +    mr = mdc->get_memory_region(md, &error_abort);
>>      g_assert(ms->device_memory);
>>  
>>      memory_region_del_subregion(&ms->device_memory->mr, mr);
>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>> index db8f71c672..915908b104 100644
>> --- a/hw/mem/pc-dimm.c
>> +++ b/hw/mem/pc-dimm.c
>> @@ -74,12 +74,10 @@ void pc_dimm_unplug(DeviceState *dev, MachineState 
>> *machine)
>>  {
>>      PCDIMMDevice *dimm = PC_DIMM(dev);
>>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>> -    MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(dev);
>>      MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
>>                                                                &error_abort);
>> -    MemoryRegion *mr = mdc->get_memory_region(MEMORY_DEVICE(dev), 
>> &error_abort);
>>  
>> -    memory_device_unplug_region(machine, mr);
>> +    memory_device_unplug(MEMORY_DEVICE(dev), machine);
>>      vmstate_unregister_ram(vmstate_mr, dev);
>>  }
>>  
>> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
>> index 5ba36f98fb..fd0b43c224 100644
>> --- a/include/hw/mem/memory-device.h
>> +++ b/include/hw/mem/memory-device.h
>> @@ -61,7 +61,7 @@ uint64_t get_plugged_memory_size(void);
>>  void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
>>                              const uint64_t *legacy_align, Error **errp);
>>  void memory_device_plug(MemoryDeviceState *md, MachineState *ms);
>> -void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr);
>> +void memory_device_unplug(MemoryDeviceState *md, MachineState *ms);
>>  uint64_t memory_device_get_region_size(const MemoryDeviceState *md,
>>                                         Error **errp);
>>  
> Otherwise looks good
> 
> Reviewed-by: Igor Mammedov <address@hidden>
> 

Thanks!


-- 

Thanks,

David / dhildenb