On Fri, 23 Apr 2021 at 14:25, Klaus Jensen <i...@irrelevant.dk> wrote:
>
> On Apr 23 14:21, Peter Maydell wrote:
> >On Fri, 23 Apr 2021 at 06:21, Klaus Jensen <i...@irrelevant.dk> wrote:
> >>
> >> From: Klaus Jensen <k.jen...@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.jen...@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
