Re: [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring.

[Michael S. Tsirkin]

On Tue, Jun 02, 2026 at 08:04:13AM +0200, Eugenio Perez Martin wrote:
> On Tue, Jun 2, 2026 at 6:34 AM yangjiale <yangjiale133@xxxxxxx> wrote:
> >
> > When a descriptor list spans across cache lines,
> > updating the flag first can lead to a scenario where the device side
> > perceives the flag as valid, yet the corresponding address and length
> > fields remain unupdated—resulting in invalid values.
> > Therefore, the flag field must be updated last.
> >
> > Signed-off-by: yangjiale <yangjiale133@xxxxxxx>
> > ---
> >  drivers/virtio/virtio_ring.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index fbca7ce1c6bf..036b4f90d30f 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -1688,6 +1688,10 @@ static inline int virtqueue_add_packed(struct vring_virtqueue *vq,
> >                                              &addr, &len, premapped, attr))
> >                                 goto unmap_release;
> >
> > +                       desc[i].addr = cpu_to_le64(addr);
> > +                       desc[i].len = cpu_to_le32(len);
> > +                       desc[i].id = cpu_to_le16(id);
> > +
> >                         flags = cpu_to_le16(vq->packed.avail_used_flags |
> >                                     (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
> >                                     (n < out_sgs ? 0 : VRING_DESC_F_WRITE));
> > @@ -1696,10 +1700,6 @@ static inline int virtqueue_add_packed(struct vring_virtqueue *vq,
> >                         else
> >                                 desc[i].flags = flags;
> >
> > -                       desc[i].addr = cpu_to_le64(addr);
> > -                       desc[i].len = cpu_to_le32(len);
> > -                       desc[i].id = cpu_to_le16(id);
> > -
> >                         if (unlikely(vq->use_map_api)) {
> >                                 vq->packed.desc_extra[curr].addr = premapped ?
> >                                         DMA_MAPPING_ERROR : addr;
> 
> These flags are updated before the flags of the head descriptor at the
> end of the function, at "vq->packed.vring.desc[head].flags =
> head_flags", so the device should not see these. Because of that, the
> relative order between the rest of the fields of the same descriptor
> or other descriptors' fields, except for the head descriptor's flags,
> should not matter. There is a write memory barrier just before
> updating the head's flags.
> 
> Also, I don't get why the cache line matters here. Can you expand? Am
> I missing something?

Oh desc is just a local thing. ENOCOFFEE )