When calling vringh_init_iotlb(), use the negotiated features which
might be different than the supported features.
Fixes: 2c53d0f64c06f ("vdpasim: vDPA device simulator")
Signed-off-by: Eli Cohen <[email protected]>
---
v0 --> v1:
Update "Fixes" line
drivers/vdpa/vdpa_sim/vdpa_sim.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 14e024de5cbf..89a474c7a096 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -66,7 +66,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,
+ vringh_init_iotlb(&vq->vring, vdpasim->features,
VDPASIM_QUEUE_MAX, false,
(struct vring_desc *)(uintptr_t)vq->desc_addr,
(struct vring_avail *)
@@ -86,7 +86,7 @@ static void vdpasim_vq_reset(struct vdpasim *vdpasim,
vq->device_addr = 0;
vq->cb = NULL;
vq->private = NULL;
- vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features,
+ vringh_init_iotlb(&vq->vring, vdpasim->features,
VDPASIM_QUEUE_MAX, false, NULL, NULL, NULL);
vq->vring.notify = NULL;
--
2.30.1
On Tue, Jul 20, 2021 at 08:25:33AM +0300, Eli Cohen wrote:
>When calling vringh_init_iotlb(), use the negotiated features which
>might be different than the supported features.
>
>Fixes: 2c53d0f64c06f ("vdpasim: vDPA device simulator")
>Signed-off-by: Eli Cohen <[email protected]>
>---
>v0 --> v1:
>Update "Fixes" line
>
> drivers/vdpa/vdpa_sim/vdpa_sim.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>index 14e024de5cbf..89a474c7a096 100644
>--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
>+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>@@ -66,7 +66,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,
>+ vringh_init_iotlb(&vq->vring, vdpasim->features,
> VDPASIM_QUEUE_MAX, false,
> (struct vring_desc *)(uintptr_t)vq->desc_addr,
> (struct vring_avail *)
>@@ -86,7 +86,7 @@ static void vdpasim_vq_reset(struct vdpasim *vdpasim,
> vq->device_addr = 0;
> vq->cb = NULL;
> vq->private = NULL;
>- vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features,
>+ vringh_init_iotlb(&vq->vring, vdpasim->features,
vdpasim_vq_reset() is called while resetting the device in
vdpasim_reset() where we also set `vdpasim->features = 0` after
resetting the vqs, so maybe it's better to use the supported features
here, since the negotiated ones are related to the previous instance.
Thanks,
Stefano
On Tue, Jul 20, 2021 at 08:57:40AM +0200, Stefano Garzarella wrote:
> On Tue, Jul 20, 2021 at 08:25:33AM +0300, Eli Cohen wrote:
> > When calling vringh_init_iotlb(), use the negotiated features which
> > might be different than the supported features.
> >
> > Fixes: 2c53d0f64c06f ("vdpasim: vDPA device simulator")
> > Signed-off-by: Eli Cohen <[email protected]>
> > ---
> > v0 --> v1:
> > Update "Fixes" line
> >
> > drivers/vdpa/vdpa_sim/vdpa_sim.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > index 14e024de5cbf..89a474c7a096 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > @@ -66,7 +66,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,
> > + vringh_init_iotlb(&vq->vring, vdpasim->features,
> > VDPASIM_QUEUE_MAX, false,
> > (struct vring_desc *)(uintptr_t)vq->desc_addr,
> > (struct vring_avail *)
> > @@ -86,7 +86,7 @@ static void vdpasim_vq_reset(struct vdpasim *vdpasim,
> > vq->device_addr = 0;
> > vq->cb = NULL;
> > vq->private = NULL;
> > - vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features,
> > + vringh_init_iotlb(&vq->vring, vdpasim->features,
>
> vdpasim_vq_reset() is called while resetting the device in vdpasim_reset()
> where we also set `vdpasim->features = 0` after resetting the vqs, so maybe
> it's better to use the supported features here, since the negotiated ones
> are related to the previous instance.
>
I don't think using supported features is valid. Better to make sure
vringh_init_iotlb() is called after the features have been negotiated.
> Thanks,
> Stefano
>
On Tue, Jul 20, 2021 at 10:14:35AM +0300, Eli Cohen wrote:
>On Tue, Jul 20, 2021 at 08:57:40AM +0200, Stefano Garzarella wrote:
>> On Tue, Jul 20, 2021 at 08:25:33AM +0300, Eli Cohen wrote:
>> > When calling vringh_init_iotlb(), use the negotiated features which
>> > might be different than the supported features.
>> >
>> > Fixes: 2c53d0f64c06f ("vdpasim: vDPA device simulator")
>> > Signed-off-by: Eli Cohen <[email protected]>
>> > ---
>> > v0 --> v1:
>> > Update "Fixes" line
>> >
>> > drivers/vdpa/vdpa_sim/vdpa_sim.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>> > index 14e024de5cbf..89a474c7a096 100644
>> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
>> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>> > @@ -66,7 +66,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,
>> > + vringh_init_iotlb(&vq->vring, vdpasim->features,
>> > VDPASIM_QUEUE_MAX, false,
>> > (struct vring_desc *)(uintptr_t)vq->desc_addr,
>> > (struct vring_avail *)
>> > @@ -86,7 +86,7 @@ static void vdpasim_vq_reset(struct vdpasim *vdpasim,
>> > vq->device_addr = 0;
>> > vq->cb = NULL;
>> > vq->private = NULL;
>> > - vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features,
>> > + vringh_init_iotlb(&vq->vring, vdpasim->features,
>>
>> vdpasim_vq_reset() is called while resetting the device in vdpasim_reset()
>> where we also set `vdpasim->features = 0` after resetting the vqs, so maybe
>> it's better to use the supported features here, since the negotiated ones
>> are related to the previous instance.
>>
>
>I don't think using supported features is valid. Better to make sure
>vringh_init_iotlb() is called after the features have been negotiated.
>
I think the vringh_init_iotlb() call in vdpasim_vq_reset() is just used
to clean up the `struct vringh`, then it will be initialized in
vdpasim_queue_ready() when features have already been negotiated.
Maybe here we can pass 0 (to the features parameter) if we don't want to
use the features supported by the device.
Thanks,
Stefano
On Tue, Jul 20, 2021 at 10:13:27AM +0200, Stefano Garzarella wrote:
> On Tue, Jul 20, 2021 at 10:14:35AM +0300, Eli Cohen wrote:
> > On Tue, Jul 20, 2021 at 08:57:40AM +0200, Stefano Garzarella wrote:
> > > On Tue, Jul 20, 2021 at 08:25:33AM +0300, Eli Cohen wrote:
> > > > When calling vringh_init_iotlb(), use the negotiated features which
> > > > might be different than the supported features.
> > > >
> > > > Fixes: 2c53d0f64c06f ("vdpasim: vDPA device simulator")
> > > > Signed-off-by: Eli Cohen <[email protected]>
> > > > ---
> > > > v0 --> v1:
> > > > Update "Fixes" line
> > > >
> > > > drivers/vdpa/vdpa_sim/vdpa_sim.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > > > index 14e024de5cbf..89a474c7a096 100644
> > > > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > > > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > > > @@ -66,7 +66,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,
> > > > + vringh_init_iotlb(&vq->vring, vdpasim->features,
> > > > VDPASIM_QUEUE_MAX, false,
> > > > (struct vring_desc *)(uintptr_t)vq->desc_addr,
> > > > (struct vring_avail *)
> > > > @@ -86,7 +86,7 @@ static void vdpasim_vq_reset(struct vdpasim *vdpasim,
> > > > vq->device_addr = 0;
> > > > vq->cb = NULL;
> > > > vq->private = NULL;
> > > > - vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features,
> > > > + vringh_init_iotlb(&vq->vring, vdpasim->features,
> > >
> > > vdpasim_vq_reset() is called while resetting the device in vdpasim_reset()
> > > where we also set `vdpasim->features = 0` after resetting the vqs, so maybe
> > > it's better to use the supported features here, since the negotiated ones
> > > are related to the previous instance.
> > >
> >
> > I don't think using supported features is valid. Better to make sure
> > vringh_init_iotlb() is called after the features have been negotiated.
> >
>
> I think the vringh_init_iotlb() call in vdpasim_vq_reset() is just used to
> clean up the `struct vringh`, then it will be initialized in
> vdpasim_queue_ready() when features have already been negotiated.
>
> Maybe here we can pass 0 (to the features parameter) if we don't want to use
> the features supported by the device.
>
> Thanks,
> Stefano
Eli? Maybe you can describe what is the observed bug the patch
is trying to fix.
Thanks!
--
MST