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

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