qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Maxim Levitsky <mlevitsk@redhat.com>
To: qemu-devel@nongnu.org
Cc: "Fam Zheng" <fam@euphon.net>,
	"Laurent Vivier" <lvivier@redhat.com>,
	"Thomas Huth" <thuth@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	qemu-block@nongnu.org, "Michael S. Tsirkin" <mst@redhat.com>,
	"Jason Wang" <jasowang@redhat.com>,
	"Markus Armbruster" <armbru@redhat.com>,
	"Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"Stefan Hajnoczi" <stefanha@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Maxim Levitsky" <mlevitsk@redhat.com>,
	"John Snow" <jsnow@redhat.com>,
	"Stefan Berger" <stefanb@linux.ibm.com>
Subject: [PATCH v7 09/13] device-core: use atomic_set on .realized property
Date: Tue,  6 Oct 2020 15:39:00 +0300	[thread overview]
Message-ID: <20201006123904.610658-10-mlevitsk@redhat.com> (raw)
In-Reply-To: <20201006123904.610658-1-mlevitsk@redhat.com>

Some code might race with placement of new devices on a bus.
We currently first place a (unrealized) device on the bus
and then realize it.

As a workaround, users that scan the child device list, can
check the realized property to see if it is safe to access such a device.
Use an atomic write here too to aid with this.

A separate discussion is what to do with devices that are unrealized:
It looks like for this case we only call the hotplug handler's unplug
callback and its up to it to unrealize the device.
An atomic operation doesn't cause harm for this code path though.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-6-mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/core/qdev.c         | 19 ++++++++++++++++++-
 include/hw/qdev-core.h |  2 ++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 59e5e710b7..fc4daa36fa 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -946,7 +946,25 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             }
        }
 
+       qatomic_store_release(&dev->realized, value);
+
     } else if (!value && dev->realized) {
+
+        /*
+         * Change the value so that any concurrent users are aware
+         * that the device is going to be unrealized
+         *
+         * TODO: change .realized property to enum that states
+         * each phase of the device realization/unrealization
+         */
+
+        qatomic_set(&dev->realized, value);
+        /*
+         * Ensure that concurrent users see this update prior to
+         * any other changes done by unrealize.
+         */
+        smp_wmb();
+
         QLIST_FOREACH(bus, &dev->child_bus, sibling) {
             qbus_unrealize(bus);
         }
@@ -961,7 +979,6 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
     }
 
     assert(local_err == NULL);
-    dev->realized = value;
     return;
 
 child_realize_fail:
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 2c6307e3ed..868973319e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -163,6 +163,8 @@ struct NamedClockList {
 /**
  * DeviceState:
  * @realized: Indicates whether the device has been fully constructed.
+ *            When accessed outsize big qemu lock, must be accessed with
+ *            atomic_load_acquire()
  * @reset: ResettableState for the device; handled by Resettable interface.
  *
  * This structure should not be accessed directly.  We declare it here
-- 
2.26.2



  parent reply	other threads:[~2020-10-06 12:52 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-06 12:38 [PATCH v7 00/13] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Maxim Levitsky
2020-10-06 12:38 ` [PATCH v7 01/13] qtest: rename qtest_qmp_receive to qtest_qmp_receive_dict Maxim Levitsky
2020-10-12 11:13   ` Thomas Huth
2020-10-06 12:38 ` [PATCH v7 02/13] qtest: Reintroduce qtest_qmp_receive Maxim Levitsky
2020-10-12 11:14   ` Thomas Huth
2020-10-12 13:47     ` Paolo Bonzini
2020-10-12 13:49       ` Thomas Huth
2020-10-12 16:56         ` Paolo Bonzini
2020-10-06 12:38 ` [PATCH v7 03/13] qtest: switch users back to qtest_qmp_receive Maxim Levitsky
2020-10-06 12:56   ` Paolo Bonzini
2020-10-06 13:17     ` Maxim Levitsky
2020-10-06 12:38 ` [PATCH v7 04/13] qdev: add "check if address free" callback for buses Maxim Levitsky
2020-10-06 12:38 ` [PATCH v7 05/13] scsi: switch to bus->check_address Maxim Levitsky
2020-10-06 12:38 ` [PATCH v7 06/13] scsi/scsi_bus: switch search direction in scsi_device_find Maxim Levitsky
2020-10-06 12:38 ` [PATCH v7 07/13] device_core: use drain_call_rcu in in qmp_device_add Maxim Levitsky
2020-10-06 12:38 ` [PATCH v7 08/13] device-core: use RCU for list of children of a bus Maxim Levitsky
2020-10-06 12:39 ` Maxim Levitsky [this message]
2020-10-06 12:39 ` [PATCH v7 10/13] scsi/scsi-bus: scsi_device_find: don't return unrealized devices Maxim Levitsky
2020-10-06 12:39 ` [PATCH v7 11/13] scsi/scsi_bus: Add scsi_device_get Maxim Levitsky
2020-10-06 12:39 ` [PATCH v7 12/13] virtio-scsi: use scsi_device_get Maxim Levitsky
2020-10-06 12:39 ` [PATCH v7 13/13] scsi/scsi_bus: fix races in REPORT LUNS Maxim Levitsky
2020-10-06 13:01 ` [PATCH v7 00/13] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread no-reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201006123904.610658-10-mlevitsk@redhat.com \
    --to=mlevitsk@redhat.com \
    --cc=armbru@redhat.com \
    --cc=berrange@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=f4bug@amsat.org \
    --cc=fam@euphon.net \
    --cc=jasowang@redhat.com \
    --cc=jsnow@redhat.com \
    --cc=lvivier@redhat.com \
    --cc=mst@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=stefanb@linux.ibm.com \
    --cc=stefanha@redhat.com \
    --cc=thuth@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).