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

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

[Peter Maydell]

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

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.

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
 * we do insist that drives aren't connected twice
 * we don't directly check that an if=ide drive is actually plugged
   into an IDE controller (ditto SCSI, virtio, etc etc)

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.

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

[This is almost what we have right now, except we do 1 only
sporadically and accidentally, and so some users are probably
using option combos we'd like to have be illegal]

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

Option C:

Any other reasonable suggestions?


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

thanks
-- PMM


Peter Maydell (3):
  blockdev: Factor out create_implicit_virtio_device
  blockdev: Defer creation of implicit PCI devices for IF_VIRTIO drives
  hw/arm/virt: Make block devices default to virtio

 blockdev.c    | 55 +++++++++++++++++++++++++++++++++++++++----------------
 hw/arm/virt.c |  1 +
 2 files changed, 40 insertions(+), 16 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/3] blockdev: Factor out create_implicit_virtio_device

[Peter Maydell]

Factor out the code which creates the implicit virtio device
for IF_VIRTIO drives, so we can call it from drive_check_orphaned().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 blockdev.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index de94a8b..9cf6123 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -212,6 +212,27 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
     return NULL;
 }
 
+/* Create the virtio device whose existence is implied by the
+ * creation of a block device with if=virtio.
+ */
+static void create_implicit_virtio_device(const char *driveid,
+                                          const char *devaddr)
+{
+    QemuOpts *devopts;
+
+    devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
+                               &error_abort);
+    if (arch_type == QEMU_ARCH_S390X) {
+        qemu_opt_set(devopts, "driver", "virtio-blk-s390", &error_abort);
+    } else {
+        qemu_opt_set(devopts, "driver", "virtio-blk-pci", &error_abort);
+    }
+    qemu_opt_set(devopts, "drive", driveid, &error_abort);
+    if (devaddr) {
+        qemu_opt_set(devopts, "addr", devaddr, &error_abort);
+    }
+}
+
 bool drive_check_orphaned(void)
 {
     BlockBackend *blk;
@@ -929,19 +950,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     }
 
     if (type == IF_VIRTIO) {
-        QemuOpts *devopts;
-        devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
-                                   &error_abort);
-        if (arch_type == QEMU_ARCH_S390X) {
-            qemu_opt_set(devopts, "driver", "virtio-blk-s390", &error_abort);
-        } else {
-            qemu_opt_set(devopts, "driver", "virtio-blk-pci", &error_abort);
-        }
-        qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
-                     &error_abort);
-        if (devaddr) {
-            qemu_opt_set(devopts, "addr", devaddr, &error_abort);
-        }
+        create_implicit_virtio_device(qdict_get_str(bs_opts, "id"), devaddr);
     }
 
     filename = qemu_opt_get(legacy_opts, "file");
-- 
1.9.1

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

[Peter Maydell]

If a user requests an IF_VIRTIO drive on the command line, don't
create the implicit PCI virtio device immediately, but wait until
the rest of the command line has been processed and only create the
device if the drive would otherwise be orphaned. This means that
if the user said drive,id=something,... -device drive=something,..,.
we'll allow the drive to be connected to the user's specified
device rather than stealing it to connect to the implicit virtio
device.

This change does reorder device creation (which will mean a
migration break, and guest visible changes like which PCI
slot implicitly created devices appear in). We can do this
because no machine currently specifies a block_default_type of
IF_VIRTIO except for the S390 machines, and those machines
do not currently support cross-version migration anyway.

Although at first glance it looks like this commit is changing
the behaviour for hotplugged devices, it is not: although
hotplugged devices used to call the code to create an implicit
virtio device, this had no effect because the code in vl.c to
create devices from the devopts list had already run once and
would not be run again.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 blockdev.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 9cf6123..68cdbfe 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -46,6 +46,7 @@
 #include "qmp-commands.h"
 #include "trace.h"
 #include "sysemu/arch_init.h"
