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

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> This is a respin of the patchset I sent out last week; the
> only change v1->v2 is to drop the special casing of S390, as
> Christian confirmed it isn't needed. This collapses the old
> patches 2/3 into one.
>
> I'm going to use this cover letter as an opportunity to (a) hopefully
> be a bit clearer about the intention and (b) try to summarise the
> current behaviour and possible improvements.
>
> Patchset Rationale
> ==================
>
> blockdev.c will create implicit virtio-blk-* devices for IF_VIRTIO
> drives.

Yes.  Let me elaborate a bit.

-drive and HMP drive_add wrap around blockdev.c's drive_new().
drive_new() is pretty baroque.  Grown, not designed.  High-level view of
what it does:

* It always creates a block backend.

* It always records block interface type (parameter "if"), bus and unit
  (parameters "bus", "unit" or "index") for later use.

* It always makes up an ID if the user didn't provide one (parameter
  "id").  How it makes it up depends on the interface type.

* For interface type IF_VIRTIO, it does the equivalent of -device
  virtio-blk-pci,drive=ID.  Except for s390x it uses virtio-blk-ccw
  instead.  If parameter "addr" is given, it adds addr=ADDR to the
  -device argument.

* More, but it's of little interest here.

The important part for us is that drive_new() creates just the backend.
Creating the frontend is somebody else's problem.

*Except* for IF_VIRTIO.  There, drive_new() adds frontend configuration
using the -device infrastructure.  This infrastructure then takes care
of creating the frontend.  Happens after machine initialization.  Can
fail.  Example:

    $ qemu-system-arm -M spitz -drive if=virtio
    qemu-system-arm: -drive if=virtio: No 'PCI' bus found for device 
'virtio-blk-pci'

For IF_NONE, creating the frontend is the user's problem.

For anything else, it's the board's problem.  That's where "records
interface type, bus and unit" comes in: that's how the board finds the
drives it's supposed to create frontends for.

However, the board may not do it for all of them.  Example:

    $ qemu-system-arm -M virt -drive if=ide
    Warning: Orphaned drive without device: 
id=ide0-hd0,file=,if=ide,bus=0,unit=0

This board silently ignores this drive.  Generic code detects that, and
warns.

Parameter "if" defaults to a board-specific interface type.  For
historical reasons, it usually defaults to IF_IDE.  Even when the board
doesn't implement IF_IDE.  Example:

    $ qemu-system-arm -M virt foo.qcow2 
    Warning: Orphaned drive without device: 
id=ide0-hd0,file=foo.qcow2,if=ide,bus=0,unit=0

Non-option argument foo.qcow2 is sugar for -drive
media=disk,index=0,file=foo.qcow2.  With machine type "virt", interface
type defaults to IF_IDE.  "virt" doesn't do IF_IDE.

Yes, that's stupid.

>         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,
> including the "-hda" very-short convenience options. In particular I'm
> getting justified complaints that the virt board's command line
> requirements for disk specification are complicated and different from x86.
>
> (The virt board currently ends up with the default default drive type,
> which is IDE, which is totally useless as there is no IDE controller.)

Exactly.

> Patch 3 is the one-liner to change the default-drive-type. Unfortunately
> just doing that alone will break commandlines that currently work and
> are I think relatively common with people using this board, like:
>
>   -drive file=arm-wheezy.qcow2,id=foo
>   -device virtio-blk-pci,drive=foo
>
> (Note the lack of an if= specifier.) This works in current master
> because nothing complains about the fact you've (implicitly) specified
> an if=ide drive on a machine with no IDE drive, or about the fact
> you've just plugged an if=ide drive into a virtio-blk device.

Yes.  It's undocumented usage, but since it has always worked, it may
have hardened into ABI.

> I *really* want to be able to set the default drive type to IF_VIRTIO;
> if I have to break command lines like the above in order to do so,
> I can put up with that, but I'm hoping we can do better than that.
>
> Current situation
> =================
>
> At the moment we somewhat under-check uses of the if=foo option to
> drives:
>  * we do check that drives are not completely orphaned

Except for if=none, because the user may want to define an "orphan"
backend now, and hot-plug its frontend later.

>  * we do insist that drives aren't connected twice

Yes.

>  * we don't directly check that an if=ide drive is actually plugged
>    into an IDE controller (ditto SCSI, virtio, etc etc)

To be precice: the if=ide backend is actually plugged into an IDE
frontend.

Two ways that could happen:

1. Board code makes the connection.  Programming error.  Unlikely.

2. The user makes the connection with -device or device_add.

Perhaps we could sort all the block device models into interface type
buckets, but right now we don't.  However, as long as we only want to
check 2., we don't have to: simply require if=none with -device &
friends.

*Except* for the -device-like frontend configuration created on behalf
 of if=virtio.

