[virtio-dev] queue_enable vs QueueReady

[virtio-dev] queue_enable vs QueueReady

[Jason Wang]

Hi:

I found ambiguity in the virtio specification:

In PCI part, it describes the queue_enable as:

The driver uses this to selectively prevent the device from executing 
requests from this virtqueue. 1 - enabled; 0 - disabled.

In MMIO part, it describes the QueueReady as:

Writing one (0x1) to this register notifies the device that it can 
execute requests from this virtual queue. Reading from this register 
returns the last value written to it. Both read and write accesses apply 
to the queue selected by writing to QueueSel.

If I understand this correctly, they have the same meaning, but the 
driver requirements section looks conflict:

PCI said: The driver MUST NOT write a 0 to queue_enable.

MMIO said:

To stop using the queue the driver MUST write zero (0x0) to this 
QueueReady and MUST read the value back to ensure synchronization.

So we can't disable a queue via queue_enable but QueueReady. Any reason 
for such inconsistency?

Thanks


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] queue_enable vs QueueReady

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1754 bytes --]

On Thu, May 28, 2020 at 09:06:36PM +0800, Jason Wang wrote:
> Hi:
> 
> I found ambiguity in the virtio specification:
> 
> In PCI part, it describes the queue_enable as:
> 
> The driver uses this to selectively prevent the device from executing
> requests from this virtqueue. 1 - enabled; 0 - disabled.
> 
> In MMIO part, it describes the QueueReady as:
> 
> Writing one (0x1) to this register notifies the device that it can execute
> requests from this virtual queue. Reading from this register returns the
> last value written to it. Both read and write accesses apply to the queue
> selected by writing to QueueSel.
> 
> If I understand this correctly, they have the same meaning, but the driver
> requirements section looks conflict:
> 
> PCI said: The driver MUST NOT write a 0 to queue_enable.
> 
> MMIO said:
> 
> To stop using the queue the driver MUST write zero (0x0) to this QueueReady
> and MUST read the value back to ensure synchronization.
> 
> So we can't disable a queue via queue_enable but QueueReady. Any reason for
> such inconsistency?

I think MMIO is the outlier here. The device emulation code in QEMU
doesn't deal with queue shutdown. That only happens during device reset,
so it's like that a if the guest disables a virtio-mmio queue then
something undefined will happen on the QEMU side. For example, writing
used elements back to the virtqueue after it has been disabled.

If the VIRTIO spec really intends to support virtqueue shutdown then the
semantics need to be spelled out clearly: what happens to in-flight
requests? How do devices behave that rely on multiple virtqueues during
normal operation (e.g. virtio-vsock where you can't really have rx-only
or tx-only)?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [virtio-dev] queue_enable vs QueueReady

[Jason Wang]

On 2020/5/28 下午10:00, Stefan Hajnoczi wrote:
> On Thu, May 28, 2020 at 09:06:36PM +0800, Jason Wang wrote:
>> Hi:
>>
>> I found ambiguity in the virtio specification:
>>
>> In PCI part, it describes the queue_enable as:
>>
>> The driver uses this to selectively prevent the device from executing
>> requests from this virtqueue. 1 - enabled; 0 - disabled.
>>
>> In MMIO part, it describes the QueueReady as:
>>
>> Writing one (0x1) to this register notifies the device that it can execute
>> requests from this virtual queue. Reading from this register returns the
>> last value written to it. Both read and write accesses apply to the queue
>> selected by writing to QueueSel.
>>
>> If I understand this correctly, they have the same meaning, but the driver
>> requirements section looks conflict:
>>
>> PCI said: The driver MUST NOT write a 0 to queue_enable.
>>
>> MMIO said:
>>
>> To stop using the queue the driver MUST write zero (0x0) to this QueueReady
>> and MUST read the value back to ensure synchronization.
>>
>> So we can't disable a queue via queue_enable but QueueReady. Any reason for
>> such inconsistency?
> I think MMIO is the outlier here. The device emulation code in QEMU
> doesn't deal with queue shutdown. That only happens during device reset,
> so it's like that a if the guest disables a virtio-mmio queue then
> something undefined will happen on the QEMU side.


Qemu's MMIO only implement the fucntion of value write and read. Its PCI 
implementation is also buggy which assumes the value is one.


>   For example, writing
> used elements back to the virtqueue after it has been disabled.


Ok, but kernel virtio-mmio driver did the following in vm_del_vq():

     /* Select and deactivate the queue */
     writel(index, vm_dev->base + VIRTIO_MMIO_QUEUE_SEL);
     if (vm_dev->version == 1) {
         writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN);
     } else {
         writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_READY);
         WARN_ON(readl(vm_dev->base + VIRTIO_MMIO_QUEUE_READY));
     }

Which tries to align with the spec ...


