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
- 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 at 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
Cornelia Huck
2019-Jan-03 16:08 UTC
[PATCH 2/2] virtio: document virtio_config_ops restrictions
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 at 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
Michael S. Tsirkin
2019-Jan-03 16:28 UTC
[PATCH 2/2] virtio: document virtio_config_ops restrictions
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 at 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 fieldThen might_sleep in virtio_cread/virtio_cwrite and friends would be appropriate? I guess we'll need to fix balloon first.> -- > 2.17.2
Cornelia Huck
2019-Jan-04 12:48 UTC
[PATCH 2/2] virtio: document virtio_config_ops restrictions
On Thu, 3 Jan 2019 18:28:49 +0100 Halil Pasic <pasic at linux.ibm.com> wrote:> On Thu, 3 Jan 2019 17:08:04 +0100 > Cornelia Huck <cohuck at 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 at 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.
Apparently Analagous Threads
- [PATCH 0/2] virtio: virtio_config_ops documentation
- [PATCH 0/2] virtio: virtio_config_ops documentation
- [PATCH 2/2] virtio: document virtio_config_ops restrictions
- [PATCH 2/2] virtio: document virtio_config_ops restrictions
- [PATCH v2 03/24] virtio: allow __virtioXX, __leXX in config space