Re: [PATCH for-6.0 v2 2/2] hw/block/nvme: disable hotplugging for subsystem-linked controllers

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 2507 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]