+#include "monitor/qdev.h"
 
 static const char *const if_name[IF_COUNT] = {
     [IF_NONE] = "none",
@@ -231,6 +232,15 @@ static void create_implicit_virtio_device(const char *driveid,
     if (devaddr) {
         qemu_opt_set(devopts, "addr", devaddr, &error_abort);
     }
+
+    /* We're called after vl.c has processed the -device options,
+     * so we need to create the device ourselves now.
+     */
+    DeviceState *dev = qdev_device_add(devopts);
+    if (!dev) {
+        exit(1);
+    }
+    object_unref(OBJECT(dev));
 }
 
 bool drive_check_orphaned(void)
@@ -245,6 +255,14 @@ bool drive_check_orphaned(void)
         /* Unless this is a default drive, this may be an oversight. */
         if (!blk_get_attached_dev(blk) && !dinfo->is_default &&
             dinfo->type != IF_NONE) {
+            if (dinfo->type == IF_VIRTIO) {
+                /* An orphaned virtio drive might be waiting for us to
+                 * create the implicit device for it.
+                 */
+                create_implicit_virtio_device(blk_name(blk), dinfo->devaddr);
+                continue;
+            }
+
             fprintf(stderr, "Warning: Orphaned drive without device: "
                     "id=%s,file=%s,if=%s,bus=%d,unit=%d\n",
                     blk_name(blk), blk_bs(blk)->filename, if_name[dinfo->type],
@@ -949,10 +967,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
         goto fail;
     }
 
-    if (type == IF_VIRTIO) {
-        create_implicit_virtio_device(qdict_get_str(bs_opts, "id"), devaddr);
-    }
-
     filename = qemu_opt_get(legacy_opts, "file");
 
     /* Check werror/rerror compatibility with if=... */
-- 
1.9.1

[Qemu-devel] [PATCH v2 3/3] hw/arm/virt: Make block devices default to virtio

[Peter Maydell]

Now we have virtio-pci, we can make the virt board's default block
device type be IF_VIRTIO. This allows users to use simplified
command lines that don't have to explicitly create virtio-pci-blk
devices; the -hda &c very short options now also work.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1b1cc71..1cb56c8 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -961,6 +961,7 @@ static void virt_class_init(ObjectClass *oc, void *data)
     mc->init = machvirt_init;
     mc->max_cpus = 8;
     mc->has_dynamic_sysbus = true;
+    mc->block_default_type = IF_VIRTIO;
 }
 
 static const TypeInfo machvirt_info = {
-- 
1.9.1

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

[Peter Maydell]

On 9 June 2015 at 18:48, Peter Maydell <peter.maydell@linaro.org> wrote:
> Patch 3 is the one-liner to change the default-drive-type. Unfortunately
> just doing that alone will break commandlines that currently work

The other problem with taking patch 3 alone is that it completely
breaks everything, because:
 * by default we create a "cdrom" drive whose type is <board-default>
   and which has no media inserted
 * the virtio-blk device barfs if you give it a drive with no media:
   "Device needs media, but drive is empty"

The S390 systems get around this by specifying no_cdrom = 1, but
it doesn't seem terribly satisfactory that this has to be manually
done by any machine with a virtio default drive type...

-- PMM

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

[Peter Maydell]

On 9 June 2015 at 18:48, Peter Maydell <peter.maydell@linaro.org> wrote:
> 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)

I had a look at getting this to be diagnosed properly, and the sketch
of the patch is:
1. add "bool auto_claimed;" to struct DriveInfo
2. everywhere we grab a DriveInfo and auto-connect it to a device,
   set dinfo->auto_claimed to true
3. in drive_check_orphaned() do this:

+        if (blk_get_attached_dev(blk) && dinfo->type != IF_NONE &&
+            !dinfo->auto_claimed) {
+            /* This drive is attached to something, but it was specified
+             * with if=<something> and it's not attached because it was
+             * automatically claimed by the board code because of the if=
+             * specification. The user probably forgot an if=none.
+             */
+            fprintf(stderr, "Warning: automatic connection of drive specified "
+                    "(using if=%s or by not specifying if= at all) "
+                    "but it was also connected manually (try using
if=none ?): "
+                    "id=%s,file=%s,if=%s,bus=%d,unit=%d\n",
+                    if_name[dinfo->type],
+                    blk_name(blk), blk_bs(blk)->filename, if_name[dinfo->type],
+                    dinfo->bus, dinfo->unit);
+            rs = true;
+        }

(We could distinguish the "using if=" and "not specifying if="
but we'd need another bool in the DriveInfo just to track that for
the benefit of the warning.)

This works, but part 2 is a bit awkward. I think it's the case that
we do auto-claiming if and only if somebody calls blk_by_legacy_dinfo()
on the dinfo, but it seems like a hack to add the "auto_claimed = true"
inside that function... (OTOH, that function's called in about 50
places, so it would be very handy not to need to change all of the
callsites!)

