From: Minwoo Im <minwoo.im.dev@gmail.com>

In NVMe, namespace is being attached to process I/O.  We register NVMe
namespace to a controller via nvme_register_namespace() during
nvme_ns_setup().  This is main reason of receiving NvmeCtrl object
instance to this function to map the namespace to a controller.

To make namespace instance more independent, it should be split into two
parts: setup and register.  This patch split them into two differnt
parts, and finally nvme_ns_setup() does not have nothing to do with
NvmeCtrl instance at all.

This patch is a former patch to introduce NVMe subsystem scheme to the
existing design especially for multi-path.  In that case, it should be
split into two to make namespace independent from a controller.

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme-ns.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index 17e876e6bc44..ce79ad4a5319 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -321,10 +321,6 @@ int nvme_ns_setup(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
         nvme_ns_init_zoned(ns, 0);
     }
 
-    if (nvme_register_namespace(n, ns, errp)) {
-        return -1;
-    }
-
     return 0;
 }
 
@@ -362,6 +358,13 @@ static void nvme_ns_realize(DeviceState *dev, Error **errp)
                                 "could not setup namespace: ");
         return;
     }
+
+    if (nvme_register_namespace(n, ns, errp)) {
+        error_propagate_prepend(errp, local_err,
+                                "could not register namespace: ");
+        return;
+    }
+
 }
 
 static Property nvme_ns_props[] = {
-- 
2.30.0

Hi Klaus,

On 09.02.21 08:30, Klaus Jensen wrote:
> From: Minwoo Im <minwoo.im.dev@gmail.com>
>
> In NVMe, namespace is being attached to process I/O.  We register NVMe
> namespace to a controller via nvme_register_namespace() during
> nvme_ns_setup().  This is main reason of receiving NvmeCtrl object
> instance to this function to map the namespace to a controller.
>
> To make namespace instance more independent, it should be split into two
> parts: setup and register.  This patch split them into two differnt
> parts, and finally nvme_ns_setup() does not have nothing to do with
> NvmeCtrl instance at all.
>
> This patch is a former patch to introduce NVMe subsystem scheme to the
> existing design especially for multi-path.  In that case, it should be
> split into two to make namespace independent from a controller.
>
> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>


In latest master, I can no longer access NVMe devices from edk2. Git 
bisect pointed me to this commit.

To reproduce:

  $ ./build/qemu-system-x86_64 -enable-kvm -pflash 
build/pc-bios/edk2-x86_64-code.fd -drive 
file=image.raw,if=none,id=d,snapshot=on -device nvme,serial=1234,drive=d 
-nographic -net none

You will see that before this commit, edk2 is able to enumerate the 
block device, while after this commit it does not find it.


good:

Mapping table
      FS0: Alias(s):HD1b:;BLK2:
        � 
PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(1,GPT,7A866FF6-0A12-4911-A4ED-D0565BEB41A2,0x80,0x64000)
     BLK0: Alias(s):
          PciRoot(0x0)/Pci(0x1,0x1)/Ata(0x0)
     BLK1: Alias(s):
        � 
PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)
     BLK3: Alias(s):
        � 
PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(2,GPT,F070566B-C58D-4F13-9B92-64F1794385E7,0x64080,0x40000)
     BLK4: Alias(s):
        � 
PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(3,GPT,EDE86BE3-C64F-4ACB-B4AA-5E6C0135D335,0xA4080,0x315BF1B)


bad:

Mapping table
     BLK0: Alias(s):
          PciRoot(0x0)/Pci(0x1,0x1)/Ata(0x0)



Thanks,

Alex

