On Thu, Sep 20, 2018 at 12:32:29PM +0200, David Hildenbrand wrote: > The memory region is necessary for plugging/unplugging a memory device. > The region size (via get_region_size()) is no longer sufficient, as > besides the alignment, also the region itself is required in order to > add it to the device memory region of the machine via > - memory_region_add_subregion > - memory_region_del_subregion > > So, to factor out plugging/unplugging of memory devices from pc-dimm > code, we have to factor out access to the memory region first. > > Signed-off-by: David Hildenbrand <da...@redhat.com>
Reviewed-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
> hw/mem/nvdimm.c | 9 ++++++---
> hw/mem/pc-dimm.c | 26 ++++++++++++++++----------
> include/hw/mem/memory-device.h | 5 +++++
> include/hw/mem/pc-dimm.h | 4 ----
> 4 files changed, 27 insertions(+), 17 deletions(-)
>
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 1c6674c4ed..49324f3fae 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -27,6 +27,7 @@
> #include "qapi/error.h"
> #include "qapi/visitor.h"
> #include "hw/mem/nvdimm.h"
> +#include "hw/mem/memory-device.h"
>
> static void nvdimm_get_label_size(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> @@ -118,9 +119,10 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice
> *nvdimm, Error **errp)
> nvdimm->nvdimm_mr->align = align;
> }
>
> -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error
> **errp)
> +static MemoryRegion *nvdimm_md_get_memory_region(MemoryDeviceState *md,
> + Error **errp)
> {
> - NVDIMMDevice *nvdimm = NVDIMM(dimm);
> + NVDIMMDevice *nvdimm = NVDIMM(md);
> Error *local_err = NULL;
>
> if (!nvdimm->nvdimm_mr) {
> @@ -190,11 +192,12 @@ static Property nvdimm_properties[] = {
> static void nvdimm_class_init(ObjectClass *oc, void *data)
> {
> PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
> + MemoryDeviceClass *mdc = MEMORY_DEVICE_CLASS(oc);
> NVDIMMClass *nvc = NVDIMM_CLASS(oc);
> DeviceClass *dc = DEVICE_CLASS(oc);
>
> ddc->realize = nvdimm_realize;
> - ddc->get_memory_region = nvdimm_get_memory_region;
> + mdc->get_memory_region = nvdimm_md_get_memory_region;
> dc->props = nvdimm_properties;
>
> nvc->read_label_data = nvdimm_read_label_data;
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index a06da7e08e..49ad8bac2d 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -32,8 +32,7 @@ static int pc_dimm_get_free_slot(const int *hint, int
> max_slots, Error **errp);
> void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine,
> const uint64_t *legacy_align, Error **errp)
> {
> - PCDIMMDevice *dimm = PC_DIMM(dev);
> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> + MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(dev);
> Error *local_err = NULL;
> MemoryRegion *mr;
> uint64_t addr, align;
> @@ -49,7 +48,7 @@ void pc_dimm_pre_plug(DeviceState *dev, MachineState
> *machine,
> 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);
> + mr = mdc->get_memory_region(MEMORY_DEVICE(dev), &local_err);
> if (local_err) {
> goto out;
> }
> @@ -73,9 +72,10 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine,
> Error **errp)
> {
> PCDIMMDevice *dimm = PC_DIMM(dev);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> + MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(dev);
> MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
> &error_abort);
> - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> + MemoryRegion *mr = mdc->get_memory_region(MEMORY_DEVICE(dev),
> &error_abort);
> uint64_t addr;
>
> addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP,
> @@ -89,9 +89,10 @@ void pc_dimm_unplug(DeviceState *dev, MachineState
> *machine)
> {
> PCDIMMDevice *dimm = PC_DIMM(dev);
> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
> + MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(dev);
> MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
> &error_abort);
> - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
> + MemoryRegion *mr = mdc->get_memory_region(MEMORY_DEVICE(dev),
> &error_abort);
>
> memory_device_unplug_region(machine, mr);
> vmstate_unregister_ram(vmstate_mr, dev);
> @@ -237,12 +238,11 @@ static uint64_t pc_dimm_md_get_addr(const
> MemoryDeviceState *md)
> static uint64_t pc_dimm_md_get_region_size(const MemoryDeviceState *md,
> Error **errp)
> {
> - /* dropping const here is fine as we don't touch the memory region */
> - PCDIMMDevice *dimm = PC_DIMM(md);
> - const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md);
> + MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md);
> MemoryRegion *mr;
>
> - mr = ddc->get_memory_region(dimm, errp);
> + /* dropping const here is fine as we don't touch the memory region */
> + mr = mdc->get_memory_region((MemoryDeviceState *)md, errp);
> if (!mr) {
> return 0;
> }
> @@ -250,6 +250,12 @@ static uint64_t pc_dimm_md_get_region_size(const
> MemoryDeviceState *md,
> return memory_region_size(mr);
> }
>
> +static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState *md,
> + Error **errp)
> +{
> + return pc_dimm_get_memory_region(PC_DIMM(md), errp);
> +}
> +
> static void pc_dimm_md_fill_device_info(const MemoryDeviceState *md,
> MemoryDeviceInfo *info)
> {
> @@ -291,13 +297,13 @@ static void pc_dimm_class_init(ObjectClass *oc, void
> *data)
> dc->props = pc_dimm_properties;
> dc->desc = "DIMM memory module";
>
> - ddc->get_memory_region = pc_dimm_get_memory_region;
> ddc->get_vmstate_memory_region = pc_dimm_get_memory_region;
>
> mdc->get_addr = pc_dimm_md_get_addr;
> /* for a dimm plugged_size == region_size */
> mdc->get_plugged_size = pc_dimm_md_get_region_size;
> mdc->get_region_size = pc_dimm_md_get_region_size;
> + mdc->get_memory_region = pc_dimm_md_get_memory_region;
> mdc->fill_device_info = pc_dimm_md_fill_device_info;
> }
>
> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
> index 12f8ca9eb1..0feed4ec0d 100644
> --- a/include/hw/mem/memory-device.h
> +++ b/include/hw/mem/memory-device.h
> @@ -38,6 +38,10 @@ typedef struct MemoryDeviceState {
> * usable ("plugged") by the guest.
> * @get_region_size: The size of the memory region of the @md that's mapped
> * in guest physical memory at @get_addr.
> + * @get_memory_region: The memory region of the @md of the @md that's
> + * mapped in guest physical memory at @get_addr. If a @md is ever composed
> + * of multiple successive memory regions, a covering memory region is to
> + * be used. Scattered memory regions are not supported for single devices.
> * @fill_device_info: Translate current @md state into #MemoryDeviceInfo.
> */
> typedef struct MemoryDeviceClass {
> @@ -48,6 +52,7 @@ typedef struct MemoryDeviceClass {
> uint64_t (*get_addr)(const MemoryDeviceState *md);
> uint64_t (*get_plugged_size)(const MemoryDeviceState *md, Error **errp);
> uint64_t (*get_region_size)(const MemoryDeviceState *md, Error **errp);
> + MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);
> void (*fill_device_info)(const MemoryDeviceState *md,
> MemoryDeviceInfo *info);
> } MemoryDeviceClass;
> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
> index b382eb4303..b445687081 100644
> --- a/include/hw/mem/pc-dimm.h
> +++ b/include/hw/mem/pc-dimm.h
> @@ -61,9 +61,6 @@ typedef struct PCDIMMDevice {
> * PCDIMMDeviceClass:
> * @realize: called after common dimm is realized so that the dimm based
> * devices get the chance to do specified operations.
> - * @get_memory_region: returns #MemoryRegion associated with @dimm which
> - * is directly mapped into the physical address space of guest. Will not
> - * fail after the device was realized.
> * @get_vmstate_memory_region: returns #MemoryRegion which indicates the
> * memory of @dimm should be kept during live migration. Will not fail
> * after the device was realized.
> @@ -74,7 +71,6 @@ typedef struct PCDIMMDeviceClass {
>
> /* public */
> void (*realize)(PCDIMMDevice *dimm, Error **errp);
> - MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp);
> MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm,
> Error **errp);
> } PCDIMMDeviceClass;
--
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
