Re: [Qemu-devel] [[RFC v3 02/12] virtio: redefine structure & memory cache for packed ring

[Jason Wang <jasowang@redhat.com>]

On 2018年10月15日 15:26, Wei Xu wrote:
> On Mon, Oct 15, 2018 at 11:03:52AM +0800, Jason Wang wrote:
>>
>> On 2018年10月11日 22:08, wexu@redhat.com wrote:
>>> From: Wei Xu <wexu@redhat.com>
>>>
>>> Redefine packed ring structure according to qemu nomenclature,
>>> also supported data(event index, wrap counter, etc) are introduced.
>>>
>>> Signed-off-by: Wei Xu <wexu@redhat.com>
>>> ---
>>>   hw/virtio/virtio.c | 26 ++++++++++++++++++++++++--
>>>   1 file changed, 24 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 94f5c8e..500eecf 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -39,6 +39,13 @@ typedef struct VRingDesc
>>>       uint16_t next;
>>>   } VRingDesc;
>>> +typedef struct VRingPackedDesc {
>>> +    uint64_t addr;
>>> +    uint32_t len;
>>> +    uint16_t id;
>>> +    uint16_t flags;
>>> +} VRingPackedDesc;
>>> +
>>>   typedef struct VRingAvail
>>>   {
>>>       uint16_t flags;
>>> @@ -62,8 +69,14 @@ typedef struct VRingUsed
>>>   typedef struct VRingMemoryRegionCaches {
>>>       struct rcu_head rcu;
>>>       MemoryRegionCache desc;
>>> -    MemoryRegionCache avail;
>>> -    MemoryRegionCache used;
>>> +    union {
>>> +        MemoryRegionCache avail;
>>> +        MemoryRegionCache driver;
>>> +    };
>> Can we reuse avail and used?
> Sure.
>
>>> +    union {
>>> +        MemoryRegionCache used;
>>> +        MemoryRegionCache device;
>>> +    };
>>>   } VRingMemoryRegionCaches;
>>>   typedef struct VRing
>>> @@ -77,6 +90,11 @@ typedef struct VRing
>>>       VRingMemoryRegionCaches *caches;
>>>   } VRing;
>>> +typedef struct VRingPackedDescEvent {
>>> +    uint16_t off_wrap;
>>> +    uint16_t flags;
>>> +} VRingPackedDescEvent ;
>>> +
>>>   struct VirtQueue
>>>   {
>>>       VRing vring;
>>> @@ -87,6 +105,10 @@ struct VirtQueue
>>>       /* Last avail_idx read from VQ. */
>>>       uint16_t shadow_avail_idx;
>>> +    uint16_t event_idx;
>> Need a comment to explain this field.
> Yes, it is the unified name for interrupt which is what I want to see
> if we can reuse 'shadow' and 'used' index in current code, for Tx
> queue, it should be the 'used' index after finished sending the last
> desc. For Rx queue, it should be the 'shadow' index when no enough
> descriptors which might be a few descriptors ahead of the 'used' index,
> there are a few indexes already so this makes code a bit redundant.

If I understand your meaning correctly, you can refer vhost codes:

https://github.com/jasowang/net/commit/9b7b0d75d88d980a3c8954db0aa754b124facd0e

It maintains both last_wrap_counter and avail_wrap_counter/avail_idx. 
avail_wrap_counter/avail_idx is guaranteed to be increased 
monotonically, this is useful when we need to move avail index backwards.

Besides this, for qemu, I think you need a better name e.g 
last_avail_idx here.

Thanks

>
> Will see if I can remove this in next version, any comments?
>
> Wei
>
>
>> Thanks
>>
>>> +    bool event_wrap_counter;
>>> +    bool avail_wrap_counter;
>>> +
>>>       uint16_t used_idx;
>>>       /* Last used index value we have signalled on */
>>