diff mbox series

[v2,13/16] nvme: factor out namespace setup

Message ID 20200415130159.611361-14-its@irrelevant.dk (mailing list archive)
State New, archived
Headers show
Series nvme: refactoring and cleanups | expand

Commit Message

Klaus Jensen April 15, 2020, 1:01 p.m. UTC 
From: Klaus Jensen <k.jensen@samsung.com>

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.c | 46 ++++++++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 20 deletions(-)

Comments

Philippe Mathieu-Daudé April 15, 2020, 1:16 p.m. UTC | #1 
On 4/15/20 3:01 PM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/block/nvme.c | 46 ++++++++++++++++++++++++++--------------------
>   1 file changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index d5244102252c..2b007115c302 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1358,6 +1358,27 @@ static void nvme_init_blk(NvmeCtrl *n, Error **errp)
>                                     false, errp);
>   }
>   
> +static void nvme_init_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> +{
> +    int64_t bs_size;
> +    NvmeIdNs *id_ns = &ns->id_ns;
> +
> +    bs_size = blk_getlength(n->conf.blk);
> +    if (bs_size < 0) {
> +        error_setg_errno(errp, -bs_size, "could not get backing file size");
> +        return;
> +    }
> +
> +    n->ns_size = bs_size;
> +
> +    id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
> +    id_ns->nsze = cpu_to_le64(nvme_ns_nlbas(n, ns));
> +
> +    /* no thin provisioning */
> +    id_ns->ncap = id_ns->nsze;
> +    id_ns->nuse = id_ns->ncap;
> +}
> +
>   static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>   {
>       NvmeCtrl *n = NVME(pci_dev);
> @@ -1365,7 +1386,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>       Error *err = NULL;
>   
>       int i;
> -    int64_t bs_size;
>       uint8_t *pci_conf;
>   
>       nvme_check_constraints(n, &err);
> @@ -1376,12 +1396,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>   
>       nvme_init_state(n);
>   
> -    bs_size = blk_getlength(n->conf.blk);
> -    if (bs_size < 0) {
> -        error_setg(errp, "could not get backing file size");
> -        return;
> -    }
> -
>       nvme_init_blk(n, &err);
>       if (err) {
>           error_propagate(errp, err);
> @@ -1394,8 +1408,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>       pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS);
>       pcie_endpoint_cap_init(pci_dev, 0x80);
>   
> -    n->ns_size = bs_size / (uint64_t)n->num_namespaces;

Valid because currently 'n->num_namespaces' = 1, OK.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>       memory_region_init_io(&n->iomem, OBJECT(n), &nvme_mmio_ops, n,
>                             "nvme", n->reg_size);
>       pci_register_bar(pci_dev, 0,
> @@ -1459,17 +1471,11 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>       }
>   
>       for (i = 0; i < n->num_namespaces; i++) {
> -        NvmeNamespace *ns = &n->namespaces[i];
> -        NvmeIdNs *id_ns = &ns->id_ns;
> -        id_ns->nsfeat = 0;
> -        id_ns->nlbaf = 0;
> -        id_ns->flbas = 0;
> -        id_ns->mc = 0;
> -        id_ns->dpc = 0;
> -        id_ns->dps = 0;
> -        id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
> -        id_ns->ncap  = id_ns->nuse = id_ns->nsze =
> -            cpu_to_le64(nvme_ns_nlbas(n, ns));
> +        nvme_init_namespace(n, &n->namespaces[i], &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
>       }
>   }
>   
>
Klaus Jensen April 15, 2020, 1:20 p.m. UTC | #2 
On Apr 15 15:16, Philippe Mathieu-Daudé wrote:
> On 4/15/20 3:01 PM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >   hw/block/nvme.c | 46 ++++++++++++++++++++++++++--------------------
> >   1 file changed, 26 insertions(+), 20 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index d5244102252c..2b007115c302 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1358,6 +1358,27 @@ static void nvme_init_blk(NvmeCtrl *n, Error **errp)
> >                                     false, errp);
> >   }
> > +static void nvme_init_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> > +{
> > +    int64_t bs_size;
> > +    NvmeIdNs *id_ns = &ns->id_ns;
> > +
> > +    bs_size = blk_getlength(n->conf.blk);
> > +    if (bs_size < 0) {
> > +        error_setg_errno(errp, -bs_size, "could not get backing file size");
> > +        return;
> > +    }
> > +
> > +    n->ns_size = bs_size;
> > +
> > +    id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
> > +    id_ns->nsze = cpu_to_le64(nvme_ns_nlbas(n, ns));
> > +
> > +    /* no thin provisioning */
> > +    id_ns->ncap = id_ns->nsze;
> > +    id_ns->nuse = id_ns->ncap;
> > +}
> > +
> >   static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> >   {
> >       NvmeCtrl *n = NVME(pci_dev);
> > @@ -1365,7 +1386,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> >       Error *err = NULL;
> >       int i;
> > -    int64_t bs_size;
> >       uint8_t *pci_conf;
> >       nvme_check_constraints(n, &err);
> > @@ -1376,12 +1396,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> >       nvme_init_state(n);
> > -    bs_size = blk_getlength(n->conf.blk);
> > -    if (bs_size < 0) {
> > -        error_setg(errp, "could not get backing file size");
> > -        return;
> > -    }
> > -
> >       nvme_init_blk(n, &err);
> >       if (err) {
> >           error_propagate(errp, err);
> > @@ -1394,8 +1408,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
> >       pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS);
> >       pcie_endpoint_cap_init(pci_dev, 0x80);
> > -    n->ns_size = bs_size / (uint64_t)n->num_namespaces;
> 
> Valid because currently 'n->num_namespaces' = 1, OK.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
 
Thank you for the reviews Philippe and the suggesting that I split up
the series :)

