Re: [Qemu-devel] [PATCH 1/3] virtio: Introduce VirtIODevice.broken

[Michael S. Tsirkin]

On Thu, Apr 24, 2014 at 11:19:14AM +0800, Fam Zheng wrote:
> On Wed, 04/23 10:17, Michael S. Tsirkin wrote:
> > On Tue, Apr 22, 2014 at 04:55:15PM +0800, Fam Zheng wrote:
> > > If guest driver behaves abnormally, emulation code could mark the device
> > > as "broken".
> > > 
> > > Once "broken" is set, device emulation will typically wait for a reset
> > > command and ignore any other operations, but it could also return error
> > > responds. In other words, whether and how does guest know about this
> > > error status is device specific.
> > > 
> > > Signed-off-by: Fam Zheng <address@hidden>
> > 
> > We really need a flag to notify guest about this state though.
> > We should add this in virtio 1.0.
> > For now, how about clearing DRIVER_OK?
> 
> >From Public Review Draft 02:
> 
>     DRIVER_OK (4) Indicates that the driver is set up and ready to drive the
>     device.
> 
> Does clearing it here have any effect on guest behavior?
> 
> We could add a DEVICE_OK bit and let the driver check it before passing vq.

Could you please send this suggestion as review comment on the draft?
We'll take it from here.


> > This way we don't need to touch so much code.
> 
> Don't understand how clearing DRIVER_OK can avoid exit(). Could you explain a
> bit?
> 
> Thanks,
> Fam
> 
> > 
> > > ---
> > >  hw/virtio/virtio.c         | 12 ++++++++++++
> > >  include/hw/virtio/virtio.h |  3 +++
> > >  2 files changed, 15 insertions(+)
> > > 
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index aeabf3a..222bb73 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -538,6 +538,16 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t 
> > > val)
> > >      vdev->status = val;
> > >  }
> > >  
> > > +bool virtio_broken(VirtIODevice *vdev)
> > > +{
> > > +    return vdev->broken;
> > > +}
> > > +
> > > +void virtio_set_broken(VirtIODevice *vdev)
> > > +{
> > > +    vdev->broken = true;
> > > +}
> > > +
> > >  void virtio_reset(void *opaque)
> > >  {
> > >      VirtIODevice *vdev = opaque;
> > > @@ -554,6 +564,7 @@ void virtio_reset(void *opaque)
> > >      vdev->queue_sel = 0;
> > >      vdev->status = 0;
> > >      vdev->isr = 0;
> > > +    vdev->broken = false;
> > >      vdev->config_vector = VIRTIO_NO_VECTOR;
> > >      virtio_notify_vector(vdev, vdev->config_vector);
> > >  
> > > @@ -995,6 +1006,7 @@ void virtio_init(VirtIODevice *vdev, const char 
> > > *name,
> > >      vdev->status = 0;
> > >      vdev->isr = 0;
> > >      vdev->queue_sel = 0;
> > > +    vdev->broken = 0;
> > >      vdev->config_vector = VIRTIO_NO_VECTOR;
> > >      vdev->vq = g_malloc0(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX);
> > >      vdev->vm_running = runstate_is_running();
> > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > > index 3e54e90..5b16faa 100644
> > > --- a/include/hw/virtio/virtio.h
> > > +++ b/include/hw/virtio/virtio.h
> > > @@ -121,6 +121,7 @@ struct VirtIODevice
> > >      bool vm_running;
> > >      VMChangeStateEntry *vmstate;
> > >      char *bus_name;
> > > +    bool broken;
> > >  };
> > >  
> > >  typedef struct VirtioDeviceClass {
> > > @@ -211,6 +212,8 @@ 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);
> > > +void virtio_set_broken(VirtIODevice *vdev);
> > > +bool virtio_broken(VirtIODevice *vdev);
> > >  void virtio_reset(void *opaque);
> > >  void virtio_update_irq(VirtIODevice *vdev);
> > >  int virtio_set_features(VirtIODevice *vdev, uint32_t val);
> > > -- 
> > > 1.9.2