Re: [Qemu-devel] [PATCH v4 12/24] memory-device: add device class function set_addr()

[Auger Eric]

Hi David,

On 9/26/18 11:42 AM, David Hildenbrand wrote:
> To be able to factor out address asignment of memory devices, we will
s/asignment/assignment
> 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.
> 
> Reviewed-by: David Gibson <address@hidden>
> Reviewed-by: Igor Mammedov <address@hidden>
> Signed-off-by: David Hildenbrand <address@hidden>
Reviewed-by: Eric Auger <address@hidden>

Thanks

Eric
> ---
>  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 4bd8c496bb..5873172175 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -236,6 +236,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)
>  {
> @@ -286,6 +292,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 642797655f..6395942b27 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. This is helpful for devices that
>   * dynamically manage the amount of memory accessible by the guest via
> @@ -51,6 +52,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,
>