thanks
-- PMM

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

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> 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.

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

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 9 June 2015 at 18:48, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Patch 3 is the one-liner to change the default-drive-type. Unfortunately
>> just doing that alone will break commandlines that currently work
>
> The other problem with taking patch 3 alone is that it completely
> breaks everything, because:
>  * by default we create a "cdrom" drive whose type is <board-default>
>    and which has no media inserted
>  * the virtio-blk device barfs if you give it a drive with no media:
>    "Device needs media, but drive is empty"
>
> The S390 systems get around this by specifying no_cdrom = 1, but
> it doesn't seem terribly satisfactory that this has to be manually
> done by any machine with a virtio default drive type...

In my opinion, no_cdrom, no_parallel, no_floppy and no_sdcard are all
daft.  Flip their sense, and the chance of boards getting them right
increases sharply.  An explicit mc->use_cdrom = 1 in a machine
initialization function for a board without a suitable controller sticks
out and gets fixed.  A missing mc->use_cdrom = 1 for a board with a
suitable controller annoys users and gets fixed.

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

[Peter Maydell]

On 20 June 2015 at 16:00, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> 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.

Yes. Did you see the series I sent out after this one, which
basically just tries to improve the warnings and diagnostics
but is otherwise "option C" ? That's my current favourite...

thanks
-- PMM

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

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 9 June 2015 at 18:48, Peter Maydell <peter.maydell@linaro.org> wrote:
>> 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)
>
> I had a look at getting this to be diagnosed properly, and the sketch
> of the patch is:
> 1. add "bool auto_claimed;" to struct DriveInfo
> 2. everywhere we grab a DriveInfo and auto-connect it to a device,
>    set dinfo->auto_claimed to true
> 3. in drive_check_orphaned() do this:
>
> +        if (blk_get_attached_dev(blk) && dinfo->type != IF_NONE &&
> +            !dinfo->auto_claimed) {
> +            /* This drive is attached to something, but it was specified
> +             * with if=<something> and it's not attached because it was
> +             * automatically claimed by the board code because of the if=
> +             * specification. The user probably forgot an if=none.
> +             */
> +            fprintf(stderr, "Warning: automatic connection of drive specified "
> +                    "(using if=%s or by not specifying if= at all) "
> +                    "but it was also connected manually (try using
> if=none ?): "
> +                    "id=%s,file=%s,if=%s,bus=%d,unit=%d\n",
> +                    if_name[dinfo->type],
> +                    blk_name(blk), blk_bs(blk)->filename, if_name[dinfo->type],
> +                    dinfo->bus, dinfo->unit);
> +            rs = true;
> +        }
>
> (We could distinguish the "using if=" and "not specifying if="
> but we'd need another bool in the DriveInfo just to track that for
> the benefit of the warning.)
>
> This works, but part 2 is a bit awkward. I think it's the case that
> we do auto-claiming if and only if somebody calls blk_by_legacy_dinfo()
> on the dinfo, but it seems like a hack to add the "auto_claimed = true"
> inside that function... (OTOH, that function's called in about 50
> places, so it would be very handy not to need to change all of the
> callsites!)

Adding it to blk_by_legacy_dinfo() is sound when it's called exactly for
the block backends the board claims.  We need to show:

(A) It's called for all block backends the board claims

    Plausible enough, because:

    * Boards claim only drives defined with interface types other than
      IF_NONE.

    * Boards find these drives with drive_get() or its wrappers.  They
      all return DriveInfo.

    * To actually connect a frontend, they need to find the DriveInfo's
      BlockBackend, with blk_by_legacy_dinfo().

(B) It's not called for any block backend the board doesn't claim

    Counter-example: hmp_drive_add().  However, that can only run later,
    so we can ignore it.  Still, not exactly inspiring confidence.

What about this instead:

1. When -device creation connects a qdev_prop_drive property to a
backend, fail when the backend has a DriveInfo and the DriveInfo has
type != IF_NONE.  Note: the connection is made in parse_drive().

2. This breaks -drive if=virtio and possibly more.  That's because for
if=virtio, we create input for -device creation that asks to connect
this IF_VIRTIO drive.  To unbreak it, mark the DriveInfo when we create
such input, and make parse_drive() accept backends so marked.  To find
the places that need to mark, examine users of option group "device".  A
quick, sloppy grep shows a bit over a dozen candidates.  Not too bad.

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

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 20 June 2015 at 16:00, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> 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.
>
> Yes. Did you see the series I sent out after this one, which
> basically just tries to improve the warnings and diagnostics
> but is otherwise "option C" ? That's my current favourite...

I guess you mean "[PATCH 0/4] block: Improve warnings for
doubly-connected drives".  I'll have a look at it next week.