[PATCH 0/2] virtio: virtio_config_ops documentation

[PATCH 0/2] virtio: virtio_config_ops documentation

[Cornelia Huck]

After the latest virtio-balloon changes, it became clear that
it is not obvious that some of the virtio operations (e.g.
reading or writing the config space) cannot be done from an
atomic context for all transports.

At least try to document that, and also fix some inconsistencies
I noticed along the way.

Cornelia Huck (2):
  virtio: fix virtio_config_ops description
  virtio: document virtio_config_ops restrictions

 include/linux/virtio_config.h | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

-- 
2.17.2

[PATCH 1/2] virtio: fix virtio_config_ops description

[Cornelia Huck]

- get_features has returned 64 bits since commit d025477368792
  ("virtio: add support for 64 bit features.")
- properly mark all optional callbacks

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 include/linux/virtio_config.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 32baf8e26735..7087ef946ba7 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -22,7 +22,7 @@ struct irq_affinity;
  *	offset: the offset of the configuration field
  *	buf: the buffer to read the field value from.
  *	len: the length of the buffer
- * @generation: config generation counter
+ * @generation: config generation counter (optional)
  *	vdev: the virtio_device
  *	Returns the config generation counter
  * @get_status: read the status byte
@@ -48,17 +48,17 @@ struct irq_affinity;
  * @del_vqs: free virtqueues found by find_vqs().
  * @get_features: get the array of feature bits for this device.
  *	vdev: the virtio_device
- *	Returns the first 32 feature bits (all we currently need).
+ *	Returns the first 64 feature bits (all we currently need).
  * @finalize_features: confirm what device features we'll be using.
  *	vdev: the virtio_device
  *	This gives the final feature bits for the device: it can change
  *	the dev->feature bits if it wants.
  *	Returns 0 on success or error status
- * @bus_name: return the bus name associated with the device
+ * @bus_name: return the bus name associated with the device (optional)
  *	vdev: the virtio_device
  *      This returns a pointer to the bus name a la pci_name from which
  *      the caller can then copy.
- * @set_vq_affinity: set the affinity for a virtqueue.
+ * @set_vq_affinity: set the affinity for a virtqueue (optional).
  * @get_vq_affinity: get the affinity for a virtqueue (optional).
  */
 typedef void vq_callback_t(struct virtqueue *);
-- 
2.17.2

[PATCH 2/2] virtio: document virtio_config_ops restrictions

[Cornelia Huck]

Some transports (e.g. virtio-ccw) implement virtio operations that
seem to be a simple read/write as something more involved that
cannot be done from an atomic context.

