diff mbox series

[RFC,1/5] block/nvme: use some NVME_CAP_* macros

Message ID 20201015115252.15582-2-eric.auger@redhat.com
State New
Headers show
Series NVMe passthrough: Support 64kB page host | expand

Commit Message

Eric Auger Oct. 15, 2020, 11:52 a.m. UTC 
let's use NVME_CAP_DSTRD, NVME_CAP_MPSMIN and NVME_CAP_TO macros

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 block/nvme.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Philippe Mathieu-Daudé Oct. 15, 2020, 1:29 p.m. UTC | #1 
On 10/15/20 1:52 PM, Eric Auger wrote:
> let's use NVME_CAP_DSTRD, NVME_CAP_MPSMIN and NVME_CAP_TO macros
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>   block/nvme.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index f4f27b6da7..e3d96f20d0 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -728,10 +728,10 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>           goto out;
>       }
>   
> -    s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));
> -    s->doorbell_scale = (4 << (((cap >> 32) & 0xF))) / sizeof(uint32_t);
> +    s->page_size = MAX(4096, 1 << (12 + NVME_CAP_MPSMIN(cap)));

Are you suggesting commit fad1eb68862 ("block/nvme: Use register
definitions from 'block/nvme.h'") is buggy?

> +    s->doorbell_scale = (4 << ((NVME_CAP_DSTRD(cap)))) / sizeof(uint32_t);
>       bs->bl.opt_mem_alignment = s->page_size;
> -    timeout_ms = MIN(500 * ((cap >> 24) & 0xFF), 30000);
> +    timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
>   
>       /* Reset device to get a clean state. */
>       s->regs->ctrl.cc = cpu_to_le32(le32_to_cpu(s->regs->ctrl.cc) & 0xFE);
>
Philippe Mathieu-Daudé Oct. 15, 2020, 1:32 p.m. UTC | #2 
On 10/15/20 3:29 PM, Philippe Mathieu-Daudé wrote:
> On 10/15/20 1:52 PM, Eric Auger wrote:

>> let's use NVME_CAP_DSTRD, NVME_CAP_MPSMIN and NVME_CAP_TO macros

>>

>> Signed-off-by: Eric Auger <eric.auger@redhat.com>

>> ---

>>   block/nvme.c | 6 +++---

>>   1 file changed, 3 insertions(+), 3 deletions(-)

>>

>> diff --git a/block/nvme.c b/block/nvme.c

>> index f4f27b6da7..e3d96f20d0 100644

>> --- a/block/nvme.c

>> +++ b/block/nvme.c

>> @@ -728,10 +728,10 @@ static int nvme_init(BlockDriverState *bs, const 

>> char *device, int namespace,

>>           goto out;

>>       }

>> -    s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));

>> -    s->doorbell_scale = (4 << (((cap >> 32) & 0xF))) / sizeof(uint32_t);

>> +    s->page_size = MAX(4096, 1 << (12 + NVME_CAP_MPSMIN(cap)));

> 

> Are you suggesting commit fad1eb68862 ("block/nvme: Use register

> definitions from 'block/nvme.h'") is buggy?


Buh I wonder how we missed that :/

> 

>> +    s->doorbell_scale = (4 << ((NVME_CAP_DSTRD(cap)))) / 

>> sizeof(uint32_t);

>>       bs->bl.opt_mem_alignment = s->page_size;

>> -    timeout_ms = MIN(500 * ((cap >> 24) & 0xFF), 30000);

>> +    timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);

>>       /* Reset device to get a clean state. */

>>       s->regs->ctrl.cc = cpu_to_le32(le32_to_cpu(s->regs->ctrl.cc) & 

>> 0xFE);

>>

>
Philippe Mathieu-Daudé Oct. 15, 2020, 1:36 p.m. UTC | #3 
On 10/15/20 3:32 PM, Philippe Mathieu-Daudé wrote:
> On 10/15/20 3:29 PM, Philippe Mathieu-Daudé wrote:

>> On 10/15/20 1:52 PM, Eric Auger wrote:

>>> let's use NVME_CAP_DSTRD, NVME_CAP_MPSMIN and NVME_CAP_TO macros

>>>

>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>

>>> ---

>>>   block/nvme.c | 6 +++---

>>>   1 file changed, 3 insertions(+), 3 deletions(-)

>>>

>>> diff --git a/block/nvme.c b/block/nvme.c

>>> index f4f27b6da7..e3d96f20d0 100644

>>> --- a/block/nvme.c

>>> +++ b/block/nvme.c

>>> @@ -728,10 +728,10 @@ static int nvme_init(BlockDriverState *bs, 

>>> const char *device, int namespace,

>>>           goto out;

>>>       }

