qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 3/3] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_me


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





reply via email to

[Prev in Thread] Current Thread [Next in Thread]