Re: [PATCH v1 2/8] virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

[Michael S. Tsirkin]

On Mon, Mar 11, 2024 at 10:53:25AM -0400, Jonah Palmer wrote:
> 
> 
> On 3/8/24 2:19 PM, Michael S. Tsirkin wrote:
> > On Fri, Mar 08, 2024 at 12:45:13PM -0500, Jonah Palmer wrote:
> > > 
> > > 
> > > On 3/8/24 12:36 PM, Eugenio Perez Martin wrote:
> > > > On Fri, Mar 8, 2024 at 6: 01 PM Michael S. Tsirkin <mst@ redhat. com>
> > > > wrote: > > On Mon, Mar 04, 2024 at 02: 46: 06PM -0500, Jonah Palmer
> > > > wrote: > > Prevent ioeventfd from being enabled/disabled when a
> > > > virtio-pci > > device
> > > > ZjQcmQRYFpfptBannerStart
> > > > This Message Is From an External Sender
> > > > This message came from outside your organization.
> > > > Report Suspicious
> > > > <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/ACWV5N9M2RV99hQ!Op20OCZE8kFi__wOXJ_Z0URZ2e_9fdaYz2tejZvKqiDgOm6ijq_imUptzxsrej_4riwCrBGeKmQ9VKXqnbV1ujbfiOV5-E2e1s3pKqpqUL-gRIuMQLDLygRD1hoX3Q$>
> > > > ZjQcmQRYFpfptBannerEnd
> > > > 
> > > > On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> 
> > > > wrote:
> > > > > 
> > > > > On Mon, Mar 04, 2024 at 02:46:06PM -0500, Jonah Palmer wrote:
> > > > > > Prevent ioeventfd from being enabled/disabled when a virtio-pci
> > > > > > device has negotiated the VIRTIO_F_NOTIFICATION_DATA transport
> > > > > > feature.
> > > > > > 
> > > > > > Due to ioeventfd not being able to carry the extra data associated 
> > > > > > with
> > > > > > this feature, the ioeventfd should be left in a disabled state for
> > > > > > emulated virtio-pci devices using this feature.
> > > > > > 
> > > > > > Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> > > > > 
> > > > > I thought hard about this. I propose that for now,
> > > > > instead of disabling ioevetfd silently we error out unless
> > > > > user disabled it for us.
> > > > > WDYT?
> > > > > 
> > > > 
> > > > Yes, error is a better plan than silently disabling it. In the
> > > > (unlikely?) case we are able to make notification data work with
> > > > eventfd in the future, it makes the change more evident.
> > > > 
> > > 
> > > Will do in v2. I assume we'll also make this the case for virtio-mmio and
> > > virtio-ccw?
> > 
> > Guess so. Pls note freeze is imminent.
> 
> Got it. Also, would you mind elaborating a bit more on "error out"? E.g. do
> we want to prevent the Qemu from starting at all if a device is attempting
> to use both VIRTIO_F_NOTIFICATION_DATA and ioeventfd? Or do you mean
> something like still keep ioeventfd disabled but also log an error message
> unless it was explicitly disabled by the user?


my preference would be to block device instance from being created.

> > > > > 
> > > > > > ---
> > > > > >   hw/virtio/virtio-pci.c | 6 ++++--
> > > > > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > > > > index d12edc567f..287b8f7720 100644
> > > > > > --- a/hw/virtio/virtio-pci.c
> > > > > > +++ b/hw/virtio/virtio-pci.c
> > > > > > @@ -417,13 +417,15 @@ 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)) {
> > > > > > +        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > > > > > +            !virtio_vdev_has_feature(vdev, 
> > > > > > VIRTIO_F_NOTIFICATION_DATA)) {
> > > > > >               virtio_pci_stop_ioeventfd(proxy);
> > > > > >           }
> > > > > > 
> > > > > >           virtio_set_status(vdev, val & 0xFF);
> > > > > > 
> > > > > > -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
> > > > > > +        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > > > > > +            !virtio_vdev_has_feature(vdev, 
> > > > > > VIRTIO_F_NOTIFICATION_DATA)) {
> > > > > >               virtio_pci_start_ioeventfd(proxy);
> > > > > >           }
> > > > > > 
> > > > > > --
> > > > > > 2.39.3
> > > > > 
> > > > 
> >