Re: [PATCH v2 4/7] device-core: use atomic_set on .realized property

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2093 bytes --]

On Mon, May 11, 2020 at 07:09:48PM +0300, Maxim Levitsky wrote:
> 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>
> ---
>  hw/core/qdev.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)

Please add a comment to struct DeviceState saying the realized field
must be accessed with atomic_load_acquire() when used outside the QEMU
global mutex.

> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 732789e2b7..d530c5922f 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -964,7 +964,20 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>              }
>         }
>  
> +       atomic_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
> +         */
> +
> +        atomic_store_release(&dev->realized, value);

I'm not sure if atomic_store_release() is strong enough in the true ->
false case:

  Operations coming after ``atomic_store_release()`` can still be
  reordered before it.

A reader may already seen changes made to unrealize the DeviceState even
though realized still appears to be true. A full write memory barrier
seems safer here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]