Re: [Qemu-devel] [PATCH v4 14/14] memory-device: factor out plug into hotplug handler

[David Hildenbrand]

On 01.06.2018 13:39, Igor Mammedov wrote:
> On Thu, 17 May 2018 10:15:27 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> Let's move the plug logic into the applicable hotplug handler for pc and
>> spapr.
>>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  hw/i386/pc.c                   | 35 ++++++++++++++++++++---------------
>>  hw/mem/memory-device.c         | 40 ++++++++++++++++++++++++++++++++++------
>>  hw/mem/pc-dimm.c               | 29 +----------------------------
>>  hw/mem/trace-events            |  2 +-
>>  hw/ppc/spapr.c                 | 15 ++++++++++++---
>>  include/hw/mem/memory-device.h |  7 ++-----
>>  include/hw/mem/pc-dimm.h       |  3 +--
>>  7 files changed, 71 insertions(+), 60 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 426fb534c2..f022eb042e 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1682,22 +1682,8 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>>      HotplugHandlerClass *hhc;
>>      Error *local_err = NULL;
>>      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>> -    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>> -    PCDIMMDevice *dimm = PC_DIMM(dev);
>> -    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>> -    MemoryRegion *mr;
>> -    uint64_t align = TARGET_PAGE_SIZE;
>>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>>  
>> -    mr = ddc->get_memory_region(dimm, &local_err);
>> -    if (local_err) {
>> -        goto out;
>> -    }
>> -
>> -    if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
>> -        align = memory_region_get_alignment(mr);
>> -    }
>> -
>>      /*
>>       * When -no-acpi is used with Q35 machine type, no ACPI is built,
>>       * but pcms->acpi_dev is still created. Check !acpi_enabled in
>> @@ -1715,7 +1701,7 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>>          goto out;
>>      }
>>  
>> -    pc_dimm_memory_plug(dev, MACHINE(pcms), align, &local_err);
>> +    pc_dimm_memory_plug(dev, MACHINE(pcms), &local_err);
>>      if (local_err) {
>>          goto out;
>>      }
>> @@ -2036,6 +2022,25 @@ static void pc_machine_device_plug_cb(HotplugHandler 
>> *hotplug_dev,
>>  {
>>      Error *local_err = NULL;
>>  
>> +    /* first stage hotplug handler */
>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>> +        const PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(hotplug_dev);
>> +        uint64_t align = 0;
>> +
>> +        /* compat handling: force to TARGET_PAGE_SIZE */
>> +        if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
>> +            !pcmc->enforce_aligned_dimm) {
>> +            align = TARGET_PAGE_SIZE;
>> +        }
>> +        memory_device_plug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev),
>> +                           align ? &align : NULL, &local_err);
>> +    }
>> +
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>>      /* final stage hotplug handler */
>>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>>          pc_dimm_plug(hotplug_dev, dev, &local_err);
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> index 8f10d613ea..04bdb30f22 100644
>> --- a/hw/mem/memory-device.c
>> +++ b/hw/mem/memory-device.c
>> @@ -69,9 +69,10 @@ static int memory_device_used_region_size(Object *obj, 
>> void *opaque)
>>      return 0;
>>  }
>>  
>> -uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
>> -                                     uint64_t align, uint64_t size,
>> -                                     Error **errp)
>> +static uint64_t memory_device_get_free_addr(MachineState *ms,
>> +                                            const uint64_t *hint,
>> +                                            uint64_t align, uint64_t size,
>> +                                            Error **errp)
>>  {
>>      uint64_t address_space_start, address_space_end;
>>      uint64_t used_region_size = 0;
>> @@ -237,11 +238,38 @@ void memory_device_pre_plug(MachineState *ms, const 
>> MemoryDeviceState *md,
>>      }
>>  }
>>  
>> -void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
>> -                               uint64_t addr)
>> +void memory_device_plug(MachineState *ms, MemoryDeviceState *md,
>> +                        uint64_t *enforced_align, Error **errp)
> enforced_align is PC machine specific compat flag
> to keep old machines with unaligned layout work (i.e. don't break 
> CLI/migration)
> it shouldn't go into a generic code.
> By default all new machines should use aligned layout. 
> 

Yes, but there has to be a way for the search to access this property.
So what do you propose in contrast to this?

-- 

Thanks,

David / dhildenb