>
> If the VIRTIO spec really intends to support virtqueue shutdown then the
> semantics need to be spelled out clearly: what happens to in-flight
> requests? How do devices behave that rely on multiple virtqueues during
> normal operation (e.g. virtio-vsock where you can't really have rx-only
> or tx-only)?


If we plan to do this, we need do something similar to status. That 
means driver must wait for a read of queue_enable to return 0.

For in-flight request we can do the same thing as device reset but just 
for a virtqueue probably.

Thanks


>
> Stefan


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: queue_enable vs QueueReady

[Michael S. Tsirkin]

On Thu, May 28, 2020 at 09:06:36PM +0800, Jason Wang wrote:
> Hi:
> 
> I found ambiguity in the virtio specification:
> 
> In PCI part, it describes the queue_enable as:
> 
> The driver uses this to selectively prevent the device from executing
> requests from this virtqueue. 1 - enabled; 0 - disabled.
> 
> In MMIO part, it describes the QueueReady as:
> 
> Writing one (0x1) to this register notifies the device that it can execute
> requests from this virtual queue. Reading from this register returns the
> last value written to it. Both read and write accesses apply to the queue
> selected by writing to QueueSel.
> 
> If I understand this correctly, they have the same meaning, but the driver
> requirements section looks conflict:
> 
> PCI said: The driver MUST NOT write a 0 to queue_enable.
> 
> MMIO said:
> 
> To stop using the queue the driver MUST write zero (0x0) to this QueueReady
> and MUST read the value back to ensure synchronization.
> 
> So we can't disable a queue via queue_enable but QueueReady. Any reason for
> such inconsistency?
> 
> Thanks

PCI assumed device reset is enough to stop all queues.  We had tons of
bugs around shutdown because of this, so in hindsight, MMIO had maybe a
better idea.

Ability to stop a queue and take back buffers would be nice, e.g. serial
is kind of messed up around port disconnect without it.

-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: queue_enable vs QueueReady

[Jason Wang]

On 2020/6/1 下午2:01, Michael S. Tsirkin wrote:
> On Thu, May 28, 2020 at 09:06:36PM +0800, Jason Wang wrote:
>> Hi:
>>
>> I found ambiguity in the virtio specification:
>>
>> In PCI part, it describes the queue_enable as:
>>
>> The driver uses this to selectively prevent the device from executing
>> requests from this virtqueue. 1 - enabled; 0 - disabled.
>>
>> In MMIO part, it describes the QueueReady as:
>>
>> Writing one (0x1) to this register notifies the device that it can execute
>> requests from this virtual queue. Reading from this register returns the
>> last value written to it. Both read and write accesses apply to the queue
>> selected by writing to QueueSel.
>>
>> If I understand this correctly, they have the same meaning, but the driver
>> requirements section looks conflict:
>>
>> PCI said: The driver MUST NOT write a 0 to queue_enable.
>>
>> MMIO said:
>>
>> To stop using the queue the driver MUST write zero (0x0) to this QueueReady
>> and MUST read the value back to ensure synchronization.
>>
>> So we can't disable a queue via queue_enable but QueueReady. Any reason for
>> such inconsistency?
>>
>> Thanks
> PCI assumed device reset is enough to stop all queues.  We had tons of
> bugs around shutdown because of this, so in hindsight, MMIO had maybe a
> better idea.
>
> Ability to stop a queue and take back buffers would be nice, e.g. serial
> is kind of messed up around port disconnect without it.


Yes, but this inconsistency brings trouble for vDPA implementation which 
has a set_queue_ready() and we do the following in virtio_vdpa as MMIO did:

     /* Select and deactivate the queue */
     ops->set_vq_ready(vdpa, index, 0);
     WARN_ON(ops->get_vq_ready(vdpa, index));

The codes seems to work fine for IFC (a violation of the spec probably) 
but not Qemu's virtio-net-pci.

Thanks



---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: queue_enable vs QueueReady

[Michael S. Tsirkin]