>>> -    s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));

>>> -    s->doorbell_scale = (4 << (((cap >> 32) & 0xF))) / 

>>> sizeof(uint32_t);

>>> +    s->page_size = MAX(4096, 1 << (12 + NVME_CAP_MPSMIN(cap)));

>>

>> Are you suggesting commit fad1eb68862 ("block/nvme: Use register

>> definitions from 'block/nvme.h'") is buggy?

> 

> Buh I wonder how we missed that :/


Since your patch doesn't apply anyway, I might fix as:

         s->page_size = 4096 << NVME_CAP_MPSMIN(cap);

> 

>>

>>> +    s->doorbell_scale = (4 << ((NVME_CAP_DSTRD(cap)))) / 

>>> sizeof(uint32_t);

>>>       bs->bl.opt_mem_alignment = s->page_size;

>>> -    timeout_ms = MIN(500 * ((cap >> 24) & 0xFF), 30000);

>>> +    timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);

>>>       /* Reset device to get a clean state. */

>>>       s->regs->ctrl.cc = cpu_to_le32(le32_to_cpu(s->regs->ctrl.cc) & 

>>> 0xFE);

>>>

>>

>
Eric Auger Oct. 15, 2020, 4:11 p.m. UTC | #4 
Hi Philippe,

On 10/15/20 3:36 PM, Philippe Mathieu-Daudé wrote:
> On 10/15/20 3:32 PM, Philippe Mathieu-Daudé wrote:
>> On 10/15/20 3:29 PM, Philippe Mathieu-Daudé wrote:
>>> On 10/15/20 1:52 PM, Eric Auger wrote:
>>>> let's use NVME_CAP_DSTRD, NVME_CAP_MPSMIN and NVME_CAP_TO macros
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>>   block/nvme.c | 6 +++---
>>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/block/nvme.c b/block/nvme.c
>>>> index f4f27b6da7..e3d96f20d0 100644
>>>> --- a/block/nvme.c
>>>> +++ b/block/nvme.c
>>>> @@ -728,10 +728,10 @@ static int nvme_init(BlockDriverState *bs,
>>>> const char *device, int namespace,
>>>>           goto out;
>>>>       }
>>>> -    s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));
>>>> -    s->doorbell_scale = (4 << (((cap >> 32) & 0xF))) /
>>>> sizeof(uint32_t);
>>>> +    s->page_size = MAX(4096, 1 << (12 + NVME_CAP_MPSMIN(cap)));
>>>
>>> Are you suggesting commit fad1eb68862 ("block/nvme: Use register
>>> definitions from 'block/nvme.h'") is buggy?
yes I think so. Sorry I messed up my rebase and failed to grab that patch.
>>
>> Buh I wonder how we missed that :/
> 
> Since your patch doesn't apply anyway, I might fix as:
> 
>         s->page_size = 4096 << NVME_CAP_MPSMIN(cap);
1 << (12 + NVME_CAP_MPSMIN(cap)) matches the spec text so personally I
would keep that.

Thanks

Eric
> 
>>
>>>
>>>> +    s->doorbell_scale = (4 << ((NVME_CAP_DSTRD(cap)))) /
>>>> sizeof(uint32_t);
>>>>       bs->bl.opt_mem_alignment = s->page_size;
>>>> -    timeout_ms = MIN(500 * ((cap >> 24) & 0xFF), 30000);
>>>> +    timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
>>>>       /* Reset device to get a clean state. */
>>>>       s->regs->ctrl.cc = cpu_to_le32(le32_to_cpu(s->regs->ctrl.cc) &
>>>> 0xFE);
>>>>
>>>
>>
> 
>
diff mbox series

Patch

diff --git a/block/nvme.c b/block/nvme.c
index f4f27b6da7..e3d96f20d0 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -728,10 +728,10 @@  static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
         goto out;
     }
 
-    s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));
-    s->doorbell_scale = (4 << (((cap >> 32) & 0xF))) / sizeof(uint32_t);
+    s->page_size = MAX(4096, 1 << (12 + NVME_CAP_MPSMIN(cap)));
+    s->doorbell_scale = (4 << ((NVME_CAP_DSTRD(cap)))) / sizeof(uint32_t);
     bs->bl.opt_mem_alignment = s->page_size;
-    timeout_ms = MIN(500 * ((cap >> 24) & 0xFF), 30000);
+    timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
 
     /* Reset device to get a clean state. */
     s->regs->ctrl.cc = cpu_to_le32(le32_to_cpu(s->regs->ctrl.cc) & 0xFE);