Re: [PATCH v5 02/10] vhost: use SVQ element ndescs instead of opaque data for desc validation
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
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
在 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;