linux-kernel - Re: [PATCH v4 04/25] virtio: defer config changed notifications

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141014085908.GA6016@redhat.com>
Date:	Tue, 14 Oct 2014 11:59:08 +0300
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	linux-kernel@...r.kernel.org,
	virtualization@...ts.linux-foundation.org,
	linux-scsi@...r.kernel.org, linux-s390@...r.kernel.org,
	v9fs-developer@...ts.sourceforge.net, netdev@...r.kernel.org,
	kvm@...r.kernel.org, Amit Shah <amit.shah@...hat.com>,
	Cornelia Huck <cornelia.huck@...ibm.com>,
	Christian Borntraeger <borntraeger@...ibm.com>,
	"David S. Miller" <davem@...emloft.net>,
	Paolo Bonzini <pbonzini@...hat.com>,
	Heinz Graalfs <graalfs@...ux.vnet.ibm.com>
Subject: Re: [PATCH v4 04/25] virtio: defer config changed notifications

On Tue, Oct 14, 2014 at 11:01:12AM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@...hat.com> writes:
> > Defer config changed notifications that arrive during
> > probe/scan/freeze/restore.
> >
> > This will allow drivers to set DRIVER_OK earlier, without worrying about
> > racing with config change interrupts.
> >
> > This change will also benefit old hypervisors (before 2009)
> > that send interrupts without checking DRIVER_OK: previously,
> > the callback could race with driver-specific initialization.
> >
> > This will also help simplify drivers.
> 
> But AFAICT you never *read* dev->config_changed.
> 
> You unconditionally trigger a config_changed event in
> virtio_config_enable().  That's a bit weird, but probably OK.
> 
> How about the following change (on top of your patch).  I
> think the renaming is clearer, and note the added if() test in
> virtio_config_enable().
> 
> If you approve, I'll fold it in.
> 
> Cheers,
> Rusty.

Hi Rusty,
I'm okay with both changes.
You can fold it in if you prefer, or just make it a patch on top.

> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 2536701b098b..df598dd8c5c8 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -122,7 +122,7 @@ static void __virtio_config_changed(struct virtio_device *dev)
>  	struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>  
>  	if (!dev->config_enabled)
> -		dev->config_changed = true;
> +		dev->config_change_pending = true;
>  	else if (drv && drv->config_changed)
>  		drv->config_changed(dev);
>  }
> @@ -148,8 +148,9 @@ static void virtio_config_enable(struct virtio_device *dev)
>  {
>  	spin_lock_irq(&dev->config_lock);
>  	dev->config_enabled = true;
> -	__virtio_config_changed(dev);
> -	dev->config_changed = false;
> +	if (dev->config_change_pending)
> +		__virtio_config_changed(dev);
> +	dev->config_change_pending = false;
>  	spin_unlock_irq(&dev->config_lock);
>  }
>  
> @@ -253,7 +254,7 @@ int register_virtio_device(struct virtio_device *dev)
>  
>  	spin_lock_init(&dev->config_lock);
>  	dev->config_enabled = false;
> -	dev->config_changed = false;
> +	dev->config_change_pending = false;
>  
>  	/* We always start by resetting the device, in case a previous
>  	 * driver messed it up.  This also tests that code path a little. */
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 5636b119dc25..65261a7244fc 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -80,7 +80,7 @@ bool virtqueue_is_broken(struct virtqueue *vq);
>   * @index: unique position on the virtio bus
>   * @failed: saved value for CONFIG_S_FAILED bit (for restore)
>   * @config_enabled: configuration change reporting enabled
> - * @config_changed: configuration change reported while disabled
> + * @config_change_pending: configuration change reported while disabled
>   * @config_lock: protects configuration change reporting
>   * @dev: underlying device.
>   * @id: the device type identification (used to match it with a driver).
> @@ -94,7 +94,7 @@ struct virtio_device {
>  	int index;
>  	bool failed;
>  	bool config_enabled;
> -	bool config_changed;
> +	bool config_change_pending;
>  	spinlock_t config_lock;
>  	struct device dev;
>  	struct virtio_device_id id;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/