lkml.org 
[lkml]   [2022]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] vdpa_sim: fix vringh initialization in vdpasim_queue_ready()
On Mon, Nov 14, 2022 at 4:11 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Mon, Nov 14, 2022 at 10:13:51AM +0100, Eugenio Perez Martin wrote:
> >On Fri, Nov 11, 2022 at 5:30 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> On Fri, Nov 11, 2022 at 04:40:33PM +0100, Eugenio Perez Martin wrote:
> >> >On Thu, Nov 10, 2022 at 3:13 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >> >>
> >> >> When we initialize vringh, we should pass the features and the
> >> >> number of elements in the virtqueue negotiated with the driver,
> >> >> otherwise operations with vringh may fail.
> >> >>
> >> >> This was discovered in a case where the driver sets a number of
> >> >> elements in the virtqueue different from the value returned by
> >> >> .get_vq_num_max().
> >> >>
> >> >> In vdpasim_vq_reset() is safe to initialize the vringh with
> >> >> default values, since the virtqueue will not be used until
> >> >> vdpasim_queue_ready() is called again.
> >> >>
> >> >> Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator")
> >> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >> >> ---
> >> >> drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 +--
> >> >> 1 file changed, 1 insertion(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >> >> index b071f0d842fb..b20689f8fe89 100644
> >> >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >> >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >> >> @@ -67,8 +67,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
> >> >> {
> >> >> struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
> >> >>
> >> >> - vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features,
> >> >> - VDPASIM_QUEUE_MAX, false,
> >> >> + vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false,
> >> >> (struct vring_desc *)(uintptr_t)vq->desc_addr,
> >> >> (struct vring_avail *)
> >> >> (uintptr_t)vq->driver_addr,
> >> >> --
> >> >> 2.38.1
> >> >>
> >> >
> >> >I think this is definitely an improvement, but I'd say we should go a
> >> >step further and rename VDPASIM_QUEUE_MAX to VDPASIM_QUEUE_DEFAULT. As
> >> >you point out in the patch message it is not a max anymore.
> >>
> >> I'm not sure about renaming since it is the value returned by
> >> vdpasim_get_vq_num_max, so IMHO the _MAX suffix is fine.
> >
> >Oh that's a very good point. But then I guess a conformant driver
> >should never set more descriptors than that.
>
> Yep, right!
>
> >
> >Would it be convenient to make the default queue size of 32768 and let
>
> Yep, I think it makes sense.
>
> >the guest specify less descriptors than that? Default configuration
> >will consume more memory then.
>
> Do you mean for the driver point of view?
>
> Because IIUC in vringh we don't allocate anything related to the queue
> size.
>

Right, I mean the driver that does not override the vring size will
start allocating bigger vrings by default. But I don't think that's a
problem actually, given that it is the simulator, just pointing it
out.

> >
> >> But I admit that initially I didn't understand whether it's the maximum
> >> number of queues or elements, so maybe VDPASIM_VQ_NUM_MAX is better.
> >>
> >> >
> >> >Another thing to note is that we don't have a way to report that
> >> >userspace indicated a bad value for queue length. With the current
> >> >code vringh will not initialize at all if I'm not wrong, so we should
> >> >prevent userspace to put a bad num.
> >>
> >> Right!
> >>
> >> >
> >> >Ideally, we should repeat the tests of vring_init_kern at
> >> >vdpasim_set_vq_num. We could either call it with NULL vring addresses
> >> >to check for -EINVAL, or simply repeat the conditional (!num || num >
> >> >0xffff || (num & (num - 1))). I'd say the first one is better to not
> >> >go out of sync.
> >>
> >> Or we could do the check in vdpasim_set_vq_ready() and set it not ready
> >> if the vq_num is wrong.
> >>
> >
> >Maybe it is the right place to do it, but the device is initiated at
> >that point so the driver needs to perform a full reset.
> >
>
> Yes, but the driver is misbehaving, so it might be okay to request a
> full reset.
>

Setting DEVICE_NEEDS_RESET in that case, right?

Thanks!

> >As a reference, qemu will retain the last valid size set to a vq, or
> >the default. This is because it ignores the bad values systematically.
> >Not sure what is more conformant actually :).
> >
>
> Me too :-)
>
> Thanks,
> Stefano
>

\
 
 \ /
  Last update: 2022-11-15 13:07    [W:0.047 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site