Re: [PATCHv2 0/5] qemu: Introduce nvme disk emulation support
On Wed, May 07, 2025 at 15:56:52 +0200, Martin Kletzander wrote:
On Sun, Apr 27, 2025 at 07:48:02PM +0800, honglei.wang(a)smartx.com
wrote:
> From: hongleiwang <honglei.wang(a)smartx.com>
>
> QEMU has supported nvme disk emulation for a long time,
> see: https://qemu-project.gitlab.io/qemu/system/devices/nvme.html.
>
> The following patches introduce nvme-ns disk bus type:
Thanks for the v2. I have some suggestions which I'd like some more
feedback on, which might be controversial, hence the added Cc's for
people who replied to any of the versions.
> A disk with nvme-ns as bus is represented as an nvme namespace
> and needs to be attached to an nvme controller. In XML, it can be
> used like this:
> <devices>
> ...
> <disk type='file' device='disk'>
> <driver name='qemu' type='raw'/>
> <source file='/tmp/data.img'/>
> <target dev='nvmensa' bus='nvme-ns'/>
Since we're not using the bus='nvme', what if we abandoned the qemu
naming and not specify the -ns; on top of that it would be nicer to have
the device naming same as it actually happens on Linux. That is kind of
biased, but we already do that for other disk types.
Keep in mind that we explicitly document that 'dev' may not match what
the device becomes in the guest ...
The above would then become:
<target dev='nvme0n1' bus='nvme'/>
... in this case it could be more problematic because the disk
target/dev doesn't force you to put nvme0n1 and nvme0n2 on the same
controller nor does the namespace bit need to correspond to the
namespace id.
Yes it is true that with other buses you still can mix stuff up if you
configure it explicitly.
Because that means a slightly bigger change to how we do things
currently (bunch of the code depends on the original numbering) I'd be
willing to submit the XML part of this series with that change. One
of the reasons is that I also need it for support in a different
driver.
I guess that is fair. I still think that just doing 'nvmea', 'nvmeb',
'nvmec' etc should be sufficient here, but the code already has
provisions for partition numbers so this shouldn't be much different.