I'll get the v1.3 series ready next.
Philippe Mathieu-Daudé April 15, 2020, 1:26 p.m. UTC | #3 
On 4/15/20 3:20 PM, Klaus Birkelund Jensen wrote:
> On Apr 15 15:16, Philippe Mathieu-Daudé wrote:
>> On 4/15/20 3:01 PM, Klaus Jensen wrote:
>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>
>>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>>> ---
>>>    hw/block/nvme.c | 46 ++++++++++++++++++++++++++--------------------
>>>    1 file changed, 26 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>> index d5244102252c..2b007115c302 100644
>>> --- a/hw/block/nvme.c
>>> +++ b/hw/block/nvme.c
>>> @@ -1358,6 +1358,27 @@ static void nvme_init_blk(NvmeCtrl *n, Error **errp)
>>>                                      false, errp);
>>>    }
>>> +static void nvme_init_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
>>> +{
>>> +    int64_t bs_size;
>>> +    NvmeIdNs *id_ns = &ns->id_ns;
>>> +
>>> +    bs_size = blk_getlength(n->conf.blk);
>>> +    if (bs_size < 0) {
>>> +        error_setg_errno(errp, -bs_size, "could not get backing file size");
>>> +        return;
>>> +    }
>>> +
>>> +    n->ns_size = bs_size;
>>> +
>>> +    id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
>>> +    id_ns->nsze = cpu_to_le64(nvme_ns_nlbas(n, ns));
>>> +
>>> +    /* no thin provisioning */
>>> +    id_ns->ncap = id_ns->nsze;
>>> +    id_ns->nuse = id_ns->ncap;
>>> +}
>>> +
>>>    static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>>>    {
>>>        NvmeCtrl *n = NVME(pci_dev);
>>> @@ -1365,7 +1386,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>>>        Error *err = NULL;
>>>        int i;
>>> -    int64_t bs_size;
>>>        uint8_t *pci_conf;
>>>        nvme_check_constraints(n, &err);
>>> @@ -1376,12 +1396,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>>>        nvme_init_state(n);
>>> -    bs_size = blk_getlength(n->conf.blk);
>>> -    if (bs_size < 0) {
>>> -        error_setg(errp, "could not get backing file size");
>>> -        return;
>>> -    }
>>> -
>>>        nvme_init_blk(n, &err);
>>>        if (err) {
>>>            error_propagate(errp, err);
>>> @@ -1394,8 +1408,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>>>        pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS);
>>>        pcie_endpoint_cap_init(pci_dev, 0x80);
>>> -    n->ns_size = bs_size / (uint64_t)n->num_namespaces;
>>
>> Valid because currently 'n->num_namespaces' = 1, OK.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>   
> Thank you for the reviews Philippe and the suggesting that I split up
> the series :)

No prob, thanks for the quick fixes. Let's see if Keith is happy with 
v2. Personally I don't have anymore comments.

> 
> I'll get the v1.3 series ready next.
> 

Cool. What really matters (to me) is seeing tests. If we can merge tests 
(without multiple namespaces) before the rest of your series, even 
better. Tests give reviewers/maintainers confidence that code isn't 
breaking ;)

