Linux Virtualization - Jan 2015 - [PATCH RFC v6 13/20] virtio: allow to fail setting status

On Thu, Dec 11, 2014 at 02:25:15PM +0100, Cornelia Huck
wrote:
> virtio-1 allow setting of the FEATURES_OK status bit to fail if
> the negotiated feature bits are inconsistent: let's fail
> virtio_set_status() in that case and update virtio-ccw to post an
> error to the guest.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
Right but a separate validate_features call is awkward.
How about we defer virtio_set_features until FEATURES_OK,
and teach virtio_set_features that it can fail?

> ---
>  hw/s390x/virtio-ccw.c      |   20 ++++++++++++--------
>  hw/virtio/virtio.c         |   24 +++++++++++++++++++++++-
>  include/hw/virtio/virtio.h |    3 ++-
>  3 files changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index e09e0da..a55e851 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -555,15 +555,19 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>              if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
>                  virtio_ccw_stop_ioeventfd(dev);
>              }
> -            virtio_set_status(vdev, status);
> -            if (vdev->status == 0) {
> -                virtio_reset(vdev);
> -            }
> -            if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> -                virtio_ccw_start_ioeventfd(dev);
> +            if (virtio_set_status(vdev, status) == 0) {
> +                if (vdev->status == 0) {
> +                    virtio_reset(vdev);
> +                }
> +                if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> +                    virtio_ccw_start_ioeventfd(dev);
> +                }
> +                sch->curr_status.scsw.count = ccw.count -
sizeof(status);
> +                ret = 0;
> +            } else {
> +                /* Trigger a command reject. */
> +                ret = -ENOSYS;
>              }
> -            sch->curr_status.scsw.count = ccw.count - sizeof(status);
> -            ret = 0;
>          }
>          break;
>      case CCW_CMD_SET_IND:
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index a3dd67b..90eedd3 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -543,15 +543,37 @@ void virtio_update_irq(VirtIODevice *vdev)
>      virtio_notify_vector(vdev, VIRTIO_NO_VECTOR);
>  }
>  
> -void virtio_set_status(VirtIODevice *vdev, uint8_t val)
> +static int virtio_validate_features(VirtIODevice *vdev)
> +{
> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> +
> +    if (k->validate_features) {
> +        return k->validate_features(vdev);
> +    } else {
> +        return 0;
> +    }
> +}
> +
> +int virtio_set_status(VirtIODevice *vdev, uint8_t val)
>  {
>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>      trace_virtio_set_status(vdev, val);
>  
> +    if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +        if (!(vdev->status & VIRTIO_CONFIG_S_FEATURES_OK)
&&
> +            val & VIRTIO_CONFIG_S_FEATURES_OK) {
> +            int ret = virtio_validate_features(vdev);
> +
> +            if (ret) {
> +                return ret;
> +            }
> +        }
> +    }
>      if (k->set_status) {
>          k->set_status(vdev, val);
>      }
>      vdev->status = val;
> +    return 0;
>  }
>  
>  bool target_words_bigendian(void);
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index a24e403..068211e 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -149,6 +149,7 @@ typedef struct VirtioDeviceClass {
>      uint64_t (*get_features)(VirtIODevice *vdev, uint64_t
requested_features);
>      uint64_t (*bad_features)(VirtIODevice *vdev);
>      void (*set_features)(VirtIODevice *vdev, uint64_t val);
> +    int (*validate_features)(VirtIODevice *vdev);
>      void (*get_config)(VirtIODevice *vdev, uint8_t *config);
>      void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
>      void (*reset)(VirtIODevice *vdev);
> @@ -233,7 +234,7 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n,
int align);
>  void virtio_queue_notify(VirtIODevice *vdev, int n);
>  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
>  void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
> -void virtio_set_status(VirtIODevice *vdev, uint8_t val);
> +int virtio_set_status(VirtIODevice *vdev, uint8_t val);
>  void virtio_reset(void *opaque);
>  void virtio_update_irq(VirtIODevice *vdev);
>  int virtio_set_features(VirtIODevice *vdev, uint64_t val);
> -- 
> 1.7.9.5

On Tue, 30 Dec 2014 14:25:37 +0200
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> On Thu, Dec 11, 2014 at 02:25:15PM +0100, Cornelia Huck wrote:
> > virtio-1 allow setting of the FEATURES_OK status bit to fail if
> > the negotiated feature bits are inconsistent: let's fail
> > virtio_set_status() in that case and update virtio-ccw to post an
> > error to the guest.
> > 
> > Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
> 
> Right but a separate validate_features call is awkward.
> How about we defer virtio_set_features until FEATURES_OK,
> and teach virtio_set_features that it can fail?
Hm. But we would need to keep virtio_set_features() where it is called
now for legacy devices, as they will never see FEATURES_OK, right? So
we need to make this depending on revisions (or whatever the equivalent
is for pci/mmio), as we cannot check for VERSION_1. Not sure whether
this makes the code easier to follow.

On Wed, Jan 07, 2015 at 05:13:32PM +0100, Cornelia Huck
wrote:
> On Tue, 30 Dec 2014 14:25:37 +0200
> "Michael S. Tsirkin" <mst at redhat.com> wrote:
> 
> > On Thu, Dec 11, 2014 at 02:25:15PM +0100, Cornelia Huck wrote:
> > > virtio-1 allow setting of the FEATURES_OK status bit to fail if
> > > the negotiated feature bits are inconsistent: let's fail
> > > virtio_set_status() in that case and update virtio-ccw to post an
> > > error to the guest.
> > > 
> > > Signed-off-by: Cornelia Huck <cornelia.huck at de.ibm.com>
> > 
> > Right but a separate validate_features call is awkward.
> > How about we defer virtio_set_features until FEATURES_OK,
> > and teach virtio_set_features that it can fail?
> 
> Hm. But we would need to keep virtio_set_features() where it is called
> now for legacy devices, as they will never see FEATURES_OK, right?
> So
> we need to make this depending on revisions (or whatever the equivalent
> is for pci/mmio), as we cannot check for VERSION_1. Not sure whether
> this makes the code easier to follow.
So let's make this a separate callback then.
virtio_legacy_set_features?