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
