On Tue, 29 Jan 2019 at 12:26, Michael S. Tsirkin <m...@redhat.com> wrote: > > On Tue, Jan 29, 2019 at 12:11:55PM +0800, Stefan Hajnoczi wrote: > > On Tue, Jan 22, 2019 at 04:31:47PM +0800, elohi...@gmail.com wrote: > > > +typedef struct DescState { > > > + uint8_t inuse; > > > + uint8_t version; > > > + uint16_t used_idx; > > > + uint16_t avail_idx; > > > + uint16_t reserved; > > > +} DescState; > > > + > > > +typedef struct QueueRegion { > > > + uint8_t valid; > > what's this? >
We can use this to check whether this buffer is reset by qemu. > > > + uint16_t desc_num; > > there's padding before this field. Pls make it explicit. > Will do it. > > > + DescState desc[0]; > > > +} QueueRegion; > > > + > > > +The struct DescState is used to describe one head-descriptor's state. The > > > +fields have following meanings: > > > + > > > + inuse: Indicate whether the descriptor is inuse or not. > > inuse by what? > Maybe inflight is better? > > > + > > > + version: Indicate whether we have an atomic update to used ring and > > > + inflight buffer when slave crash at that point. This field should be > > > + increased by one before and after this two updates. An odd version > > > + indicates an in-progress update. > > I'm not sure I understand what does the above say. Also does this > require two atomics? Seems pretty expensive. And why is it called > version? > > > > + > > > + used_idx: Store old index of used ring before we update used ring and > > > + inflight buffer so that slave can know whether an odd version > > > inflight > > > + head-descriptor in inflight buffer is processed or not. > > Here too. > Sorry, the above description may be not clear. This two fields are used to indicate whether we have an in-progress update to used ring and inflight buffer. If slave crash before the update to used_ring and after the update to inflight buffer, the version should be odd and used_idx should be equal to used_ring.idx. Then we need to roll back the update to inflight buffer. As for the name of the version filed, actually I didn't find a good one, so I just copy it from struct kvm_steal_time... > > > + > > > + avail_idx: Used to preserve the descriptor's order in avail ring so > > > that > > > + slave can resubmit descriptors in order. > > Why would that be necessary? > Maybe some devices will be able to use it to preserve order after reconnecting in future? > > > > Will a completely new "packed vring" inflight shm layout be necessary to > > support the packed vring layout in VIRTIO 1.1? > > > > https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-610007 > > Probably. > How about supporting packed virtqueue in guest driver? Thanks, Yongji