diff mbox series

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

Message ID 20210423052127.512489-2-its@irrelevant.dk
State New
Headers show
Series hw/block/nvme: fix msix uninit | expand

Commit Message

Klaus Jensen April 23, 2021, 5:21 a.m. UTC 
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(-)

Comments

Peter Maydell April 23, 2021, 3:46 p.m. UTC | #1 
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
Klaus Jensen April 26, 2021, 4:40 a.m. UTC | #2 
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 :)
Philippe Mathieu-Daudé April 26, 2021, 9:27 a.m. UTC | #3 
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)
Klaus Jensen April 26, 2021, 9:39 a.m. UTC | #4 
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!
Michael S. Tsirkin April 26, 2021, 1:08 p.m. UTC | #5 
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)
Peter Maydell April 26, 2021, 3:23 p.m. UTC | #6 
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
diff mbox series

Patch

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