Re: [PATCH v5 02/10] vhost: use SVQ element ndescs instead of opaque data for desc validation

[Jason Wang]

On Thu, Aug 4, 2022 at 1:57 PM Eugenio Perez Martin  wrote:
>
> On Thu, Aug 4, 2022 at 5:01 AM Jason Wang  wrote:
> >
> >
> > 在 2022/8/3 01:57, Eugenio Pérez 写道:
> > > Since we're going to allow SVQ to add elements without the guest's
> > > knowledge and without its own VirtQueueElement, it's easier to check if
> > > an element is a valid head checking a different thing than the
> > > VirtQueueElement.
> > >
> > > Signed-off-by: Eugenio Pérez 
> > > ---
> >
> >
> > Patch looks good to me. But I spot several other issues:
> >
> > 1) vhost_svq_add() use size_t for in_num and out_num, is this intended?
>
> Would you prefer them to be unsigned? To me size_t fits better, but
> VirtQueueElement uses unsigned anyway.

Yes, the problem I ask is because the in_num were from
VirtQueueElement which is unsigned and cast to size_t and then cast
back to unsigned.

>
> > 2) do we need to fail vhost_svq_add() if in_num + out_num == 0?
> >
>
> We can recover from it, but it's a failure of qemu code.
> - In the case of loading the state to the destination device, we
> already know the layout (it's always 1 out, 1 in).
> - In the case of forwarding buffers, there is no way to get a
> VirtQueueElement with 0 out and 0 in descriptors, due to the virtqueue
> way of working.

So I think it's better to fail the svq_add in this case.

Thanks

>
> Would you prefer to return success in this case?
>
> Thanks!
>

Re: [PATCH v5 02/10] vhost: use SVQ element ndescs instead of opaque data for desc validation

[Eugenio Perez Martin]

On Thu, Aug 4, 2022 at 5:01 AM Jason Wang  wrote:
>
>
> 在 2022/8/3 01:57, Eugenio Pérez 写道:
> > Since we're going to allow SVQ to add elements without the guest's
> > knowledge and without its own VirtQueueElement, it's easier to check if
> > an element is a valid head checking a different thing than the
> > VirtQueueElement.
> >
> > Signed-off-by: Eugenio Pérez 
> > ---
>
>
> Patch looks good to me. But I spot several other issues:
>
> 1) vhost_svq_add() use size_t for in_num and out_num, is this intended?

Would you prefer them to be unsigned? To me size_t fits better, but
VirtQueueElement uses unsigned anyway.

> 2) do we need to fail vhost_svq_add() if in_num + out_num == 0?
>

We can recover from it, but it's a failure of qemu code.
- In the case of loading the state to the destination device, we
already know the layout (it's always 1 out, 1 in).
- In the case of forwarding buffers, there is no way to get a
VirtQueueElement with 0 out and 0 in descriptors, due to the virtqueue
way of working.

Would you prefer to return success in this case?

Thanks!

Re: [PATCH v5 02/10] vhost: use SVQ element ndescs instead of opaque data for desc validation

[Jason Wang]

在 2022/8/3 01:57, Eugenio Pérez 写道:

Since we're going to allow SVQ to add elements without the guest's
knowledge and without its own VirtQueueElement, it's easier to check if
an element is a valid head checking a different thing than the
VirtQueueElement.

Signed-off-by: Eugenio Pérez 
---



Patch looks good to me. But I spot several other issues:

1) vhost_svq_add() use size_t for in_num and out_num, is this intended?
2) do we need to fail vhost_svq_add() if in_num + out_num == 0?

Thanks



  hw/virtio/vhost-shadow-virtqueue.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
b/hw/virtio/vhost-shadow-virtqueue.c
index ffd2b2c972..e6eebd0e8d 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -414,7 +414,7 @@ static VirtQueueElement 
*vhost_svq_get_buf(VhostShadowVirtqueue *svq,
  return NULL;
  }
  
-if (unlikely(!svq->desc_state[used_elem.id].elem)) {

+if (unlikely(!svq->desc_state[used_elem.id].ndescs)) {
  qemu_log_mask(LOG_GUEST_ERROR,
  "Device %s says index %u is used, but it was not available",
  svq->vdev->name, used_elem.id);
@@ -422,6 +422,7 @@ static VirtQueueElement 
*vhost_svq_get_buf(VhostShadowVirtqueue *svq,
  }
  
  num = svq->desc_state[used_elem.id].ndescs;

+svq->desc_state[used_elem.id].ndescs = 0;
  last_used_chain = vhost_svq_last_desc_of_chain(svq, num, used_elem.id);
  svq->desc_next[last_used_chain] = svq->free_head;
  svq->free_head = used_elem.id;

[PATCH v5 02/10] vhost: use SVQ element ndescs instead of opaque data for desc validation

[Eugenio Pérez]

Since we're going to allow SVQ to add elements without the guest's
knowledge and without its own VirtQueueElement, it's easier to check if
an element is a valid head checking a different thing than the
VirtQueueElement.

Signed-off-by: Eugenio Pérez 
---
 hw/virtio/vhost-shadow-virtqueue.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
b/hw/virtio/vhost-shadow-virtqueue.c
index ffd2b2c972..e6eebd0e8d 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -414,7 +414,7 @@ static VirtQueueElement 
*vhost_svq_get_buf(VhostShadowVirtqueue *svq,
 return NULL;
 }
 
-if (unlikely(!svq->desc_state[used_elem.id].elem)) {
+if (unlikely(!svq->desc_state[used_elem.id].ndescs)) {
 qemu_log_mask(LOG_GUEST_ERROR,
 "Device %s says index %u is used, but it was not available",
 svq->vdev->name, used_elem.id);
@@ -422,6 +422,7 @@ static VirtQueueElement 
*vhost_svq_get_buf(VhostShadowVirtqueue *svq,
 }
 
 num = svq->desc_state[used_elem.id].ndescs;
+svq->desc_state[used_elem.id].ndescs = 0;
 last_used_chain = vhost_svq_last_desc_of_chain(svq, num, used_elem.id);
 svq->desc_next[last_used_chain] = svq->free_head;
 svq->free_head = used_elem.id;
-- 
2.31.1