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

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 <[email protected]> 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 <[email protected]>
>> > Signed-off-by: Jonah Palmer <[email protected]>
>>
>> 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?

>>
>> > ---
>> >  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
>>
>