On 2026-06-01 11:59 PM, Michael S. Tsirkin wrote:
> On Tue, Jun 02, 2026 at 08:04:13AM +0200, Eugenio Perez Martin wrote:
>> On Tue, Jun 2, 2026 at 6:34 AM yangjiale <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 )
>
>
Whether or not this can happen in practice, I notice
virtqueue_add_packed_in_order() has the same write pattern.
Dongli Zhang