On Feb 11 10:53, Alexander Graf wrote:
> Hi Klaus,
> 
> On 09.02.21 08:30, Klaus Jensen wrote:
> > From: Minwoo Im <minwoo.im.dev@gmail.com>
> > 
> > In NVMe, namespace is being attached to process I/O.  We register NVMe
> > namespace to a controller via nvme_register_namespace() during
> > nvme_ns_setup().  This is main reason of receiving NvmeCtrl object
> > instance to this function to map the namespace to a controller.
> > 
> > To make namespace instance more independent, it should be split into two
> > parts: setup and register.  This patch split them into two differnt
> > parts, and finally nvme_ns_setup() does not have nothing to do with
> > NvmeCtrl instance at all.
> > 
> > This patch is a former patch to introduce NVMe subsystem scheme to the
> > existing design especially for multi-path.  In that case, it should be
> > split into two to make namespace independent from a controller.
> > 
> > Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> 
> 
> In latest master, I can no longer access NVMe devices from edk2. Git bisect
> pointed me to this commit.
> 

Hi Alexander,

Thanks for reporting this. This patch should fix it, I'll queue it up.

diff --git i/hw/block/nvme.c w/hw/block/nvme.c
index 5ce21b7100b3..d36e14fe13e2 100644
--- i/hw/block/nvme.c
+++ w/hw/block/nvme.c
@@ -4507,6 +4507,12 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
         if (nvme_ns_setup(ns, errp)) {
             return;
         }
+
+        if (nvme_register_namespace(n, ns, errp)) {
+            error_propagate_prepend(errp, local_err,
+                                    "could not register namespace: ");
+            return;
+        }
     }
 }

On 2/11/21 10:53 AM, Alexander Graf wrote:
> Hi Klaus,
> 
> On 09.02.21 08:30, Klaus Jensen wrote:
>> From: Minwoo Im <minwoo.im.dev@gmail.com>
>>
>> In NVMe, namespace is being attached to process I/O.  We register NVMe
>> namespace to a controller via nvme_register_namespace() during
>> nvme_ns_setup().  This is main reason of receiving NvmeCtrl object
>> instance to this function to map the namespace to a controller.
>>
>> To make namespace instance more independent, it should be split into two
>> parts: setup and register.  This patch split them into two differnt
>> parts, and finally nvme_ns_setup() does not have nothing to do with
>> NvmeCtrl instance at all.
>>
>> This patch is a former patch to introduce NVMe subsystem scheme to the
>> existing design especially for multi-path.  In that case, it should be
>> split into two to make namespace independent from a controller.
>>
>> Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> 
> 
> In latest master, I can no longer access NVMe devices from edk2. Git
> bisect pointed me to this commit.
> 
> To reproduce:
> 
>   $ ./build/qemu-system-x86_64 -enable-kvm -pflash
> build/pc-bios/edk2-x86_64-code.fd -drive
> file=image.raw,if=none,id=d,snapshot=on -device nvme,serial=1234,drive=d
> -nographic -net none
> 
> You will see that before this commit, edk2 is able to enumerate the
> block device, while after this commit it does not find it.
> 
> 
> good:
> 
> Mapping table
>       FS0: Alias(s):HD1b:;BLK2:
>         �
> PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(1,GPT,7A866FF6-0A12-4911-A4ED-D0565BEB41A2,0x80,0x64000)
> 
>      BLK0: Alias(s):
>           PciRoot(0x0)/Pci(0x1,0x1)/Ata(0x0)
>      BLK1: Alias(s):
>         � PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)
>      BLK3: Alias(s):
>         �
> PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(2,GPT,F070566B-C58D-4F13-9B92-64F1794385E7,0x64080,0x40000)
> 
>      BLK4: Alias(s):
>         �
> PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(3,GPT,EDE86BE3-C64F-4ACB-B4AA-5E6C0135D335,0xA4080,0x315BF1B)

Good integration test for the emulated NVMe subsystem!

tests/acceptance/boot_linux_console.py should provide trivial template.

> 
> 
> 
> bad:
> 
> Mapping table
>      BLK0: Alias(s):
>           PciRoot(0x0)/Pci(0x1,0x1)/Ata(0x0)
> 
> 
> 
> Thanks,
> 
> Alex
> 
> 
>