Linux Virtualization - Jan 2015 - [PATCH v3 04/16] virtio/console: verify device has config space

On (Wed) 14 Jan 2015 [19:27:35], Michael S. Tsirkin
wrote:
> Some devices might not implement config space access
> (e.g. remoteproc used not to - before 3.9).
> virtio/console needs config space access so make it
> fail gracefully if not there.
Do we know any such devices?  Wondering what prompted this patch.  If
it's just theoretical, I'd rather let it be like this, and pull this
in when there's a device that doesn't have config space.

Also, just the console functionality (i.e. F_MULTIPORT is unset) is
available w/o config space access.  In fact, getting this patch in
would mean remoteproc wouldn't even run in its pre-config days...
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index de03df9..26afb56 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1986,6 +1986,12 @@ static int virtcons_probe(struct virtio_device
*vdev)
>  	bool multiport;
>  	bool early = early_put_chars != NULL;
>  
> +	if (!vdev->config->get) {
> +		dev_err(&vdev->dev, "%s failure: config access
disabled\n",
> +			__func__);
> +		return -EINVAL;
> +	}
> +

		Amit

On Tue, Jan 20, 2015 at 04:10:40PM +0530, Amit Shah
wrote:
> On (Wed) 14 Jan 2015 [19:27:35], Michael S. Tsirkin wrote:
> > Some devices might not implement config space access
> > (e.g. remoteproc used not to - before 3.9).
> > virtio/console needs config space access so make it
> > fail gracefully if not there.
> 
> Do we know any such devices?  Wondering what prompted this patch.  If
> it's just theoretical, I'd rather let it be like this, and pull
this
> in when there's a device that doesn't have config space.
Yes, with virtio 1.0 config space can be in a separate BAR now.  If
that's not enabled by BIOS (e.g. out of space), we won't have config
space.
> Also, just the console functionality (i.e. F_MULTIPORT is unset) is
> available w/o config space access.
Supporting this by gracefully disabling F_MULTIPORT
would require getting this info from driver before
features are finalized.
Alternatively, check F_MULTIPORT and only fail if set?
Let me know, I'll cook up a patch.
> In fact, getting this patch in
> would mean remoteproc wouldn't even run in its pre-config days...
It seems to have get callback unconditionally now - or did I miss
something?

> > diff --git a/drivers/char/virtio_console.c
b/drivers/char/virtio_console.c
> > index de03df9..26afb56 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -1986,6 +1986,12 @@ static int virtcons_probe(struct virtio_device
*vdev)
> >  	bool multiport;
> >  	bool early = early_put_chars != NULL;
> >  
> > +	if (!vdev->config->get) {
> > +		dev_err(&vdev->dev, "%s failure: config access
disabled\n",
> > +			__func__);
> > +		return -EINVAL;
> > +	}
> > +
> 
> 
> 		Amit

On (Tue) 20 Jan 2015 [13:09:55], Michael S. Tsirkin
wrote:
> On Tue, Jan 20, 2015 at 04:10:40PM +0530, Amit Shah wrote:
> > On (Wed) 14 Jan 2015 [19:27:35], Michael S. Tsirkin wrote:
> > > Some devices might not implement config space access
> > > (e.g. remoteproc used not to - before 3.9).
> > > virtio/console needs config space access so make it
> > > fail gracefully if not there.
> > 
> > Do we know any such devices?  Wondering what prompted this patch.  If
> > it's just theoretical, I'd rather let it be like this, and
pull this
> > in when there's a device that doesn't have config space.
> 
> Yes, with virtio 1.0 config space can be in a separate BAR now.  If
> that's not enabled by BIOS (e.g. out of space), we won't have
config
> space.
I'm still not sure whether we should pull in this patch before
actually seeing a failure.

You do have a dev_err which tells why the probe failed, so it's an
acceptable compromise I suppose.
> > Also, just the console functionality (i.e. F_MULTIPORT is unset) is
> > available w/o config space access.
> 
> Supporting this by gracefully disabling F_MULTIPORT
> would require getting this info from driver before
> features are finalized.
> Alternatively, check F_MULTIPORT and only fail if set?
> Let me know, I'll cook up a patch.
Yes, failing only if F_MULTIPORT is set is a better option (if we have
to fail).
> > In fact, getting this patch in
> > would mean remoteproc wouldn't even run in its pre-config days...
> 
> It seems to have get callback unconditionally now - or did I miss
> something?
What I meant was remoteproc doesn't depend on the config space, only
uses the console functionality.  If remoteproc devices didn't expose a
config space, this patch would cause it to lose its console
functionality for no apparent reason.


		Amit