Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] blockdev: Defer creation of implicit PCI devices for IF_VIRTIO drives

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> blockdev.c will create implicit virtio-blk-* devices for IF_VIRTIO
> drives. I want to turn this on for the ARM virt board (now it has PCI),
> so that users can use shorter and more comprehensible command lines.

I had to read further until understood your goal: you want to make
-drive default to if=virtio for this machine type.  It currently
defaults to if=ide, but the board doesn't pick those up.

> Unfortunately, the code as it stands will always create an implicit
> device, which means that setting the virt block_default_type to IF_VIRTIO
> would break existing command lines like:
>
>   -drive file=arm-wheezy.qcow2,id=foo
>   -device virtio-blk-pci,drive=foo

This works basically by accident.  The documented way to do it is -drive
if=none.  Omitting if= like you do defaults to the board's
block_default_type, in this case if=ide.

With a board that actually implements if=ide, such as a PC, your command
line is correctly rejected:

    -device virtio-blk-pci,drive=foo: Property 'virtio-blk-device.drive' can't 
take value 'foo', it's in use

Below the hood, the board creates a suitable device model and attaches
the drive to it.  When -device tries to attach, it fails.

With a board that doesn't implement if=ide, such as ARM virt, the drive
remains unconnected.  Evidence: without the -device using it, we get

    Warning: Orphaned drive without device: 
id=foo,file=arm-wheezy.qcow2,if=ide,bus=0,unit=0

Since the drive remains unconnected, -device using it succeeds, even
though if=ide.  -device should really require if=none.  But since it has
never bothered to, this misuse may have hardened into de facto ABI.

Same for any block_default_type other than IF_NONE.

> because the creation of the drive causes us to create an implied
> virtio-blk-pci which steals the drive, and then the explicit
> device creation fails because 'foo' is already in use.

Yes, changing a board's block_default_type changes the meaning of -drive
without if=none.  Works as designed :)

> This patchset fixes this problem by deferring the creation of the
> implicit devices to drive_check_orphaned(), which means we can do
> it only for the drives which haven't been explicitly connected to
> a device by the user.

This changes QEMU from rejecting a certain pattern of incorrect usage to
guessing what the user meant.  Example:

    $ qemu-system-x86_64 -nodefaults -display none -drive 
if=virtio,file=tmp.qcow2,id=foo -device ide-hd,drive=foo

The user asks for the drive to be both a virtio and an IDE device, which
is of course nonsense.

Before your patch, we reject the nonsense:

    qemu-system-x86_64: -drive if=virtio,file=tmp.qcow2,id=foo: Property 
'virtio-blk-device.drive' can't take value 'foo', it's in use

Afterwards, we accept it, guessing the user really meant -device ide=hd,
and not if=virtio.

But if you try the same with if=scsi, we still reject it.

Is this really a good idea?

> The slight downside of deferral is that it is effectively rearranging
> the order in which the devices are created, which means they will
> end up in different PCI slots, etc. We can get away with this for PCI,
> because at the moment the only machines which set their block_default_type
> to IF_VIRTIO are the S390X ones. I have special-cased S390X to retain
> the old behaviour, which is a bit ugly but functional. If S390X doesn't
> care about cross-version migration we can probably drop that and
> just have everything defer. S390X people?
>
> The code in master didn't seem to take much account of the possibility
> of hotplug -- if the user created a drive via the monitor

Full monitor command, please?

>                                                           we would
> apparently try to create the implicit drive, but in fact not do so
> because vl.c had already done device creation long before. I've included
> a patch that makes it more explicit that hotplug does not get you the
> magic implicit devices.

PATCH 2/4.  Haven't thought through its implications.

> The last patch is the oneliner to enable the default for virt once
> the underlying stuff lets us do this without breaking existing user
> command lines.