| Message ID | 20221028160251.268607-3-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Endianess and coding style fixes for SVQ event idx support | expand |
On 28/10/22 18:02, Eugenio Pérez wrote: > This causes errors on virtio modern devices on big endian hosts > > Fixes: 01f8beacea2a ("vhost: toggle device callbacks using used event idx") > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > hw/virtio/vhost-shadow-virtqueue.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > index 70766ea740..467099f5d9 100644 > --- a/hw/virtio/vhost-shadow-virtqueue.c > +++ b/hw/virtio/vhost-shadow-virtqueue.c > @@ -382,7 +382,7 @@ static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq) > { > if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) { > uint16_t *used_event = &svq->vring.avail->ring[svq->vring.num]; > - *used_event = svq->shadow_used_idx; > + *used_event = cpu_to_le16(svq->shadow_used_idx); This looks correct, but what about: virtio_stw_p(svq->vdev, used_event, svq->shadow_used_idx); > } else { > svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT); > }
On Sat, Oct 29, 2022 at 12:48:43AM +0200, Philippe Mathieu-Daudé wrote: > On 28/10/22 18:02, Eugenio Pérez wrote: > > This causes errors on virtio modern devices on big endian hosts > > > > Fixes: 01f8beacea2a ("vhost: toggle device callbacks using used event idx") > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > hw/virtio/vhost-shadow-virtqueue.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > > index 70766ea740..467099f5d9 100644 > > --- a/hw/virtio/vhost-shadow-virtqueue.c > > +++ b/hw/virtio/vhost-shadow-virtqueue.c > > @@ -382,7 +382,7 @@ static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq) > > { > > if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) { > > uint16_t *used_event = &svq->vring.avail->ring[svq->vring.num]; > > - *used_event = svq->shadow_used_idx; > > + *used_event = cpu_to_le16(svq->shadow_used_idx); > > This looks correct, but what about: > > virtio_stw_p(svq->vdev, used_event, svq->shadow_used_idx); Philippe thanks for review but this comment isn't all that clear. I think you meant something like: this won't handle endian-ness for legacy virtio devices on BE correctly. I think virtio_stw_p would be better. which would make sense. Yes contributors should document motivation for changes but so should reviewers. > > } else { > > svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT); > > }
On Sat, Oct 29, 2022 at 12:48 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 28/10/22 18:02, Eugenio Pérez wrote: > > This causes errors on virtio modern devices on big endian hosts > > > > Fixes: 01f8beacea2a ("vhost: toggle device callbacks using used event idx") > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > hw/virtio/vhost-shadow-virtqueue.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > > index 70766ea740..467099f5d9 100644 > > --- a/hw/virtio/vhost-shadow-virtqueue.c > > +++ b/hw/virtio/vhost-shadow-virtqueue.c > > @@ -382,7 +382,7 @@ static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq) > > { > > if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) { > > uint16_t *used_event = &svq->vring.avail->ring[svq->vring.num]; > > - *used_event = svq->shadow_used_idx; > > + *used_event = cpu_to_le16(svq->shadow_used_idx); > > This looks correct, but what about: > > virtio_stw_p(svq->vdev, used_event, svq->shadow_used_idx); > Hi Philippe, I think this has the same answer as [1], the endianness conversion from the guest to the host may not be the same as the one needed from qemu SVQ to the vdpa device. Please let me know if it is not the case. Thanks! [1] https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg06081.html > > } else { > > svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT); > > } >
On 31/10/22 09:17, Michael S. Tsirkin wrote: > On Sat, Oct 29, 2022 at 12:48:43AM +0200, Philippe Mathieu-Daudé wrote: >> On 28/10/22 18:02, Eugenio Pérez wrote: >>> This causes errors on virtio modern devices on big endian hosts >>> >>> Fixes: 01f8beacea2a ("vhost: toggle device callbacks using used event idx") >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> >>> --- >>> hw/virtio/vhost-shadow-virtqueue.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c >>> index 70766ea740..467099f5d9 100644 >>> --- a/hw/virtio/vhost-shadow-virtqueue.c >>> +++ b/hw/virtio/vhost-shadow-virtqueue.c >>> @@ -382,7 +382,7 @@ static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq) >>> { >>> if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) { >>> uint16_t *used_event = &svq->vring.avail->ring[svq->vring.num]; >>> - *used_event = svq->shadow_used_idx; >>> + *used_event = cpu_to_le16(svq->shadow_used_idx); >> >> This looks correct, but what about: >> >> virtio_stw_p(svq->vdev, used_event, svq->shadow_used_idx); > > > Philippe thanks for review but this comment isn't all that clear. > I think you meant something like: > this won't handle endian-ness for legacy virtio devices > on BE correctly. I think virtio_stw_p would be better. > > which would make sense. > > Yes contributors should document motivation for changes but so > should reviewers. Yes, fair enough :)
On Mon, Oct 31, 2022 at 09:54:34AM +0100, Eugenio Perez Martin wrote: > On Sat, Oct 29, 2022 at 12:48 AM Philippe Mathieu-Daudé > <philmd@linaro.org> wrote: > > > > On 28/10/22 18:02, Eugenio Pérez wrote: > > > This causes errors on virtio modern devices on big endian hosts > > > > > > Fixes: 01f8beacea2a ("vhost: toggle device callbacks using used event idx") > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > --- > > > hw/virtio/vhost-shadow-virtqueue.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > > > index 70766ea740..467099f5d9 100644 > > > --- a/hw/virtio/vhost-shadow-virtqueue.c > > > +++ b/hw/virtio/vhost-shadow-virtqueue.c > > > @@ -382,7 +382,7 @@ static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq) > > > { > > > if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) { > > > uint16_t *used_event = &svq->vring.avail->ring[svq->vring.num]; > > > - *used_event = svq->shadow_used_idx; > > > + *used_event = cpu_to_le16(svq->shadow_used_idx); > > > > This looks correct, but what about: > > > > virtio_stw_p(svq->vdev, used_event, svq->shadow_used_idx); > > > > Hi Philippe, > > I think this has the same answer as [1], the endianness conversion > from the guest to the host may not be the same as the one needed from > qemu SVQ to the vdpa device. Please let me know if it is not the case. > > Thanks! > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg06081.html So considering legacy, i do not belive you can make a legacy device on top of modern one using SVQ alone. So I'd say SVQ should follow virtio endian-ness, not LE. > > > } else { > > > svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT); > > > } > >
On Mon, Oct 31, 2022 at 1:33 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Oct 31, 2022 at 09:54:34AM +0100, Eugenio Perez Martin wrote: > > On Sat, Oct 29, 2022 at 12:48 AM Philippe Mathieu-Daudé > > <philmd@linaro.org> wrote: > > > > > > On 28/10/22 18:02, Eugenio Pérez wrote: > > > > This causes errors on virtio modern devices on big endian hosts > > > > > > > > Fixes: 01f8beacea2a ("vhost: toggle device callbacks using used event idx") > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > > --- > > > > hw/virtio/vhost-shadow-virtqueue.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > > > > index 70766ea740..467099f5d9 100644 > > > > --- a/hw/virtio/vhost-shadow-virtqueue.c > > > > +++ b/hw/virtio/vhost-shadow-virtqueue.c > > > > @@ -382,7 +382,7 @@ static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq) > > > > { > > > > if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) { > > > > uint16_t *used_event = &svq->vring.avail->ring[svq->vring.num]; > > > > - *used_event = svq->shadow_used_idx; > > > > + *used_event = cpu_to_le16(svq->shadow_used_idx); > > > > > > This looks correct, but what about: > > > > > > virtio_stw_p(svq->vdev, used_event, svq->shadow_used_idx); > > > > > > > Hi Philippe, > > > > I think this has the same answer as [1], the endianness conversion > > from the guest to the host may not be the same as the one needed from > > qemu SVQ to the vdpa device. Please let me know if it is not the case. > > > > Thanks! > > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg06081.html > > So considering legacy, i do not belive you can make a legacy > device on top of modern one using SVQ alone. > Right, more work is needed. For example, config r/w conversions. But it's a valid use case where SVQ helps too. > So I'd say SVQ should follow virtio endian-ness, not LE. At this moment both the device that the guest sees and the vdpa device must be modern ones to enable SVQ. So the event idx must be stored in the vring in LE. Similar access functions as virtio_ld* and virtio_st* are needed if SVQ supports legacy vdpa devices in the future. The point is that svq->shadow_avail_idx is decoupled from the guest's avail ring, event idx, etc. It will always be in the host's CPU endianness, regardless of the guest's one. And, for the moment, the event idx write must be in LE. There is a more fundamental problem about using virtio_{st,ld}* here: These read from and write to guest's memory, but neither svq->shadow_used_idx or shadow vring are in guest's memory but only in qemu's VA. To start the support of legacy vdpa devices would involve a deeper change here, since all shadow vring writes and reads are written this way. Thanks! > > > > > > } else { > > > > svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT); > > > > } > > > >
On Mon, Oct 31, 2022 at 02:02:16PM +0100, Eugenio Perez Martin wrote: > On Mon, Oct 31, 2022 at 1:33 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Oct 31, 2022 at 09:54:34AM +0100, Eugenio Perez Martin wrote: > > > On Sat, Oct 29, 2022 at 12:48 AM Philippe Mathieu-Daudé > > > <philmd@linaro.org> wrote: > > > > > > > > On 28/10/22 18:02, Eugenio Pérez wrote: > > > > > This causes errors on virtio modern devices on big endian hosts > > > > > > > > > > Fixes: 01f8beacea2a ("vhost: toggle device callbacks using used event idx") > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > > > --- > > > > > hw/virtio/vhost-shadow-virtqueue.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > > > > > index 70766ea740..467099f5d9 100644 > > > > > --- a/hw/virtio/vhost-shadow-virtqueue.c > > > > > +++ b/hw/virtio/vhost-shadow-virtqueue.c > > > > > @@ -382,7 +382,7 @@ static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq) > > > > > { > > > > > if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) { > > > > > uint16_t *used_event = &svq->vring.avail->ring[svq->vring.num]; > > > > > - *used_event = svq->shadow_used_idx; > > > > > + *used_event = cpu_to_le16(svq->shadow_used_idx); > > > > > > > > This looks correct, but what about: > > > > > > > > virtio_stw_p(svq->vdev, used_event, svq->shadow_used_idx); > > > > > > > > > > Hi Philippe, > > > > > > I think this has the same answer as [1], the endianness conversion > > > from the guest to the host may not be the same as the one needed from > > > qemu SVQ to the vdpa device. Please let me know if it is not the case. > > > > > > Thanks! > > > > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg06081.html > > > > So considering legacy, i do not belive you can make a legacy > > device on top of modern one using SVQ alone. > > > > Right, more work is needed. For example, config r/w conversions. But > it's a valid use case where SVQ helps too. I am not sure why it's valid frankly. > > So I'd say SVQ should follow virtio endian-ness, not LE. > > At this moment both the device that the guest sees and the vdpa device > must be modern ones to enable SVQ. So the event idx must be stored in > the vring in LE. Similar access functions as virtio_ld* and virtio_st* > are needed if SVQ supports legacy vdpa devices in the future. > > The point is that svq->shadow_avail_idx is decoupled from the guest's > avail ring, event idx, etc. It will always be in the host's CPU > endianness, regardless of the guest's one. And, for the moment, the > event idx write must be in LE. > > There is a more fundamental problem about using virtio_{st,ld}* here: > These read from and write to guest's memory, but neither > svq->shadow_used_idx or shadow vring are in guest's memory but only in > qemu's VA. To start the support of legacy vdpa devices would involve a > deeper change here, since all shadow vring writes and reads are > written this way. > > Thanks! Yea generally, I don't know how it can work given legacy will never attach a PASID to a VQ. But maybe given we add yet another variant of endian-ness it is time to actually use sparse tags for this stuff. > > > > > > > > > } else { > > > > > svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT); > > > > > } > > > > > >
On Mon, Oct 31, 2022 at 4:09 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Oct 31, 2022 at 02:02:16PM +0100, Eugenio Perez Martin wrote: > > On Mon, Oct 31, 2022 at 1:33 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Mon, Oct 31, 2022 at 09:54:34AM +0100, Eugenio Perez Martin wrote: > > > > On Sat, Oct 29, 2022 at 12:48 AM Philippe Mathieu-Daudé > > > > <philmd@linaro.org> wrote: > > > > > > > > > > On 28/10/22 18:02, Eugenio Pérez wrote: > > > > > > This causes errors on virtio modern devices on big endian hosts > > > > > > > > > > > > Fixes: 01f8beacea2a ("vhost: toggle device callbacks using used event idx") > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > --- > > > > > > hw/virtio/vhost-shadow-virtqueue.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > > > > > > index 70766ea740..467099f5d9 100644 > > > > > > --- a/hw/virtio/vhost-shadow-virtqueue.c > > > > > > +++ b/hw/virtio/vhost-shadow-virtqueue.c > > > > > > @@ -382,7 +382,7 @@ static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq) > > > > > > { > > > > > > if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) { > > > > > > uint16_t *used_event = &svq->vring.avail->ring[svq->vring.num]; > > > > > > - *used_event = svq->shadow_used_idx; > > > > > > + *used_event = cpu_to_le16(svq->shadow_used_idx); > > > > > > > > > > This looks correct, but what about: > > > > > > > > > > virtio_stw_p(svq->vdev, used_event, svq->shadow_used_idx); > > > > > > > > > > > > > Hi Philippe, > > > > > > > > I think this has the same answer as [1], the endianness conversion > > > > from the guest to the host may not be the same as the one needed from > > > > qemu SVQ to the vdpa device. Please let me know if it is not the case. > > > > > > > > Thanks! > > > > > > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg06081.html > > > > > > So considering legacy, i do not belive you can make a legacy > > > device on top of modern one using SVQ alone. > > > > > > > Right, more work is needed. For example, config r/w conversions. But > > it's a valid use case where SVQ helps too. > > I am not sure why it's valid frankly. > > > > So I'd say SVQ should follow virtio endian-ness, not LE. > > > > At this moment both the device that the guest sees and the vdpa device > > must be modern ones to enable SVQ. So the event idx must be stored in > > the vring in LE. Similar access functions as virtio_ld* and virtio_st* > > are needed if SVQ supports legacy vdpa devices in the future. > > > > The point is that svq->shadow_avail_idx is decoupled from the guest's > > avail ring, event idx, etc. It will always be in the host's CPU > > endianness, regardless of the guest's one. And, for the moment, the > > event idx write must be in LE. > > > > There is a more fundamental problem about using virtio_{st,ld}* here: > > These read from and write to guest's memory, but neither > > svq->shadow_used_idx or shadow vring are in guest's memory but only in > > qemu's VA. To start the support of legacy vdpa devices would involve a > > deeper change here, since all shadow vring writes and reads are > > written this way. > > > > Thanks! > > Yea generally, I don't know how it can work given legacy > will never attach a PASID to a VQ. > The conversion I tried to put here was legacy guests communicating in big endian with qemu, and then qemu communicating in little endian with modern devices. For this to work SVQ should be enabled for all the queues all the time. Then the simplest conversion function here should be cpu_to_leNN, isn't it? The only device we support here is a modern, little endian, But maybe my example just added more noise. My point is that this write and all the writes and loads added on these patches, have nothing to do with the guest's endianness. They are only from the SVQ vring to the device. And they are not forwarding the used_event of the guest, but another decoupled one that may or may not match. That's why the endianness we should take into account is not the vdev one, but only the CPU and little endian. > But maybe given we add yet another variant of endian-ness > it is time to actually use sparse tags for this stuff. > I agree with this. I can try to do a fast POC. Thanks! > > > > > > > > > > > > } else { > > > > > > svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT); > > > > > > } > > > > > > > > >
On Mon, Oct 31, 2022 at 05:05:42PM +0100, Eugenio Perez Martin wrote: > On Mon, Oct 31, 2022 at 4:09 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Oct 31, 2022 at 02:02:16PM +0100, Eugenio Perez Martin wrote: > > > On Mon, Oct 31, 2022 at 1:33 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Mon, Oct 31, 2022 at 09:54:34AM +0100, Eugenio Perez Martin wrote: > > > > > On Sat, Oct 29, 2022 at 12:48 AM Philippe Mathieu-Daudé > > > > > <philmd@linaro.org> wrote: > > > > > > > > > > > > On 28/10/22 18:02, Eugenio Pérez wrote: > > > > > > > This causes errors on virtio modern devices on big endian hosts > > > > > > > > > > > > > > Fixes: 01f8beacea2a ("vhost: toggle device callbacks using used event idx") > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > > --- > > > > > > > hw/virtio/vhost-shadow-virtqueue.c | 2 +- > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > > > > > > > index 70766ea740..467099f5d9 100644 > > > > > > > --- a/hw/virtio/vhost-shadow-virtqueue.c > > > > > > > +++ b/hw/virtio/vhost-shadow-virtqueue.c > > > > > > > @@ -382,7 +382,7 @@ static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq) > > > > > > > { > > > > > > > if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) { > > > > > > > uint16_t *used_event = &svq->vring.avail->ring[svq->vring.num]; > > > > > > > - *used_event = svq->shadow_used_idx; > > > > > > > + *used_event = cpu_to_le16(svq->shadow_used_idx); > > > > > > > > > > > > This looks correct, but what about: > > > > > > > > > > > > virtio_stw_p(svq->vdev, used_event, svq->shadow_used_idx); > > > > > > > > > > > > > > > > Hi Philippe, > > > > > > > > > > I think this has the same answer as [1], the endianness conversion > > > > > from the guest to the host may not be the same as the one needed from > > > > > qemu SVQ to the vdpa device. Please let me know if it is not the case. > > > > > > > > > > Thanks! > > > > > > > > > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg06081.html > > > > > > > > So considering legacy, i do not belive you can make a legacy > > > > device on top of modern one using SVQ alone. > > > > > > > > > > Right, more work is needed. For example, config r/w conversions. But > > > it's a valid use case where SVQ helps too. > > > > I am not sure why it's valid frankly. > > > > > > So I'd say SVQ should follow virtio endian-ness, not LE. > > > > > > At this moment both the device that the guest sees and the vdpa device > > > must be modern ones to enable SVQ. So the event idx must be stored in > > > the vring in LE. Similar access functions as virtio_ld* and virtio_st* > > > are needed if SVQ supports legacy vdpa devices in the future. > > > > > > The point is that svq->shadow_avail_idx is decoupled from the guest's > > > avail ring, event idx, etc. It will always be in the host's CPU > > > endianness, regardless of the guest's one. And, for the moment, the > > > event idx write must be in LE. > > > > > > There is a more fundamental problem about using virtio_{st,ld}* here: > > > These read from and write to guest's memory, but neither > > > svq->shadow_used_idx or shadow vring are in guest's memory but only in > > > qemu's VA. To start the support of legacy vdpa devices would involve a > > > deeper change here, since all shadow vring writes and reads are > > > written this way. > > > > > > Thanks! > > > > Yea generally, I don't know how it can work given legacy > > will never attach a PASID to a VQ. > > > > The conversion I tried to put here was legacy guests communicating in > big endian with qemu, and then qemu communicating in little endian > with modern devices. For this to work SVQ should be enabled for all > the queues all the time. Yes I got that. This won't work so easily just because e.g. network header is slightly different, so it's more than just descriptor translations even just on data path. > Then the simplest conversion function here should be cpu_to_leNN, > isn't it? The only device we support here is a modern, little endian, At the moment vdpa only properly works with modern. But really another way to support legacy is if a device has support, and to fix vdpa to support legacy. Will we ever do that? Will anyone bother? I don't know. > But maybe my example just added more noise. My point is that this > write and all the writes and loads added on these patches, have > nothing to do with the guest's endianness. Device might support programmable endian-ness. If it does, then yes we could thinkably have yet another type of endian-ness "device endian" but practically setting it to anything except guest endian is just inviting pain. > They are only from the SVQ > vring to the device. And they are not forwarding the used_event of the > guest, but another decoupled one that may or may not match. That's why > the endianness we should take into account is not the vdev one, but > only the CPU and little endian. > > > But maybe given we add yet another variant of endian-ness > > it is time to actually use sparse tags for this stuff. > > > > I agree with this. I can try to do a fast POC. > > Thanks! > > > > > > > > > > > > > > > > > } else { > > > > > > > svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT); > > > > > > > } > > > > > > > > > > > >
diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c index 70766ea740..467099f5d9 100644 --- a/hw/virtio/vhost-shadow-virtqueue.c +++ b/hw/virtio/vhost-shadow-virtqueue.c @@ -382,7 +382,7 @@ static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq) { if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) { uint16_t *used_event = &svq->vring.avail->ring[svq->vring.num]; - *used_event = svq->shadow_used_idx; + *used_event = cpu_to_le16(svq->shadow_used_idx); } else { svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT); }
This causes errors on virtio modern devices on big endian hosts Fixes: 01f8beacea2a ("vhost: toggle device callbacks using used event idx") Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- hw/virtio/vhost-shadow-virtqueue.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)