On Tue, Jun 02, 2020 at 10:57:19AM +0800, Jason Wang wrote:
> 
> On 2020/6/1 下午2:01, Michael S. Tsirkin wrote:
> > On Thu, May 28, 2020 at 09:06:36PM +0800, Jason Wang wrote:
> > > Hi:
> > > 
> > > I found ambiguity in the virtio specification:
> > > 
> > > In PCI part, it describes the queue_enable as:
> > > 
> > > The driver uses this to selectively prevent the device from executing
> > > requests from this virtqueue. 1 - enabled; 0 - disabled.
> > > 
> > > In MMIO part, it describes the QueueReady as:
> > > 
> > > Writing one (0x1) to this register notifies the device that it can execute
> > > requests from this virtual queue. Reading from this register returns the
> > > last value written to it. Both read and write accesses apply to the queue
> > > selected by writing to QueueSel.
> > > 
> > > If I understand this correctly, they have the same meaning, but the driver
> > > requirements section looks conflict:
> > > 
> > > PCI said: The driver MUST NOT write a 0 to queue_enable.
> > > 
> > > MMIO said:
> > > 
> > > To stop using the queue the driver MUST write zero (0x0) to this QueueReady
> > > and MUST read the value back to ensure synchronization.
> > > 
> > > So we can't disable a queue via queue_enable but QueueReady. Any reason for
> > > such inconsistency?
> > > 
> > > Thanks
> > PCI assumed device reset is enough to stop all queues.  We had tons of
> > bugs around shutdown because of this, so in hindsight, MMIO had maybe a
> > better idea.
> > 
> > Ability to stop a queue and take back buffers would be nice, e.g. serial
> > is kind of messed up around port disconnect without it.
> 
> 
> Yes, but this inconsistency brings trouble for vDPA implementation which has
> a set_queue_ready() and we do the following in virtio_vdpa as MMIO did:
> 
>     /* Select and deactivate the queue */
>     ops->set_vq_ready(vdpa, index, 0);
>     WARN_ON(ops->get_vq_ready(vdpa, index));
> 
> The codes seems to work fine for IFC (a violation of the spec probably) but
> not Qemu's virtio-net-pci.
> 
> Thanks

What do you mean "not Qemu's virtio-net-pci"? Which implementation
of .set_vq_ready do you use?

According to spec, I would expect this callback to do something
other that writing into queue_enable.

BTW I noticed this:

        if (cmd == VHOST_VDPA_SET_VRING_ENABLE) {
                if (copy_from_user(&s, argp, sizeof(s)))
                        return -EFAULT;
                ops->set_vq_ready(vdpa, idx, s.num);
                return 0;
        }

I'm guessing s.num should be 1, right?


-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: queue_enable vs QueueReady

[Jason Wang]

On 2020/6/2 下午12:20, Michael S. Tsirkin wrote:
> On Tue, Jun 02, 2020 at 10:57:19AM +0800, Jason Wang wrote:
>> On 2020/6/1 下午2:01, Michael S. Tsirkin wrote:
>>> On Thu, May 28, 2020 at 09:06:36PM +0800, Jason Wang wrote:
>>>> Hi:
>>>>
>>>> I found ambiguity in the virtio specification:
>>>>
>>>> In PCI part, it describes the queue_enable as:
>>>>
>>>> The driver uses this to selectively prevent the device from executing
>>>> requests from this virtqueue. 1 - enabled; 0 - disabled.
>>>>
>>>> In MMIO part, it describes the QueueReady as:
>>>>
>>>> Writing one (0x1) to this register notifies the device that it can execute
>>>> requests from this virtual queue. Reading from this register returns the
>>>> last value written to it. Both read and write accesses apply to the queue
>>>> selected by writing to QueueSel.
>>>>
>>>> If I understand this correctly, they have the same meaning, but the driver
>>>> requirements section looks conflict:
>>>>
>>>> PCI said: The driver MUST NOT write a 0 to queue_enable.
>>>>
>>>> MMIO said:
>>>>
>>>> To stop using the queue the driver MUST write zero (0x0) to this QueueReady
>>>> and MUST read the value back to ensure synchronization.
>>>>
>>>> So we can't disable a queue via queue_enable but QueueReady. Any reason for
>>>> such inconsistency?
>>>>
>>>> Thanks
>>> PCI assumed device reset is enough to stop all queues.  We had tons of
>>> bugs around shutdown because of this, so in hindsight, MMIO had maybe a
>>> better idea.
>>>
>>> Ability to stop a queue and take back buffers would be nice, e.g. serial
>>> is kind of messed up around port disconnect without it.
>>
>> Yes, but this inconsistency brings trouble for vDPA implementation which has
>> a set_queue_ready() and we do the following in virtio_vdpa as MMIO did:
>>
>>      /* Select and deactivate the queue */
>>      ops->set_vq_ready(vdpa, index, 0);
>>      WARN_ON(ops->get_vq_ready(vdpa, index));
>>
>> The codes seems to work fine for IFC (a violation of the spec probably) but
>> not Qemu's virtio-net-pci.
>>
>> Thanks
> What do you mean "not Qemu's virtio-net-pci"? Which implementation
> of .set_vq_ready do you use?


I meant the queue_enable implemented in qemu virtio-net-pci.

See my patch: [PATCH] virtio-pci: fix queue_enable write


>
> According to spec, I would expect this callback to do something
> other that writing into queue_enable.
>
> BTW I noticed this:
>
>          if (cmd == VHOST_VDPA_SET_VRING_ENABLE) {
>                  if (copy_from_user(&s, argp, sizeof(s)))
>                          return -EFAULT;
>                  ops->set_vq_ready(vdpa, idx, s.num);
>                  return 0;
>          }
>
> I'm guessing s.num should be 1, right?


It was a value set by userspace, so I think we can't have this assumption.

Thanks


>


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org