[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 4/4] pc-dimm: assign and verify the "addr" pr
|
From: |
David Gibson |
|
Subject: |
Re: [Qemu-devel] [PATCH v2 4/4] pc-dimm: assign and verify the "addr" property during pre_plug |
|
Date: |
Fri, 29 Jun 2018 11:19:33 +1000 |
|
User-agent: |
Mutt/1.10.0 (2018-05-17) |
On Thu, Jun 28, 2018 at 02:14:17PM +0200, David Hildenbrand 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.
>
> 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.
>
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
> 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;
>
> /*
> * 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);
Why would you pass a const pointer, rather than just a value?
> }
>
> 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);
> + }
> +
> + 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);
> 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
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature