On Apr 23 14:25, Peter Maydell wrote:
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...
Agree, patch 1 is what I would like to see in if an -rc5 is spun.
signature.asc
Description: PGP signature
