diff mbox series

[v4,14/14] memory-device: factor out plug into hotplug handler

Message ID 20180517081527.14410-15-david@redhat.com
State New
Headers show
Series MemoryDevice: use multi stage hotplug handlers | expand

Commit Message

David Hildenbrand May 17, 2018, 8:15 a.m. UTC 
Let's move the plug logic into the applicable hotplug handler for pc and
spapr.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 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(-)

Comments

Igor Mammedov June 1, 2018, 11:39 a.m. UTC | #1 
On Thu, 17 May 2018 10:15:27 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's move the plug logic into the applicable hotplug handler for pc and
> spapr.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  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. 

>  {
> -    /* we expect a previous call to memory_device_get_free_addr() */
> +    const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
> +    const uint64_t size = mdc->get_region_size(md);
> +    MemoryRegion *mr = mdc->get_memory_region(md);
> +    uint64_t addr = mdc->get_addr(md);
> +    uint64_t align;
> +
> +    /* we expect a previous call to memory_device_pre_plug */
>      g_assert(ms->device_memory);
> +    g_assert(mr && !memory_region_is_mapped(mr));
> +
> +    /* compat handling, some alignment has to be enforced for DIMMs */
> +    if (enforced_align) {
> +        align = *enforced_align;
> +    } else {
> +        align = memory_region_get_alignment(mr);
> +    }
> +
> +    /* our device might have stronger alignment requirements */
> +    if (mdc->get_align) {
> +        align = MAX(align, mdc->get_align(md));
> +    }
> +
> +    addr = memory_device_get_free_addr(ms, !addr ? NULL : &addr, align,
> +                                       size, errp);
> +    if (*errp) {
> +        return;
> +    }
> +    trace_memory_device_assign_address(addr);
> +    mdc->set_addr(md, addr);
>  
>      memory_region_add_subregion(&ms->device_memory->mr,
>                                  addr - ms->device_memory->base, mr);
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index d487bb513b..8b1dcb3260 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -32,39 +32,13 @@ typedef struct pc_dimms_capacity {
>       Error    **errp;
>  } pc_dimms_capacity;
>  
> -void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
> -                         uint64_t align, Error **errp)
> +void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, Error **errp)
>  {
>      int slot;
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
>      Error *local_err = NULL;
> -    MemoryRegion *mr;
> -    uint64_t addr;
> -
> -    mr = ddc->get_memory_region(dimm, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    addr = object_property_get_uint(OBJECT(dimm),
> -                                    PC_DIMM_ADDR_PROP, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    addr = memory_device_get_free_addr(machine, !addr ? NULL : &addr, align,
> -                                       memory_region_size(mr), &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -
> -    object_property_set_uint(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) {
> @@ -82,7 +56,6 @@ void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
>      }
>      trace_mhp_pc_dimm_assigned_slot(slot);
>  
> -    memory_device_plug_region(machine, mr, addr);
>      vmstate_register_ram(vmstate_mr, dev);
>  
>  out:
> diff --git a/hw/mem/trace-events b/hw/mem/trace-events
> index a661ee49a3..930b6aa6ea 100644
> --- a/hw/mem/trace-events
> +++ b/hw/mem/trace-events
> @@ -2,6 +2,6 @@
>  
>  # hw/mem/pc-dimm.c
>  mhp_pc_dimm_assigned_slot(int slot) "%d"
> -mhp_pc_dimm_assigned_address(uint64_t addr) "0x%"PRIx64
>  # hw/mem/memory-device.c
> +memory_device_assign_address(uint64_t addr) "0x%"PRIx64
>  memory_device_unassign_address(uint64_t addr) "0x%"PRIx64
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index abdd38a6b5..5a4dbbf31e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3144,16 +3144,15 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *mr;
> -    uint64_t align, size, addr;
> +    uint64_t size, addr;
>  
>      mr = ddc->get_memory_region(dimm, &local_err);
>      if (local_err) {
>          goto out;
>      }
> -    align = memory_region_get_alignment(mr);
>      size = memory_region_size(mr);
>  
> -    pc_dimm_memory_plug(dev, MACHINE(ms), align, &local_err);
> +    pc_dimm_memory_plug(dev, MACHINE(ms), &local_err);
>      if (local_err) {
>          goto out;
>      }
> @@ -3595,6 +3594,16 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>      sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>      Error *local_err = NULL;
>  
> +    /* first stage hotplug handler */
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
> +        memory_device_plug(ms, MEMORY_DEVICE(dev), 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)) {
>          int node;
> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
> index b8365959e7..a7408597fd 100644
> --- a/include/hw/mem/memory-device.h
> +++ b/include/hw/mem/memory-device.h
> @@ -53,11 +53,8 @@ MemoryDeviceInfoList *qmp_memory_device_list(void);
>  uint64_t get_plugged_memory_size(void);
>  void memory_device_pre_plug(MachineState *ms, const MemoryDeviceState *md,
>                              Error **errp);
> -uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
> -                                     uint64_t align, uint64_t size,
> -                                     Error **errp);
> -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);
>  void memory_device_unplug(MachineState *ms, MemoryDeviceState *md);
>  
>  #endif
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 627c8601d9..006c80fb2e 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -78,7 +78,6 @@ typedef struct PCDIMMDeviceClass {
>  
>  int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
>  
> -void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
> -                         uint64_t align, Error **errp);
> +void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, Error **errp);
>  void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine);
>  #endif
David Hildenbrand June 4, 2018, 11:47 a.m. UTC | #2 
On 01.06.2018 13:39, Igor Mammedov wrote:
> On Thu, 17 May 2018 10:15:27 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Let's move the plug logic into the applicable hotplug handler for pc and
>> spapr.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  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?
David Hildenbrand June 7, 2018, 10:44 a.m. UTC | #3 
On 04.06.2018 13:47, David Hildenbrand wrote:
> On 01.06.2018 13:39, Igor Mammedov wrote:
>> On Thu, 17 May 2018 10:15:27 +0200
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> Let's move the plug logic into the applicable hotplug handler for pc and
>>> spapr.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  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?
> 

