* [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring.
@ 2026-06-02 4:31 yangjiale
2026-06-02 6:04 ` Eugenio Perez Martin
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: yangjiale @ 2026-06-02 4:31 UTC (permalink / raw)
To: Michael S . Tsirkin
Cc: Jason Wang, Xuan Zhuo, Eugenio Pérez, virtualization,
linux-kernel, yangjiale
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@163.com>
---
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;
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring.
2026-06-02 4:31 [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring yangjiale
@ 2026-06-02 6:04 ` Eugenio Perez Martin
2026-06-02 6:59 ` Michael S. Tsirkin
` (2 more replies)
2026-06-02 6:59 ` Michael S. Tsirkin
2026-06-03 1:09 ` Xuan Zhuo
2 siblings, 3 replies; 9+ messages in thread
From: Eugenio Perez Martin @ 2026-06-02 6:04 UTC (permalink / raw)
To: yangjiale
Cc: Michael S . Tsirkin, Jason Wang, Xuan Zhuo, virtualization,
linux-kernel
On Tue, Jun 2, 2026 at 6:34 AM yangjiale <yangjiale133@163.com> 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@163.com>
> ---
> 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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring.
2026-06-02 4:31 [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring yangjiale
2026-06-02 6:04 ` Eugenio Perez Martin
@ 2026-06-02 6:59 ` Michael S. Tsirkin
2026-06-03 1:09 ` Xuan Zhuo
2 siblings, 0 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2026-06-02 6:59 UTC (permalink / raw)
To: yangjiale
Cc: Jason Wang, Xuan Zhuo, Eugenio Pérez, virtualization,
linux-kernel
On Tue, Jun 02, 2026 at 12:31:23PM +0800, yangjiale 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@163.com>
> ---
> 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;
Good catch! And presumably we need a write barrier then?
> --
> 2.25.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring.
2026-06-02 6:04 ` Eugenio Perez Martin
@ 2026-06-02 6:59 ` Michael S. Tsirkin
2026-06-02 9:21 ` Dongli Zhang
2026-06-03 1:58 ` yangjiale133
[not found] ` <5a3e06d5.103d.19e8b1863dd.Coremail.yangjiale133@163.com>
2 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2026-06-02 6:59 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: yangjiale, Jason Wang, Xuan Zhuo, virtualization, linux-kernel
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@163.com> 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@163.com>
> > ---
> > 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 )
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring.
2026-06-02 6:59 ` Michael S. Tsirkin
@ 2026-06-02 9:21 ` Dongli Zhang
0 siblings, 0 replies; 9+ messages in thread
From: Dongli Zhang @ 2026-06-02 9:21 UTC (permalink / raw)
To: Michael S. Tsirkin, Eugenio Perez Martin, yangjiale
Cc: Jason Wang, Xuan Zhuo, virtualization, linux-kernel
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 <yangjiale133@163.com> 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@163.com>
>>> ---
>>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring.
2026-06-02 4:31 [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring yangjiale
2026-06-02 6:04 ` Eugenio Perez Martin
2026-06-02 6:59 ` Michael S. Tsirkin
@ 2026-06-03 1:09 ` Xuan Zhuo
2 siblings, 0 replies; 9+ messages in thread
From: Xuan Zhuo @ 2026-06-03 1:09 UTC (permalink / raw)
To: yangjiale
Cc: Jason Wang, Eugenio Pérez, virtualization, linux-kernel,
yangjiale, Michael S . Tsirkin
On Tue, 2 Jun 2026 12:31:23 +0800, yangjiale <yangjiale133@163.com> 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.
Are you raising this based on your code review, or did you actually encounter an
issue during testing?
I don't think there is a problem here, as all operations are protected by the
head desc flag. Even if cacheline effects cause data to be written prematurely,
it shouldn't be an issue. The device must wait until the head desc flags
are updated before it can start any subsequent work.
Thanks.
>
> Signed-off-by: yangjiale <yangjiale133@163.com>
> ---
> 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;
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re:Re: [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring.
2026-06-02 6:04 ` Eugenio Perez Martin
2026-06-02 6:59 ` Michael S. Tsirkin
@ 2026-06-03 1:58 ` yangjiale133
2026-06-03 2:08 ` Xuan Zhuo
[not found] ` <5a3e06d5.103d.19e8b1863dd.Coremail.yangjiale133@163.com>
2 siblings, 1 reply; 9+ messages in thread
From: yangjiale133 @ 2026-06-03 1:58 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: Michael S . Tsirkin, Jason Wang, Xuan Zhuo,
virtualization@lists.linux.dev, linux-kernel@vger.kernel.org
From the device's perspective, during a single read of the descriptor
list-and that list spans across cache lines.
the retrieved data will show `desc[head].flags` as valid,
and `desc[i].flags` as valid as well; however,
the `desc[i].addr` and `len` fields may be invalid.
I suspect this occurs because a single DMA operation is internally
split into multiple parallel read transactions,
aligned to cache line boundaries.
I apologize that I currently lack the necessary environment to verify
whether this modification definitively resolves the issue or
merely reduces the probability of its occurrence;
therefore, this patch can be discarded.
yangjiale
At 2026-06-02 14:04:13, "Eugenio Perez Martin" <eperezma@redhat.com> wrote:
>On Tue, Jun 2, 2026 at 6:34 AM yangjiale <yangjiale133@163.com> 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@163.com>
>> ---
>> 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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re:Re: [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring.
2026-06-03 1:58 ` yangjiale133
@ 2026-06-03 2:08 ` Xuan Zhuo
0 siblings, 0 replies; 9+ messages in thread
From: Xuan Zhuo @ 2026-06-03 2:08 UTC (permalink / raw)
To: yangjiale133
Cc: Michael S . Tsirkin, Jason Wang, virtualization@lists.linux.dev,
linux-kernel@vger.kernel.org, Eugenio Perez Martin
On Wed, 3 Jun 2026 09:58:56 +0800 (CST), "yangjiale133" <yangjiale133@163.com> wrote:
> From the device's perspective, during a single read of the descriptor
> list-and that list spans across cache lines.
> the retrieved data will show `desc[head].flags` as valid,
> and `desc[i].flags` as valid as well; however,
> the `desc[i].addr` and `len` fields may be invalid.
>
> I suspect this occurs because a single DMA operation is internally
> split into multiple parallel read transactions,
> aligned to cache line boundaries.
I am aware of one case. When reading, the device reads multiple descriptors
at once. If the head descriptor's flag is 'avail', it then checks the data of
the subsequent descriptors from that same batch. However, this does not comply
with the specification. The device should only read the subsequent descriptors
after the head descriptor's flag is confirmed to be 'avail'. Although this might
impact performance, it is the correct behavior.
Thanks
>
> I apologize that I currently lack the necessary environment to verify
> whether this modification definitively resolves the issue or
> merely reduces the probability of its occurrence;
> therefore, this patch can be discarded.
>
> yangjiale
>
>
>
> At 2026-06-02 14:04:13, "Eugenio Perez Martin" <eperezma@redhat.com> wrote:
> >On Tue, Jun 2, 2026 at 6:34 AM yangjiale <yangjiale133@163.com> 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@163.com>
> >> ---
> >> 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?
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring.
[not found] ` <5a3e06d5.103d.19e8b1863dd.Coremail.yangjiale133@163.com>
@ 2026-06-03 5:10 ` Michael S. Tsirkin
0 siblings, 0 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2026-06-03 5:10 UTC (permalink / raw)
To: yangjiale133
Cc: Eugenio Perez Martin, Jason Wang, Xuan Zhuo, virtualization,
linux-kernel
On Wed, Jun 03, 2026 at 09:28:11AM +0800, yangjiale133 wrote:
> From the device's perspective, during a single read of the descriptor list
> specifically when that list spans across cache lines.
> the retrieved data will show `desc[head].flags` as valid,
> and `desc[i].flags` as valid as well; however,
> the `desc[i].addr` and `len` fields may be invalid.
>
> I apologize that I currently lack the necessary environment to verify
> whether this modification definitively resolves the issue or
> merely reduces the probability of its occurrence;
> therefore, this patch can be discarded.
>
> yangjiale
it could be that your device does a single read of the descriptor.
the pci spec is explicit that after getting valid flags,
device must read the descriptor again.
>
> At 2026-06-02 14:04:13, "Eugenio Perez Martin" <eperezma@redhat.com> wrote:
> >On Tue, Jun 2, 2026 at 6:34 AM yangjiale <yangjiale133@163.com> 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@163.com>
> >> ---
> >> 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?
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-06-03 5:10 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-02 4:31 [PATCH] VIRTIO: Update the desc 'flag' fied last in packed ring yangjiale
2026-06-02 6:04 ` Eugenio Perez Martin
2026-06-02 6:59 ` Michael S. Tsirkin
2026-06-02 9:21 ` Dongli Zhang
2026-06-03 1:58 ` yangjiale133
2026-06-03 2:08 ` Xuan Zhuo
[not found] ` <5a3e06d5.103d.19e8b1863dd.Coremail.yangjiale133@163.com>
2026-06-03 5:10 ` Michael S. Tsirkin
2026-06-02 6:59 ` Michael S. Tsirkin
2026-06-03 1:09 ` Xuan Zhuo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox