Re: [RFC v2 3/3] vhost: Allocate memory for packed vring.

[Sahil <icegambit91@gmail.com>]

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 <icegambit91@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