Give at least a hint about that.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 include/linux/virtio_config.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 7087ef946ba7..987b6491b946 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -12,6 +12,11 @@ struct irq_affinity;
 
 /**
  * virtio_config_ops - operations for configuring a virtio device
+ * Note: Do not assume that a transport implements all of the operations
+ *       getting/setting a value as a simple read/write! Generally speaking,
+ *       any of @get/@set, @get_status/@set_status, or @get_features/
+ *       @finalize_features are NOT safe to be called from an atomic
+ *       context.
  * @get: read the value of a configuration field
  *	vdev: the virtio_device
  *	offset: the offset of the configuration field
-- 
2.17.2

Re: [PATCH 2/2] virtio: document virtio_config_ops restrictions

[Michael S. Tsirkin]

On Thu, Jan 03, 2019 at 05:08:04PM +0100, Cornelia Huck wrote:
> Some transports (e.g. virtio-ccw) implement virtio operations that
> seem to be a simple read/write as something more involved that
> cannot be done from an atomic context.
> 
> Give at least a hint about that.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  include/linux/virtio_config.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 7087ef946ba7..987b6491b946 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -12,6 +12,11 @@ struct irq_affinity;
>  
>  /**
>   * virtio_config_ops - operations for configuring a virtio device
> + * Note: Do not assume that a transport implements all of the operations
> + *       getting/setting a value as a simple read/write! Generally speaking,
> + *       any of @get/@set, @get_status/@set_status, or @get_features/
> + *       @finalize_features are NOT safe to be called from an atomic
> + *       context.
>   * @get: read the value of a configuration field
>   *	vdev: the virtio_device
>   *	offset: the offset of the configuration field

Then might_sleep in virtio_cread/virtio_cwrite and
friends would be appropriate? I guess we'll need to fix
balloon first.

> -- 
> 2.17.2

Re: [PATCH 2/2] virtio: document virtio_config_ops restrictions

[Halil Pasic]

On Thu,  3 Jan 2019 17:08:04 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> Some transports (e.g. virtio-ccw) implement virtio operations that
> seem to be a simple read/write as something more involved that
> cannot be done from an atomic context.
> 
> Give at least a hint about that.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  include/linux/virtio_config.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 7087ef946ba7..987b6491b946 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -12,6 +12,11 @@ struct irq_affinity;
>  
>  /**
>   * virtio_config_ops - operations for configuring a virtio device
> + * Note: Do not assume that a transport implements all of the operations
> + *       getting/setting a value as a simple read/write! Generally speaking,
> + *       any of @get/@set, @get_status/@set_status, or @get_features/
> + *       @finalize_features are NOT safe to be called from an atomic
> + *       context.

I think the only exception is @bus_name (and maybe @generation, I don't
know) because it does not have to 'speak' with the hypervisor. If a
transport operation has to 'speak' with the hypervisor, we do it by
making it interpret a channel program. That means not safe to be called
form atomic context. Or am I missing something?

Regards,
Halil


>   * @get: read the value of a configuration field
>   *	vdev: the virtio_device
>   *	offset: the offset of the configuration field

Re: [PATCH 2/2] virtio: document virtio_config_ops restrictions

[Cornelia Huck]

On Thu, 3 Jan 2019 11:28:28 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jan 03, 2019 at 05:08:04PM +0100, Cornelia Huck wrote:
> > Some transports (e.g. virtio-ccw) implement virtio operations that
> > seem to be a simple read/write as something more involved that
> > cannot be done from an atomic context.
> > 
> > Give at least a hint about that.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  include/linux/virtio_config.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> > index 7087ef946ba7..987b6491b946 100644
> > --- a/include/linux/virtio_config.h
> > +++ b/include/linux/virtio_config.h
> > @@ -12,6 +12,11 @@ struct irq_affinity;
> >  
> >  /**
> >   * virtio_config_ops - operations for configuring a virtio device
> > + * Note: Do not assume that a transport implements all of the operations
> > + *       getting/setting a value as a simple read/write! Generally speaking,
> > + *       any of @get/@set, @get_status/@set_status, or @get_features/
> > + *       @finalize_features are NOT safe to be called from an atomic
> > + *       context.
> >   * @get: read the value of a configuration field
> >   *	vdev: the virtio_device
> >   *	offset: the offset of the configuration field  
> 
> Then might_sleep in virtio_cread/virtio_cwrite and
> friends would be appropriate? I guess we'll need to fix
> balloon first.

Yes, it makes sense to go over the code and add might_sleep to
functions where it makes sense after the balloon changes have been
merged.

Re: [PATCH 2/2] virtio: document virtio_config_ops restrictions

[Cornelia Huck]

On Thu, 3 Jan 2019 18:28:49 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Thu,  3 Jan 2019 17:08:04 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > Some transports (e.g. virtio-ccw) implement virtio operations that
> > seem to be a simple read/write as something more involved that
> > cannot be done from an atomic context.
> > 
> > Give at least a hint about that.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  include/linux/virtio_config.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> > index 7087ef946ba7..987b6491b946 100644
> > --- a/include/linux/virtio_config.h
> > +++ b/include/linux/virtio_config.h
> > @@ -12,6 +12,11 @@ struct irq_affinity;
> >  
> >  /**
> >   * virtio_config_ops - operations for configuring a virtio device
> > + * Note: Do not assume that a transport implements all of the operations
> > + *       getting/setting a value as a simple read/write! Generally speaking,
> > + *       any of @get/@set, @get_status/@set_status, or @get_features/
> > + *       @finalize_features are NOT safe to be called from an atomic
> > + *       context.  
> 
> I think the only exception is @bus_name (and maybe @generation, I don't
> know) because it does not have to 'speak' with the hypervisor. If a
> transport operation has to 'speak' with the hypervisor, we do it by
> making it interpret a channel program. That means not safe to be called
> form atomic context. Or am I missing something?

I explicitly singled out the listed callbacks because they read/write a
value and there might be more to them than meets the eye. I would
assume that nobody expects calling e.g. find_vqs (allocating queues)
from an atomic context to be a good idea :) Maybe I should do
s/Generally speaking/In particular/ ?

That said, it's only a hint; we should add might_sleep as well to
interfaces where it makes sense.

Re: [PATCH 1/2] virtio: fix virtio_config_ops description

[Cornelia Huck]

On Thu,  3 Jan 2019 17:08:03 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> - get_features has returned 64 bits since commit d025477368792
>   ("virtio: add support for 64 bit features.")
> - properly mark all optional callbacks
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  include/linux/virtio_config.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 32baf8e26735..7087ef946ba7 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -22,7 +22,7 @@ struct irq_affinity;
>   *	offset: the offset of the configuration field
>   *	buf: the buffer to read the field value from.
>   *	len: the length of the buffer
> - * @generation: config generation counter
> + * @generation: config generation counter (optional)
>   *	vdev: the virtio_device
>   *	Returns the config generation counter
>   * @get_status: read the status byte
> @@ -48,17 +48,17 @@ struct irq_affinity;
>   * @del_vqs: free virtqueues found by find_vqs().
>   * @get_features: get the array of feature bits for this device.
>   *	vdev: the virtio_device
> - *	Returns the first 32 feature bits (all we currently need).
> + *	Returns the first 64 feature bits (all we currently need).
>   * @finalize_features: confirm what device features we'll be using.
>   *	vdev: the virtio_device
>   *	This gives the final feature bits for the device: it can change
>   *	the dev->feature bits if it wants.
>   *	Returns 0 on success or error status
> - * @bus_name: return the bus name associated with the device
> + * @bus_name: return the bus name associated with the device (optional)
>   *	vdev: the virtio_device
>   *      This returns a pointer to the bus name a la pci_name from which
>   *      the caller can then copy.
> - * @set_vq_affinity: set the affinity for a virtqueue.
> + * @set_vq_affinity: set the affinity for a virtqueue (optional).
>   * @get_vq_affinity: get the affinity for a virtqueue (optional).
>   */
>  typedef void vq_callback_t(struct virtqueue *);

