Message ID | 20210423052127.512489-2-its@irrelevant.dk |
---|---|
State | New |
Headers | show |
Series | hw/block/nvme: fix msix uninit | expand |
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
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 :)
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)
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!
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)
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 --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[] = {