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