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

[Peter Maydell]

On 8 June 2015 at 15:18, Markus Armbruster <address@hidden> wrote:
> 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.

Yes, but since we didn't reject these command lines, they
worked anyway. (We should ideally have rejected them as "hey
you asked for an IDE drive but it wasn't connected to anything".)
So we should avoid breaking them now.

> 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.

Indeed.

>> 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?

We're only rejecting it by accident, I think, because the PC machine
happens to support SCSI. If you remove the pc_pci_device_init()
code in pc.c which creates SCSI adaptors, then we will happily
accept the command line and plug the if=scsi drive into the
IDE adaptor.

Similarly, the pc machine will currently accept the combination
 -drive if=sd,...,id=foo -device ide=hd,drive=foo

If we care about catching mismatched drive-types and devices, we
should actually check that, rather than rejecting them if the
machine supports devices for that drive type and ignoring the
mismatch if it does not.

>> 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?

I haven't tested creating anything via the monitor. I merely
noted that the other code path calling drive_new() is via
hmp_drive_add(). (I notice now that that will actually fail
on anything other than an IF_NONE drive, so it's just as well
we weren't creating devices for IF_VIRTIO, or we'd probably
have leaked them.)

>>                                                           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.

It's not supposed to have implications, it's supposed to be
a "no behaviour change" patch :-)

Without the s390 special casing patches 2 and 3 collapse down
into a single patch, incidentally.

-- PMM