Hi,
On Friday, July 26, 2024 7:58:49 PM GMT+5:30 Eugenio Perez Martin wrote:
> On Fri, Jul 26, 2024 at 11:59 AM Sahil Siddiq <icegambi...@gmail.com> wrote:
> > [...]
> > @@ -759,19 +780,34 @@ void vhost_svq_start(VhostShadowVirtqueue *svq,
> > VirtIODevice *vdev,>
> > svq->vq = vq;
> > svq->iova_tree = iova_tree;
> >
> > - svq->vring.num = virtio_queue_get_num(vdev,
> > virtio_get_queue_index(vq)); - svq->num_free = svq->vring.num;
> > - svq->vring.desc = mmap(NULL, vhost_svq_driver_area_size(svq),
> > - PROT_READ | PROT_WRITE, MAP_SHARED |
> > MAP_ANONYMOUS, - -1, 0);
> > - desc_size = sizeof(vring_desc_t) * svq->vring.num;
> > - svq->vring.avail = (void *)((char *)svq->vring.desc + desc_size);
> > - svq->vring.used = mmap(NULL, vhost_svq_device_area_size(svq),
> > - PROT_READ | PROT_WRITE, MAP_SHARED |
> > MAP_ANONYMOUS, - -1, 0);
> > - svq->desc_state = g_new0(SVQDescState, svq->vring.num);
> > - svq->desc_next = g_new0(uint16_t, svq->vring.num);
> > - for (unsigned i = 0; i < svq->vring.num - 1; i++) {
> > + if (virtio_vdev_has_feature(svq->vdev, VIRTIO_F_RING_PACKED)) {
> > + svq->is_packed = true;
> > + svq->vring_packed.vring.num = virtio_queue_get_num(vdev,
> > virtio_get_queue_index(vq)); + svq->num_free =
> > svq->vring_packed.vring.num;
> > + svq->vring_packed.vring.desc = mmap(NULL,
> > vhost_svq_memory_packed(svq), +
> > PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, +
> > -1, 0);
> > + desc_size = sizeof(struct vring_packed_desc) * svq->vring.num;
> > + svq->vring_packed.vring.driver = (void *)((char
> > *)svq->vring_packed.vring.desc + desc_size);
> (Expanding on the cover letter comment) Here the driver area is
> aligned properly too as each descriptor is 16 bytes length, and
> required alignment for the driver area is 4 bytes by VirtIO standard.
Ok, this makes sense now.
> > + svq->vring_packed.vring.device = (void *)((char *)svq-
>vring_packed.vring.driver +
> > + sizeof(struct
> > vring_packed_desc_event)); + } else {
> > + svq->is_packed = false;
> > + svq->vring.num = virtio_queue_get_num(vdev,
> > virtio_get_queue_index(vq));
> > + svq->num_free = svq->vring.num;
>
> Nitpicks,
>
> The variables is_packed and num_free can be merged out of the if/else
> to reduce code duplication.
>
> Also it is clearer to me to assign svq->is_packed =
> virtio_vdev_has_feature(...) and then check the variable in the if.
> But other parts of QEMU do as you do here so I don't have a strong
> opinion.
I think your suggestion will reduce code duplication. I'll change this.
> > [...]
> > @@ -146,10 +149,13 @@ size_t vhost_svq_poll(VhostShadowVirtqueue *svq,
> > size_t num);
> >
> > void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd);
> > void vhost_svq_set_svq_call_fd(VhostShadowVirtqueue *svq, int call_fd);
> > -void vhost_svq_get_vring_addr(const VhostShadowVirtqueue *svq,
> > - struct vhost_vring_addr *addr);
> > +void vhost_svq_get_vring_addr_split(const VhostShadowVirtqueue *svq,
> > + struct vhost_vring_addr *addr);
> > +void vhost_svq_get_vring_addr_packed(const VhostShadowVirtqueue *svq,
> > + struct vhost_vring_addr *addr);
> >
> > size_t vhost_svq_driver_area_size(const VhostShadowVirtqueue *svq);
> > size_t vhost_svq_device_area_size(const VhostShadowVirtqueue *svq);
> >
> > +size_t vhost_svq_memory_packed(const VhostShadowVirtqueue *svq);
>
> Ok now I get the question on the cover letter better,
>
> It is ok to reuse the already present functions, no need to create new
> ones to export in this header.
I think "vhost_svq_memory_packed" will be required while the others can be
removed.
Thanks,
Sahil
