From patchwork Mon Dec 17 10:40:23 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Michael S. Tsirkin" X-Patchwork-Id: 206892 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id CFB292C007D for ; Tue, 18 Dec 2012 02:11:33 +1100 (EST) Received: from localhost ([::1]:46957 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TkcLr-0004Sm-U0 for incoming@patchwork.ozlabs.org; Mon, 17 Dec 2012 10:11:31 -0500 Received: from eggs.gnu.org ([208.118.235.92]:60470) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TkcLf-0004Rz-29 for qemu-devel@nongnu.org; Mon, 17 Dec 2012 10:11:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TkcLX-0005UB-OM for qemu-devel@nongnu.org; Mon, 17 Dec 2012 10:11:18 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55367) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TkcLX-0005U5-GD for qemu-devel@nongnu.org; Mon, 17 Dec 2012 10:11:11 -0500 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qBHFBAVT018304 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 17 Dec 2012 10:11:11 -0500 Received: from redhat.com (vpn1-6-198.ams2.redhat.com [10.36.6.198]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with SMTP id qBHFB8Cl010098 for ; Mon, 17 Dec 2012 10:11:09 -0500 Resent-From: "Michael S. Tsirkin" Resent-Date: Mon, 17 Dec 2012 17:14:20 +0200 Resent-Message-ID: <20121217151420.GB28088@redhat.com> Resent-To: qemu-devel@nongnu.org Date: Mon, 17 Dec 2012 12:40:23 +0200 From: "Michael S. Tsirkin" To: Paolo Bonzini Message-ID: <20121217104023.GC27072@redhat.com> References: <1355322396-32026-1-git-send-email-pbonzini@redhat.com> <20121212153821.GA17446@redhat.com> <50C8B2FD.5030200@redhat.com> <20121212171847.GC18597@redhat.com> <50C8BF83.4010307@redhat.com> <20121212212720.GC23087@redhat.com> <50C997BF.80200@redhat.com> <20121216170418.GG15790@redhat.com> <50CE2184.9020908@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <50CE2184.9020908@redhat.com> X-Scanned-By: MIMEDefang 2.67 on 10.5.11.12 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: agraf@suse.de, qemu-devel Subject: Re: [Qemu-devel] [PATCH 0/2] virtio: reset all qbuses too when writing to the status field X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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. 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;