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

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

riastradh@ wrote:

> What I'm still not sure about -- until further thought after a lunch
> barrier at least -- is:
> 
> 1. Does a _polling loop_ require additional PREREADs?
> 
> 	PREREAD;
> 	trigger DMA;
> 	POSTREAD;
> 	while (!buf->ready) {
> 		PREREAD;	// <--- ???
> 		POSTREAD;
> 	}
> 	POSTREAD;
> 	use(buf->data);
> 
>    And, can the rule about this be phrased in terms of higher-level
>    semantics of DMA operations than details of the 68030 cache
>    implementation?

I would like to first clarify the PREREAD/POSTREAD rule for a polling
loop, because I think this is not specific to virtio.

For a DMA READ buffer, my understanding is that POSTREAD allows the CPU
to inspect the buffer.  If the device may still write to the same buffer
after that, the driver should issue PREREAD again before the next device
update may happen.

This seems to match existing drivers.  For example, src/sys/dev/ici82557.c
does this kind of sequence while polling command completion:

```
        for (i = 1000; i > 0; i--) {
                FXP_CDCONFIGSYNC(sc,
                    BUS_DMASYNC_POSTREAD | BUS_DMASYNC_POSTWRITE);
                status = le16toh(cbp->cb_status);
                FXP_CDCONFIGSYNC(sc, BUS_DMASYNC_PREREAD);
                if ((status & FXP_CB_STATUS_C) != 0)
                        break;
                DELAY(1);
        }
```

and IIRC these PREREAD ops were necessary for sgimips O2 (which had
non-COHERENT DMA).

So my current understanding is that this is an existing bus_dma ownership
handoff pattern, not a virtio-specific special case.

> 2. Does the VIRTIO_F_RING_EVENT_IDX case require PREREAD too?
> 
> --- src/sys/dev/pci/virtio.c
> +++ src/sys/dev/pci/virtio.c
> @@ -1269,6 +1269,7 @@
>  		if (sc->sc_active_features & VIRTIO_F_RING_EVENT_IDX) {
>  			vq_sync_uring_avail(sc, vq, BUS_DMASYNC_POSTREAD);
>  			t = virtio_rw16(sc, *vq->vq_avail_event) + 1;
> +			vq_sync_uring_avail(sc, vq, BUS_DMASYNC_PREREAD);
>  			if ((uint16_t) (n - t) < (uint16_t) (n - o))
>  				sc->sc_ops->kick(sc, vq->vq_index);
>  		} else {

I think this is correct.

---
Izumi Tsutsui