Message ID | 20210209073101.548811-33-its@irrelevant.dk |
---|---|
State | New |
Headers | show |
Series | [PULL,01/56] hw/block/nvme: remove superfluous NvmeCtrl parameter | expand |
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 > > >
On Feb 11 12:40, Philippe Mathieu-Daudé wrote: > 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. > Yes. This is something we really need. I will priorities this together with documentation :) Thanks for pointing me in the direction of this Philippe.
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[] = {