[PATCH for-6.0? 0/1] hw/block/nvme: fix msix uninit

[PATCH for-6.0? 0/1] hw/block/nvme: fix msix uninit

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Hi Peter,

The commit message on the patch describes the issue. This is a QEMU
crashing bug in -rc4 that I introduced early in the cycle and never
found in time. Lack of testing device hotplugging is the cause for that.

I'm not sure what to say other than I'm terribly sorry for introducing
this and if this means an -rc5 needs to be rolled, then I'm even more
sorry.

I think an acceptance test could have caught this, and I am already
working on an acceptance test suite for the nvme device, so I'll add
something that test this as well. But, well, it doesn't help now.

Klaus Jensen (1):
  hw/block/nvme: fix invalid msix exclusive uninit

 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.31.1

[PATCH for-6.0? 1/1] hw/block/nvme: fix invalid msix exclusive uninit

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Commit 1901b4967c3f changed the nvme device from using a bar exclusive
for MSI-x to sharing it on bar0.

Unfortunately, the msix_uninit_exclusive_bar() call remains in
nvme_exit() which causes havoc when the device is removed with, say,
device_del.

Fix this.

Fixes: 1901b4967c3f ("hw/block/nvme: move msix table and pba to BAR 0")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 624a1431d072..31a1a59d88c9 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -6235,7 +6235,7 @@ static void nvme_exit(PCIDevice *pci_dev)
     if (n->pmr.dev) {
         host_memory_backend_set_mapped(n->pmr.dev, false);
     }
-    msix_uninit_exclusive_bar(pci_dev);
+    msix_uninit(pci_dev, &n->bar0, &n->bar0);
 }
 
 static Property nvme_props[] = {
-- 
2.31.1

Re: [PATCH for-6.0? 1/1] hw/block/nvme: fix invalid msix exclusive uninit

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1840 bytes --]

On Apr 22 15:58, Klaus Jensen wrote:
>From: Klaus Jensen <k.jensen@samsung.com>
>
>Commit 1901b4967c3f changed the nvme device from using a bar exclusive
>for MSI-x to sharing it on bar0.
>
>Unfortunately, the msix_uninit_exclusive_bar() call remains in
>nvme_exit() which causes havoc when the device is removed with, say,
>device_del.
>
>Fix this.
>
>Fixes: 1901b4967c3f ("hw/block/nvme: move msix table and pba to BAR 0")
>Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>---
> hw/block/nvme.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>index 624a1431d072..31a1a59d88c9 100644
>--- a/hw/block/nvme.c
>+++ b/hw/block/nvme.c
>@@ -6235,7 +6235,7 @@ static void nvme_exit(PCIDevice *pci_dev)
>     if (n->pmr.dev) {
>         host_memory_backend_set_mapped(n->pmr.dev, false);
>     }
>-    msix_uninit_exclusive_bar(pci_dev);
>+    msix_uninit(pci_dev, &n->bar0, &n->bar0);
> }
>
> static Property nvme_props[] = {
>-- 
>2.31.1
>

Having investigated more, the proper fix must also delete the subregion 
that 1901b4967c3f introduced. Otherwise a reference lingers, causing the 
attached drive to never be "unlocked".

my_bad_counter++;

I found this screw-up while investigating Bug 1925496, so credit to Oguz 
Bektas for reporting this!


diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 624a1431d072..5fe082ec34c5 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -6235,7 +6235,8 @@ static void nvme_exit(PCIDevice *pci_dev)
      if (n->pmr.dev) {
          host_memory_backend_set_mapped(n->pmr.dev, false);
      }
-    msix_uninit_exclusive_bar(pci_dev);
+    msix_uninit(pci_dev, &n->bar0, &n->bar0);
+    memory_region_del_subregion(&n->bar0, &n->iomem);
  }

  static Property nvme_props[] = {



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH for-6.0? 0/1] hw/block/nvme: fix msix uninit

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

On Apr 22 15:58, Klaus Jensen wrote:
>From: Klaus Jensen <k.jensen@samsung.com>
>
>Hi Peter,
>
>The commit message on the patch describes the issue. This is a QEMU
>crashing bug in -rc4 that I introduced early in the cycle and never
>found in time. Lack of testing device hotplugging is the cause for that.
>
>I'm not sure what to say other than I'm terribly sorry for introducing
>this and if this means an -rc5 needs to be rolled, then I'm even more
>sorry.
>
>I think an acceptance test could have caught this, and I am already
>working on an acceptance test suite for the nvme device, so I'll add
>something that test this as well. But, well, it doesn't help now.
>
>Klaus Jensen (1):
>  hw/block/nvme: fix invalid msix exclusive uninit
>
> hw/block/nvme.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>-- 
>2.31.1
>
>

As far as I can tell, to cause this crash, monitor access is required, 
so I am not sure if we can get away with a note on this in the release 
notes and fix this in a potential stable release or next.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH for-6.0? 0/1] hw/block/nvme: fix msix uninit

[Peter Maydell]

On Thu, 22 Apr 2021 at 15:07, Klaus Jensen <its@irrelevant.dk> wrote:
>
> On Apr 22 15:58, Klaus Jensen wrote:
> >From: Klaus Jensen <k.jensen@samsung.com>
> >
> >Hi Peter,
> >
> >The commit message on the patch describes the issue. This is a QEMU
> >crashing bug in -rc4 that I introduced early in the cycle and never
> >found in time. Lack of testing device hotplugging is the cause for that.
> >
> >I'm not sure what to say other than I'm terribly sorry for introducing
> >this and if this means an -rc5 needs to be rolled, then I'm even more
> >sorry.
> >
> >I think an acceptance test could have caught this, and I am already
> >working on an acceptance test suite for the nvme device, so I'll add
> >something that test this as well. But, well, it doesn't help now.

> As far as I can tell, to cause this crash, monitor access is required,
> so I am not sure if we can get away with a note on this in the release
> notes and fix this in a potential stable release or next.

Is this a regression since 5.2 ?

thanks
-- PMM

Re: [PATCH for-6.0? 0/1] hw/block/nvme: fix msix uninit

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

On Apr 22 19:58, Peter Maydell wrote:
>On Thu, 22 Apr 2021 at 15:07, Klaus Jensen <its@irrelevant.dk> wrote:
>>
>> On Apr 22 15:58, Klaus Jensen wrote:
>> >From: Klaus Jensen <k.jensen@samsung.com>
>> >
>> >Hi Peter,
>> >
>> >The commit message on the patch describes the issue. This is a QEMU
>> >crashing bug in -rc4 that I introduced early in the cycle and never
>> >found in time. Lack of testing device hotplugging is the cause for that.
>> >
>> >I'm not sure what to say other than I'm terribly sorry for introducing
>> >this and if this means an -rc5 needs to be rolled, then I'm even more
>> >sorry.
>> >
>> >I think an acceptance test could have caught this, and I am already
>> >working on an acceptance test suite for the nvme device, so I'll add
>> >something that test this as well. But, well, it doesn't help now.
>
>> As far as I can tell, to cause this crash, monitor access is required,
>> so I am not sure if we can get away with a note on this in the release
>> notes and fix this in a potential stable release or next.
>
>Is this a regression since 5.2 ?
>

Yes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]