On Thu, 28 Jun 2018 14:14:17 +0200
David Hildenbrand <da...@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 <da...@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
