diff mbox

[0/2] virtio: reset all qbuses too when writing to the status field

Message ID 20121217104023.GC27072@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Dec. 17, 2012, 10:40 a.m. UTC 
On Sun, Dec 16, 2012 at 08:31:16PM +0100, Paolo Bonzini wrote:
> Il 16/12/2012 18:04, Michael S. Tsirkin ha scritto:
> > On Thu, Dec 13, 2012 at 09:54:23AM +0100, Paolo Bonzini wrote:
> >> Il 12/12/2012 22:27, Michael S. Tsirkin ha scritto:
> >>>>> Maybe it's obvious to you that qdev_reset_all(x)
> >>>>> does a soft reset and what soft reset means
> >>>>> for each bus type
> >>>>
> >>>> We can define soft reset to be *independent* of the bus type.  As you
> >>>> said, you access it with a device register.
> >>>
> >>> I think qemu has one type of reset ATM which is the hard reset.
> >>
> >> A hard reset would kill BARs and configuration space too.
> >> qdev_reset_all doesn't.  Ergo, it is not a hard reset.
> >>
> >> But hey, I'm not wed to names.  Let's call it device-level reset and
> >> bus-level reset.  Whatever.
> > 
> > It's not a question of a name.
> > 
> > ATM qemu supports one kind of reset because it's a kind of reset all
> > hardware supports.  The moment we start inventing
> > our own one we need to document exactly what it means.
> 
> We have two, DeviceClass's and BusClass's reset members.
> 
> I'll make a patch to document them.

And qdev_reset_all needs documentation too.
Another issue is that this really should be
part of generic code, not transport-specific one.

The reason we can't do this is because virtio
does not have access to DeviceState: all it has
is a void * binding opaque.
However this is not really a feature - it seems we
should be able to pass DeviceState instead.

How about the following? Then we can put reset
in generic code where it belongs.
It's untested - really kind of pseudo code - and
s390 is still to be updated.

Posting to see what does everyone thinks.

---

Comments

Paolo Bonzini Dec. 17, 2012, 3:14 p.m. UTC | #1 
Il 17/12/2012 11:40, Michael S. Tsirkin ha scritto:
> How about the following? Then we can put reset
> in generic code where it belongs.
> It's untested - really kind of pseudo code - and
> s390 is still to be updated.
> 
> Posting to see what does everyone thinks.

I'm not (yet) sure how that helps my problem, but it is definitely a
step in the right direction!

Paolo
Michael S. Tsirkin Dec. 17, 2012, 3:24 p.m. UTC | #2 
On Mon, Dec 17, 2012 at 04:14:00PM +0100, Paolo Bonzini wrote:
> Il 17/12/2012 11:40, Michael S. Tsirkin ha scritto:
> > How about the following? Then we can put reset
> > in generic code where it belongs.
> > It's untested - really kind of pseudo code - and
> > s390 is still to be updated.
> > 
> > Posting to see what does everyone thinks.
> 
> I'm not (yet) sure how that helps my problem,

It makes it possible for virtio.c to get at the
device state through the binding pointer.
So you will be able to qdev_reset_all from virtio.c
where it belongs, instead of duplicating code
in all bindings.

> but it is definitely a
> step in the right direction!
> 
> Paolo
Paolo Bonzini Dec. 17, 2012, 3:37 p.m. UTC | #3 
Il 17/12/2012 16:24, Michael S. Tsirkin ha scritto:
> On Mon, Dec 17, 2012 at 04:14:00PM +0100, Paolo Bonzini wrote:
>> Il 17/12/2012 11:40, Michael S. Tsirkin ha scritto:
>>> How about the following? Then we can put reset
>>> in generic code where it belongs.
>>> It's untested - really kind of pseudo code - and
>>> s390 is still to be updated.
>>>
>>> Posting to see what does everyone thinks.
>>
>> I'm not (yet) sure how that helps my problem,
> 
> It makes it possible for virtio.c to get at the
> device state through the binding pointer.
> So you will be able to qdev_reset_all from virtio.c
> where it belongs, instead of duplicating code
> in all bindings.

Yes, but where does it belong?  Do you want to move handling of the
status register (and others) to hw/virtio.c?

Also, you're proposing that I do qdev_reset_all(vdev->binding_opaque)
but that would be a layering violation.  Generic virtio code should not
be able to reset the transport-specific setup (e.g. MSIs).

Paolo

>> but it is definitely a
>> step in the right direction!
>>
>> Paolo
Michael S. Tsirkin Dec. 17, 2012, 4:01 p.m. UTC | #4 
On Mon, Dec 17, 2012 at 04:37:36PM +0100, Paolo Bonzini wrote:
> Il 17/12/2012 16:24, Michael S. Tsirkin ha scritto:
> > On Mon, Dec 17, 2012 at 04:14:00PM +0100, Paolo Bonzini wrote:
> >> Il 17/12/2012 11:40, Michael S. Tsirkin ha scritto:
> >>> How about the following? Then we can put reset
> >>> in generic code where it belongs.
> >>> It's untested - really kind of pseudo code - and
> >>> s390 is still to be updated.
> >>>
> >>> Posting to see what does everyone thinks.
> >>
> >> I'm not (yet) sure how that helps my problem,
> > 
> > It makes it possible for virtio.c to get at the
> > device state through the binding pointer.
> > So you will be able to qdev_reset_all from virtio.c
> > where it belongs, instead of duplicating code
> > in all bindings.
> 
> Yes, but where does it belong?  Do you want to move handling of the
> status register (and others) to hw/virtio.c?

