* [virtio-dev] queue_enable vs QueueReady @ 2020-05-28 13:06 Jason Wang 2020-05-28 14:00 ` Stefan Hajnoczi 2020-06-01 6:01 ` [virtio-dev] " Michael S. Tsirkin 0 siblings, 2 replies; 7+ messages in thread From: Jason Wang @ 2020-05-28 13:06 UTC (permalink / raw) To: Michael S. Tsirkin, Virtio-Dev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [virtio-dev] queue_enable vs QueueReady 2020-05-28 13:06 [virtio-dev] queue_enable vs QueueReady Jason Wang @ 2020-05-28 14:00 ` Stefan Hajnoczi 2020-05-29 2:57 ` Jason Wang 2020-06-01 6:01 ` [virtio-dev] " Michael S. Tsirkin 1 sibling, 1 reply; 7+ messages in thread From: Stefan Hajnoczi @ 2020-05-28 14:00 UTC (permalink / raw) To: Jason Wang; +Cc: Michael S. Tsirkin, Virtio-Dev [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [virtio-dev] queue_enable vs QueueReady 2020-05-28 14:00 ` Stefan Hajnoczi @ 2020-05-29 2:57 ` Jason Wang 0 siblings, 0 replies; 7+ messages in thread From: Jason Wang @ 2020-05-29 2:57 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: Michael S. Tsirkin, Virtio-Dev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [virtio-dev] Re: queue_enable vs QueueReady 2020-05-28 13:06 [virtio-dev] queue_enable vs QueueReady Jason Wang 2020-05-28 14:00 ` Stefan Hajnoczi @ 2020-06-01 6:01 ` Michael S. Tsirkin 2020-06-02 2:57 ` Jason Wang 1 sibling, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2020-06-01 6:01 UTC (permalink / raw) To: Jason Wang; +Cc: Virtio-Dev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [virtio-dev] Re: queue_enable vs QueueReady 2020-06-01 6:01 ` [virtio-dev] " Michael S. Tsirkin @ 2020-06-02 2:57 ` Jason Wang 2020-06-02 4:20 ` Michael S. Tsirkin 0 siblings, 1 reply; 7+ messages in thread From: Jason Wang @ 2020-06-02 2:57 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Virtio-Dev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [virtio-dev] Re: queue_enable vs QueueReady 2020-06-02 2:57 ` Jason Wang @ 2020-06-02 4:20 ` Michael S. Tsirkin 2020-06-02 6:46 ` Jason Wang 0 siblings, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2020-06-02 4:20 UTC (permalink / raw) To: Jason Wang; +Cc: Virtio-Dev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [virtio-dev] Re: queue_enable vs QueueReady 2020-06-02 4:20 ` Michael S. Tsirkin @ 2020-06-02 6:46 ` Jason Wang 0 siblings, 0 replies; 7+ messages in thread From: Jason Wang @ 2020-06-02 6:46 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Virtio-Dev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-06-02 6:46 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-28 13:06 [virtio-dev] queue_enable vs QueueReady Jason Wang 2020-05-28 14:00 ` Stefan Hajnoczi 2020-05-29 2:57 ` Jason Wang 2020-06-01 6:01 ` [virtio-dev] " Michael S. Tsirkin 2020-06-02 2:57 ` Jason Wang 2020-06-02 4:20 ` Michael S. Tsirkin 2020-06-02 6:46 ` Jason Wang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.