From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49872) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TkdL7-0003Mt-HO for qemu-devel@nongnu.org; Mon, 17 Dec 2012 11:14:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TkdL6-00058h-92 for qemu-devel@nongnu.org; Mon, 17 Dec 2012 11:14:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:24910) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TkdL6-00058a-0K for qemu-devel@nongnu.org; Mon, 17 Dec 2012 11:14:48 -0500 Message-ID: <50CF44F4.7050506@redhat.com> Date: Mon, 17 Dec 2012 17:14:44 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <20121212171847.GC18597@redhat.com> <50C8BF83.4010307@redhat.com> <20121212212720.GC23087@redhat.com> <50C997BF.80200@redhat.com> <20121216170418.GG15790@redhat.com> <50CE2184.9020908@redhat.com> <20121217104023.GC27072@redhat.com> <50CF36B8.4070203@redhat.com> <20121217152404.GA28235@redhat.com> <50CF3C40.1010202@redhat.com> <20121217160108.GA28909@redhat.com> In-Reply-To: <20121217160108.GA28909@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 0/2] virtio: reset all qbuses too when writing to the status field List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: agraf@suse.de, qemu-devel 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