I thought we can have some kind of generic function that all
transports can call. It would call qdev_reset_all internally,
and we would invoke it from all transports.

> Also, you're proposing that I do qdev_reset_all(vdev->binding_opaque)
> but that would be a layering violation.  Generic virtio code should not
> be able to reset the transport-specific setup (e.g. MSIs).
> 
> Paolo

Bus reset looks like this:

	qdev -> pci -> virtio pci reset -> virtio reset

status reset looks like this:

	 virtio pci reset -> virtio reset

You original patch was basically calling back to
qdev from virtio pci (bypassing pci).

If that is OK and not a layering violation,
why calling from virtio back to virtio pci not OK?

How do you think reset should be layered?


> >> but it is definitely a
> >> step in the right direction!
> >>
> >> Paolo
Paolo Bonzini Dec. 17, 2012, 4:14 p.m. UTC | #5 
Il 17/12/2012 17:01, Michael S. Tsirkin ha scritto:
> On Mon, Dec 17, 2012 at 04:37:36PM +0100, Paolo Bonzini wrote:
>> Il 17/12/2012 16:24, Michael S. Tsirkin ha scritto:
>>> On Mon, Dec 17, 2012 at 04:14:00PM +0100, Paolo Bonzini wrote:
>>>> Il 17/12/2012 11:40, Michael S. Tsirkin ha scritto:
>>>>> How about the following? Then we can put reset
>>>>> in generic code where it belongs.
>>>>> It's untested - really kind of pseudo code - and
>>>>> s390 is still to be updated.
>>>>>
>>>>> Posting to see what does everyone thinks.
>>>>
>>>> I'm not (yet) sure how that helps my problem,
>>>
>>> It makes it possible for virtio.c to get at the
>>> device state through the binding pointer.
>>> So you will be able to qdev_reset_all from virtio.c
>>> where it belongs, instead of duplicating code
>>> in all bindings.
>>
>> Yes, but where does it belong?  Do you want to move handling of the
>> status register (and others) to hw/virtio.c?
> 
> I thought we can have some kind of generic function that all
> transports can call. It would call qdev_reset_all internally,
> and we would invoke it from all transports.
> 
>> Also, you're proposing that I do qdev_reset_all(vdev->binding_opaque)
>> but that would be a layering violation.  Generic virtio code should not
>> be able to reset the transport-specific setup (e.g. MSIs).
> 
> Bus reset looks like this:
> 
> 	qdev -> pci -> virtio pci reset -> virtio reset
> 
> status reset looks like this:
> 
> 	 virtio pci reset -> virtio reset
> 
> You original patch was basically calling back to
> qdev from virtio pci (bypassing pci).

Because it is actually correct to not involve PCI.  This is not
bypassing: PCI is above in the qdev tree, and never learns about a reset
that is triggered by a register write.  So, a device can ask qdev and be
reset, but it device cannot ask its parent to do a bus reset of itself.
 That would be like doing an FLR when writing zero to status.  Wrong,
and a layering violation.

> If that is OK and not a layering violation,
> why calling from virtio back to virtio pci not OK?

Because I'm calling qdev_reset_all on the same device that received the
reset.  I'm not calling qdev_reset_all on a parent device.  Calling
qdev_reset_all(vdev->binding_opaque) is equivalent calling it on a
parent device.

Still, the extra typesafety of your patch is good to have.

Paolo

> How do you think reset should be layered?
> 
> 
>>>> but it is definitely a
>>>> step in the right direction!
>>>>
>>>> Paolo
diff mbox

Patch

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 3ea4140..63ae888 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -98,34 +98,34 @@  bool virtio_is_big_endian(void);
 
 /* virtio device */
 
-static void virtio_pci_notify(void *opaque, uint16_t vector)
+static void virtio_pci_notify(DeviceState *d, uint16_t vector)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     if (msix_enabled(&proxy->pci_dev))
         msix_notify(&proxy->pci_dev, vector);
     else
         qemu_set_irq(proxy->pci_dev.irq[0], proxy->vdev->isr & 1);
 }
 
-static void virtio_pci_save_config(void * opaque, QEMUFile *f)
+static void virtio_pci_save_config(DeviceState *d, QEMUFile *f)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     pci_device_save(&proxy->pci_dev, f);
     msix_save(&proxy->pci_dev, f);
     if (msix_present(&proxy->pci_dev))
         qemu_put_be16(f, proxy->vdev->config_vector);
 }
 
-static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f)
+static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     if (msix_present(&proxy->pci_dev))
         qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n));
 }
 
