Re: [PATCH 06/10] device-core: use atomic_set on .realized property

[Maxim Levitsky]

On Fri, 2020-09-25 at 13:26 -0400, Paolo Bonzini wrote:
> From: Maxim Levitsky <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 |  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