Messages in this thread | | | From | Eugenio Perez Martin <> | Date | Tue, 15 Nov 2022 13:04:49 +0100 | Subject | Re: [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 >
| |