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 <[email protected]> > --- > 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.
signature.asc
Description: PGP signature