FYI, I now have a patch like this:

commit 64cf8b1c210ffc86283ce2f677d425c6569b9358
Author: David Hildenbrand <david@redhat.com>
Date:   Wed Jun 6 21:00:27 2018 +0200

    machine: introduce memory_device_align (factor out enforce_aligned_dimm)
    
    We will handle memory device alignment completely in memory-device.c,
    without passing compatibility parameters ("*align").
    
    As x86 and Power use different strategies to determine an alignment and
    we need clean support for compat handling, let's introduce an enum on
    the machine class level.
    
    The three introduced types represent what is being done on x86 and Power
    right now.
    
    The x86 part is only changed to temporarily keep working, we'll factor
    this out into common code soon.
    
    Signed-off-by: David Hildenbrand <david@redhat.com>

...
diff --git a/include/hw/boards.h b/include/hw/boards.h
index ef7457f5dd..3f151207c1 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -105,6 +105,15 @@ typedef struct {
     CPUArchId cpus[0];
 } CPUArchIdList;
 
+typedef enum MemoryDeviceAlign {
+    /* use specified memory region alignment */
+    MEMORY_DEVICE_ALIGN_REGION = 0,
+    /* use target page size as alignment */
+    MEMORY_DEVICE_ALIGN_PAGE,
+    /* use target page size if no memory region alignment has been specified */
+    MEMORY_DEVICE_ALIGN_REGION_OR_PAGE,
+} MemoryDeviceAlign;
+
 /**
  * MachineClass:
  * @max_cpus: maximum number of CPUs supported. Default: 1
@@ -156,6 +165,9 @@ typedef struct {
  *    should instead use "unimplemented-device" for all memory ranges where
  *    the guest will attempt to probe for a device that QEMU doesn't
  *    implement and a stub device is required.
+ * @memory_device_align: The alignment that will be used as default when
+ *    searching for a guest physical memory address while plugging a
+ *    memory device. This is relevant for compatibility handling.
  */
 struct MachineClass {
     /*< private >*/
@@ -202,6 +214,7 @@ struct MachineClass {
     const char **valid_cpu_types;
     strList *allowed_dynamic_sysbus_devices;
     bool auto_enable_numa_with_memhp;
+    MemoryDeviceAlign memory_device_align;
     void (*numa_auto_assign_ram)(MachineClass *mc, NodeInfo *nodes,
                                  int nb_nodes, ram_addr_t size);
 
...
diff mbox series

Patch

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)
 {
-    /* we expect a previous call to memory_device_get_free_addr() */
+    const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
+    const uint64_t size = mdc->get_region_size(md);
+    MemoryRegion *mr = mdc->get_memory_region(md);
+    uint64_t addr = mdc->get_addr(md);
+    uint64_t align;
+
+    /* we expect a previous call to memory_device_pre_plug */
     g_assert(ms->device_memory);
+    g_assert(mr && !memory_region_is_mapped(mr));
+
+    /* compat handling, some alignment has to be enforced for DIMMs */
+    if (enforced_align) {
+        align = *enforced_align;
+    } else {
+        align = memory_region_get_alignment(mr);
+    }
+
+    /* our device might have stronger alignment requirements */
+    if (mdc->get_align) {
+        align = MAX(align, mdc->get_align(md));
+    }
+
+    addr = memory_device_get_free_addr(ms, !addr ? NULL : &addr, align,
+                                       size, errp);
+    if (*errp) {
+        return;
+    }
+    trace_memory_device_assign_address(addr);
+    mdc->set_addr(md, addr);
 
     memory_region_add_subregion(&ms->device_memory->mr,
                                 addr - ms->device_memory->base, mr);
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index d487bb513b..8b1dcb3260 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -32,39 +32,13 @@  typedef struct pc_dimms_capacity {
      Error    **errp;
 } pc_dimms_capacity;
 
-void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
-                         uint64_t align, Error **errp)
+void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, Error **errp)
 {
     int slot;
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
     Error *local_err = NULL;
-    MemoryRegion *mr;
-    uint64_t addr;
-
-    mr = ddc->get_memory_region(dimm, &local_err);
-    if (local_err) {
-        goto out;
-    }
-
-    addr = object_property_get_uint(OBJECT(dimm),
-                                    PC_DIMM_ADDR_PROP, &local_err);
-    if (local_err) {
-        goto out;
-    }
-
-    addr = memory_device_get_free_addr(machine, !addr ? NULL : &addr, align,
-                                       memory_region_size(mr), &local_err);
-    if (local_err) {
-        goto out;
-    }
-
-    object_property_set_uint(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) {
@@ -82,7 +56,6 @@  void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
     }
     trace_mhp_pc_dimm_assigned_slot(slot);
 
-    memory_device_plug_region(machine, mr, addr);
     vmstate_register_ram(vmstate_mr, dev);
 
 out:
diff --git a/hw/mem/trace-events b/hw/mem/trace-events
index a661ee49a3..930b6aa6ea 100644
--- a/hw/mem/trace-events
+++ b/hw/mem/trace-events
@@ -2,6 +2,6 @@ 
 
 # hw/mem/pc-dimm.c
 mhp_pc_dimm_assigned_slot(int slot) "%d"
-mhp_pc_dimm_assigned_address(uint64_t addr) "0x%"PRIx64
 # hw/mem/memory-device.c
+memory_device_assign_address(uint64_t addr) "0x%"PRIx64
 memory_device_unassign_address(uint64_t addr) "0x%"PRIx64
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index abdd38a6b5..5a4dbbf31e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3144,16 +3144,15 @@  static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *mr;
-    uint64_t align, size, addr;
+    uint64_t size, addr;
 
     mr = ddc->get_memory_region(dimm, &local_err);
     if (local_err) {
         goto out;
     }
-    align = memory_region_get_alignment(mr);
     size = memory_region_size(mr);
 
-    pc_dimm_memory_plug(dev, MACHINE(ms), align, &local_err);
+    pc_dimm_memory_plug(dev, MACHINE(ms), &local_err);
     if (local_err) {
         goto out;
     }
@@ -3595,6 +3594,16 @@  static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     Error *local_err = NULL;
 
+    /* first stage hotplug handler */
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        memory_device_plug(ms, MEMORY_DEVICE(dev), 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)) {
         int node;
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index b8365959e7..a7408597fd 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -53,11 +53,8 @@  MemoryDeviceInfoList *qmp_memory_device_list(void);
 uint64_t get_plugged_memory_size(void);
 void memory_device_pre_plug(MachineState *ms, const MemoryDeviceState *md,
                             Error **errp);
-uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
-                                     uint64_t align, uint64_t size,
-                                     Error **errp);
-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);
 void memory_device_unplug(MachineState *ms, MemoryDeviceState *md);
 
 #endif
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 627c8601d9..006c80fb2e 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -78,7 +78,6 @@  typedef struct PCDIMMDeviceClass {
 
 int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
 
-void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
-                         uint64_t align, Error **errp);
+void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine, Error **errp);
 void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine);
 #endif