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.

Yes I'm thinking some new bit might be useful to report internal device
errors. Not sure what "let the driver check it before passing vq"
means here - such an internal error would have to be reported by sending
a config interrupt to guest.

> > 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

It does not by itself but with IIUC in existing qemu code clearing DRIVER_OK
will stop the device until reset which is what you wanted to
do, right?

> > 
> > > ---
> > >  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