diff mbox series

[v3,10/22] memory-device: add device class function set_addr()

Message ID 20180920103243.28474-11-david@redhat.com
State New
Headers show
Series memory-device: complete refactoring + virtio-pmem | expand

Commit Message

David Hildenbrand Sept. 20, 2018, 10:32 a.m. UTC
To be able to factor out address asignment of memory devices, we will
have to read (get_addr()) and write (set_addr()) the address.

We can't use properties for this purpose, as properties are device
specific. E.g. while the address property for a DIMM is called "addr", it
might be called differently (e.g. "memaddr") for other devices.

Especially virtio based memory devices cannot use "addr" as that is already
reserved and used for the address on the bus (for the proxy device).

Also, it might be possible to have memory devices without address
properties (e.g. internal DIMM-like thingies).

In contrast to get_addr(), we expect that set_addr() can fail.

Keep it simple for now for pc-dimm and simply set the static property, that
will fail once realized from the callback.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/mem/pc-dimm.c               | 7 +++++++
 include/hw/mem/memory-device.h | 2 ++
 2 files changed, 9 insertions(+)

Comments

David Gibson Sept. 21, 2018, 6:51 a.m. UTC | #1
On Thu, Sep 20, 2018 at 12:32:31PM +0200, David Hildenbrand wrote:
1;5202;0c> To be able to factor out address asignment of memory devices, we will
> have to read (get_addr()) and write (set_addr()) the address.
> 
> We can't use properties for this purpose, as properties are device
> specific. E.g. while the address property for a DIMM is called "addr", it
> might be called differently (e.g. "memaddr") for other devices.
> 
> Especially virtio based memory devices cannot use "addr" as that is already
> reserved and used for the address on the bus (for the proxy device).
> 
> Also, it might be possible to have memory devices without address
> properties (e.g. internal DIMM-like thingies).
> 
> In contrast to get_addr(), we expect that set_addr() can fail.
> 
> Keep it simple for now for pc-dimm and simply set the static property, that
> will fail once realized from the callback.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/mem/pc-dimm.c               | 7 +++++++
>  include/hw/mem/memory-device.h | 2 ++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 95c3c4bd76..3474b810ef 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -235,6 +235,12 @@ static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
>      return dimm->addr;
>  }
>  
> +static void pc_dimm_md_set_addr(MemoryDeviceState *md, uint64_t addr,
> +                                Error **errp)
> +{
> +    object_property_set_uint(OBJECT(md), addr, PC_DIMM_ADDR_PROP, errp);
> +}
> +
>  static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState *md,
>                                                    Error **errp)
>  {
> @@ -285,6 +291,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
>      ddc->get_vmstate_memory_region = pc_dimm_get_memory_region;
>  
>      mdc->get_addr = pc_dimm_md_get_addr;
> +    mdc->set_addr = pc_dimm_md_set_addr;
>      /* for a dimm plugged_size == region_size */
>      mdc->get_plugged_size = memory_device_get_region_size;
>      mdc->get_memory_region = pc_dimm_md_get_memory_region;
> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
> index 64df232919..b2fd26c262 100644
> --- a/include/hw/mem/memory-device.h
> +++ b/include/hw/mem/memory-device.h
> @@ -34,6 +34,7 @@ typedef struct MemoryDeviceState {
>   * @get_addr: The address of the @md in guest physical memory. "0" means that
>   * no address has been specified by the user and that no address has been
>   * assigned yet.
> + * @set_addr: Set the address of the @md in guest physical memory.
>   * @get_plugged_size: The amount of memory provided by this @md currently
>   * usable ("plugged") by the guest.
>   * @get_memory_region: The memory region of the @md of the @md that's
> @@ -48,6 +49,7 @@ typedef struct MemoryDeviceClass {
>  
>      /* public */
>      uint64_t (*get_addr)(const MemoryDeviceState *md);
> +    void (*set_addr)(MemoryDeviceState *md, uint64_t addr, Error **errp);
>      uint64_t (*get_plugged_size)(const MemoryDeviceState *md, Error **errp);
>      MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);
>      void (*fill_device_info)(const MemoryDeviceState *md,
Igor Mammedov Sept. 24, 2018, 1:36 p.m. UTC | #2
On Thu, 20 Sep 2018 12:32:31 +0200
David Hildenbrand <david@redhat.com> wrote:

> To be able to factor out address asignment of memory devices, we will
> have to read (get_addr()) and write (set_addr()) the address.
> 
> We can't use properties for this purpose, as properties are device
> specific. E.g. while the address property for a DIMM is called "addr", it
> might be called differently (e.g. "memaddr") for other devices.
> 
> Especially virtio based memory devices cannot use "addr" as that is already
> reserved and used for the address on the bus (for the proxy device).
> 
> Also, it might be possible to have memory devices without address
> properties (e.g. internal DIMM-like thingies).
> 
> In contrast to get_addr(), we expect that set_addr() can fail.
> 
> Keep it simple for now for pc-dimm and simply set the static property, that
> will fail once realized from the callback.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/mem/pc-dimm.c               | 7 +++++++
>  include/hw/mem/memory-device.h | 2 ++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 95c3c4bd76..3474b810ef 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -235,6 +235,12 @@ static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
>      return dimm->addr;
>  }
>  
> +static void pc_dimm_md_set_addr(MemoryDeviceState *md, uint64_t addr,
> +                                Error **errp)
> +{
> +    object_property_set_uint(OBJECT(md), addr, PC_DIMM_ADDR_PROP, errp);
> +}
> +
>  static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState *md,
>                                                    Error **errp)
>  {
> @@ -285,6 +291,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
>      ddc->get_vmstate_memory_region = pc_dimm_get_memory_region;
>  
>      mdc->get_addr = pc_dimm_md_get_addr;
> +    mdc->set_addr = pc_dimm_md_set_addr;
>      /* for a dimm plugged_size == region_size */
>      mdc->get_plugged_size = memory_device_get_region_size;
>      mdc->get_memory_region = pc_dimm_md_get_memory_region;
> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
> index 64df232919..b2fd26c262 100644
> --- a/include/hw/mem/memory-device.h
> +++ b/include/hw/mem/memory-device.h
> @@ -34,6 +34,7 @@ typedef struct MemoryDeviceState {
>   * @get_addr: The address of the @md in guest physical memory. "0" means that
>   * no address has been specified by the user and that no address has been
>   * assigned yet.
> + * @set_addr: Set the address of the @md in guest physical memory.
>   * @get_plugged_size: The amount of memory provided by this @md currently
>   * usable ("plugged") by the guest.
>   * @get_memory_region: The memory region of the @md of the @md that's
> @@ -48,6 +49,7 @@ typedef struct MemoryDeviceClass {
>  
>      /* public */
>      uint64_t (*get_addr)(const MemoryDeviceState *md);
> +    void (*set_addr)(MemoryDeviceState *md, uint64_t addr, Error **errp);
>      uint64_t (*get_plugged_size)(const MemoryDeviceState *md, Error **errp);
>      MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);
>      void (*fill_device_info)(const MemoryDeviceState *md,
diff mbox series

Patch

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 95c3c4bd76..3474b810ef 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -235,6 +235,12 @@  static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
     return dimm->addr;
 }
 
+static void pc_dimm_md_set_addr(MemoryDeviceState *md, uint64_t addr,
+                                Error **errp)
+{
+    object_property_set_uint(OBJECT(md), addr, PC_DIMM_ADDR_PROP, errp);
+}
+
 static MemoryRegion *pc_dimm_md_get_memory_region(MemoryDeviceState *md,
                                                   Error **errp)
 {
@@ -285,6 +291,7 @@  static void pc_dimm_class_init(ObjectClass *oc, void *data)
     ddc->get_vmstate_memory_region = pc_dimm_get_memory_region;
 
     mdc->get_addr = pc_dimm_md_get_addr;
+    mdc->set_addr = pc_dimm_md_set_addr;
     /* for a dimm plugged_size == region_size */
     mdc->get_plugged_size = memory_device_get_region_size;
     mdc->get_memory_region = pc_dimm_md_get_memory_region;
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
index 64df232919..b2fd26c262 100644
--- a/include/hw/mem/memory-device.h
+++ b/include/hw/mem/memory-device.h
@@ -34,6 +34,7 @@  typedef struct MemoryDeviceState {
  * @get_addr: The address of the @md in guest physical memory. "0" means that
  * no address has been specified by the user and that no address has been
  * assigned yet.
+ * @set_addr: Set the address of the @md in guest physical memory.
  * @get_plugged_size: The amount of memory provided by this @md currently
  * usable ("plugged") by the guest.
  * @get_memory_region: The memory region of the @md of the @md that's
@@ -48,6 +49,7 @@  typedef struct MemoryDeviceClass {
 
     /* public */
     uint64_t (*get_addr)(const MemoryDeviceState *md);
+    void (*set_addr)(MemoryDeviceState *md, uint64_t addr, Error **errp);
     uint64_t (*get_plugged_size)(const MemoryDeviceState *md, Error **errp);
     MemoryRegion *(*get_memory_region)(MemoryDeviceState *md, Error **errp);
     void (*fill_device_info)(const MemoryDeviceState *md,