Cornelia Huck
2022-Apr-06 13:04 UTC
[PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst at redhat.com> wrote:> On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote: >> This patch implements PCI version of synchronize_vqs(). >> >> Cc: Thomas Gleixner <tglx at linutronix.de> >> Cc: Peter Zijlstra <peterz at infradead.org> >> Cc: "Paul E. McKenney" <paulmck at kernel.org> >> Cc: Marc Zyngier <maz at kernel.org> >> Signed-off-by: Jason Wang <jasowang at redhat.com> > > Please add implementations at least for ccw and mmio.I'm not sure what (if anything) can/should be done for ccw...> >> --- >> drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++ >> drivers/virtio/virtio_pci_common.h | 2 ++ >> drivers/virtio/virtio_pci_legacy.c | 1 + >> drivers/virtio/virtio_pci_modern.c | 2 ++ >> 4 files changed, 19 insertions(+) >> >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c >> index d724f676608b..b78c8bc93a97 100644 >> --- a/drivers/virtio/virtio_pci_common.c >> +++ b/drivers/virtio/virtio_pci_common.c >> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev) >> synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i)); >> } >> >> +void vp_synchronize_vqs(struct virtio_device *vdev) >> +{ >> + struct virtio_pci_device *vp_dev = to_vp_device(vdev); >> + int i; >> + >> + if (vp_dev->intx_enabled) { >> + synchronize_irq(vp_dev->pci_dev->irq); >> + return; >> + } >> + >> + for (i = 0; i < vp_dev->msix_vectors; ++i) >> + synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i)); >> +} >> +...given that this seems to synchronize threaded interrupt handlers? Halil, do you think ccw needs to do anything? (AFAICS, we only have one 'irq' for channel devices anyway, and the handler just calls the relevant callbacks directly.)>> /* the notify function used when creating a virt queue */ >> bool vp_notify(struct virtqueue *vq) >> {
Michael S. Tsirkin
2022-Apr-06 15:31 UTC
[PATCH V2 4/5] virtio-pci: implement synchronize_vqs()
On Wed, Apr 06, 2022 at 03:04:32PM +0200, Cornelia Huck wrote:> On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst at redhat.com> wrote: > > > On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote: > >> This patch implements PCI version of synchronize_vqs(). > >> > >> Cc: Thomas Gleixner <tglx at linutronix.de> > >> Cc: Peter Zijlstra <peterz at infradead.org> > >> Cc: "Paul E. McKenney" <paulmck at kernel.org> > >> Cc: Marc Zyngier <maz at kernel.org> > >> Signed-off-by: Jason Wang <jasowang at redhat.com> > > > > Please add implementations at least for ccw and mmio. > > I'm not sure what (if anything) can/should be done for ccw... > > > > >> --- > >> drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++ > >> drivers/virtio/virtio_pci_common.h | 2 ++ > >> drivers/virtio/virtio_pci_legacy.c | 1 + > >> drivers/virtio/virtio_pci_modern.c | 2 ++ > >> 4 files changed, 19 insertions(+) > >> > >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > >> index d724f676608b..b78c8bc93a97 100644 > >> --- a/drivers/virtio/virtio_pci_common.c > >> +++ b/drivers/virtio/virtio_pci_common.c > >> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev) > >> synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i)); > >> } > >> > >> +void vp_synchronize_vqs(struct virtio_device *vdev) > >> +{ > >> + struct virtio_pci_device *vp_dev = to_vp_device(vdev); > >> + int i; > >> + > >> + if (vp_dev->intx_enabled) { > >> + synchronize_irq(vp_dev->pci_dev->irq); > >> + return; > >> + } > >> + > >> + for (i = 0; i < vp_dev->msix_vectors; ++i) > >> + synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i)); > >> +} > >> + > > ...given that this seems to synchronize threaded interrupt handlers?No, any handlers at all. The point is to make sure any memory changes made prior to this op are visible to callbacks. Jason, maybe add that to the documentation?> Halil, do you think ccw needs to do anything? (AFAICS, we only have one > 'irq' for channel devices anyway, and the handler just calls the > relevant callbacks directly.)Then you need to synchronize with that.> >> /* the notify function used when creating a virt queue */ > >> bool vp_notify(struct virtqueue *vq) > >> {
On Wed, 06 Apr 2022 15:04:32 +0200 Cornelia Huck <cohuck at redhat.com> wrote:> On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst at redhat.com> wrote: > > > On Wed, Apr 06, 2022 at 04:35:37PM +0800, Jason Wang wrote: > >> This patch implements PCI version of synchronize_vqs(). > >> > >> Cc: Thomas Gleixner <tglx at linutronix.de> > >> Cc: Peter Zijlstra <peterz at infradead.org> > >> Cc: "Paul E. McKenney" <paulmck at kernel.org> > >> Cc: Marc Zyngier <maz at kernel.org> > >> Signed-off-by: Jason Wang <jasowang at redhat.com> > > > > Please add implementations at least for ccw and mmio. > > I'm not sure what (if anything) can/should be done for ccw...If nothing needs to be done I would like to have at least a comment in the code that explains why. So that somebody who reads the code doesn't wonder: why is virtio-ccw not implementing that callback.> > > > >> --- > >> drivers/virtio/virtio_pci_common.c | 14 ++++++++++++++ > >> drivers/virtio/virtio_pci_common.h | 2 ++ > >> drivers/virtio/virtio_pci_legacy.c | 1 + > >> drivers/virtio/virtio_pci_modern.c | 2 ++ > >> 4 files changed, 19 insertions(+) > >> > >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > >> index d724f676608b..b78c8bc93a97 100644 > >> --- a/drivers/virtio/virtio_pci_common.c > >> +++ b/drivers/virtio/virtio_pci_common.c > >> @@ -37,6 +37,20 @@ void vp_synchronize_vectors(struct virtio_device *vdev) > >> synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i)); > >> } > >> > >> +void vp_synchronize_vqs(struct virtio_device *vdev) > >> +{ > >> + struct virtio_pci_device *vp_dev = to_vp_device(vdev); > >> + int i; > >> + > >> + if (vp_dev->intx_enabled) { > >> + synchronize_irq(vp_dev->pci_dev->irq); > >> + return; > >> + } > >> + > >> + for (i = 0; i < vp_dev->msix_vectors; ++i) > >> + synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i)); > >> +} > >> + > > ...given that this seems to synchronize threaded interrupt handlers? > Halil, do you think ccw needs to do anything? (AFAICS, we only have one > 'irq' for channel devices anyway, and the handler just calls the > relevant callbacks directly.)Sorry I don't understand enough yet. A more verbose documentation on "virtio_synchronize_vqs - synchronize with virtqueue callbacks" would surely benefit me. It may be more than enough for a back-belt but it ain't enough for me to tell what is the callback supposed to accomplish. I will have to study this discussion and the code more thoroughly. Tentatively I side with Jason and Michael in a sense, that I don't believe virtio-ccw is safe against rough interrupts. Sorry for the late response. I intend to revisit this on Monday. If I don't please feel encouraged to ping. Regards, Halil