[Qemu-devel] [PATCH for-4.1?] nvme: Limit blkshift to 12 (for 4 kB blocks)

Max Reitz posted 1 patch 4 years, 8 months ago
Test asan passed
Test docker-mingw@fedora passed
Test FreeBSD passed
Test docker-clang@ubuntu passed
Test checkpatch passed
Test s390x passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190730114812.10493-1-mreitz@redhat.com
Maintainers: Fam Zheng <fam@euphon.net>, Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>
block/nvme.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
[Qemu-devel] [PATCH for-4.1?] nvme: Limit blkshift to 12 (for 4 kB blocks)
Posted by Max Reitz 4 years, 8 months ago
Linux does not support blocks greater than 4 kB anyway, so we might as
well limit blkshift to 12 and thus save us from some potential trouble.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Suggested-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
I won't be around for too long today, so I thought I'd just write a
patch myself now.
---
 block/nvme.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index c28755cc31..2c85713519 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -105,7 +105,7 @@ typedef struct {
 
     uint64_t nsze; /* Namespace size reported by identify command */
     int nsid;      /* The namespace id to read/write data. */
-    size_t blkshift;
+    int blkshift;
 
     uint64_t max_transfer;
     bool plugged;
@@ -420,7 +420,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
     NvmeIdNs *idns;
     NvmeLBAF *lbaf;
     uint8_t *resp;
-    int r, hwsect_size;
+    int r;
     uint64_t iova;
     NvmeCmd cmd = {
         .opcode = NVME_ADM_CMD_IDENTIFY,
@@ -474,11 +474,11 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
         goto out;
     }
 
-    hwsect_size = 1 << lbaf->ds;
-
-    if (hwsect_size < BDRV_SECTOR_SIZE || hwsect_size > s->page_size) {
-        error_setg(errp, "Namespace has unsupported block size (%d)",
-                hwsect_size);
+    if (lbaf->ds < BDRV_SECTOR_BITS || lbaf->ds > 12 ||
+        (1 << lbaf->ds) > s->page_size)
+    {
+        error_setg(errp, "Namespace has unsupported block size (2^%d)",
+                   lbaf->ds);
         goto out;
     }
 
@@ -804,16 +804,16 @@ static int64_t nvme_getlength(BlockDriverState *bs)
     return s->nsze << s->blkshift;
 }
 
-static int64_t nvme_get_blocksize(BlockDriverState *bs)
+static uint32_t nvme_get_blocksize(BlockDriverState *bs)
 {
     BDRVNVMeState *s = bs->opaque;
-    assert(s->blkshift >= BDRV_SECTOR_BITS);
-    return 1 << s->blkshift;
+    assert(s->blkshift >= BDRV_SECTOR_BITS && s->blkshift <= 12);
+    return UINT32_C(1) << s->blkshift;
 }
 
 static int nvme_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
 {
-    int64_t blocksize = nvme_get_blocksize(bs);
+    uint32_t blocksize = nvme_get_blocksize(bs);
     bsz->phys = blocksize;
     bsz->log = blocksize;
     return 0;
-- 
2.21.0


Re: [Qemu-devel] [PATCH for-4.1?] nvme: Limit blkshift to 12 (for 4 kB blocks)
Posted by Maxim Levitsky 4 years, 8 months ago
On Tue, 2019-07-30 at 13:48 +0200, Max Reitz wrote:
> Linux does not support blocks greater than 4 kB anyway, so we might as
> well limit blkshift to 12 and thus save us from some potential trouble.

Well in theory its not 4K but PAGE_SIZE, thus on some IBM machines that I heard have
64K page size that might work, but again, I don't think any hardware vendor
dared yet to sell devices with sector size > 4K.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> I won't be around for too long today, so I thought I'd just write a
> patch myself now.
> ---
>  block/nvme.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index c28755cc31..2c85713519 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -105,7 +105,7 @@ typedef struct {
>  
>      uint64_t nsze; /* Namespace size reported by identify command */
>      int nsid;      /* The namespace id to read/write data. */
> -    size_t blkshift;
> +    int blkshift;
>  
>      uint64_t max_transfer;
>      bool plugged;
> @@ -420,7 +420,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>      NvmeIdNs *idns;
>      NvmeLBAF *lbaf;
>      uint8_t *resp;
> -    int r, hwsect_size;
> +    int r;
>      uint64_t iova;
>      NvmeCmd cmd = {
>          .opcode = NVME_ADM_CMD_IDENTIFY,
> @@ -474,11 +474,11 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>          goto out;
>      }
>  
> -    hwsect_size = 1 << lbaf->ds;
> -
> -    if (hwsect_size < BDRV_SECTOR_SIZE || hwsect_size > s->page_size) {
> -        error_setg(errp, "Namespace has unsupported block size (%d)",
> -                hwsect_size);
> +    if (lbaf->ds < BDRV_SECTOR_BITS || lbaf->ds > 12 ||
> +        (1 << lbaf->ds) > s->page_size)
> +    {
> +        error_setg(errp, "Namespace has unsupported block size (2^%d)",
> +                   lbaf->ds);
>          goto out;
>      }
>  
> @@ -804,16 +804,16 @@ static int64_t nvme_getlength(BlockDriverState *bs)
>      return s->nsze << s->blkshift;
>  }
>  
> -static int64_t nvme_get_blocksize(BlockDriverState *bs)
> +static uint32_t nvme_get_blocksize(BlockDriverState *bs)
>  {
>      BDRVNVMeState *s = bs->opaque;
> -    assert(s->blkshift >= BDRV_SECTOR_BITS);
> -    return 1 << s->blkshift;
> +    assert(s->blkshift >= BDRV_SECTOR_BITS && s->blkshift <= 12);
> +    return UINT32_C(1) << s->blkshift;
>  }
>  
>  static int nvme_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
>  {
> -    int64_t blocksize = nvme_get_blocksize(bs);
> +    uint32_t blocksize = nvme_get_blocksize(bs);
>      bsz->phys = blocksize;
>      bsz->log = blocksize;
>      return 0;