linux-kernel - Re: [PATCH V2 4/5] virtio-pci: implement synchronize

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220411045011-mutt-send-email-mst@kernel.org>
Date:   Mon, 11 Apr 2022 04:56:33 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     Halil Pasic <pasic@...ux.ibm.com>,
        Cornelia Huck <cohuck@...hat.com>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Marc Zyngier <maz@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        virtualization <virtualization@...ts.linux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH V2 4/5] virtio-pci: implement synchronize_vqs()

On Mon, Apr 11, 2022 at 04:22:19PM +0800, Jason Wang wrote:
> On Sun, Apr 10, 2022 at 3:51 PM Michael S. Tsirkin <mst@...hat.com> wrote:
> >
> > On Fri, Apr 08, 2022 at 03:03:07PM +0200, Halil Pasic wrote:
> > > On Wed, 06 Apr 2022 15:04:32 +0200
> > > Cornelia Huck <cohuck@...hat.com> wrote:
> > >
> > > > On Wed, Apr 06 2022, "Michael S. Tsirkin" <mst@...hat.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@...utronix.de>
> > > > >> Cc: Peter Zijlstra <peterz@...radead.org>
> > > > >> Cc: "Paul E. McKenney" <paulmck@...nel.org>
> > > > >> Cc: Marc Zyngier <maz@...nel.org>
> > > > >> Signed-off-by: Jason Wang <jasowang@...hat.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.
> >
> > Right.
> >
> > I am currently thinking instead of making this optional in the
> > core we should make it mandatory, and have transports which do not
> > need to sync have an empty stub with documentation explaining why.
> >
> > Also, do we want to document this sync is explicitly for irq enable/disable?
> > synchronize_irq_enable_disable?
> 
> I would not since the transport is not guaranteed to use an interrupt
> for callbacks.

OK but let's then document this in more detail.
More readers will wonder about what is the callback
trying to accomplish, and Halil requested that as well.

For example, let's document why is sync required on enable.

> >
> >
> > > >
> > > > >
> > > > >> ---
> > > > >>  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.
> 
> That's my feeling as well.
> 
> Thanks
> 
> > >
> > > Sorry for the late response. I intend to revisit this on Monday. If
> > > I don't please feel encouraged to ping.
> > >
> > > Regards,
> > > Halil
> >