Hi David,
On 9/26/18 11:42 AM, David Hildenbrand wrote:
> Let's trace the address when pre_pluggin/plugging/unplugging a memory device.
>
> Trace it when pre_plugging as well as when plugging, so we really know
> when a specific address is actually used.
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> hw/mem/memory-device.c | 6 ++++++
> hw/mem/pc-dimm.c | 8 --------
> hw/mem/trace-events | 5 ++++-
> 3 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index e8e282bf5e..cf85199a72 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -17,6 +17,7 @@
> #include "qemu/range.h"
> #include "hw/virtio/vhost.h"
> #include "sysemu/kvm.h"
> +#include "trace.h"
>
> static gint memory_device_addr_sort(gconstpointer a, gconstpointer b)
> {
> @@ -271,6 +272,9 @@ void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
> goto out;
> }
> mdc->set_addr(md, addr, &local_err);
> + if (!local_err) {
> + trace_memory_device_pre_assign_address(addr);
> + }
> out:
> error_propagate(errp, local_err);
> }
> @@ -290,6 +294,7 @@ void memory_device_plug(MemoryDeviceState *md, MachineState *ms)
>
> memory_region_add_subregion(&ms->device_memory->mr,
> addr - ms->device_memory->base, mr);
> + trace_memory_device_assign_address(addr);
> }
>
> void memory_device_unplug(MemoryDeviceState *md, MachineState *ms)
> @@ -305,6 +310,7 @@ void memory_device_unplug(MemoryDeviceState *md, MachineState *ms)
> g_assert(ms->device_memory);
>
> memory_region_del_subregion(&ms->device_memory->mr, mr);
> + trace_memory_device_unassign_address(mdc->get_addr(md));
> }
>
> uint64_t memory_device_get_region_size(const MemoryDeviceState *md,
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 5a9a3d831d..9c0c487cb6 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -34,7 +34,6 @@ void pc_dimm_pre_plug(PCDIMMDevice *dimm, MachineState *machine,
> {
> DeviceState *dev = DEVICE(dimm);
> Error *local_err = NULL;
> - uint64_t addr;
> int slot;
>
> slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
> @@ -49,13 +48,6 @@ void pc_dimm_pre_plug(PCDIMMDevice *dimm, MachineState *machine,
>
> memory_device_pre_plug(MEMORY_DEVICE(dev), machine, legacy_align,
> &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);
> out:
> error_propagate(errp, local_err);
> }
> diff --git a/hw/mem/trace-events b/hw/mem/trace-events
> index e150dcc497..b40482077e 100644
> --- a/hw/mem/trace-events
> +++ b/hw/mem/trace-events
> @@ -2,4 +2,7 @@
>
> # hw/mem/pc-dimm.c
> mhp_pc_dimm_assigned_slot(int slot) "%d"
> -mhp_pc_dimm_assigned_address(uint64_t addr) "0x%"PRIx64
> +# hw/mem/memory-device.c
> +memory_device_pre_assign_address(uint64_t addr) "0x%"PRIx64
> +memory_device_assign_address(uint64_t addr) "0x%"PRIx64
> +memory_device_unassign_address(uint64_t addr) "0x%"PRIx64
In case we use several memory devices, wouldn't it make sense to pass
the mr name or id as well to uniquely identify the device?
nit: For debugging purpose it would look simpler to me to use the
original function name for the associated trace (all the more so there
is a single trace within) than the "assign_address" naming.
Thanks
Eric
>