-static int virtio_pci_load_config(void * opaque, QEMUFile *f)
+static int virtio_pci_load_config(DeviceState *d, QEMUFile *f)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     int ret;
     ret = pci_device_load(&proxy->pci_dev, f);
     if (ret) {
@@ -144,9 +144,9 @@  static int virtio_pci_load_config(void * opaque, QEMUFile *f)
     return 0;
 }
 
-static int virtio_pci_load_queue(void * opaque, int n, QEMUFile *f)
+static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     uint16_t vector;
     if (msix_present(&proxy->pci_dev)) {
         qemu_get_be16s(f, &vector);
@@ -464,9 +464,9 @@  static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
     }
 }
 
-static unsigned virtio_pci_get_features(void *opaque)
+static unsigned virtio_pci_get_features(DeviceState *d)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     return proxy->host_features;
 }
 
@@ -568,9 +568,9 @@  static void kvm_virtio_pci_vector_release(PCIDevice *dev, unsigned vector)
     }
 }
 
-static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign)
+static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     VirtQueue *vq = virtio_get_queue(proxy->vdev, n);
     EventNotifier *notifier = virtio_queue_get_guest_notifier(vq);
 
@@ -588,15 +588,15 @@  static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign)
     return 0;
 }
 
-static bool virtio_pci_query_guest_notifiers(void *opaque)
+static bool virtio_pci_query_guest_notifiers(DeviceState *d)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     return msix_enabled(&proxy->pci_dev);
 }
 
-static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
+static int virtio_pci_set_guest_notifiers(DeviceState *d, bool assign)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     VirtIODevice *vdev = proxy->vdev;
     int r, n;
 
@@ -612,7 +612,7 @@  static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
             break;
         }
 
-        r = virtio_pci_set_guest_notifier(opaque, n, assign);
+        r = virtio_pci_set_guest_notifier(d, n, assign);
         if (r < 0) {
             goto assign_error;
         }
@@ -638,14 +638,14 @@  assign_error:
     /* We get here on assignment failure. Recover by undoing for VQs 0 .. n. */
     assert(assign);
     while (--n >= 0) {
-        virtio_pci_set_guest_notifier(opaque, n, !assign);
+        virtio_pci_set_guest_notifier(d, n, !assign);
     }
     return r;
 }
 
-static int virtio_pci_set_host_notifier(void *opaque, int n, bool assign)
+static int virtio_pci_set_host_notifier(DeviceState *d, int n, bool assign)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
 
     /* Stop using ioeventfd for virtqueue kick if the device starts using host
      * notifiers.  This makes it easy to avoid stepping on each others' toes.
@@ -661,9 +661,9 @@  static int virtio_pci_set_host_notifier(void *opaque, int n, bool assign)
     return virtio_pci_set_host_notifier_internal(proxy, n, assign, false);
 }
 
-static void virtio_pci_vmstate_change(void *opaque, bool running)
+static void virtio_pci_vmstate_change(DeviceState *d, bool running)
 {
-    VirtIOPCIProxy *proxy = opaque;
+    VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
 
     if (running) {
         /* Try to find out if the guest has bus master disabled, but is
diff --git a/hw/virtio.h b/hw/virtio.h
index 7c17f7b..b9eeaa5 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -91,17 +91,17 @@  typedef struct VirtQueueElement
 } VirtQueueElement;
 
 typedef struct {
-    void (*notify)(void * opaque, uint16_t vector);
-    void (*save_config)(void * opaque, QEMUFile *f);
-    void (*save_queue)(void * opaque, int n, QEMUFile *f);
-    int (*load_config)(void * opaque, QEMUFile *f);
-    int (*load_queue)(void * opaque, int n, QEMUFile *f);
-    int (*load_done)(void * opaque, QEMUFile *f);
-    unsigned (*get_features)(void * opaque);
-    bool (*query_guest_notifiers)(void * opaque);
-    int (*set_guest_notifiers)(void * opaque, bool assigned);
-    int (*set_host_notifier)(void * opaque, int n, bool assigned);
-    void (*vmstate_change)(void * opaque, bool running);
+    void (*notify)(DeviceState *d, uint16_t vector);
+    void (*save_config)(DeviceState *d, QEMUFile *f);
+    void (*save_queue)(DeviceState *d, int n, QEMUFile *f);
+    int (*load_config)(DeviceState *d, QEMUFile *f);
+    int (*load_queue)(DeviceState *d, int n, QEMUFile *f);
+    int (*load_done)(DeviceState *d, QEMUFile *f);
+    unsigned (*get_features)(DeviceState *d);
+    bool (*query_guest_notifiers)(DeviceState *d);
+    int (*set_guest_notifiers)(DeviceState *d, bool assigned);
+    int (*set_host_notifier)(DeviceState *d, int n, bool assigned);
+    void (*vmstate_change)(DeviceState *d, bool running);
 } VirtIOBindings;
 
 #define VIRTIO_PCI_QUEUE_MAX 64
@@ -128,7 +128,7 @@  struct VirtIODevice
     void (*set_status)(VirtIODevice *vdev, uint8_t val);
     VirtQueue *vq;
     const VirtIOBindings *binding;
-    void *binding_opaque;
+    DeviceState *binding_opaque;
     uint16_t device_id;
     bool vm_running;
     VMChangeStateEntry *vmstate;