From patchwork Wed Dec 12 17:29:54 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 205587 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 8D7B02C0080 for ; Thu, 13 Dec 2012 04:37:43 +1100 (EST) Received: from localhost ([::1]:34171 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tiq8k-0006Is-7M for incoming@patchwork.ozlabs.org; Wed, 12 Dec 2012 12:30:38 -0500 Received: from eggs.gnu.org ([208.118.235.92]:43590) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tiq8I-0005aS-D8 for qemu-devel@nongnu.org; Wed, 12 Dec 2012 12:30:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tiq86-0006Xc-Hj for qemu-devel@nongnu.org; Wed, 12 Dec 2012 12:30:05 -0500 Received: from mx1.redhat.com ([209.132.183.28]:2376) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tiq86-0006XH-9A for qemu-devel@nongnu.org; Wed, 12 Dec 2012 12:29:58 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qBCHTvc3010447 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 12 Dec 2012 12:29:57 -0500 Received: from yakj.usersys.redhat.com (ovpn-112-38.ams2.redhat.com [10.36.112.38]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id qBCHTtta020652; Wed, 12 Dec 2012 12:29:55 -0500 Message-ID: <50C8BF12.1070906@redhat.com> Date: Wed, 12 Dec 2012 18:29:54 +0100 From: Paolo Bonzini User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: "Michael S. Tsirkin" References: <1355322396-32026-1-git-send-email-pbonzini@redhat.com> <1355322396-32026-2-git-send-email-pbonzini@redhat.com> <20121212144448.GE15555@redhat.com> <50C8A0A8.3000303@redhat.com> <20121212153333.GC16750@redhat.com> <50C8B2BB.50606@redhat.com> <20121212170522.GA18597@redhat.com> In-Reply-To: <20121212170522.GA18597@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: qemu-devel@nongnu.org, agraf@suse.de Subject: Re: [Qemu-devel] [PATCH 1/2] virtio-pci: 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 >> No, it's a generic thing. Other virtio devices may have a bus, and they >> should reset it just as well. virtio-serial's guest_reset function >> closes each port from its own reset callback manually (since commit >> 4399722, virtio-serial-bus: Unset guest_connected at reset and driver >> reset, 2012-04-24). That shouldn't even exist. The ports should close >> themselves when a reset signal reaches them, and the propagation of the >> reset should happen automatically just like IMO it should for virtio-scsi. >> >> Let's not hide the basic concepts behind "it's a SCSI thing". The >> concept here is that of a soft and hard reset, and these is the definition: >> >> - you soft-reset a device by writing to a register which is in the spec >> of the device (e.g., write zero to the status register); >> >> - you hard-reset a device by writing to a register which is in the spec >> of the bus (e.g., FLR); >> >> - hard reset is a superset of soft reset; >> >> - soft-resetting device X will also do a hard reset of all devices below X. >> >> For example, a soft-reset of a PCI-to-PCI bridge does a hard-reset of >> all devices below (qdev_reset_all calls pcibus_reset calls >> pci_device_reset). >> >> In QEMU, qdev_reset_all is a soft reset of a device. qbus_reset_all_fn >> is a hard reset of all devices below a particular device. > > I might be convinced if it's renamed qdev_reset_soft, > qbus_reset_hard. Let's rename it. > As it is it, these look like internal interfaces > that one needs to poke at implementation to figure out. Point taken. >> There is no >> generic qdev function for a hard reset of a particular device >> (pci_device_reset is it for the PCI case, just to complete the >> nomenclature with something you're familiar with). >> >> These are all generic concepts. Nothing virtio-specific, nothing >> SCSI-specific. It's not open for questioning. :) > > When we have a similar issue in another device it > will be easier to see how to do it right. > At the moment let's fix it locally. We have a similar issue in virtio-serial.c, and with my patch you can do this: i.e. just implement a method on the bus to do the hard-reset, and let generic infrastructure call it. > OK the right thing would be to to qdev_reset_all > at the virtio scsi level. The modeling > is wrong though and the devices are not the > children of the right device. > > Sticking the reset in virtio-pci just propagates this bug. Sticking a bus reset in virtio_pci_reset would. But that's not what my patch does. >>> but apparently same applies to virtio-blk which >>> really has no block bus. >> >> virtio-blk has no bus, so it does the reset from its own virtio_reset >> callback. >> >> virtio-scsi needs to ask the SCSIDevices to reset themselves. Whatever >> virtio-blk does in its reset callback, the SCSIDevices will do---only in >> a "regular" qdev reset rather than a special-purpose reset callback. >> >> I can of course iterate on all the devices, but it is pointless when >> there is already a perfectly good callback to do a soft reset, and it's >> called qdev_reset_all. > > Fine bug call it from virtio_scsi_reset. No, because that would also reset the virtio-pci bits (ioeventfd etc.) which virtio-scsi has no business with. What I could do is to call qbus_reset_all, but it makes no sense to me when there is a generic solution. Paolo diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 155da58..1564482 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -532,12 +532,13 @@ static void set_config(VirtIODevice *vdev, const uint8_t *config_data) memcpy(&config, config_data, sizeof(config)); } -static void guest_reset(VirtIOSerial *vser) +static int virtser_bus_reset(BusState *qbus) { + VirtIOSerialBus *bus = DO_UPCAST(VirtIOSerialBus, qbus, qbus); VirtIOSerialPort *port; VirtIOSerialPortClass *vsc; - QTAILQ_FOREACH(port, &vser->ports, next) { + QTAILQ_FOREACH(port, &bus->vser->ports, next) { vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port); if (port->guest_connected) { port->guest_connected = false; @@ -546,6 +547,7 @@ static void guest_reset(VirtIOSerial *vser) vsc->guest_close(port); } } + return 0; } static void set_status(VirtIODevice *vdev, uint8_t status) @@ -566,17 +568,6 @@ static void set_status(VirtIODevice *vdev, uint8_t status) */ port->guest_connected = true; } - if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) { - guest_reset(vser); - } -} - -static void vser_reset(VirtIODevice *vdev) -{ - VirtIOSerial *vser; - - vser = DO_UPCAST(VirtIOSerial, vdev, vdev); - guest_reset(vser); } static void virtio_serial_save(QEMUFile *f, void *opaque) @@ -766,6 +757,7 @@ static void virtser_bus_class_init(ObjectClass *klass, void *data) { BusClass *k = BUS_CLASS(klass); k->print_dev = virtser_bus_dev_print; + k->reset = virtser_bus_reset; } static const TypeInfo virtser_bus_info = { @@ -985,7 +977,6 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, virtio_serial_conf *conf) vser->vdev.get_config = get_config; vser->vdev.set_config = set_config; vser->vdev.set_status = set_status; - vser->vdev.reset = vser_reset; vser->qdev = dev;