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

2024-03-12 Thread Jonah Palmer 




On 3/12/24 10:58 AM, Michael S. Tsirkin wrote:

On Tue, Mar 12, 2024 at 10:33:51AM -0400, Jonah Palmer wrote:



On 3/11/24 11:47 AM, Michael S. Tsirkin wrote:

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

ZjQcmQRYFpfptBannerEnd

On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin  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 
Signed-off-by: Jonah Palmer 


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.



I could very well be missing something here, but I was looking to see how I
could block the device from being created (realized) given the functional
mismatch between negotiating the VIRTIO_F_NOTIFICATION_DATA feature and
ioeventfd being enabled.

However, I realized that feature negotiation only happens after the virtio
device has been realized and it's one of the last steps before the device
becomes fully operational. In other words, we don't know if the guest
(driver) also supports this feature until the feature negotiation phase,
which is after realization.

So, during realization (e.g. virtio_device_realize), we know if the virtio
device (1) intends to negotiate the VIRTIO_F_NOTIFICATION_DATA feature and
(2) has enabled ioeventfd, however, we don't know if the driver will
actually support this notification data feature.

Given this, we could block the device from being created if the device is
*intending* to use the notification data feature along with ioeventfd, but
this seems premature since we don't know if the feature will actually be
successfully negotiated.


Yes this is the option I had in mind. Many devices with this feature
do not actually work if they do not get the extra data
so they fail FEATURES_OK, anyway.




Ah, okay I see. This was the extra context I was missing.

Will do, thanks Michael!


Another option might be check this during/immediately after feature
negotiation, and then unrealize the device. However, I'm not sure if by this
point it's "too late" to unrealize it.

There's also other options like defaulting to using notification data over
ioeventfd (since a user would need to explicitly enable it, showing intent
to actually use the feature), which is what we're doing now, except we could
add some kind of warning message for the user. Another option could be
setting the device to broken. However, these options don't align with your
suggestion of removing the device completely.

Let me know how you'd like me to proceed with this. Thanks!




---
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);
}

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

2024-03-12 Thread Michael S. Tsirkin 
On Tue, Mar 12, 2024 at 10:33:51AM -0400, Jonah Palmer wrote:
> 
> 
> On 3/11/24 11:47 AM, Michael S. Tsirkin wrote:
> > 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  > > > > > 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
> > > > > > 
> > > > > > ZjQcmQRYFpfptBannerEnd
> > > > > > 
> > > > > > On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin  
> > > > > > 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 
> > > > > > > > Signed-off-by: Jonah Palmer 
> > > > > > > 
> > > > > > > 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.
> > 
> 
> I could very well be missing something here, but I was looking to see how I
> could block the device from being created (realized) given the functional
> mismatch between negotiating the VIRTIO_F_NOTIFICATION_DATA feature and
> ioeventfd being enabled.
> 
> However, I realized that feature negotiation only happens after the virtio
> device has been realized and it's one of the last steps before the device
> becomes fully operational. In other words, we don't know if the guest
> (driver) also supports this feature until the feature negotiation phase,
> which is after realization.
> 
> So, during realization (e.g. virtio_device_realize), we know if the virtio
> device (1) intends to negotiate the VIRTIO_F_NOTIFICATION_DATA feature and
> (2) has enabled ioeventfd, however, we don't know if the driver will
> actually support this notification data feature.
> 
> Given this, we could block the device from being created if the device is
> *intending* to use the notification data feature along with ioeventfd, but
> this seems premature since we don't know if the feature will actually be
> successfully negotiated.

Yes this is the option I had in mind. Many devices with this feature
do not actually work if they do not get the extra data
so they fail FEATURES_OK, anyway.


> Another option might be check this during/immediately after feature
> negotiation, and then unrealize the device. However, I'm not sure if by this
> point it's "too late" to unrealize it.
> 
> There's also other options like defaulting to using notification data over
> ioeventfd (since a user would need to explicitly enable it, showing intent
> to actually use the feature), which is what we're doing now, except we could
> add some kind of warning message for the user. Another option could be
> setting the device to broken. However, these options don't align with your
> suggestion of removing the device completely.
> 
> Let me know how you'd like me to proceed with this. Thanks!
> 
> > > > > > > 
> > > > > > > > ---

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

2024-03-12 Thread Jonah Palmer 




On 3/11/24 11:47 AM, Michael S. Tsirkin wrote:

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

ZjQcmQRYFpfptBannerEnd

On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin  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 
Signed-off-by: Jonah Palmer 


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.



I could very well be missing something here, but I was looking to see 
how I could block the device from being created (realized) given the 
functional mismatch between negotiating the VIRTIO_F_NOTIFICATION_DATA 
feature and ioeventfd being enabled.


However, I realized that feature negotiation only happens after the 
virtio device has been realized and it's one of the last steps before 
the device becomes fully operational. In other words, we don't know if 
the guest (driver) also supports this feature until the feature 
negotiation phase, which is after realization.


So, during realization (e.g. virtio_device_realize), we know if the 
virtio device (1) intends to negotiate the VIRTIO_F_NOTIFICATION_DATA 
feature and (2) has enabled ioeventfd, however, we don't know if the 
driver will actually support this notification data feature.


Given this, we could block the device from being created if the device 
is *intending* to use the notification data feature along with 
ioeventfd, but this seems premature since we don't know if the feature 
will actually be successfully negotiated.


Another option might be check this during/immediately after feature 
negotiation, and then unrealize the device. However, I'm not sure if by 
this point it's "too late" to unrealize it.


There's also other options like defaulting to using notification data 
over ioeventfd (since a user would need to explicitly enable it, showing 
intent to actually use the feature), which is what we're doing now, 
except we could add some kind of warning message for the user. Another 
option could be setting the device to broken. However, these options 
don't align with your suggestion of removing the device completely.


Let me know how you'd like me to proceed with this. Thanks!




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

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

2024-03-11 Thread 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 
> > > > 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
> > > > 
> > > > ZjQcmQRYFpfptBannerEnd
> > > > 
> > > > On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin  
> > > > 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 
> > > > > > Signed-off-by: Jonah Palmer 
> > > > > 
> > > > > 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
> > > > > 
> > > > 
> >

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

2024-03-11 Thread Jonah Palmer 




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

ZjQcmQRYFpfptBannerEnd

On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin  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 
Signed-off-by: Jonah Palmer 


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?





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

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

2024-03-08 Thread Michael S. Tsirkin 
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 
> > 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
> > 
> > ZjQcmQRYFpfptBannerEnd
> > 
> > On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin  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 
> > > > Signed-off-by: Jonah Palmer 
> > > 
> > > 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.
> > > 
> > > > ---
> > > >  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
> > > 
> >

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

2024-03-08 Thread Jonah Palmer 




On 3/8/24 12:36 PM, Eugenio Perez Martin wrote:
On Fri, Mar 8, 2024 at 6: 01 PM Michael S. Tsirkin  
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

ZjQcmQRYFpfptBannerEnd

On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin  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 
> Signed-off-by: Jonah Palmer 

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

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

2024-03-08 Thread Eugenio Perez Martin 
On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin  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 
> > Signed-off-by: Jonah Palmer 
>
> 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.

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

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

2024-03-08 Thread Michael S. Tsirkin 
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 
> Signed-off-by: Jonah Palmer 

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?


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