[PATCH for-6.0 v2 0/2] hw/block/nvme: fix msix uninit

[PATCH for-6.0 v2 0/2] hw/block/nvme: fix msix uninit

[Klaus Jensen]

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

First patch fixes a regression where msix is not correctly uninit'ed
when an nvme device is hotplugged with device_del. When viewed in
conjunction with the commit that introduced the bug (commit
1901b4967c3f), I think the fix looks relatively obvious.

Second patch disables hotplugging for nvme controllers that are
connected to subsystems since the way namespaces are connected to the
nvme controller bus is messed up by removing the device. This bug causes
a segfault but is *not* a regression and is related to an experimental
feature.

v2:
  - remove memory subregion as well
  - add (possible) patch to disable hotplugging on subsystem connected
    controllers

Klaus Jensen (2):
  hw/block/nvme: fix invalid msix exclusive uninit
  hw/block/nvme: disable hotplugging for subsystem-linked controllers

 hw/block/nvme.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH for-6.0 v2 1/2] 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.

Additionally, a subregion is added but it is not removed on exit which
causes a reference to linger and the drive to never be unlocked.

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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[] = {
-- 
2.31.1

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

[Peter Maydell]

On Fri, 23 Apr 2021 at 06:21, Klaus Jensen <its@irrelevant.dk> 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.
>
> Additionally, a subregion is added but it is not removed on exit which
> causes a reference to linger and the drive to never be unlocked.
>
> 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> 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[] = {

Looks plausible, but if you want this in rc5 could somebody review
it, please ?

thanks
-- PMM

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

[Klaus Jensen]

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

On Apr 23 07:21, 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.
>
>Additionally, a subregion is added but it is not removed on exit which
>causes a reference to linger and the drive to never be unlocked.
>
>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 | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>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[] = {
>-- 
>2.31.1
>

Ping for a review on this please :)

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

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

[Philippe Mathieu-Daudé]

On 4/26/21 6:40 AM, Klaus Jensen wrote:
> On Apr 23 07:21, 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.
>>
>> Additionally, a subregion is added but it is not removed on exit which
>> causes a reference to linger and the drive to never be unlocked.
>>
>> 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 | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> 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[] = {
>> -- 
>> 2.31.1
>>
> 
> Ping for a review on this please :)

You forgot to Cc the maintainers :/ (doing it now).

$ ./scripts/get_maintainer.pl -f include/hw/pci/msix.h
"Michael S. Tsirkin" <mst@redhat.com> (supporter:PCI)
Marcel Apfelbaum <marcel.apfelbaum@gmail.com> (supporter:PCI)

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

[Klaus Jensen]

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

On Apr 26 11:27, Philippe Mathieu-Daudé wrote:
>On 4/26/21 6:40 AM, Klaus Jensen wrote:
>> On Apr 23 07:21, 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.
>>>
>>> Additionally, a subregion is added but it is not removed on exit which
>>> causes a reference to linger and the drive to never be unlocked.
>>>
>>> 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 | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> 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[] = {
>>> -- 
>>> 2.31.1
>>>
>>
>> Ping for a review on this please :)
>
>You forgot to Cc the maintainers :/ (doing it now).
>
>$ ./scripts/get_maintainer.pl -f include/hw/pci/msix.h
>"Michael S. Tsirkin" <mst@redhat.com> (supporter:PCI)
>Marcel Apfelbaum <marcel.apfelbaum@gmail.com> (supporter:PCI)
>

I didnt consider CC'ing the PCI maintainers directly, but makes total 
sense here, thanks!

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

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

[Michael S. Tsirkin]

On Mon, Apr 26, 2021 at 11:27:04AM +0200, Philippe Mathieu-Daudé wrote:
> On 4/26/21 6:40 AM, Klaus Jensen wrote:
> > On Apr 23 07:21, 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.
> >>
> >> Additionally, a subregion is added but it is not removed on exit which
> >> causes a reference to linger and the drive to never be unlocked.
> >>
> >> Fixes: 1901b4967c3f ("hw/block/nvme: move msix table and pba to BAR 0")
> >> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> >> ---
> >> hw/block/nvme.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> 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[] = {
> >> -- 
> >> 2.31.1
> >>
> > 
> > Ping for a review on this please :)
> 
> You forgot to Cc the maintainers :/ (doing it now).
> 
> $ ./scripts/get_maintainer.pl -f include/hw/pci/msix.h
> "Michael S. Tsirkin" <mst@redhat.com> (supporter:PCI)
> Marcel Apfelbaum <marcel.apfelbaum@gmail.com> (supporter:PCI)

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

[Peter Maydell]

On Fri, 23 Apr 2021 at 06:21, Klaus Jensen <its@irrelevant.dk> 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.
>
> Additionally, a subregion is added but it is not removed on exit which
> causes a reference to linger and the drive to never be unlocked.
>
> 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> 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[] = {
> --

Applied this patch (but not patch 2) to master for 6.0; thanks.

-- PMM

[PATCH for-6.0 v2 2/2] hw/block/nvme: disable hotplugging for subsystem-linked controllers

[Klaus Jensen]

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

If a controller is linked to a subsystem, do not allow it to be
hotplugged since this will mess up the (possibly shared) namespaces.

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

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5fe082ec34c5..7606b58a39b9 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -6140,12 +6140,16 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
 
 static int nvme_init_subsys(NvmeCtrl *n, Error **errp)
 {
+    DeviceClass *dc;
     int cntlid;
 
     if (!n->subsys) {
         return 0;
     }
 
+    dc = DEVICE_GET_CLASS(n);
+    dc->hotpluggable = false;
+
     cntlid = nvme_subsys_register_ctrl(n, errp);
     if (cntlid < 0) {
         return -1;
-- 
2.31.1

Re: [PATCH for-6.0 v2 2/2] hw/block/nvme: disable hotplugging for subsystem-linked controllers

[Peter Maydell]

On Fri, 23 Apr 2021 at 06:21, Klaus Jensen <its@irrelevant.dk> wrote:
>
> From: Klaus Jensen <k.jensen@samsung.com>
>
> If a controller is linked to a subsystem, do not allow it to be
> hotplugged since this will mess up the (possibly shared) namespaces.
>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 5fe082ec34c5..7606b58a39b9 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -6140,12 +6140,16 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
>
>  static int nvme_init_subsys(NvmeCtrl *n, Error **errp)
>  {
> +    DeviceClass *dc;
>      int cntlid;
>
>      if (!n->subsys) {
>          return 0;
>      }
>
> +    dc = DEVICE_GET_CLASS(n);
> +    dc->hotpluggable = false;
> +
>      cntlid = nvme_subsys_register_ctrl(n, errp);
>      if (cntlid < 0) {
>          return -1;

I'm not sure this is right -- the DeviceClass is the
class struct, which there's only one of for every instance
of the device in the system. So this is saying "if this instance
is linked to a subsystem, don't let any *future* instances ever
be hotpluggable". I'm not even sure if it will do the right
thing for the current device, because this function is called
from the device's realize method, and the device_set_realized()
function does the "forbid if dc->hotpluggable is false" check
before calling the realize method.

Possibly what you want to do here is to call the
device_get_hotplugged() function and just make the realize
method fail with a suitable error if the device is both (a) being
hotplugged and (b) has a subsystem link; but I'm not an expert on
hotplug, so I might be wrong.

thanks
-- PMM

Re: [PATCH for-6.0 v2 2/2] hw/block/nvme: disable hotplugging for subsystem-linked controllers

[Klaus Jensen]

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

On Apr 23 14:21, Peter Maydell wrote:
>On Fri, 23 Apr 2021 at 06:21, Klaus Jensen <its@irrelevant.dk> wrote:
>>
>> From: Klaus Jensen <k.jensen@samsung.com>
>>
>> If a controller is linked to a subsystem, do not allow it to be
>> hotplugged since this will mess up the (possibly shared) namespaces.
>>
>> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> ---
>>  hw/block/nvme.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> index 5fe082ec34c5..7606b58a39b9 100644
>> --- a/hw/block/nvme.c
>> +++ b/hw/block/nvme.c
>> @@ -6140,12 +6140,16 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
>>
>>  static int nvme_init_subsys(NvmeCtrl *n, Error **errp)
>>  {
>> +    DeviceClass *dc;
>>      int cntlid;
>>
>>      if (!n->subsys) {
>>          return 0;
>>      }
>>
>> +    dc = DEVICE_GET_CLASS(n);
>> +    dc->hotpluggable = false;
>> +
>>      cntlid = nvme_subsys_register_ctrl(n, errp);
>>      if (cntlid < 0) {
>>          return -1;
>
>I'm not sure this is right -- the DeviceClass is the
>class struct, which there's only one of for every instance
>of the device in the system. So this is saying "if this instance
>is linked to a subsystem, don't let any *future* instances ever
>be hotpluggable". I'm not even sure if it will do the right
>thing for the current device, because this function is called
>from the device's realize method, and the device_set_realized()
>function does the "forbid if dc->hotpluggable is false" check
>before calling the realize method.
>
>Possibly what you want to do here is to call the
>device_get_hotplugged() function and just make the realize
>method fail with a suitable error if the device is both (a) being
>hotplugged and (b) has a subsystem link; but I'm not an expert on
>hotplug, so I might be wrong.
>

Thanks Peter, this sounds exactly like what I want. I'll respin!

I have a "full" fix that actually makes the device hotpluggable in the 
context of subsystems, but it is definitely not an -rc thing.

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

Re: [PATCH for-6.0 v2 2/2] hw/block/nvme: disable hotplugging for subsystem-linked controllers

[Peter Maydell]

On Fri, 23 Apr 2021 at 14:25, Klaus Jensen <its@irrelevant.dk> wrote:
>
> On Apr 23 14:21, Peter Maydell wrote:
> >On Fri, 23 Apr 2021 at 06:21, Klaus Jensen <its@irrelevant.dk> wrote:
> >>
> >> From: Klaus Jensen <k.jensen@samsung.com>
> >>
> >> If a controller is linked to a subsystem, do not allow it to be
> >> hotplugged since this will mess up the (possibly shared) namespaces.
> >>
> >> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> >> ---
> >>  hw/block/nvme.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> >> index 5fe082ec34c5..7606b58a39b9 100644
> >> --- a/hw/block/nvme.c
> >> +++ b/hw/block/nvme.c
> >> @@ -6140,12 +6140,16 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
> >>
> >>  static int nvme_init_subsys(NvmeCtrl *n, Error **errp)
> >>  {
> >> +    DeviceClass *dc;
> >>      int cntlid;
> >>
> >>      if (!n->subsys) {
> >>          return 0;
> >>      }
> >>
> >> +    dc = DEVICE_GET_CLASS(n);
> >> +    dc->hotpluggable = false;
> >> +
> >>      cntlid = nvme_subsys_register_ctrl(n, errp);
> >>      if (cntlid < 0) {
> >>          return -1;
> >
> >I'm not sure this is right -- the DeviceClass is the
> >class struct, which there's only one of for every instance
> >of the device in the system. So this is saying "if this instance
> >is linked to a subsystem, don't let any *future* instances ever
> >be hotpluggable". I'm not even sure if it will do the right
> >thing for the current device, because this function is called
> >from the device's realize method, and the device_set_realized()
> >function does the "forbid if dc->hotpluggable is false" check
> >before calling the realize method.
> >
> >Possibly what you want to do here is to call the
> >device_get_hotplugged() function and just make the realize
> >method fail with a suitable error if the device is both (a) being
> >hotplugged and (b) has a subsystem link; but I'm not an expert on
> >hotplug, so I might be wrong.
> >
>
> Thanks Peter, this sounds exactly like what I want. I'll respin!
>
> I have a "full" fix that actually makes the device hotpluggable in the
> context of subsystems, but it is definitely not an -rc thing.

For 6.0 I don't think we should put this in anyway -- it's not
a regression and in any case it sounds like it needs more work...

-- PMM

Re: [PATCH for-6.0 v2 2/2] hw/block/nvme: disable hotplugging for subsystem-linked controllers

[Klaus Jensen]

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

On Apr 23 14:25, Peter Maydell wrote:
>On Fri, 23 Apr 2021 at 14:25, Klaus Jensen <its@irrelevant.dk> wrote:
>>
>> On Apr 23 14:21, Peter Maydell wrote:
>> >On Fri, 23 Apr 2021 at 06:21, Klaus Jensen <its@irrelevant.dk> wrote:
>> >>
>> >> From: Klaus Jensen <k.jensen@samsung.com>
>> >>
>> >> If a controller is linked to a subsystem, do not allow it to be
>> >> hotplugged since this will mess up the (possibly shared) namespaces.
>> >>
>> >> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>> >> ---
>> >>  hw/block/nvme.c | 4 ++++
>> >>  1 file changed, 4 insertions(+)
>> >>
>> >> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>> >> index 5fe082ec34c5..7606b58a39b9 100644
>> >> --- a/hw/block/nvme.c
>> >> +++ b/hw/block/nvme.c
>> >> @@ -6140,12 +6140,16 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
>> >>
>> >>  static int nvme_init_subsys(NvmeCtrl *n, Error **errp)
>> >>  {
>> >> +    DeviceClass *dc;
>> >>      int cntlid;
>> >>
>> >>      if (!n->subsys) {
>> >>          return 0;
>> >>      }
>> >>
>> >> +    dc = DEVICE_GET_CLASS(n);
>> >> +    dc->hotpluggable = false;
>> >> +
>> >>      cntlid = nvme_subsys_register_ctrl(n, errp);
>> >>      if (cntlid < 0) {
>> >>          return -1;
>> >
>> >I'm not sure this is right -- the DeviceClass is the
>> >class struct, which there's only one of for every instance
>> >of the device in the system. So this is saying "if this instance
>> >is linked to a subsystem, don't let any *future* instances ever
>> >be hotpluggable". I'm not even sure if it will do the right
>> >thing for the current device, because this function is called
>> >from the device's realize method, and the device_set_realized()
>> >function does the "forbid if dc->hotpluggable is false" check
>> >before calling the realize method.
>> >
>> >Possibly what you want to do here is to call the
>> >device_get_hotplugged() function and just make the realize
>> >method fail with a suitable error if the device is both (a) being
>> >hotplugged and (b) has a subsystem link; but I'm not an expert on
>> >hotplug, so I might be wrong.
>> >
>>
>> Thanks Peter, this sounds exactly like what I want. I'll respin!
>>
>> I have a "full" fix that actually makes the device hotpluggable in the
>> context of subsystems, but it is definitely not an -rc thing.
>
>For 6.0 I don't think we should put this in anyway -- it's not
>a regression and in any case it sounds like it needs more work...
>

Agree, patch 1 is what I would like to see in if an -rc5 is spun.

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

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

[Klaus Jensen]

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

On Apr 23 07:21, Klaus Jensen wrote:
>From: Klaus Jensen <k.jensen@samsung.com>
>
>First patch fixes a regression where msix is not correctly uninit'ed
>when an nvme device is hotplugged with device_del. When viewed in
>conjunction with the commit that introduced the bug (commit
>1901b4967c3f), I think the fix looks relatively obvious.
>
>Second patch disables hotplugging for nvme controllers that are
>connected to subsystems since the way namespaces are connected to the
>nvme controller bus is messed up by removing the device. This bug causes
>a segfault but is *not* a regression and is related to an experimental
>feature.
>
>v2:
>  - remove memory subregion as well
>  - add (possible) patch to disable hotplugging on subsystem connected
>    controllers
>
>Klaus Jensen (2):
>  hw/block/nvme: fix invalid msix exclusive uninit
>  hw/block/nvme: disable hotplugging for subsystem-linked controllers
>
> hw/block/nvme.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
>-- 
>2.31.1
>
>

Peter,

I know you have a lot of crap on your plate right now, so for the 
record, yes, this is a regression, but not release critical, right?

I am not aware of anyone depending on this unplugging functionality 
(which according to Bug 1925496 is and have always been flaky) in 
production. Basically, as far as I know, all known uses of this device 
are for development and/or testing.

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

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

[Peter Maydell]

On Fri, 23 Apr 2021 at 11:38, Klaus Jensen <its@irrelevant.dk> wrote:
>
> On Apr 23 07:21, Klaus Jensen wrote:
> >From: Klaus Jensen <k.jensen@samsung.com>
> >
> >First patch fixes a regression where msix is not correctly uninit'ed
> >when an nvme device is hotplugged with device_del. When viewed in
> >conjunction with the commit that introduced the bug (commit
> >1901b4967c3f), I think the fix looks relatively obvious.
> >
> >Second patch disables hotplugging for nvme controllers that are
> >connected to subsystems since the way namespaces are connected to the
> >nvme controller bus is messed up by removing the device. This bug causes
> >a segfault but is *not* a regression and is related to an experimental
> >feature.

> I know you have a lot of crap on your plate right now, so for the
> record, yes, this is a regression, but not release critical, right?
>
> I am not aware of anyone depending on this unplugging functionality
> (which according to Bug 1925496 is and have always been flaky) in
> production. Basically, as far as I know, all known uses of this device
> are for development and/or testing.

Thanks for the update. We probably are going to have to roll an rc5
for the network-interface hotplug crash, but it sounds from your
description (especially if hotunplug has always been broken) as if
we could safely leave theses fixes until 6.1.

thanks
-- PMM