Re: kern/60144: virtio(4) cache coherence issue

[Tetsuya Isaki <isaki%pastel-flower.jp@localhost>]

At Thu, 2 Apr 2026 05:42:10 -0700,
Jason Thorpe wrote:
> If thatʼs the case, then Iʼd be concerned that thereʼs some other
> issue in the virtio code that could cause a problem on 68040 as
> well, because what it indicates is that, after the cache was
> invalidated, something accessed the relevant memory (or memory
> adjacent enough to the relevant memory) that caused a cache line
> fill to take place, thus putting data thatʼs about to be stale back
> into the cache.

Thank you for your suggestion.
I found it.

sys/dev/pci/virtio.c:
 1221 int
 1222 virtio_enqueue_commit(struct virtio_softc *sc, struct virtqueue *vq, int
 1222  slot,
 1223     bool notifynow)
 1224 {
  :
 1247         /*
 1248          * Prepare for `device->CPU' (host->guest) transfer
 1249          * into the buffer.  This must happen before we commit
 1250          * the vq->vq_avail->idx update to ensure we're not
 1251          * still using the buffer in case program-prior loads
 1252          * or stores in it get delayed past the store to
 1253          * vq->vq_avail->idx.
 1254          */
 1255         vq_sync_uring_all(sc, vq, BUS_DMASYNC_PREREAD);

   ^^-- Issues PREREAD.  On 68030/-current, it invalidates the data cache
        (containing vq->vq_used at least).

 1256
 1257         /* ensure payload is published, then avail idx */
 1258         vq_sync_aring_payload(sc, vq, BUS_DMASYNC_PREWRITE);
 1259         vq->vq_avail->idx = virtio_rw16(sc, vq->vq_avail_idx);
 1260         vq_sync_aring_header(sc, vq, BUS_DMASYNC_PREWRITE);
 1261         vq->vq_queued++;
 1262
 1263         /*
 1264          * Ensure we publish the avail idx _before_ we check whether
 1265          * the host needs to notified.
 1266          */
 1267         paravirt_membar_sync();
 1268
 1269         if (sc->sc_active_features & VIRTIO_F_RING_EVENT_IDX) {
 1270             vq_sync_uring_avail(sc, vq, BUS_DMASYNC_POSTREAD);
 1271             t = virtio_rw16(sc, *vq->vq_avail_event) + 1;
 1272             if ((uint16_t) (n - t) < (uint16_t) (n - o))
 1273                 sc->sc_ops->kick(sc, vq->vq_index);
 1274         } else {
 1275             vq_sync_uring_header(sc, vq, BUS_DMASYNC_POSTREAD);
 1276             flags = virtio_rw16(sc, vq->vq_used->flags);

   ^^-- Reads vq->vq_used region again(!!)
        See text below.

 1277             if (!(flags & VRING_USED_F_NO_NOTIFY))
 1278                 sc->sc_ops->kick(sc, vq->vq_index);

   ^^-- And then starts the virtio HW process.

 1279         }
 1280     }
 1281     mutex_exit(&vq->vq_aring_lock);
 1282
 1283     return 0;
 1284 }
 

vq->vq_used is shared region between the driver(CPU) and the device(HW),
and is defined as:

  struct vring_used {
        uint16_t flags;
        uint16_t idx;
        struct vring_used_elem ring[];
	/* trailed by uint16_t avail_event when VIRTIO_F_RING_EVENT_IDX */
  } __packed;

Reading vq->vq_used->flags here will also load the adjacent
vq->vq_used->idx into the cache (at least on the 68030 cache system).
This cached idx may still remain in the cache in the next interrupt
handler even though the virtio device has already updated idx in memory.
---
Tetsuya Isaki <isaki%pastel-flower.jp@localhost / isaki%NetBSD.org@localhost>