|
From: | Daniel Henrique Barboza |
Subject: | Re: [PATCH v4 3/3] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write() |
Date: | Sun, 11 Jul 2021 05:49:44 -0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 |
On 7/10/21 3:57 AM, Markus Armbruster wrote:
Igor Mammedov <imammedo@redhat.com> writes:On Fri, 09 Jul 2021 13:25:43 +0200 Markus Armbruster <armbru@redhat.com> wrote:Igor Mammedov <imammedo@redhat.com> writes:On Thu, 08 Jul 2021 15:08:57 +0200 Markus Armbruster <armbru@redhat.com> wrote:Daniel Henrique Barboza <danielhb413@gmail.com> writes:MEM_UNPLUG_ERROR is deprecated since the introduction of DEVICE_UNPLUG_ERROR. Keep emitting both while the deprecation of MEM_UNPLUG_ERROR is pending. CC: Michael S. Tsirkin <mst@redhat.com> CC: Igor Mammedov <imammedo@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/acpi/memory_hotplug.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c index af37889423..fb9f4d2de7 100644 --- a/hw/acpi/memory_hotplug.c +++ b/hw/acpi/memory_hotplug.c @@ -8,6 +8,7 @@ #include "qapi/error.h" #include "qapi/qapi-events-acpi.h" #include "qapi/qapi-events-machine.h" +#include "qapi/qapi-events-qdev.h"#define MEMORY_SLOTS_NUMBER "MDNR"#define MEMORY_HOTPLUG_IO_REGION "HPMR" @@ -177,9 +178,17 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, /* call pc-dimm unplug cb */ hotplug_handler_unplug(hotplug_ctrl, dev, &local_err); if (local_err) { + const char *error_pretty = error_get_pretty(local_err); + trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector); - qapi_event_send_mem_unplug_error(dev->id, - error_get_pretty(local_err)); + + /* + * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR + * while the deprecation of MEM_UNPLUG_ERROR is + * pending. + */ + qapi_event_send_mem_unplug_error(dev->id, error_pretty); + qapi_event_send_device_unplug_error(dev->id, error_pretty); error_free(local_err); break; }Same question as for PATCH 2: can dev->id be null?only theoretically (if memory device were created directly without using device_add), which as far as I know is not the case as all memory devices are created using -device/device_add so far. ( for device_add case see qdev_device_add->qdev_set_id where 'id' is set to user provided or to generated "device[%d]" value)Something is set to a generated value, but it's not dev->id :) void qdev_set_id(DeviceState *dev, const char *id) @id is the value of id=... It may be null. dev->id still is null here. { if (id) { dev->id = id; } dev->id is now the value of id=... It may be null. if (dev->id) { object_property_add_child(qdev_get_peripheral(), dev->id, OBJECT(dev)); If the user specified id=..., add @dev as child of /peripheral. The child's name is the (non-null) value of id=... } else { static int anon_count; gchar *name = g_strdup_printf("device[%d]", anon_count++); object_property_add_child(qdev_get_peripheral_anon(), name, OBJECT(dev)); g_free(name); Else, add @dev as child of /peripheral-anon. The child's name is made up. } } dev->id is still the value of id=..., i.e. it may be null.yep, I was wrong and confused it child name in QOM tree.Sure dereferencing dev->id in acpi_memory_hotplug_write() is safe?it aren't safe since guest may trigger this error when memory-device is created without id.Thanks! Daniel, the issue predates your series, but your series adds instances. We need a patch fixing the existing instances before your series, and fix up your series. Can you take care of that?
Sure. I'll add a patch to handle the dev->id == NULL case before calling qapi_event_send_mem_unplug_error() in acpi_memory_hotplug_write(). Daniel
[Prev in Thread] | Current Thread | [Next in Thread] |