On Fri, 2020-09-25 at 13:26 -0400, Paolo Bonzini wrote:
> From: Maxim Levitsky <mlevi...@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 <mlevi...@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
> Message-Id: <20200913160259.32145-6-mlevi...@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> hw/core/qdev.c | 19 ++++++++++++++++++-
> include/hw/qdev-core.h | 3 +++
> 2 files changed, 21 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();
I''l probably never fully understand where to use read/write/full barrier.
If I understand corrctly, read barrier prevents reads done by this thread to be
reordered,
by the CPU and write barrier prevents writes done by this CPU to be re-ordered.
Both (depending on the macro) usually imply compiler barrier (to avoid
compilier re-ordering
stuff...)
> +
> 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 8067497074..39490e76ee 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -163,6 +163,9 @@ struct NamedClockList {
> /**
> * DeviceState:
> * @realized: Indicates whether the device has been fully constructed.
> + * When accessed without the iothread mutex, consider using
> + * qatomic_load_acquire() before accessing any other field in
> + * the device.
This sounds way better that what I wrote. As you probably noticed recently
(but not this patchset yet) I started to review the spelling/comments
more throughfully, so hopefully we see less of stuff that I myself read
and wonder why I did that many spelling/grammar mistakes.... :-)
> * @reset: ResettableState for the device; handled by Resettable interface.
> *
> * This structure should not be accessed directly. We declare it here
Best regards,
Maxim Levitsky