> The "no double connection" rule means we will indirectly notice
> some if= mismatches: if the machine has a bus of the correct type
> and claims an if=foo drive, then this will get a connected-twice
> error (from the automatic-claiming, and from the user-specified
> commandline connection). However if the machine doesn't have a bus
> of the relevant type and so doesn't auto-claim the drive, we won't
> detect any problem. So x86_64 is inconsistent:
>
>  qemu-system-x86_64 -nodefaults -display none -drive 
> if=scsi,file=tmp.qcow2,id=foo -device ide-hd,drive=foo
>
> fails, but
>
>  qemu-system-x86_64 -nodefaults -display none -drive 
> if=sd,file=tmp.qcow2,id=foo -device ide-hd,drive=foo
>
> does not, and will connect an "if=sd" drive to the IDE controller.

Yes, and that's stupid.  Both should fail.

Trouble is making -device drive=foo fail when foo's interface type isn't
IF_NONE could break some (ill-advised) existing command lines.  That's
why we're talking.

Things that would break in decreasing order of silliness:

* Using backends defined with anything but -drive (e.g. -hda) with
  -device.  So silly I wouldn't worry about it.

* Using backends defined with -drive and an explicit if=T with -device.
  Silly enough not to worry?

* Using backends defined with -drive and *no* if= with -device.
  Unfortunately, this one's probably not silly enough not to worry.

> Where we might like to be
> =========================
>
> Some possibilities, presented without regard to back-compat:
>
> Option A:
>
> 1) explicit if=foo drive manually wired up to a device is an error
>    (and always diagnosed as such, not indirectly via it being double-used)
> 2) explicit if=none drive is never auto-claimed
> 3a) drive with no if= is exactly as if the user explicitly stated
>     if=<board-default-type> and must be auto-claimed

No board currently has default type IF_NONE.  If we had such boards,
we'd have to qualify "must be auto-claimed" with "when the board's
default type is IF_NONE".

Interface types other IF_NONE not claimed by the board become useless.
That's intentional.  Somewhat embarrassing when the interface type is
the board's default.  Therefore, you may want to add

  4) change board default interface types to actually make sense

> [This is almost what we have right now, except we do 1 only
> sporadically and accidentally,

and 3a) is currently only a warning, not an error.

>                                and so some users are probably
> using option combos we'd like to have be illegal]

In particular, it breaks the example you gave above:

    $ qemu-system-arm -M virt -drive file=arm-wheezy.qcow2,id=foo -device 
virtio-blk-pci,drive=foo

To fix, add if=none to -drive.

> Option B:
>
> 1) and 2) as above, but instead of 3a):
>
> 3b) drive with no if=... means "if user wired it up to something on the
>     command line, honour that; otherwise treat as if=<board default> and
>     auto-claim
>
> [This tries to make at least some of the option-combos do the
> plausibly right thing. This series is trying to add that for virtio,
> but other if= types are probably harder to deal with. I think the
> semantics here are nice but I'm struggling to see how to implement it.]

Device creation for -device (both the user's and implicit additions like
the stuff added by drive_new() for if=virtio) happens after machine
initialization.  By that time the board has already claimed the
backends.

As long as we stick to that order, the board needs to predict what the
-device creation is going to do to avoid claiming the backends the
-device creation will want to claim.

All we have then is device configuration (QemuOpts in option group
"device") and device model meta-data.  We know how qdev_device_add()
uses QemuOpts parameters to select and create the device model, and to
set its object properties.

I'm afraid we'd have to basically dry-run that code, either by hacking
it up to be dry-run-capable, or hacking up a copy of it.  Sounds like a
fairly big chunk of work to me.

Fragile hack: instead, assume any parameter matching /^drive.*$/ in
option group "device" will correspond to a qdev_prop_drive device model
property.  I'm not sure this is even true, and even if it is, I'd rather
not bet on it remaining true.

We could try flipping the order by splitting onboard block device
creation off machine initialization, so we can run it after -device
creation.  I'm afraid that's a Big Chunk of Nasty Work.  Could be prone
to subtle robustness bugs where some unlucky -device trips up the
onboard block device creation that follows it.

In addition to being hard or/and fragile, the proposed special treatment
of -drive without if= feels uncomfortably magical to me.

> Option C:
>
> Any other reasonable suggestions?

Do nothing, just change the default interface types for some boards you
care about.

Less breakage than option A (not sure how much in practice).  Less
consistency, too.

Spreads out the pain until we're done changing default interface types.
Option A concentrates the pain, and adds consistency.

Option B tries to trade a bit of consistency and a whole lot of work and
complexity for pain minimization.  Not sure it's a good deal, even if we
can pull it off.

> If we can decide what we'd like the semantics to be (ideally with
> some idea of how to implement them ;-)) I can have a stab at
> writing some patches.
>
> I do definitely want to enable short-options for virt for 2.4...

"Enable short options" = change virt's default block interface type from
IF_IDE to IF_VIRTIO, I presume.

"For 2.4" rules out solutions requiring big chunks work, I'm afraid.

Option C is obviously feasible.

Option A may well be.  We might have to bend the freeze rules a bit.

Option B is not, as far as I can tell.  Beware, I don't like it, which
means bias might cloud my judgement.

Hope this helps at least a little.