| Message ID | 20210423052127.512489-3-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> > > 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
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.
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
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.
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;