Ping. Any feedback on that patch?

Re: [virtio-dev] Re: [PATCH 1/2] virtio: fix virtio_config_ops description

[Halil Pasic]

On Mon, 14 Jan 2019 15:09:58 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu,  3 Jan 2019 17:08:03 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > - get_features has returned 64 bits since commit d025477368792
> >   ("virtio: add support for 64 bit features.")
> > - properly mark all optional callbacks
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>

Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

> > ---
> >  include/linux/virtio_config.h | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> > index 32baf8e26735..7087ef946ba7 100644
> > --- a/include/linux/virtio_config.h
> > +++ b/include/linux/virtio_config.h
> > @@ -22,7 +22,7 @@ struct irq_affinity;
> >   *	offset: the offset of the configuration field
> >   *	buf: the buffer to read the field value from.
> >   *	len: the length of the buffer
> > - * @generation: config generation counter
> > + * @generation: config generation counter (optional)
> >   *	vdev: the virtio_device
> >   *	Returns the config generation counter
> >   * @get_status: read the status byte
> > @@ -48,17 +48,17 @@ struct irq_affinity;
> >   * @del_vqs: free virtqueues found by find_vqs().
> >   * @get_features: get the array of feature bits for this device.
> >   *	vdev: the virtio_device
> > - *	Returns the first 32 feature bits (all we currently need).
> > + *	Returns the first 64 feature bits (all we currently need).
> >   * @finalize_features: confirm what device features we'll be using.
> >   *	vdev: the virtio_device
> >   *	This gives the final feature bits for the device: it can change
> >   *	the dev->feature bits if it wants.
> >   *	Returns 0 on success or error status
> > - * @bus_name: return the bus name associated with the device
> > + * @bus_name: return the bus name associated with the device (optional)
> >   *	vdev: the virtio_device
> >   *      This returns a pointer to the bus name a la pci_name from which
> >   *      the caller can then copy.
> > - * @set_vq_affinity: set the affinity for a virtqueue.
> > + * @set_vq_affinity: set the affinity for a virtqueue (optional).
> >   * @get_vq_affinity: get the affinity for a virtqueue (optional).
> >   */
> >  typedef void vq_callback_t(struct virtqueue *);
> 
> Ping. Any feedback on that patch?
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 

Re: [PATCH 1/2] virtio: fix virtio_config_ops description

[Wei Wang]

On 01/14/2019 10:09 PM, Cornelia Huck wrote:
> On Thu,  3 Jan 2019 17:08:03 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> - get_features has returned 64 bits since commit d025477368792
>>    ("virtio: add support for 64 bit features.")
>> - properly mark all optional callbacks
>>
>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>   include/linux/virtio_config.h | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
>> index 32baf8e26735..7087ef946ba7 100644
>> --- a/include/linux/virtio_config.h
>> +++ b/include/linux/virtio_config.h
>> @@ -22,7 +22,7 @@ struct irq_affinity;
>>    *	offset: the offset of the configuration field
>>    *	buf: the buffer to read the field value from.
>>    *	len: the length of the buffer
>> - * @generation: config generation counter
>> + * @generation: config generation counter (optional)
>>    *	vdev: the virtio_device
>>    *	Returns the config generation counter
>>    * @get_status: read the status byte
>> @@ -48,17 +48,17 @@ struct irq_affinity;
>>    * @del_vqs: free virtqueues found by find_vqs().
>>    * @get_features: get the array of feature bits for this device.
>>    *	vdev: the virtio_device
>> - *	Returns the first 32 feature bits (all we currently need).
>> + *	Returns the first 64 feature bits (all we currently need).
>>    * @finalize_features: confirm what device features we'll be using.
>>    *	vdev: the virtio_device
>>    *	This gives the final feature bits for the device: it can change
>>    *	the dev->feature bits if it wants.
>>    *	Returns 0 on success or error status
>> - * @bus_name: return the bus name associated with the device
>> + * @bus_name: return the bus name associated with the device (optional)
>>    *	vdev: the virtio_device
>>    *      This returns a pointer to the bus name a la pci_name from which
>>    *      the caller can then copy.
>> - * @set_vq_affinity: set the affinity for a virtqueue.
>> + * @set_vq_affinity: set the affinity for a virtqueue (optional).
>>    * @get_vq_affinity: get the affinity for a virtqueue (optional).
>>    */
>>   typedef void vq_callback_t(struct virtqueue *);
> Ping. Any feedback on that patch?

