by Jason Wang

[permalink] [raw]

Subject: Re: [RFC v5 2/5] virtio_ring: support creating packed ring

On 2018年05月29日 13:24, Tiwei Bie wrote:
> On Tue, May 29, 2018 at 10:49:11AM +0800, Jason Wang wrote:
>> On 2018年05月22日 16:16, Tiwei Bie wrote:
>>> This commit introduces the support for creating packed ring.
>>> All split ring specific functions are added _split suffix.
>>> Some necessary stubs for packed ring are also added.
>>>
>>> Signed-off-by: Tiwei Bie <[email protected]>
>>> ---
>>> drivers/virtio/virtio_ring.c | 801 +++++++++++++++++++++++------------
>>> include/linux/virtio_ring.h | 8 +-
>>> 2 files changed, 546 insertions(+), 263 deletions(-)
>>>
>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>> index 71458f493cf8..f5ef5f42a7cf 100644
>>> --- a/drivers/virtio/virtio_ring.c
>>> +++ b/drivers/virtio/virtio_ring.c
>>> @@ -61,11 +61,15 @@ struct vring_desc_state {
>>> struct vring_desc *indir_desc; /* Indirect descriptor, if any. */
>>> };
>>> +struct vring_desc_state_packed {
>>> + int next; /* The next desc state. */
>>> +};
>>> +
>>> struct vring_virtqueue {
>>> struct virtqueue vq;
>>> - /* Actual memory layout for this queue */
>>> - struct vring vring;
>>> + /* Is this a packed ring? */
>>> + bool packed;
>>> /* Can we use weak barriers? */
>>> bool weak_barriers;
>>> @@ -87,11 +91,39 @@ struct vring_virtqueue {
>>> /* Last used index we've seen. */
>>> u16 last_used_idx;
>>> - /* Last written value to avail->flags */
>>> - u16 avail_flags_shadow;
>>> + union {
>>> + /* Available for split ring */
>>> + struct {
>>> + /* Actual memory layout for this queue. */
>>> + struct vring vring;
>>> - /* Last written value to avail->idx in guest byte order */
>>> - u16 avail_idx_shadow;
>>> + /* Last written value to avail->flags */
>>> + u16 avail_flags_shadow;
>>> +
>>> + /* Last written value to avail->idx in
>>> + * guest byte order. */
>>> + u16 avail_idx_shadow;
>>> + };
>>> +
>>> + /* Available for packed ring */
>>> + struct {
>>> + /* Actual memory layout for this queue. */
>>> + struct vring_packed vring_packed;
>>> +
>>> + /* Driver ring wrap counter. */
>>> + u8 avail_wrap_counter;
>>> +
>>> + /* Device ring wrap counter. */
>>> + u8 used_wrap_counter;
>> How about just use boolean?
> I can change it to bool if you want.

Yes, please.

>
> [...]
>>> -static int vring_mapping_error(const struct vring_virtqueue *vq,
>>> - dma_addr_t addr)
>>> -{
>>> - if (!vring_use_dma_api(vq->vq.vdev))
>>> - return 0;
>>> -
>>> - return dma_mapping_error(vring_dma_dev(vq), addr);
>>> -}
>> It looks to me if you keep vring_mapping_error behind
>> vring_unmap_one_split(), lots of changes were unncessary.
>>
> [...]
>>> + }
>>> + /* That should have freed everything. */
>>> + BUG_ON(vq->vq.num_free != vq->vring.num);
>>> +
>>> + END_USE(vq);
>>> + return NULL;
>>> +}
>> I think the those copy-and-paste hunks could be avoided and the diff should
>> only contains renaming of the function. If yes, it would be very welcomed
>> since it requires to compare the changes verbatim otherwise.
> Michael suggested to lay out the code as:
>
> XXX_split
>
> XXX_packed
>
> XXX wrappers
>
> https://lkml.org/lkml/2018/4/13/410
>
> That's why I moved some code.

I see, then no need to change but it still looks unnecessary.

>
>>> +
>>> +/*
>>> + * The layout for the packed ring is a continuous chunk of memory
>>> + * which looks like this.
>>> + *
>>> + * struct vring_packed {
>>> + * // The actual descriptors (16 bytes each)
>>> + * struct vring_packed_desc desc[num];
>>> + *
>>> + * // Padding to the next align boundary.
>>> + * char pad[];
>>> + *
>>> + * // Driver Event Suppression
>>> + * struct vring_packed_desc_event driver;
>>> + *
>>> + * // Device Event Suppression
>>> + * struct vring_packed_desc_event device;
>>> + * };
>>> + */
>>> +static inline void vring_init_packed(struct vring_packed *vr, unsigned int num,
>>> + void *p, unsigned long align)
>>> +{
>>> + vr->num = num;
>>> + vr->desc = p;
>>> + vr->driver = (void *)(((uintptr_t)p + sizeof(struct vring_packed_desc)
>>> + * num + align - 1) & ~(align - 1));
>> If we choose not to go uapi, maybe we can use ALIGN() macro here?
> Okay.
>
>>> + vr->device = vr->driver + 1;
>>> +}
> [...]
>>> +/* Only available for split ring */
>>> const struct vring *virtqueue_get_vring(struct virtqueue *vq)
>>> {
>> A possible issue with this is:
>>
>> After commit d4674240f31f8c4289abba07d64291c6ddce51bc ("KVM: s390:
>> virtio-ccw revision 1 SET_VQ"). CCW tries to use
>> virtqueue_get_avail()/virtqueue_get_used(). Looks like a bug either here or
>> ccw code.
> Do we still need to support:
>
> include/linux/virtio.h
> /*
> * Legacy accessors -- in almost all cases, these are the wrong functions
> * to use.
> */
> static inline void *virtqueue_get_desc(struct virtqueue *vq)
> {
> return virtqueue_get_vring(vq)->desc;
> }
> static inline void *virtqueue_get_avail(struct virtqueue *vq)
> {
> return virtqueue_get_vring(vq)->avail;
> }
> static inline void *virtqueue_get_used(struct virtqueue *vq)
> {
> return virtqueue_get_vring(vq)->used;
> }
>
> in packed ring?

I think it was probably a bug in ccw, they should use e.g
virtqueue_get_desc_addr() instead.

Thanks

>
> If so, I think maybe it's better to expose them as
> symbols and implement them in virtio_ring.c.
>
> Best regards,
> Tiwei Bie

2018-05-29 06:17:13

by Jason Wang

[permalink] [raw]

Subject: Re: [RFC v5 3/5] virtio_ring: add packed ring support

On 2018年05月29日 13:11, Tiwei Bie wrote:
> On Tue, May 29, 2018 at 11:18:57AM +0800, Jason Wang wrote:
>> On 2018年05月22日 16:16, Tiwei Bie wrote:
> [...]
>>> +static void detach_buf_packed(struct vring_virtqueue *vq,
>>> + unsigned int id, void **ctx)
>>> +{
>>> + struct vring_desc_state_packed *state;
>>> + struct vring_packed_desc *desc;
>>> + unsigned int i;
>>> + int *next;
>>> +
>>> + /* Clear data ptr. */
>>> + vq->desc_state_packed[id].data = NULL;
>>> +
>>> + next = &id;
>>> + for (i = 0; i < vq->desc_state_packed[id].num; i++) {
>>> + state = &vq->desc_state_packed[*next];
>>> + vring_unmap_state_packed(vq, state);
>>> + next = &vq->desc_state_packed[*next].next;
>> Have a short discussion with Michael. It looks like the only thing we care
>> is DMA address and length here.
> The length tracked by desc_state_packed[] is also necessary
> when INDIRECT is used and the buffers are writable.
>
>> So a thought is to avoid using desc_state_packed() is vring_use_dma_api() is
>> false? Probably need another id allocator or just use desc_state_packed for
>> free id list.
> I think it's a good suggestion. Thanks!
>
> I won't track the addr/len/flags for non-indirect descs
> when vring_use_dma_api() returns false.
>
>>> + }
>>> +
>>> + vq->vq.num_free += vq->desc_state_packed[id].num;
>>> + *next = vq->free_head;
>> Using pointer seems not elegant and unnecessary here. You can just track
>> next in integer I think?
> It's possible to use integer, but will need one more var
> to track `prev` (desc_state_packed[prev].next == next in
> this case).

Yes, please do this.

>
>>> + vq->free_head = id;
>>> +
>>> + if (vq->indirect) {
>>> + u32 len;
>>> +
>>> + /* Free the indirect table, if any, now that it's unmapped. */
>>> + desc = vq->desc_state_packed[id].indir_desc;
>>> + if (!desc)
>>> + return;
>>> +
>>> + len = vq->desc_state_packed[id].len;
>>> + for (i = 0; i < len / sizeof(struct vring_packed_desc); i++)
>>> + vring_unmap_desc_packed(vq, &desc[i]);
>>> +
>>> + kfree(desc);
>>> + vq->desc_state_packed[id].indir_desc = NULL;
>>> + } else if (ctx) {
>>> + *ctx = vq->desc_state_packed[id].indir_desc;
>>> + }
>>> }
>>> static inline bool more_used_packed(const struct vring_virtqueue *vq)
>>> {
>>> - return false;
>>> + u16 last_used, flags;
>>> + u8 avail, used;
>>> +
>>> + last_used = vq->last_used_idx;
>>> + flags = virtio16_to_cpu(vq->vq.vdev,
>>> + vq->vring_packed.desc[last_used].flags);
>>> + avail = !!(flags & VRING_DESC_F_AVAIL(1));
>>> + used = !!(flags & VRING_DESC_F_USED(1));
>>> +
>>> + return avail == used && used == vq->used_wrap_counter;
>> Spec does not check avail == used? So there's probably a bug in either of
>> the two places.
>>
>> In what condition that avail != used but used == vq_used_wrap_counter? A
>> buggy device or device set the two bits in two writes?
> Currently, spec doesn't forbid devices to set the status
> bits as: avail != used but used == vq_used_wrap_counter.
> So I think driver cannot assume this won't happen.

Right, but I could not find a situation that device need to something
like this. We should either forbid this in the spec or change the
example code in the spec.

Let's see how Michael think about this.

Thanks