Regards,

Phil.
Klaus Jensen April 16, 2020, 6:03 a.m. UTC | #4 
On Apr 15 15:26, Philippe Mathieu-Daudé wrote:
> On 4/15/20 3:20 PM, Klaus Birkelund Jensen wrote:
> > 
> > I'll get the v1.3 series ready next.
> > 
> 
> Cool. What really matters (to me) is seeing tests. If we can merge tests
> (without multiple namespaces) before the rest of your series, even better.
> Tests give reviewers/maintainers confidence that code isn't breaking ;)
> 

The patches that I contribute have been pretty extensively tested by
various means in a "host setting" (e.g. blktests and some internal
tools), which really exercise the device by doing heavy I/O, testing for
compliance and also just being mean to it (e.g. tripping bus mastering
while doing I/O).

Don't misunderstand me as trying to weasel my way out of writing tests,
but I just want to understand the scope of the tests that you are
looking for? I believe (hope!) that you are not asking me to implement a
user-space NVMe driver in the test, so I assume the tests should varify
more low level details?
Maxim Levitsky April 21, 2020, 3:57 p.m. UTC | #5 
On Wed, 2020-04-15 at 15:01 +0200, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.c | 46 ++++++++++++++++++++++++++--------------------
>  1 file changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index d5244102252c..2b007115c302 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1358,6 +1358,27 @@ static void nvme_init_blk(NvmeCtrl *n, Error **errp)
>                                    false, errp);
>  }
>  
> +static void nvme_init_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
> +{
> +    int64_t bs_size;
> +    NvmeIdNs *id_ns = &ns->id_ns;
> +
> +    bs_size = blk_getlength(n->conf.blk);
> +    if (bs_size < 0) {
> +        error_setg_errno(errp, -bs_size, "could not get backing file size");
> +        return;
> +    }
> +
> +    n->ns_size = bs_size;
> +
> +    id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
> +    id_ns->nsze = cpu_to_le64(nvme_ns_nlbas(n, ns));
> +
> +    /* no thin provisioning */
> +    id_ns->ncap = id_ns->nsze;
> +    id_ns->nuse = id_ns->ncap;
> +}
> +
>  static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>  {
>      NvmeCtrl *n = NVME(pci_dev);
> @@ -1365,7 +1386,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>      Error *err = NULL;
>  
>      int i;
> -    int64_t bs_size;
>      uint8_t *pci_conf;
>  
>      nvme_check_constraints(n, &err);
> @@ -1376,12 +1396,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>  
>      nvme_init_state(n);
>  
> -    bs_size = blk_getlength(n->conf.blk);
> -    if (bs_size < 0) {
> -        error_setg(errp, "could not get backing file size");
> -        return;
> -    }
> -
>      nvme_init_blk(n, &err);
>      if (err) {
>          error_propagate(errp, err);
> @@ -1394,8 +1408,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>      pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS);
>      pcie_endpoint_cap_init(pci_dev, 0x80);
>  
> -    n->ns_size = bs_size / (uint64_t)n->num_namespaces;
> -
>      memory_region_init_io(&n->iomem, OBJECT(n), &nvme_mmio_ops, n,
>                            "nvme", n->reg_size);
>      pci_register_bar(pci_dev, 0,
> @@ -1459,17 +1471,11 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
>      }
>  
>      for (i = 0; i < n->num_namespaces; i++) {
> -        NvmeNamespace *ns = &n->namespaces[i];
> -        NvmeIdNs *id_ns = &ns->id_ns;
> -        id_ns->nsfeat = 0;
> -        id_ns->nlbaf = 0;
> -        id_ns->flbas = 0;
> -        id_ns->mc = 0;
> -        id_ns->dpc = 0;
> -        id_ns->dps = 0;
> -        id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
> -        id_ns->ncap  = id_ns->nuse = id_ns->nsze =
> -            cpu_to_le64(nvme_ns_nlbas(n, ns));
> +        nvme_init_namespace(n, &n->namespaces[i], &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
>      }
>  }
>  


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

Best regards,
	Maxim Levitsky
Philippe Mathieu-Daudé April 24, 2020, 10:13 a.m. UTC | #6 
On 4/16/20 8:03 AM, Klaus Birkelund Jensen wrote:
> On Apr 15 15:26, Philippe Mathieu-Daudé wrote:
>> On 4/15/20 3:20 PM, Klaus Birkelund Jensen wrote:
>>>
>>> I'll get the v1.3 series ready next.
>>>
>>
>> Cool. What really matters (to me) is seeing tests. If we can merge tests
>> (without multiple namespaces) before the rest of your series, even better.
>> Tests give reviewers/maintainers confidence that code isn't breaking ;)
>>
> 
> The patches that I contribute have been pretty extensively tested by
> various means in a "host setting" (e.g. blktests and some internal
> tools), which really exercise the device by doing heavy I/O, testing for
> compliance and also just being mean to it (e.g. tripping bus mastering
> while doing I/O).
> 
> Don't misunderstand me as trying to weasel my way out of writing tests,
> but I just want to understand the scope of the tests that you are
> looking for? I believe (hope!) that you are not asking me to implement a
> user-space NVMe driver in the test, so I assume the tests should varify
> more low level details?

I was thinking about something rather simple.

So you are adding the "multiple namespaces" feature, we want to test it.

If you can demonstrate it works with few I/O calls you could try with 
qtest, such:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg695421.html

If it requires complex commands, since the user-space tools already 
exist, you can use an acceptance test booting Linux, installing the NVMe 
tools and use them. See tests/acceptance/linux_ssh_mips_malta.py or
https://www.mail-archive.com/qemu-devel@nongnu.org/msg656319.html

Regards,

Phil.
diff mbox series

Patch

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index d5244102252c..2b007115c302 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1358,6 +1358,27 @@  static void nvme_init_blk(NvmeCtrl *n, Error **errp)
                                   false, errp);
 }
 