Reviewed-by: Wei Wang <wei.w.wang@intel.com>

Best,
Wei

Re: [virtio-dev] [PATCH 2/2] virtio: document virtio_config_ops restrictions

[Wei Wang]

On 01/04/2019 12:08 AM, Cornelia Huck wrote:
> Some transports (e.g. virtio-ccw) implement virtio operations that
> seem to be a simple read/write as something more involved that
> cannot be done from an atomic context.
>
> Give at least a hint about that.
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   include/linux/virtio_config.h | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 7087ef946ba7..987b6491b946 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -12,6 +12,11 @@ struct irq_affinity;
>   
>   /**
>    * virtio_config_ops - operations for configuring a virtio device
> + * Note: Do not assume that a transport implements all of the operations
> + *       getting/setting a value as a simple read/write! Generally speaking,
> + *       any of @get/@set, @get_status/@set_status, or @get_features/
> + *       @finalize_features are NOT safe to be called from an atomic
> + *       context.
>    * @get: read the value of a configuration field
>    *	vdev: the virtio_device
>    *	offset: the offset of the configuration field


Reviewed-by: Wei Wang <wei.w.wang@intel.com>

Best,
Wei

Re: [PATCH 1/2] virtio: fix virtio_config_ops description

[Michael S. Tsirkin]

On Tue, Jan 15, 2019 at 10:15:39AM +0800, Wei Wang wrote:
> On 01/14/2019 10:09 PM, Cornelia Huck wrote:
> > On Thu,  3 Jan 2019 17:08:03 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > 
> > > - get_features has returned 64 bits since commit d025477368792
> > >    ("virtio: add support for 64 bit features.")
> > > - properly mark all optional callbacks
> > > 
> > > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > > ---
> > >   include/linux/virtio_config.h | 8 ++++----
> > >   1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> > > index 32baf8e26735..7087ef946ba7 100644
> > > --- a/include/linux/virtio_config.h
> > > +++ b/include/linux/virtio_config.h
> > > @@ -22,7 +22,7 @@ struct irq_affinity;
> > >    *	offset: the offset of the configuration field
> > >    *	buf: the buffer to read the field value from.
> > >    *	len: the length of the buffer
> > > - * @generation: config generation counter
> > > + * @generation: config generation counter (optional)
> > >    *	vdev: the virtio_device
> > >    *	Returns the config generation counter
> > >    * @get_status: read the status byte
> > > @@ -48,17 +48,17 @@ struct irq_affinity;
> > >    * @del_vqs: free virtqueues found by find_vqs().
> > >    * @get_features: get the array of feature bits for this device.
> > >    *	vdev: the virtio_device
> > > - *	Returns the first 32 feature bits (all we currently need).
> > > + *	Returns the first 64 feature bits (all we currently need).
> > >    * @finalize_features: confirm what device features we'll be using.
> > >    *	vdev: the virtio_device
> > >    *	This gives the final feature bits for the device: it can change
> > >    *	the dev->feature bits if it wants.
> > >    *	Returns 0 on success or error status
> > > - * @bus_name: return the bus name associated with the device
> > > + * @bus_name: return the bus name associated with the device (optional)
> > >    *	vdev: the virtio_device
> > >    *      This returns a pointer to the bus name a la pci_name from which
> > >    *      the caller can then copy.
> > > - * @set_vq_affinity: set the affinity for a virtqueue.
> > > + * @set_vq_affinity: set the affinity for a virtqueue (optional).
> > >    * @get_vq_affinity: get the affinity for a virtqueue (optional).
> > >    */
> > >   typedef void vq_callback_t(struct virtqueue *);
> > Ping. Any feedback on that patch?
> 
> Reviewed-by: Wei Wang <wei.w.wang@intel.com>
> 
> Best,
> Wei

Pushed already, can't add tags.
Sorry about that.

-- 
MST