[v3,2/6] block/nvme: fix doorbell stride
diff mbox series

Message ID 20190703155944.9637-3-mlevitsk@redhat.com
State New
Headers show
Series
  • Few fixes for userspace NVME driver
Related show

Commit Message

Maxim Levitsky July 3, 2019, 3:59 p.m. UTC
Fix the math involving non standard doorbell stride

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Max Reitz July 5, 2019, 11:09 a.m. UTC | #1
On 03.07.19 17:59, Maxim Levitsky wrote:
> Fix the math involving non standard doorbell stride
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 6d4e7f3d83..52798081b2 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -217,7 +217,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
>          error_propagate(errp, local_err);
>          goto fail;
>      }
> -    q->cq.doorbell = &s->regs->doorbells[idx * 2 * s->doorbell_scale + 1];
> +    q->cq.doorbell = &s->regs->doorbells[(idx * 2 + 1) * s->doorbell_scale];
>  
>      return q;
>  fail:

Hm.  How has this ever worked?

Reviewed-by: Max Reitz <mreitz@redhat.com>
Max Reitz July 5, 2019, 11:10 a.m. UTC | #2
On 05.07.19 13:09, Max Reitz wrote:
> On 03.07.19 17:59, Maxim Levitsky wrote:
>> Fix the math involving non standard doorbell stride
>>
>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
>> ---
>>  block/nvme.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 6d4e7f3d83..52798081b2 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -217,7 +217,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
>>          error_propagate(errp, local_err);
>>          goto fail;
>>      }
>> -    q->cq.doorbell = &s->regs->doorbells[idx * 2 * s->doorbell_scale + 1];
>> +    q->cq.doorbell = &s->regs->doorbells[(idx * 2 + 1) * s->doorbell_scale];
>>  
>>      return q;
>>  fail:
> 
> Hm.  How has this ever worked?

(Ah, because CAP.DSTRD has probably been 0 in most devices.)
Maxim Levitsky July 7, 2019, 8:47 a.m. UTC | #3
On Fri, 2019-07-05 at 13:10 +0200, Max Reitz wrote:
> On 05.07.19 13:09, Max Reitz wrote:
> > On 03.07.19 17:59, Maxim Levitsky wrote:
> > > Fix the math involving non standard doorbell stride
> > > 
> > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > ---
> > >  block/nvme.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/block/nvme.c b/block/nvme.c
> > > index 6d4e7f3d83..52798081b2 100644
> > > --- a/block/nvme.c
> > > +++ b/block/nvme.c
> > > @@ -217,7 +217,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
> > >          error_propagate(errp, local_err);
> > >          goto fail;
> > >      }
> > > -    q->cq.doorbell = &s->regs->doorbells[idx * 2 * s->doorbell_scale + 1];
> > > +    q->cq.doorbell = &s->regs->doorbells[(idx * 2 + 1) * s->doorbell_scale];
> > >  
> > >      return q;
> > >  fail:
> > 
> > Hm.  How has this ever worked?
> 
> (Ah, because CAP.DSTRD has probably been 0 in most devices.)
> 
Exactly, and I used cache line stride in my nvme-mdev, which broke this and I spend an evening figuring out
what is going on. I was sure that there is some memory ordering bug or something even weirder before (as usual)
finding that this is a very simple bug.
I tested nvme-mdev pretty much with everything I could get my hands on, including this driver.


Best regards,
	Maxim Levitsky

Patch
diff mbox series

diff --git a/block/nvme.c b/block/nvme.c
index 6d4e7f3d83..52798081b2 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -217,7 +217,7 @@  static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
         error_propagate(errp, local_err);
         goto fail;
     }
-    q->cq.doorbell = &s->regs->doorbells[idx * 2 * s->doorbell_scale + 1];
+    q->cq.doorbell = &s->regs->doorbells[(idx * 2 + 1) * s->doorbell_scale];
 
     return q;
 fail: