Re: [PATCH] hw/block/nvme: re-enable NVMe PCI hotplug

From: Klaus Jensen <its@irrelevant.dk>
To: Hannes Reinecke <hare@suse.de>
Cc: Kevin Wolf <kwolf@redhat.com>,
	qemu-block@nongnu.org, qemu-devel@nongnu.org,
	Keith Busch <keith.busch@wdc.com>
Subject: Re: [PATCH] hw/block/nvme: re-enable NVMe PCI hotplug
Date: Tue, 11 May 2021 18:03:54 +0200	[thread overview]
Message-ID: <YJqq6rTRTL3mxMK6@apples.localdomain> (raw)
In-Reply-To: <27cc0341-3a32-4a75-f5fd-9987b1b37799@suse.de>

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

On May 11 16:54, Hannes Reinecke wrote:
>On 5/11/21 3:37 PM, Klaus Jensen wrote:
>> On May 11 15:12, Hannes Reinecke wrote:
>>> On 5/11/21 2:22 PM, Klaus Jensen wrote:
>[ .. ]
>>>> The hotplug fix looks good - I'll post a series that tries to integrate
>>>> both.
>>>>
>>> Ta.
>>>
>>> The more I think about it, the more I think we should be looking into
>>> reparenting the namespaces to the subsystem.
>>> That would have the _immediate_ benefit that 'device_del' and
>>> 'device_add' becomes symmetric (ie one doesn't have to do a separate
>>> 'device_add nvme-ns'), as the nvme namespace is not affected by the
>>> hotplug event.
>>>
>>
>> I have that working, but I'm struggling with a QEMU API technicality in
>> that I apparently cannot simply move the NvmeBus creation to the
>> nvme-subsys device. For some reason the bus is not available for the
>> nvme-ns devices. That is, if one does something like this:
>>
>>   -device nvme-subsys,...
>>   -device nvme-ns,...
>>
>> Then I get an error that "no 'nvme-bus' bus found for device 'nvme'ns".
>> This is probably just me not grok'ing the qdev well enough, so I'll keep
>> trying to fix that. What works now is to have the regular setup:
>>
>_Normally_ the 'id' of the parent device spans a bus, so the syntax
>should be
>
>-device nvme-subsys,id=subsys1,...
>-device nvme-ns,bus=subsys1,...

Yeah, I know, I just oversimplified the example. This *is* how I wanted 
it to work ;)

>
>As for the nvme device I would initially expose any namespace from the
>subsystem to the controller; the nvme spec has some concept of 'active'
>or 'inactive' namespaces which would allow us to blank out individual
>namespaces on a per-controller basis, but I fear that's not easy to
>model with qdev and the structure above.
>

The nvme-ns device already supports the boolean 'detached' parameter to 
support the concept of an inactive namespace.

>>   -device nvme-subsys,...
>>   -device nvme,...
>>   -device nvme-ns,...
>>
>> And the nvme-ns device will then reparent to the NvmeBus on nvme-subsys
>> (which magically now IS available when nvme-ns is realized). This has
>> the same end result, but I really would like that the namespaces could
>> be specified as children of the subsys directly.
>>
>Shudder.
>Automatic reparenting.
>To my understanding from qdev that shouldn't even be possible.
>Please don't.
>

It's perfectly possible with the API and used to implement stuff like 
failover. We are not changing the parent object, we are changing the 
parent bus. hw/sd does something like this (but does mention that its a 
bit of a hack). In this case I'd say we could argue to get away with it 
as well. 

Allowing the nvme-ns device to be a child of the controller allows the 
initially attached controller of non-shared namespaces to be easily 
expressible. But I agree, the approach is a bit wacky, which is why I 
havnt posted anything yet.

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