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

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?

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