| Message ID | 1355322396-32026-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 12, 2012 at 03:26:35PM +0100, Paolo Bonzini wrote: > virtio-pci devices do not perform a full reset when zero is written > to the status field. While PCI-specific status is initialized, the > reset does not propagate down the qdev bus hierarchy. Because of > this, a virtio reset does not cancel in-flight I/O for virtio-scsi > (where the cancellation is handled automatically by the SCSI > devices underneath virtio-scsi-pci). > > The patch calls qdev_reset_all, which calls virtio_pci_reset, > instead of basically inlining the contents of the latter. > > Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org> > Cc: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> This is a device specific register, IMO it should reset very specific things not what happens to be on the bus. For example qdev resets the PCI header: will or will not this reset it? It should not but no easy way to figure out. Can't the required code just go into the virtio-scsi reset callback? > --- > hw/virtio-pci.c | 25 ++++++++++--------------- > 1 file changed, 10 insertions(+), 15 deletions(-) > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index 71f4fb5..a1685f1 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -268,12 +268,10 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) > case VIRTIO_PCI_QUEUE_PFN: > pa = (hwaddr)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT; > if (pa == 0) { > - virtio_pci_stop_ioeventfd(proxy); > - virtio_reset(proxy->vdev); > - msix_unuse_all_vectors(&proxy->pci_dev); > - } > - else > + qdev_reset_all(&proxy->pci_dev.qdev); > + } else { > virtio_queue_set_addr(vdev, vdev->queue_sel, pa); > + } > break; > case VIRTIO_PCI_QUEUE_SEL: > if (val < VIRTIO_PCI_QUEUE_MAX) > @@ -285,19 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) > } > break; > case VIRTIO_PCI_STATUS: > - if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) { > - virtio_pci_stop_ioeventfd(proxy); > - } > - > virtio_set_status(vdev, val & 0xFF); > > - if (val & VIRTIO_CONFIG_S_DRIVER_OK) { > - virtio_pci_start_ioeventfd(proxy); > - } > - > if (vdev->status == 0) { > - virtio_reset(proxy->vdev); > - msix_unuse_all_vectors(&proxy->pci_dev); > + qdev_reset_all(&proxy->pci_dev.qdev); > + } else { > + if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) { > + virtio_pci_stop_ioeventfd(proxy); > + } else { > + virtio_pci_start_ioeventfd(proxy); > + } > } > > /* Linux before 2.6.34 sets the device as OK without enabling > -- > 1.8.0.1 >
Il 12/12/2012 15:44, Michael S. Tsirkin ha scritto: > On Wed, Dec 12, 2012 at 03:26:35PM +0100, Paolo Bonzini wrote: >> virtio-pci devices do not perform a full reset when zero is written >> to the status field. While PCI-specific status is initialized, the >> reset does not propagate down the qdev bus hierarchy. Because of >> this, a virtio reset does not cancel in-flight I/O for virtio-scsi >> (where the cancellation is handled automatically by the SCSI >> devices underneath virtio-scsi-pci). >> >> The patch calls qdev_reset_all, which calls virtio_pci_reset, >> instead of basically inlining the contents of the latter. >> >> Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org> >> Cc: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > This is a device specific register, IMO it should reset > very specific things not what happens to be on the bus. > For example qdev resets the PCI header: will or > will not this reset it? qdev does not reset the PCI header. Only pci_device_reset (called when resetting the whole bus and also by FLR) does. > It should not but no easy way to figure out. qdev_reset_all is not FLR. If you don't like direct usage of qdev_reset_all, let's add a pci_device_soft_reset function that just calls qdev_reset_all. This way it is more self-documenting. Other virtio implementations may not have an equivalent of FLR on their bus and do a hard-reset when 0 is written to the status field. The virtio spec is silent on this---they can do it if they want. > Can't the required code just go into the virtio-scsi > reset callback? Of course yes, but it'd be different from all other SCSI adapters then. They don't expect that they need to do anything to reset devices on their SCSI bus. Paolo >> --- >> hw/virtio-pci.c | 25 ++++++++++--------------- >> 1 file changed, 10 insertions(+), 15 deletions(-) >> >> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c >> index 71f4fb5..a1685f1 100644 >> --- a/hw/virtio-pci.c >> +++ b/hw/virtio-pci.c >> @@ -268,12 +268,10 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) >> case VIRTIO_PCI_QUEUE_PFN: >> pa = (hwaddr)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT; >> if (pa == 0) { >> - virtio_pci_stop_ioeventfd(proxy); >> - virtio_reset(proxy->vdev); >> - msix_unuse_all_vectors(&proxy->pci_dev); >> - } >> - else >> + qdev_reset_all(&proxy->pci_dev.qdev); >> + } else { >> virtio_queue_set_addr(vdev, vdev->queue_sel, pa); >> + } >> break; >> case VIRTIO_PCI_QUEUE_SEL: >> if (val < VIRTIO_PCI_QUEUE_MAX) >> @@ -285,19 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) >> } >> break; >> case VIRTIO_PCI_STATUS: >> - if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) { >> - virtio_pci_stop_ioeventfd(proxy); >> - } >> - >> virtio_set_status(vdev, val & 0xFF); >> >> - if (val & VIRTIO_CONFIG_S_DRIVER_OK) { >> - virtio_pci_start_ioeventfd(proxy); >> - } >> - >> if (vdev->status == 0) { >> - virtio_reset(proxy->vdev); >> - msix_unuse_all_vectors(&proxy->pci_dev); >> + qdev_reset_all(&proxy->pci_dev.qdev); >> + } else { >> + if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) { >> + virtio_pci_stop_ioeventfd(proxy); >> + } else { >> + virtio_pci_start_ioeventfd(proxy); >> + } >> } >> >> /* Linux before 2.6.34 sets the device as OK without enabling >> -- >> 1.8.0.1 >>
On Wed, Dec 12, 2012 at 04:20:08PM +0100, Paolo Bonzini wrote: > Il 12/12/2012 15:44, Michael S. Tsirkin ha scritto: > > On Wed, Dec 12, 2012 at 03:26:35PM +0100, Paolo Bonzini wrote: > >> virtio-pci devices do not perform a full reset when zero is written > >> to the status field. While PCI-specific status is initialized, the > >> reset does not propagate down the qdev bus hierarchy. Because of > >> this, a virtio reset does not cancel in-flight I/O for virtio-scsi > >> (where the cancellation is handled automatically by the SCSI > >> devices underneath virtio-scsi-pci). > >> > >> The patch calls qdev_reset_all, which calls virtio_pci_reset, > >> instead of basically inlining the contents of the latter. > >> > >> Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org> > >> Cc: Michael S. Tsirkin <mst@redhat.com> > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > > > This is a device specific register, IMO it should reset > > very specific things not what happens to be on the bus. > > For example qdev resets the PCI header: will or > > will not this reset it? > > qdev does not reset the PCI header. Only pci_device_reset (called when > resetting the whole bus and also by FLR) does. Point is it's a simple register, the easier it is to understand what happens on this write the better. > > It should not but no easy way to figure out. > > qdev_reset_all is not FLR. If you don't like direct usage of > qdev_reset_all, let's add a pci_device_soft_reset function that just > calls qdev_reset_all. This way it is more self-documenting. > > Other virtio implementations may not have an equivalent of FLR on their > bus and do a hard-reset when 0 is written to the status field. The > virtio spec is silent on this---they can do it if they want. guests expect sane behaviour, losing bios-assigned registers on a device specific write wouldn't be sane. > > Can't the required code just go into the virtio-scsi > > reset callback? > > Of course yes, but it'd be different from all other SCSI adapters then. > They don't expect that they need to do anything to reset devices on > their SCSI bus. > > Paolo On virtio level, it's a device specific register, there's nothing standard about it. If you are talking about some scsi thing here it belongs in virtio scsi, but apparently same applies to virtio-blk which really has no block bus. > >> --- > >> hw/virtio-pci.c | 25 ++++++++++--------------- > >> 1 file changed, 10 insertions(+), 15 deletions(-) > >> > >> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > >> index 71f4fb5..a1685f1 100644 > >> --- a/hw/virtio-pci.c > >> +++ b/hw/virtio-pci.c > >> @@ -268,12 +268,10 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) > >> case VIRTIO_PCI_QUEUE_PFN: > >> pa = (hwaddr)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT; > >> if (pa == 0) { > >> - virtio_pci_stop_ioeventfd(proxy); > >> - virtio_reset(proxy->vdev); > >> - msix_unuse_all_vectors(&proxy->pci_dev); > >> - } > >> - else > >> + qdev_reset_all(&proxy->pci_dev.qdev); > >> + } else { > >> virtio_queue_set_addr(vdev, vdev->queue_sel, pa); > >> + } > >> break; > >> case VIRTIO_PCI_QUEUE_SEL: > >> if (val < VIRTIO_PCI_QUEUE_MAX) > >> @@ -285,19 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) > >> } > >> break; > >> case VIRTIO_PCI_STATUS: > >> - if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) { > >> - virtio_pci_stop_ioeventfd(proxy); > >> - } > >> - > >> virtio_set_status(vdev, val & 0xFF); > >> > >> - if (val & VIRTIO_CONFIG_S_DRIVER_OK) { > >> - virtio_pci_start_ioeventfd(proxy); > >> - } > >> - > >> if (vdev->status == 0) { > >> - virtio_reset(proxy->vdev); > >> - msix_unuse_all_vectors(&proxy->pci_dev); > >> + qdev_reset_all(&proxy->pci_dev.qdev); > >> + } else { > >> + if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) { > >> + virtio_pci_stop_ioeventfd(proxy); > >> + } else { > >> + virtio_pci_start_ioeventfd(proxy); > >> + } > >> } > >> > >> /* Linux before 2.6.34 sets the device as OK without enabling > >> -- > >> 1.8.0.1 > >>
Il 12/12/2012 16:33, Michael S. Tsirkin ha scritto: >>> This is a device specific register, IMO it should reset >>> very specific things not what happens to be on the bus. >>> For example qdev resets the PCI header: will or >>> will not this reset it? >> >> qdev does not reset the PCI header. Only pci_device_reset (called when >> resetting the whole bus and also by FLR) does. > > Point is it's a simple register, the easier it is to understand > what happens on this write the better. The easiest way to understand what happens is to have a definition in the spec. There is none, so to me the simplest definition of resetting the device is "soft-reset the device", and in qdev a soft-reset is qdev_reset_all (see below). Whether it involves function pointers or not, I don't care. >>> Can't the required code just go into the virtio-scsi >>> reset callback? >> >> Of course yes, but it'd be different from all other SCSI adapters then. >> They don't expect that they need to do anything to reset devices on >> their SCSI bus. > > On virtio level, it's a device specific register, there's nothing > standard about it. If you are talking about some scsi thing here it > belongs in virtio scsi 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. 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. :) In the virtio case you have (logically, not at the qdev level): virtio-pci | virtio-scsi / | \ disk disk disk Writing zero to the status register does a soft reset of the virtio-pci device. This includes a hard reset of the virtio-scsi device, and a hard reset of the disks. That can be easily modeled by qdev_reset_all(pci_dev). What we actually have at the qdev level is: virtio-pci ---> virtio-scsi (via ->vdev pointer) / | \ disk disk disk The soft reset of the virtio-scsi device is done by virtio_reset instead of walking the qdev bus, but even in this case the soft reset is perfectly described by qdev_reset_all. Hence, writing zero to the status register should use qdev_reset_all. > 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. Paolo
On Wed, Dec 12, 2012 at 05:37:15PM +0100, Paolo Bonzini wrote: > Il 12/12/2012 16:33, Michael S. Tsirkin ha scritto: > >>> This is a device specific register, IMO it should reset > >>> very specific things not what happens to be on the bus. > >>> For example qdev resets the PCI header: will or > >>> will not this reset it? > >> > >> qdev does not reset the PCI header. Only pci_device_reset (called when > >> resetting the whole bus and also by FLR) does. > > > > Point is it's a simple register, the easier it is to understand > > what happens on this write the better. > > The easiest way to understand what happens is to have a definition in > the spec. There is none, I think we should improve the spec. > so to me the simplest definition of resetting > the device is "soft-reset the device", and in qdev a soft-reset is > qdev_reset_all (see below). That's kind of vague. > Whether it involves function pointers or not, I don't care. > > >>> Can't the required code just go into the virtio-scsi > >>> reset callback? > >> > >> Of course yes, but it'd be different from all other SCSI adapters then. > >> They don't expect that they need to do anything to reset devices on > >> their SCSI bus. > > > > On virtio level, it's a device specific register, there's nothing > > standard about it. If you are talking about some scsi thing here it > > belongs in virtio scsi > > 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. As it is it, these look like internal interfaces that one needs to poke at implementation to figure out. > 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. > In the virtio case you have (logically, not at the qdev level): > > virtio-pci > | > virtio-scsi > / | \ > disk disk disk > > Writing zero to the status register does a soft reset of the virtio-pci > device. This includes a hard reset of the virtio-scsi device, and a > hard reset of the disks. That can be easily modeled by > qdev_reset_all(pci_dev). > > What we actually have at the qdev level is: > > virtio-pci ---> virtio-scsi (via ->vdev pointer) > / | \ > disk disk disk > > The soft reset of the virtio-scsi device is done by virtio_reset instead > of walking the qdev bus, but even in this case the soft reset is > perfectly described by qdev_reset_all. > > Hence, writing zero to the status register should use qdev_reset_all. 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. > > 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. > > Paolo Fine bug call it from virtio_scsi_reset.
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index 71f4fb5..a1685f1 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -268,12 +268,10 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) case VIRTIO_PCI_QUEUE_PFN: pa = (hwaddr)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT; if (pa == 0) { - virtio_pci_stop_ioeventfd(proxy); - virtio_reset(proxy->vdev); - msix_unuse_all_vectors(&proxy->pci_dev); - } - else + qdev_reset_all(&proxy->pci_dev.qdev); + } else { virtio_queue_set_addr(vdev, vdev->queue_sel, pa); + } break; case VIRTIO_PCI_QUEUE_SEL: if (val < VIRTIO_PCI_QUEUE_MAX) @@ -285,19 +283,16 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val) } break; case VIRTIO_PCI_STATUS: - if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) { - virtio_pci_stop_ioeventfd(proxy); - } - virtio_set_status(vdev, val & 0xFF); - if (val & VIRTIO_CONFIG_S_DRIVER_OK) { - virtio_pci_start_ioeventfd(proxy); - } - if (vdev->status == 0) { - virtio_reset(proxy->vdev); - msix_unuse_all_vectors(&proxy->pci_dev); + qdev_reset_all(&proxy->pci_dev.qdev); + } else { + if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) { + virtio_pci_stop_ioeventfd(proxy); + } else { + virtio_pci_start_ioeventfd(proxy); + } } /* Linux before 2.6.34 sets the device as OK without enabling
virtio-pci devices do not perform a full reset when zero is written to the status field. While PCI-specific status is initialized, the reset does not propagate down the qdev bus hierarchy. Because of this, a virtio reset does not cancel in-flight I/O for virtio-scsi (where the cancellation is handled automatically by the SCSI devices underneath virtio-scsi-pci). The patch calls qdev_reset_all, which calls virtio_pci_reset, instead of basically inlining the contents of the latter. Reported-by: Bryan Venteicher <bryanv@daemoninthecloset.org> Cc: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/virtio-pci.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-)