Re: [Qemu-devel] [PATCH v2 4/4] pc-dimm: assign and verify the "addr" property during pre_plug

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 28 Jun 2018 14:14:17 +0200
David Hildenbrand <david@redhat.com> wrote:

> We can assign and verify the slot before realizing and trying to plug.
> reading/writing the address property should never fail, so let's reduce
> error handling a bit by using &error_abort. Getting access to the memory
> region now might however fail. So forward errors from
> get_memory_region() properly.
> 
> Keep tracing the assigned address for now in the plug code, as that's the
> point we are sure plugging succeeded and the address willa ctually be
                                                       ^^^^^^^^^^^^^   
> used.
I'd move tracing to pre_plug as well, so even if pre_plug fails we could
see in trace what addr was during the failure

> As all memory devices should use the alignment of the underlying memory
> region for guest physical address asignment, do detection of the
> alignment in pc_dimm_pre_plug(), but allow pc.c to overwrite the
> alignment for compatibility handling.

patch look ok,
see some nits below

 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/i386/pc.c             | 16 ++++--------
>  hw/mem/pc-dimm.c         | 53 +++++++++++++++++++++++-----------------
>  hw/ppc/spapr.c           |  7 +++---
>  include/hw/mem/pc-dimm.h |  6 ++---
>  4 files changed, 41 insertions(+), 41 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 934b7155b1..16e4b5baff 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1678,7 +1678,9 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                 Error **errp)
>  {
>      const PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> +    const PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>      const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> +    const uint64_t compat_align = TARGET_PAGE_SIZE;
s/compat_align/legacy_align/

>  
>      /*
>       * When -no-acpi is used with Q35 machine type, no ACPI is built,
> @@ -1696,7 +1698,8 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
> +    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev),
> +                     pcmc->enforce_aligned_dimm ? NULL : &compat_align, errp);
>  }
>  
>  static void pc_memory_plug(HotplugHandler *hotplug_dev,
> @@ -1705,18 +1708,9 @@ static void pc_memory_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 = ddc->get_memory_region(dimm, &error_abort);
> -    uint64_t align = TARGET_PAGE_SIZE;
>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
> -    if (pcmc->enforce_aligned_dimm) {
> -        align = memory_region_get_alignment(mr);
> -    }
> -
> -    pc_dimm_plug(dev, MACHINE(pcms), align, &local_err);
> +    pc_dimm_plug(dev, MACHINE(pcms), &local_err);
>      if (local_err) {
>          goto out;
>      }
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index e56c4daef2..8e2e8549dc 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -29,9 +29,14 @@
>  
>  static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
>  
> -void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp)
> +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine,
> +                      const uint64_t *compat_align, Error **errp)
>  {
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      Error *local_err = NULL;
> +    MemoryRegion *mr;
> +    uint64_t addr, align;
>      int slot;
>  
>      slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
> @@ -43,44 +48,46 @@ void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp)
>      }
>      object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &error_abort);
>      trace_mhp_pc_dimm_assigned_slot(slot);
> +
> +    mr = ddc->get_memory_region(dimm, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    if (compat_align) {
> +        align = *compat_align;
> +    } else {
> +        align = memory_region_get_alignment(mr);
> +    }
might be:

       align =  compat_align ? *compat_align : memory_region_get_alignment(mr);

> +
> +    addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP,
> +                                    &error_abort);
> +    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,
> +                             &error_abort);
if one considers memory device as separate black box, then set might fail
(there is no guaranties that later on set could return a error)

So I'd keep local_err & co for set actions through out all *_plug handlers


>  out:
>      error_propagate(errp, local_err);
>  }
>  
> -void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
> -                  Error **errp)
> +void pc_dimm_plug(DeviceState *dev, MachineState *machine, Error **errp)
>  {
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
>                                                                &error_abort);
>      MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> -    Error *local_err = NULL;
>      uint64_t addr;
>  
> -    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;
> -    }
> +    addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP,
> +                                    &error_abort);
>      trace_mhp_pc_dimm_assigned_address(addr);
>  
>      memory_device_plug_region(machine, mr, addr);
>      vmstate_register_ram(vmstate_mr, dev);
> -
> -out:
> -    error_propagate(errp, local_err);
>  }
>  
>  void pc_dimm_unplug(DeviceState *dev, MachineState *machine)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bf012235b6..33543c6373 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3150,13 +3150,12 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      PCDIMMDevice *dimm = PC_DIMM(dev);
>      PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>      MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> -    uint64_t align, size, addr;
> +    uint64_t size, addr;
>      uint32_t node;
>  
> -    align = memory_region_get_alignment(mr);
>      size = memory_region_size(mr);
>  
> -    pc_dimm_plug(dev, MACHINE(ms), align, &local_err);
> +    pc_dimm_plug(dev, MACHINE(ms), &local_err);
>      if (local_err) {
>          goto out;
>      }
> @@ -3223,7 +3222,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
> +    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), NULL, errp);
>  }
>  
>  struct sPAPRDIMMState {
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index 7b120416d1..6457b2a25f 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -79,8 +79,8 @@ typedef struct PCDIMMDeviceClass {
>                                                 Error **errp);
>  } PCDIMMDeviceClass;
>  
> -void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp);
> -void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
> -                  Error **errp);
> +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine,
> +                      const uint64_t *compat_align, Error **errp);
> +void pc_dimm_plug(DeviceState *dev, MachineState *machine, Error **errp);
>  void pc_dimm_unplug(DeviceState *dev, MachineState *machine);
>  #endif