+static void nvme_init_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
+{
+    int64_t bs_size;
+    NvmeIdNs *id_ns = &ns->id_ns;
+
+    bs_size = blk_getlength(n->conf.blk);
+    if (bs_size < 0) {
+        error_setg_errno(errp, -bs_size, "could not get backing file size");
+        return;
+    }
+
+    n->ns_size = bs_size;
+
+    id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
+    id_ns->nsze = cpu_to_le64(nvme_ns_nlbas(n, ns));
+
+    /* no thin provisioning */
+    id_ns->ncap = id_ns->nsze;
+    id_ns->nuse = id_ns->ncap;
+}
+
 static void nvme_realize(PCIDevice *pci_dev, Error **errp)
 {
     NvmeCtrl *n = NVME(pci_dev);
@@ -1365,7 +1386,6 @@  static void nvme_realize(PCIDevice *pci_dev, Error **errp)
     Error *err = NULL;
 
     int i;
-    int64_t bs_size;
     uint8_t *pci_conf;
 
     nvme_check_constraints(n, &err);
@@ -1376,12 +1396,6 @@  static void nvme_realize(PCIDevice *pci_dev, Error **errp)
 
     nvme_init_state(n);
 
-    bs_size = blk_getlength(n->conf.blk);
-    if (bs_size < 0) {
-        error_setg(errp, "could not get backing file size");
-        return;
-    }
-
     nvme_init_blk(n, &err);
     if (err) {
         error_propagate(errp, err);
@@ -1394,8 +1408,6 @@  static void nvme_realize(PCIDevice *pci_dev, Error **errp)
     pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_EXPRESS);
     pcie_endpoint_cap_init(pci_dev, 0x80);
 
-    n->ns_size = bs_size / (uint64_t)n->num_namespaces;
-
     memory_region_init_io(&n->iomem, OBJECT(n), &nvme_mmio_ops, n,
                           "nvme", n->reg_size);
     pci_register_bar(pci_dev, 0,
@@ -1459,17 +1471,11 @@  static void nvme_realize(PCIDevice *pci_dev, Error **errp)
     }
 
     for (i = 0; i < n->num_namespaces; i++) {
-        NvmeNamespace *ns = &n->namespaces[i];
-        NvmeIdNs *id_ns = &ns->id_ns;
-        id_ns->nsfeat = 0;
-        id_ns->nlbaf = 0;
-        id_ns->flbas = 0;
-        id_ns->mc = 0;
-        id_ns->dpc = 0;
-        id_ns->dps = 0;
-        id_ns->lbaf[0].ds = BDRV_SECTOR_BITS;
-        id_ns->ncap  = id_ns->nuse = id_ns->nsze =
-            cpu_to_le64(nvme_ns_nlbas(n, ns));
+        nvme_init_namespace(n, &n->namespaces[i], &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
     }
 }