Message ID | 20121217104023.GC27072@redhat.com |
---|---|
State | New |
Headers | show |
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
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
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
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
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 --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;