[Qemu-devel] [PATCH v2 0/8] More sensible default for -drive interface type

[Qemu-devel] [PATCH v2 0/8] More sensible default for -drive interface type

[Markus Armbruster]

Block backends defined with -drive if=T, T!=none are meant to be
picked up by machine initialization code: a suitable frontend gets
created and wired up automatically.

if=T drives not picked up that way can still be used with -device as
if they had if=none, but that's unclean and best avoided.  Unused ones
produce an "Orphaned drive without device" warning.

Many machine types default to if=ide, even though they don't actually
have an IDE controller.  A few default to if=scsi, even though they
lack a SCSI HBA.  Change their default to if=none.

While their, fix handling of index and unit for a few machines with
AHCI controllers.

With the improved default, unintentional definition of orphaned drives
should be unlikely.  Improve the "orphaned drive" warning and turn it
into an error.  Drop a few special cases of this error that are now
redundant.

v2:
* PATCH v1 6/6 dropped; if=scsi eccentricities will be addressed in a
  series of its own
* PATCH 6-8 new

Markus Armbruster (8):
  hw: Default -drive to if=ide explicitly where it works
  hw/arm/cubieboard hw/arm/xlnx-ep108: Fix units_per_default_bus
  hw: Default -drive to if=none instead of ide when ide cannot work
  hw: Default -drive to if=none instead of scsi when scsi cannot work
  hw/arm/highbank: Default -drive to if=ide instead of if=scsi
  blockdev: Improve message for orphaned -drive
  blockdev: Make orphaned -drive fatal
  hw: Drop superfluous special checks for orphaned -drive

 blockdev.c                | 21 +++++++++++----------
 hw/alpha/dp264.c          |  1 +
 hw/arm/cubieboard.c       |  4 ++++
 hw/arm/highbank.c         |  8 ++++++--
 hw/arm/realview.c         |  1 -
 hw/arm/spitz.c            |  3 +++
 hw/arm/tosa.c             |  1 +
 hw/arm/vexpress.c         |  1 -
 hw/arm/xilinx_zynq.c      |  1 -
 hw/arm/xlnx-ep108.c       |  6 ++++++
 hw/i386/pc.c              |  1 +
 hw/ide/core.c             | 17 -----------------
 hw/mips/mips_fulong2e.c   |  1 +
 hw/mips/mips_jazz.c       |  4 ----
 hw/mips/mips_malta.c      |  1 +
 hw/mips/mips_r4k.c        |  1 +
 hw/ppc/mac_newworld.c     |  1 +
 hw/ppc/mac_oldworld.c     |  1 +
 hw/ppc/prep.c             |  1 +
 hw/sh4/r2d.c              |  1 +
 hw/sparc/sun4m.c          |  5 -----
 hw/sparc64/sun4u.c        |  2 ++
 include/sysemu/blockdev.h | 11 +++++------
 23 files changed, 47 insertions(+), 47 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/8] hw: Default -drive to if=ide explicitly where it works

[Markus Armbruster]

Block backends defined with -drive if=ide are meant to be picked up by
machine initialization code: a suitable frontend gets created and
wired up automatically.

if=ide drives not picked up that way can still be used with -device as
if they had if=none, but that's unclean and best avoided.  Unused ones
produce an "Orphaned drive without device" warning.

-drive parameter "if" is optional, and the default depends on the
machine type.  If a machine type doesn't specify a default, the
default is "ide".

Many machine types default to if=ide, even though they don't actually
have an IDE controller.  A future patch will change these defaults to
something more sensible.  To prepare for it, this patch makes default
"ide" explicit for the machines that actually pick up if=ide drives:

* alpha: clipper
* arm/aarch64: spitz borzoi terrier tosa
* i386/x86_64: generic-pc-machine (with concrete subtypes pc-q35-*
  pc-i440fx-* pc-* isapc xenfv)
* mips64el: fulong2e
* mips/mipsel/mips64el: malta mips
* ppc/ppc64: mac99 g3beige prep
* sh4/sh4eb: r2d
* sparc64: sun4u sun4v

Note that ppc64 machine powernv already sets an "ide" default
explicitly.  Its IDE controller isn't implemented, yet.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/alpha/dp264.c        | 1 +
 hw/arm/spitz.c          | 3 +++
 hw/arm/tosa.c           | 1 +
 hw/i386/pc.c            | 1 +
 hw/mips/mips_fulong2e.c | 1 +
 hw/mips/mips_malta.c    | 1 +
 hw/mips/mips_r4k.c      | 1 +
 hw/ppc/mac_newworld.c   | 1 +
 hw/ppc/mac_oldworld.c   | 1 +
 hw/ppc/prep.c           | 1 +
 hw/sh4/r2d.c            | 1 +
 hw/sparc64/sun4u.c      | 2 ++
 12 files changed, 15 insertions(+)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index d6431fd..85405da 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -177,6 +177,7 @@ static void clipper_machine_init(MachineClass *mc)
 {
     mc->desc = "Alpha DP264/CLIPPER";
     mc->init = clipper_init;
+    mc->block_default_type = IF_IDE;
     mc->max_cpus = 4;
     mc->is_default = 1;
 }
diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index 949a15a..fe2d5a7 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -998,6 +998,7 @@ static void spitzpda_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "Sharp SL-C3000 (Spitz) PDA (PXA270)";
     mc->init = spitz_init;
+    mc->block_default_type = IF_IDE;
 }
 
 static const TypeInfo spitzpda_type = {
@@ -1012,6 +1013,7 @@ static void borzoipda_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "Sharp SL-C3100 (Borzoi) PDA (PXA270)";
     mc->init = borzoi_init;
+    mc->block_default_type = IF_IDE;
 }
 
 static const TypeInfo borzoipda_type = {
@@ -1026,6 +1028,7 @@ static void terrierpda_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "Sharp SL-C3200 (Terrier) PDA (PXA270)";
     mc->init = terrier_init;
+    mc->block_default_type = IF_IDE;
 }
 
 static const TypeInfo terrierpda_type = {
diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index c3db996..9f58a23 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -263,6 +263,7 @@ static void tosapda_machine_init(MachineClass *mc)
 {
     mc->desc = "Sharp SL-6000 (Tosa) PDA (PXA255)";
     mc->init = tosa_init;
+    mc->block_default_type = IF_IDE;
 }
 
 DEFINE_MACHINE("tosa", tosapda_machine_init)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 706e233..8d2a848 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2334,6 +2334,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->query_hotpluggable_cpus = pc_query_hotpluggable_cpus;
     mc->default_boot_order = "cad";
     mc->hot_add_cpu = pc_hot_add_cpu;
+    mc->block_default_type = IF_IDE;
     mc->max_cpus = 255;
     mc->reset = pc_machine_reset;
     hc->pre_plug = pc_machine_device_pre_plug_cb;
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 9a4dae4..e636c3a 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -387,6 +387,7 @@ static void mips_fulong2e_machine_init(MachineClass *mc)
 {
     mc->desc = "Fulong 2e mini pc";
     mc->init = mips_fulong2e_init;
+    mc->block_default_type = IF_IDE;
 }
 
 DEFINE_MACHINE("fulong2e", mips_fulong2e_machine_init)
diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index cf48f42..6b63179 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1264,6 +1264,7 @@ static void mips_malta_machine_init(MachineClass *mc)
 {
     mc->desc = "MIPS Malta Core LV";
     mc->init = mips_malta_init;
+    mc->block_default_type = IF_IDE;
     mc->max_cpus = 16;
     mc->is_default = 1;
 }
diff --git a/hw/mips/mips_r4k.c b/hw/mips/mips_r4k.c
index 27548c4..748586e 100644
--- a/hw/mips/mips_r4k.c
+++ b/hw/mips/mips_r4k.c
@@ -306,6 +306,7 @@ static void mips_machine_init(MachineClass *mc)
 {
     mc->desc = "mips r4k platform";
     mc->init = mips_r4k_init;
+    mc->block_default_type = IF_IDE;
 }
 
 DEFINE_MACHINE("mips", mips_machine_init)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 2bfdb64..716aea6 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -518,6 +518,7 @@ static void core99_machine_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "Mac99 based PowerMAC";
     mc->init = ppc_core99_init;
+    mc->block_default_type = IF_IDE;
     mc->max_cpus = MAX_CPUS;
     mc->default_boot_order = "cd";
     mc->kvm_type = core99_kvm_type;
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 56282c5..5df94e2 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -368,6 +368,7 @@ static void heathrow_machine_init(MachineClass *mc)
 {
     mc->desc = "Heathrow based PowerMAC";
     mc->init = ppc_heathrow_init;
+    mc->block_default_type = IF_IDE;
     mc->max_cpus = MAX_CPUS;
 #ifndef TARGET_PPC64
     mc->is_default = 1;
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 054af1e..ebf6af5 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -673,6 +673,7 @@ static void prep_machine_init(MachineClass *mc)
 {
     mc->desc = "PowerPC PREP platform";
     mc->init = ppc_prep_init;
+    mc->block_default_type = IF_IDE;
     mc->max_cpus = MAX_CPUS;
     mc->default_boot_order = "cad";
 }
diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index db373c7..6d06968 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -362,6 +362,7 @@ static void r2d_machine_init(MachineClass *mc)
 {
     mc->desc = "r2d-plus board";
     mc->init = r2d_init;
+    mc->block_default_type = IF_IDE;
 }
 
 DEFINE_MACHINE("r2d", r2d_machine_init)
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index d1a6bca..d347b66 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -579,6 +579,7 @@ static void sun4u_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "Sun4u platform";
     mc->init = sun4u_init;
+    mc->block_default_type = IF_IDE;
     mc->max_cpus = 1; /* XXX for now */
     mc->is_default = 1;
     mc->default_boot_order = "c";
@@ -596,6 +597,7 @@ static void sun4v_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "Sun4v platform";
     mc->init = sun4v_init;
+    mc->block_default_type = IF_IDE;
     mc->max_cpus = 1; /* XXX for now */
     mc->default_boot_order = "c";
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 1/8] hw: Default -drive to if=ide explicitly where it works

[Thomas Huth]

On 26.01.2017 16:09, Markus Armbruster wrote:
> Block backends defined with -drive if=ide are meant to be picked up by
> machine initialization code: a suitable frontend gets created and
> wired up automatically.
> 
> if=ide drives not picked up that way can still be used with -device as
> if they had if=none, but that's unclean and best avoided.  Unused ones
> produce an "Orphaned drive without device" warning.
> 
> -drive parameter "if" is optional, and the default depends on the
> machine type.  If a machine type doesn't specify a default, the
> default is "ide".
> 
> Many machine types default to if=ide, even though they don't actually
> have an IDE controller.  A future patch will change these defaults to
> something more sensible.  To prepare for it, this patch makes default
> "ide" explicit for the machines that actually pick up if=ide drives:
> 
> * alpha: clipper
> * arm/aarch64: spitz borzoi terrier tosa
> * i386/x86_64: generic-pc-machine (with concrete subtypes pc-q35-*
>   pc-i440fx-* pc-* isapc xenfv)
> * mips64el: fulong2e

Slightly off-topic, but: Is fulong2e still maintained? I did not spot an
entry in MAINTAINERS...?

> * mips/mipsel/mips64el: malta mips
> * ppc/ppc64: mac99 g3beige prep
> * sh4/sh4eb: r2d
> * sparc64: sun4u sun4v
> 
> Note that ppc64 machine powernv already sets an "ide" default
> explicitly.  Its IDE controller isn't implemented, yet.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/alpha/dp264.c        | 1 +
>  hw/arm/spitz.c          | 3 +++
>  hw/arm/tosa.c           | 1 +
>  hw/i386/pc.c            | 1 +
>  hw/mips/mips_fulong2e.c | 1 +
>  hw/mips/mips_malta.c    | 1 +
>  hw/mips/mips_r4k.c      | 1 +
>  hw/ppc/mac_newworld.c   | 1 +
>  hw/ppc/mac_oldworld.c   | 1 +
>  hw/ppc/prep.c           | 1 +
>  hw/sh4/r2d.c            | 1 +
>  hw/sparc64/sun4u.c      | 2 ++
>  12 files changed, 15 insertions(+)

Patch looks right to me.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/8] hw: Default -drive to if=ide explicitly where it works

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 26.01.2017 16:09, Markus Armbruster wrote:
>> Block backends defined with -drive if=ide are meant to be picked up by
>> machine initialization code: a suitable frontend gets created and
>> wired up automatically.
>> 
>> if=ide drives not picked up that way can still be used with -device as
>> if they had if=none, but that's unclean and best avoided.  Unused ones
>> produce an "Orphaned drive without device" warning.
>> 
>> -drive parameter "if" is optional, and the default depends on the
>> machine type.  If a machine type doesn't specify a default, the
>> default is "ide".
>> 
>> Many machine types default to if=ide, even though they don't actually
>> have an IDE controller.  A future patch will change these defaults to
>> something more sensible.  To prepare for it, this patch makes default
>> "ide" explicit for the machines that actually pick up if=ide drives:
>> 
>> * alpha: clipper
>> * arm/aarch64: spitz borzoi terrier tosa
>> * i386/x86_64: generic-pc-machine (with concrete subtypes pc-q35-*
>>   pc-i440fx-* pc-* isapc xenfv)
>> * mips64el: fulong2e
>
> Slightly off-topic, but: Is fulong2e still maintained? I did not spot an
> entry in MAINTAINERS...?

It's covered by the general MIPS stanza:

    $ scripts/get_maintainer.pl -f hw/mips/mips_fulong2e.c 
    Aurelien Jarno <aurelien@aurel32.net> (maintainer:MIPS)
    Yongbok Kim <yongbok.kim@imgtec.com> (maintainer:MIPS)
    qemu-devel@nongnu.org (open list:All patches CC here)

>> * mips/mipsel/mips64el: malta mips
>> * ppc/ppc64: mac99 g3beige prep
>> * sh4/sh4eb: r2d
>> * sparc64: sun4u sun4v
>> 
>> Note that ppc64 machine powernv already sets an "ide" default
>> explicitly.  Its IDE controller isn't implemented, yet.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/alpha/dp264.c        | 1 +
>>  hw/arm/spitz.c          | 3 +++
>>  hw/arm/tosa.c           | 1 +
>>  hw/i386/pc.c            | 1 +
>>  hw/mips/mips_fulong2e.c | 1 +
>>  hw/mips/mips_malta.c    | 1 +
>>  hw/mips/mips_r4k.c      | 1 +
>>  hw/ppc/mac_newworld.c   | 1 +
>>  hw/ppc/mac_oldworld.c   | 1 +
>>  hw/ppc/prep.c           | 1 +
>>  hw/sh4/r2d.c            | 1 +
>>  hw/sparc64/sun4u.c      | 2 ++
>>  12 files changed, 15 insertions(+)
>
> Patch looks right to me.
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH v2 1/8] hw: Default -drive to if=ide explicitly where it works

[Yongbok Kim]

>> Slightly off-topic, but: Is fulong2e still maintained? I did not spot an
>> entry in MAINTAINERS...?
> 
> It's covered by the general MIPS stanza:
> 
>     $ scripts/get_maintainer.pl -f hw/mips/mips_fulong2e.c 
>     Aurelien Jarno <aurelien@aurel32.net> (maintainer:MIPS)
>     Yongbok Kim <yongbok.kim@imgtec.com> (maintainer:MIPS)
>     qemu-devel@nongnu.org (open list:All patches CC here)
> 

I'm not actively looking after the device at the moment but if it has any
issues I love to handle that.

Regards,
Yongbok

Re: [Qemu-devel] MIPS machines (was: [PATCH v2 1/8] hw: Default -drive to if=ide explicitly where it works)

[Thomas Huth]

On 27.01.2017 11:21, Yongbok Kim wrote:
> 
>>> Slightly off-topic, but: Is fulong2e still maintained? I did not spot an
>>> entry in MAINTAINERS...?
>>
>> It's covered by the general MIPS stanza:
>>
>>     $ scripts/get_maintainer.pl -f hw/mips/mips_fulong2e.c 
>>     Aurelien Jarno <aurelien@aurel32.net> (maintainer:MIPS)
>>     Yongbok Kim <yongbok.kim@imgtec.com> (maintainer:MIPS)
>>     qemu-devel@nongnu.org (open list:All patches CC here)
>>
> 
> I'm not actively looking after the device at the moment but if it has any
> issues I love to handle that.

Great! Then could you maybe send a patch for the MAINTAINERS file to add
an entry for that machine?

Also it's a little bit confusing that "magnum" and "pica61" do not show
up in MAINTAINERS, but I guess that's what is meant by the "Jazz" entry?

 Thanks,
  Thomas

Re: [Qemu-devel] MIPS machines

[Yongbok Kim]

On 27/01/2017 10:31, Thomas Huth wrote:
> On 27.01.2017 11:21, Yongbok Kim wrote:
>>
>>>> Slightly off-topic, but: Is fulong2e still maintained? I did not spot an
>>>> entry in MAINTAINERS...?
>>>
>>> It's covered by the general MIPS stanza:
>>>
>>>     $ scripts/get_maintainer.pl -f hw/mips/mips_fulong2e.c 
>>>     Aurelien Jarno <aurelien@aurel32.net> (maintainer:MIPS)
>>>     Yongbok Kim <yongbok.kim@imgtec.com> (maintainer:MIPS)
>>>     qemu-devel@nongnu.org (open list:All patches CC here)
>>>
>>
>> I'm not actively looking after the device at the moment but if it has any
>> issues I love to handle that.
> 
> Great! Then could you maybe send a patch for the MAINTAINERS file to add
> an entry for that machine?
> 

Sure I will send a patch for that.

> Also it's a little bit confusing that "magnum" and "pica61" do not show
> up in MAINTAINERS, but I guess that's what is meant by the "Jazz" entry?
> 
>  Thanks,
>   Thomas
> 

I believe that is the case. The file listed in the Jazz entry
"hw/mips/mips_jazz.c" is the one to support these two machines.

Regards,
Yongbok

[Qemu-devel] [PATCH v2 2/8] hw/arm/cubieboard hw/arm/xlnx-ep108: Fix units_per_default_bus

[Markus Armbruster]

Machine types cubieboard, xlnx-ep108, xlnx-zcu102 have an onboard AHCI
controller, but neglect to set their MachineClass member
units_per_default_bus = 1.  This permits -drive if=ide,unit=1, which
makes no sense for AHCI.  It also screws up index=N for odd N, because
it gets desugared to unit=1,bus=N/2

Doesn't really matter, because these machine types fail to honor
-drive if=ide.  Add the missing units_per_default_bus = 1 anyway,
along with a TODO comment on what needs to be done for -drive if=ide.

Also set block_default_type = IF_IDE explicitly.  It's currently the
default, but the next commit will change it to something more
sensible, and we want to keep the IF_IDE default for these three
machines.  See also the previous commit.

Cc: Beniamino Galvani <b.galvani@gmail.com>
Cc: Alistair Francis <alistair.francis@xilinx.com>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/cubieboard.c | 4 ++++
 hw/arm/xlnx-ep108.c | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
index dd19ba3..b98e1c4 100644
--- a/hw/arm/cubieboard.c
+++ b/hw/arm/cubieboard.c
@@ -71,6 +71,8 @@ static void cubieboard_init(MachineState *machine)
     memory_region_add_subregion(get_system_memory(), AW_A10_SDRAM_BASE,
                                 &s->sdram);
 
+    /* TODO create and connect IDE devices for ide_drive_get() */
+
     cubieboard_binfo.ram_size = machine->ram_size;
     cubieboard_binfo.kernel_filename = machine->kernel_filename;
     cubieboard_binfo.kernel_cmdline = machine->kernel_cmdline;
@@ -82,6 +84,8 @@ static void cubieboard_machine_init(MachineClass *mc)
 {
     mc->desc = "cubietech cubieboard";
     mc->init = cubieboard_init;
+    mc->block_default_type = IF_IDE;
+    mc->units_per_default_bus = 1;
 }
 
 DEFINE_MACHINE("cubieboard", cubieboard_machine_init)
diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
index 4ec590a..860780a 100644
--- a/hw/arm/xlnx-ep108.c
+++ b/hw/arm/xlnx-ep108.c
@@ -106,6 +106,8 @@ static void xlnx_ep108_init(MachineState *machine)
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->soc.spi[i]), 1, cs_line);
     }
 
+    /* TODO create and connect IDE devices for ide_drive_get() */
+
     xlnx_ep108_binfo.ram_size = ram_size;
     xlnx_ep108_binfo.kernel_filename = machine->kernel_filename;
     xlnx_ep108_binfo.kernel_cmdline = machine->kernel_cmdline;
@@ -118,6 +120,8 @@ static void xlnx_ep108_machine_init(MachineClass *mc)
 {
     mc->desc = "Xilinx ZynqMP EP108 board";
     mc->init = xlnx_ep108_init;
+    mc->block_default_type = IF_IDE;
+    mc->units_per_default_bus = 1;
 }
 
 DEFINE_MACHINE("xlnx-ep108", xlnx_ep108_machine_init)
@@ -126,6 +130,8 @@ static void xlnx_zcu102_machine_init(MachineClass *mc)
 {
     mc->desc = "Xilinx ZynqMP ZCU102 board";
     mc->init = xlnx_ep108_init;
+    mc->block_default_type = IF_IDE;
+    mc->units_per_default_bus = 1;
 }
 
 DEFINE_MACHINE("xlnx-zcu102", xlnx_zcu102_machine_init)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 2/8] hw/arm/cubieboard hw/arm/xlnx-ep108: Fix units_per_default_bus

[Alistair Francis]

On Thu, Jan 26, 2017 at 7:09 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Machine types cubieboard, xlnx-ep108, xlnx-zcu102 have an onboard AHCI
> controller, but neglect to set their MachineClass member
> units_per_default_bus = 1.  This permits -drive if=ide,unit=1, which
> makes no sense for AHCI.  It also screws up index=N for odd N, because
> it gets desugared to unit=1,bus=N/2
>
> Doesn't really matter, because these machine types fail to honor
> -drive if=ide.  Add the missing units_per_default_bus = 1 anyway,
> along with a TODO comment on what needs to be done for -drive if=ide.
>
> Also set block_default_type = IF_IDE explicitly.  It's currently the
> default, but the next commit will change it to something more
> sensible, and we want to keep the IF_IDE default for these three
> machines.  See also the previous commit.
>
> Cc: Beniamino Galvani <b.galvani@gmail.com>
> Cc: Alistair Francis <alistair.francis@xilinx.com>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Looks fine to me.

Acked-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> ---
>  hw/arm/cubieboard.c | 4 ++++
>  hw/arm/xlnx-ep108.c | 6 ++++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
> index dd19ba3..b98e1c4 100644
> --- a/hw/arm/cubieboard.c
> +++ b/hw/arm/cubieboard.c
> @@ -71,6 +71,8 @@ static void cubieboard_init(MachineState *machine)
>      memory_region_add_subregion(get_system_memory(), AW_A10_SDRAM_BASE,
>                                  &s->sdram);
>
> +    /* TODO create and connect IDE devices for ide_drive_get() */
> +
>      cubieboard_binfo.ram_size = machine->ram_size;
>      cubieboard_binfo.kernel_filename = machine->kernel_filename;
>      cubieboard_binfo.kernel_cmdline = machine->kernel_cmdline;
> @@ -82,6 +84,8 @@ static void cubieboard_machine_init(MachineClass *mc)
>  {
>      mc->desc = "cubietech cubieboard";
>      mc->init = cubieboard_init;
> +    mc->block_default_type = IF_IDE;
> +    mc->units_per_default_bus = 1;
>  }
>
>  DEFINE_MACHINE("cubieboard", cubieboard_machine_init)
> diff --git a/hw/arm/xlnx-ep108.c b/hw/arm/xlnx-ep108.c
> index 4ec590a..860780a 100644
> --- a/hw/arm/xlnx-ep108.c
> +++ b/hw/arm/xlnx-ep108.c
> @@ -106,6 +106,8 @@ static void xlnx_ep108_init(MachineState *machine)
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->soc.spi[i]), 1, cs_line);
>      }
>
> +    /* TODO create and connect IDE devices for ide_drive_get() */
> +
>      xlnx_ep108_binfo.ram_size = ram_size;
>      xlnx_ep108_binfo.kernel_filename = machine->kernel_filename;
>      xlnx_ep108_binfo.kernel_cmdline = machine->kernel_cmdline;
> @@ -118,6 +120,8 @@ static void xlnx_ep108_machine_init(MachineClass *mc)
>  {
>      mc->desc = "Xilinx ZynqMP EP108 board";
>      mc->init = xlnx_ep108_init;
> +    mc->block_default_type = IF_IDE;
> +    mc->units_per_default_bus = 1;
>  }
>
>  DEFINE_MACHINE("xlnx-ep108", xlnx_ep108_machine_init)
> @@ -126,6 +130,8 @@ static void xlnx_zcu102_machine_init(MachineClass *mc)
>  {
>      mc->desc = "Xilinx ZynqMP ZCU102 board";
>      mc->init = xlnx_ep108_init;
> +    mc->block_default_type = IF_IDE;
> +    mc->units_per_default_bus = 1;
>  }
>
>  DEFINE_MACHINE("xlnx-zcu102", xlnx_zcu102_machine_init)
> --
> 2.7.4
>
>

[Qemu-devel] [PATCH v2 3/8] hw: Default -drive to if=none instead of ide when ide cannot work

[Markus Armbruster]

Block backends defined with -drive if=ide are meant to be picked up by
machine initialization code: a suitable frontend gets created and
wired up automatically.

if=ide drives not picked up that way can still be used with -device as
if they had if=none, but that's unclean and best avoided.  Unused ones
produce an "Orphaned drive without device" warning.

-drive parameter "if" is optional, and the default depends on the
machine type.  If a machine type doesn't specify a default, the
default is "ide".

Many machine types implicitly default to if=ide that way, even though
they don't actually have an IDE controller.  This makes no sense.

Change the implicit default to if=none.  Affected machines:

* all targets: none
* aarch64/arm: akita ast2500 canon cheetah collie connex imx25
  integratorcp kzm lm3s6965evb lm3s811evb mainstone musicpal n800 n810
  netduino2 nuri palmetto realview romulus sabrelite smdkc210 sx1 sx1
  verdex z2
* cris: axis-dev88
* i386/x86_64: xenpv
* lm32: lm32-evr lm32-uclinux milkymist
* m68k: an5206 dummy mcf5208evb
* microblaze/microblazeel: petalogix-ml605 petalogix-s3adsp1800
* mips/mips64/mips64el/mipsel: mipssim
* moxie: moxiesim
* or32: or32-sim
* ppc/ppc64/ppcemb: bamboo ref405ep taihu virtex-ml507
* ppc/ppc64: mpc8544ds ppce500
* sh4/sh4eb: shix
* sparc: leon3_generic
* sparc64: niagara
* tricore: tricore_testboard
* unicore32: puv3
* xtensa/xtensaeb: kc705 lx200 lx60 ml605 sim

None of these machines have an IDE controller, let alone code to
honor if=ide.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: xen-devel@lists.xensource.com
Cc: Michael Walle <michael@walle.cc>
Cc: Laurent Vivier <laurent@vivier.eu>
Cc: Anthony Green <green@moxielogic.com>
Cc: Jia Liu <proljc@gmail.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Fabien Chouteau <chouteau@adacore.com>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: Artyom Tarasenko <atar4qemu@gmail.com>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Acked-By: Artyom Tarasenko <atar4qemu@gmail.com>
---
 include/sysemu/blockdev.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 16432f3..351a039 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -19,12 +19,11 @@ void blockdev_auto_del(BlockBackend *blk);
 typedef enum {
     IF_DEFAULT = -1,            /* for use with drive_add() only */
     /*
-     * IF_IDE must be zero, because we want MachineClass member
-     * block_default_type to default-initialize to IF_IDE
+     * IF_NONE must be zero, because we want MachineClass member
+     * block_default_type to default-initialize to IF_NONE
      */
-    IF_IDE = 0,
-    IF_NONE,
-    IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
+    IF_NONE = 0,
+    IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
     IF_COUNT
 } BlockInterfaceType;
 
-- 
2.7.4

[PATCH v2 3/8] hw: Default -drive to if=none instead of ide when ide cannot work

[Markus Armbruster]

Block backends defined with -drive if=ide are meant to be picked up by
machine initialization code: a suitable frontend gets created and
wired up automatically.

if=ide drives not picked up that way can still be used with -device as
if they had if=none, but that's unclean and best avoided.  Unused ones
produce an "Orphaned drive without device" warning.

-drive parameter "if" is optional, and the default depends on the
machine type.  If a machine type doesn't specify a default, the
default is "ide".

Many machine types implicitly default to if=ide that way, even though
they don't actually have an IDE controller.  This makes no sense.

Change the implicit default to if=none.  Affected machines:

* all targets: none
* aarch64/arm: akita ast2500 canon cheetah collie connex imx25
  integratorcp kzm lm3s6965evb lm3s811evb mainstone musicpal n800 n810
  netduino2 nuri palmetto realview romulus sabrelite smdkc210 sx1 sx1
  verdex z2
* cris: axis-dev88
* i386/x86_64: xenpv
* lm32: lm32-evr lm32-uclinux milkymist
* m68k: an5206 dummy mcf5208evb
* microblaze/microblazeel: petalogix-ml605 petalogix-s3adsp1800
* mips/mips64/mips64el/mipsel: mipssim
* moxie: moxiesim
* or32: or32-sim
* ppc/ppc64/ppcemb: bamboo ref405ep taihu virtex-ml507
* ppc/ppc64: mpc8544ds ppce500
* sh4/sh4eb: shix
* sparc: leon3_generic
* sparc64: niagara
* tricore: tricore_testboard
* unicore32: puv3
* xtensa/xtensaeb: kc705 lx200 lx60 ml605 sim

None of these machines have an IDE controller, let alone code to
honor if=ide.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: xen-devel@lists.xensource.com
Cc: Michael Walle <michael@walle.cc>
Cc: Laurent Vivier <laurent@vivier.eu>
Cc: Anthony Green <green@moxielogic.com>
Cc: Jia Liu <proljc@gmail.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Fabien Chouteau <chouteau@adacore.com>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: Artyom Tarasenko <atar4qemu@gmail.com>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Acked-By: Artyom Tarasenko <atar4qemu@gmail.com>
---
 include/sysemu/blockdev.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 16432f3..351a039 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -19,12 +19,11 @@ void blockdev_auto_del(BlockBackend *blk);
 typedef enum {
     IF_DEFAULT = -1,            /* for use with drive_add() only */
     /*
-     * IF_IDE must be zero, because we want MachineClass member
-     * block_default_type to default-initialize to IF_IDE
+     * IF_NONE must be zero, because we want MachineClass member
+     * block_default_type to default-initialize to IF_NONE
      */
-    IF_IDE = 0,
-    IF_NONE,
-    IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
+    IF_NONE = 0,
+    IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
     IF_COUNT
 } BlockInterfaceType;
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 3/8] hw: Default -drive to if=none instead of ide when ide cannot work

[Laurent Vivier]

Le 26/01/2017 à 16:09, Markus Armbruster a écrit :
> Block backends defined with -drive if=ide are meant to be picked up by
> machine initialization code: a suitable frontend gets created and
> wired up automatically.
> 
> if=ide drives not picked up that way can still be used with -device as
> if they had if=none, but that's unclean and best avoided.  Unused ones
> produce an "Orphaned drive without device" warning.
> 
> -drive parameter "if" is optional, and the default depends on the
> machine type.  If a machine type doesn't specify a default, the
> default is "ide".
> 
> Many machine types implicitly default to if=ide that way, even though
> they don't actually have an IDE controller.  This makes no sense.
> 
> Change the implicit default to if=none.  Affected machines:
> 
> * all targets: none
> * aarch64/arm: akita ast2500 canon cheetah collie connex imx25
>   integratorcp kzm lm3s6965evb lm3s811evb mainstone musicpal n800 n810
>   netduino2 nuri palmetto realview romulus sabrelite smdkc210 sx1 sx1
>   verdex z2
> * cris: axis-dev88
> * i386/x86_64: xenpv
> * lm32: lm32-evr lm32-uclinux milkymist
> * m68k: an5206 dummy mcf5208evb
> * microblaze/microblazeel: petalogix-ml605 petalogix-s3adsp1800
> * mips/mips64/mips64el/mipsel: mipssim
> * moxie: moxiesim
> * or32: or32-sim
> * ppc/ppc64/ppcemb: bamboo ref405ep taihu virtex-ml507
> * ppc/ppc64: mpc8544ds ppce500
> * sh4/sh4eb: shix
> * sparc: leon3_generic
> * sparc64: niagara
> * tricore: tricore_testboard
> * unicore32: puv3
> * xtensa/xtensaeb: kc705 lx200 lx60 ml605 sim
> 
> None of these machines have an IDE controller, let alone code to
> honor if=ide.
> 
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: xen-devel@lists.xensource.com
> Cc: Michael Walle <michael@walle.cc>
> Cc: Laurent Vivier <laurent@vivier.eu>
> Cc: Anthony Green <green@moxielogic.com>
> Cc: Jia Liu <proljc@gmail.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: qemu-ppc@nongnu.org
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Fabien Chouteau <chouteau@adacore.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Acked-By: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  include/sysemu/blockdev.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 16432f3..351a039 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -19,12 +19,11 @@ void blockdev_auto_del(BlockBackend *blk);
>  typedef enum {
>      IF_DEFAULT = -1,            /* for use with drive_add() only */
>      /*
> -     * IF_IDE must be zero, because we want MachineClass member
> -     * block_default_type to default-initialize to IF_IDE
> +     * IF_NONE must be zero, because we want MachineClass member
> +     * block_default_type to default-initialize to IF_NONE
>       */
> -    IF_IDE = 0,
> -    IF_NONE,
> -    IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
> +    IF_NONE = 0,
> +    IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
>      IF_COUNT
>  } BlockInterfaceType;
>  
> 
Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH v2 3/8] hw: Default -drive to if=none instead of ide when ide cannot work

[Laurent Vivier]

Le 26/01/2017 à 16:09, Markus Armbruster a écrit :
> Block backends defined with -drive if=ide are meant to be picked up by
> machine initialization code: a suitable frontend gets created and
> wired up automatically.
> 
> if=ide drives not picked up that way can still be used with -device as
> if they had if=none, but that's unclean and best avoided.  Unused ones
> produce an "Orphaned drive without device" warning.
> 
> -drive parameter "if" is optional, and the default depends on the
> machine type.  If a machine type doesn't specify a default, the
> default is "ide".
> 
> Many machine types implicitly default to if=ide that way, even though
> they don't actually have an IDE controller.  This makes no sense.
> 
> Change the implicit default to if=none.  Affected machines:
> 
> * all targets: none
> * aarch64/arm: akita ast2500 canon cheetah collie connex imx25
>   integratorcp kzm lm3s6965evb lm3s811evb mainstone musicpal n800 n810
>   netduino2 nuri palmetto realview romulus sabrelite smdkc210 sx1 sx1
>   verdex z2
> * cris: axis-dev88
> * i386/x86_64: xenpv
> * lm32: lm32-evr lm32-uclinux milkymist
> * m68k: an5206 dummy mcf5208evb
> * microblaze/microblazeel: petalogix-ml605 petalogix-s3adsp1800
> * mips/mips64/mips64el/mipsel: mipssim
> * moxie: moxiesim
> * or32: or32-sim
> * ppc/ppc64/ppcemb: bamboo ref405ep taihu virtex-ml507
> * ppc/ppc64: mpc8544ds ppce500
> * sh4/sh4eb: shix
> * sparc: leon3_generic
> * sparc64: niagara
> * tricore: tricore_testboard
> * unicore32: puv3
> * xtensa/xtensaeb: kc705 lx200 lx60 ml605 sim
> 
> None of these machines have an IDE controller, let alone code to
> honor if=ide.
> 
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: xen-devel@lists.xensource.com
> Cc: Michael Walle <michael@walle.cc>
> Cc: Laurent Vivier <laurent@vivier.eu>
> Cc: Anthony Green <green@moxielogic.com>
> Cc: Jia Liu <proljc@gmail.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: qemu-ppc@nongnu.org
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Fabien Chouteau <chouteau@adacore.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Acked-By: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  include/sysemu/blockdev.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 16432f3..351a039 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -19,12 +19,11 @@ void blockdev_auto_del(BlockBackend *blk);
>  typedef enum {
>      IF_DEFAULT = -1,            /* for use with drive_add() only */
>      /*
> -     * IF_IDE must be zero, because we want MachineClass member
> -     * block_default_type to default-initialize to IF_IDE
> +     * IF_NONE must be zero, because we want MachineClass member
> +     * block_default_type to default-initialize to IF_NONE
>       */
> -    IF_IDE = 0,
> -    IF_NONE,
> -    IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
> +    IF_NONE = 0,
> +    IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
>      IF_COUNT
>  } BlockInterfaceType;
>  
> 
Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [PATCH v2 3/8] hw: Default -drive to if=none instead of ide when ide cannot work

[Thomas Huth]

On 26.01.2017 16:09, Markus Armbruster wrote:
> Block backends defined with -drive if=ide are meant to be picked up by
> machine initialization code: a suitable frontend gets created and
> wired up automatically.
> 
> if=ide drives not picked up that way can still be used with -device as
> if they had if=none, but that's unclean and best avoided.  Unused ones
> produce an "Orphaned drive without device" warning.
> 
> -drive parameter "if" is optional, and the default depends on the
> machine type.  If a machine type doesn't specify a default, the
> default is "ide".
> 
> Many machine types implicitly default to if=ide that way, even though
> they don't actually have an IDE controller.  This makes no sense.
> 
> Change the implicit default to if=none.  Affected machines:
> 
> * all targets: none
> * aarch64/arm: akita ast2500 canon cheetah collie connex imx25
>   integratorcp kzm lm3s6965evb lm3s811evb mainstone musicpal n800 n810
>   netduino2 nuri palmetto realview romulus sabrelite smdkc210 sx1 sx1
>   verdex z2
> * cris: axis-dev88
> * i386/x86_64: xenpv
> * lm32: lm32-evr lm32-uclinux milkymist
> * m68k: an5206 dummy mcf5208evb
> * microblaze/microblazeel: petalogix-ml605 petalogix-s3adsp1800
> * mips/mips64/mips64el/mipsel: mipssim
> * moxie: moxiesim
> * or32: or32-sim
> * ppc/ppc64/ppcemb: bamboo ref405ep taihu virtex-ml507
> * ppc/ppc64: mpc8544ds ppce500
> * sh4/sh4eb: shix
> * sparc: leon3_generic
> * sparc64: niagara
> * tricore: tricore_testboard
> * unicore32: puv3
> * xtensa/xtensaeb: kc705 lx200 lx60 ml605 sim
> 
> None of these machines have an IDE controller, let alone code to
> honor if=ide.
> 
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: xen-devel@lists.xensource.com
> Cc: Michael Walle <michael@walle.cc>
> Cc: Laurent Vivier <laurent@vivier.eu>
> Cc: Anthony Green <green@moxielogic.com>
> Cc: Jia Liu <proljc@gmail.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: qemu-ppc@nongnu.org
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Fabien Chouteau <chouteau@adacore.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Acked-By: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  include/sysemu/blockdev.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 16432f3..351a039 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -19,12 +19,11 @@ void blockdev_auto_del(BlockBackend *blk);
>  typedef enum {
>      IF_DEFAULT = -1,            /* for use with drive_add() only */
>      /*
> -     * IF_IDE must be zero, because we want MachineClass member
> -     * block_default_type to default-initialize to IF_IDE
> +     * IF_NONE must be zero, because we want MachineClass member
> +     * block_default_type to default-initialize to IF_NONE
>       */
> -    IF_IDE = 0,
> -    IF_NONE,
> -    IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
> +    IF_NONE = 0,
> +    IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
>      IF_COUNT
>  } BlockInterfaceType;
>  
> 

Makes sense.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/8] hw: Default -drive to if=none instead of ide when ide cannot work

[Thomas Huth]

On 26.01.2017 16:09, Markus Armbruster wrote:
> Block backends defined with -drive if=ide are meant to be picked up by
> machine initialization code: a suitable frontend gets created and
> wired up automatically.
> 
> if=ide drives not picked up that way can still be used with -device as
> if they had if=none, but that's unclean and best avoided.  Unused ones
> produce an "Orphaned drive without device" warning.
> 
> -drive parameter "if" is optional, and the default depends on the
> machine type.  If a machine type doesn't specify a default, the
> default is "ide".
> 
> Many machine types implicitly default to if=ide that way, even though
> they don't actually have an IDE controller.  This makes no sense.
> 
> Change the implicit default to if=none.  Affected machines:
> 
> * all targets: none
> * aarch64/arm: akita ast2500 canon cheetah collie connex imx25
>   integratorcp kzm lm3s6965evb lm3s811evb mainstone musicpal n800 n810
>   netduino2 nuri palmetto realview romulus sabrelite smdkc210 sx1 sx1
>   verdex z2
> * cris: axis-dev88
> * i386/x86_64: xenpv
> * lm32: lm32-evr lm32-uclinux milkymist
> * m68k: an5206 dummy mcf5208evb
> * microblaze/microblazeel: petalogix-ml605 petalogix-s3adsp1800
> * mips/mips64/mips64el/mipsel: mipssim
> * moxie: moxiesim
> * or32: or32-sim
> * ppc/ppc64/ppcemb: bamboo ref405ep taihu virtex-ml507
> * ppc/ppc64: mpc8544ds ppce500
> * sh4/sh4eb: shix
> * sparc: leon3_generic
> * sparc64: niagara
> * tricore: tricore_testboard
> * unicore32: puv3
> * xtensa/xtensaeb: kc705 lx200 lx60 ml605 sim
> 
> None of these machines have an IDE controller, let alone code to
> honor if=ide.
> 
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: xen-devel@lists.xensource.com
> Cc: Michael Walle <michael@walle.cc>
> Cc: Laurent Vivier <laurent@vivier.eu>
> Cc: Anthony Green <green@moxielogic.com>
> Cc: Jia Liu <proljc@gmail.com>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: qemu-ppc@nongnu.org
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Fabien Chouteau <chouteau@adacore.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Acked-By: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  include/sysemu/blockdev.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 16432f3..351a039 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -19,12 +19,11 @@ void blockdev_auto_del(BlockBackend *blk);
>  typedef enum {
>      IF_DEFAULT = -1,            /* for use with drive_add() only */
>      /*
> -     * IF_IDE must be zero, because we want MachineClass member
> -     * block_default_type to default-initialize to IF_IDE
> +     * IF_NONE must be zero, because we want MachineClass member
> +     * block_default_type to default-initialize to IF_NONE
>       */
> -    IF_IDE = 0,
> -    IF_NONE,
> -    IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
> +    IF_NONE = 0,
> +    IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
>      IF_COUNT
>  } BlockInterfaceType;
>  
> 

Makes sense.

Reviewed-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH v2 4/8] hw: Default -drive to if=none instead of scsi when scsi cannot work

[Markus Armbruster]

Block backends defined with -drive if=scsi are meant to be picked up
by machine initialization code: a suitable frontend gets created and
wired up automatically.

if=scsi drives not picked up that way can still be used with -device
as if they had if=none, but that's unclean and best avoided.  Unused
ones produce an "Orphaned drive without device" warning.

A few machine types default to if=scsi, even though they don't
actually have a SCSI HBA.  This makes no sense.  Change their default
to if=none.  Affected machines:

* aarch64/arm: realview-pbx-a9 vexpress-a9 vexpress-a15 xilinx-zynq-a9

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Alistair Francis <alistair.francis@xilinx.com>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/realview.c    | 1 -
 hw/arm/vexpress.c    | 1 -
 hw/arm/xilinx_zynq.c | 1 -
 3 files changed, 3 deletions(-)

diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index 8eafcca..8c11c7a 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -443,7 +443,6 @@ static void realview_pbx_a9_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "ARM RealView Platform Baseboard Explore for Cortex-A9";
     mc->init = realview_pbx_a9_init;
-    mc->block_default_type = IF_SCSI;
     mc->max_cpus = 4;
 }
 
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 58760f4..5756470 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -751,7 +751,6 @@ static void vexpress_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "ARM Versatile Express";
     mc->init = vexpress_common_init;
-    mc->block_default_type = IF_SCSI;
     mc->max_cpus = 4;
 }
 
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 7dac20d..3985356 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -323,7 +323,6 @@ static void zynq_machine_init(MachineClass *mc)
 {
     mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9";
     mc->init = zynq_init;
-    mc->block_default_type = IF_SCSI;
     mc->max_cpus = 1;
     mc->no_sdcard = 1;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 4/8] hw: Default -drive to if=none instead of scsi when scsi cannot work

[Thomas Huth]

On 26.01.2017 16:09, Markus Armbruster wrote:
> Block backends defined with -drive if=scsi are meant to be picked up
> by machine initialization code: a suitable frontend gets created and
> wired up automatically.
> 
> if=scsi drives not picked up that way can still be used with -device
> as if they had if=none, but that's unclean and best avoided.  Unused
> ones produce an "Orphaned drive without device" warning.
> 
> A few machine types default to if=scsi, even though they don't
> actually have a SCSI HBA.  This makes no sense.  Change their default
> to if=none.  Affected machines:
> 
> * aarch64/arm: realview-pbx-a9 vexpress-a9 vexpress-a15 xilinx-zynq-a9
> 
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Alistair Francis <alistair.francis@xilinx.com>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/arm/realview.c    | 1 -
>  hw/arm/vexpress.c    | 1 -
>  hw/arm/xilinx_zynq.c | 1 -
>  3 files changed, 3 deletions(-)
> 
> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
> index 8eafcca..8c11c7a 100644
> --- a/hw/arm/realview.c
> +++ b/hw/arm/realview.c
> @@ -443,7 +443,6 @@ static void realview_pbx_a9_class_init(ObjectClass *oc, void *data)
>  
>      mc->desc = "ARM RealView Platform Baseboard Explore for Cortex-A9";
>      mc->init = realview_pbx_a9_init;
> -    mc->block_default_type = IF_SCSI;
>      mc->max_cpus = 4;
>  }
>  
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index 58760f4..5756470 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -751,7 +751,6 @@ static void vexpress_class_init(ObjectClass *oc, void *data)
>  
>      mc->desc = "ARM Versatile Express";
>      mc->init = vexpress_common_init;
> -    mc->block_default_type = IF_SCSI;
>      mc->max_cpus = 4;
>  }
>  
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 7dac20d..3985356 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -323,7 +323,6 @@ static void zynq_machine_init(MachineClass *mc)
>  {
>      mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9";
>      mc->init = zynq_init;
> -    mc->block_default_type = IF_SCSI;
>      mc->max_cpus = 1;
>      mc->no_sdcard = 1;
>  }

Right, that looks like old copy-n-paste bugs from other machine types.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 4/8] hw: Default -drive to if=none instead of scsi when scsi cannot work

[Alistair Francis]

On Thu, Jan 26, 2017 at 9:59 AM, Thomas Huth <thuth@redhat.com> wrote:
> On 26.01.2017 16:09, Markus Armbruster wrote:
>> Block backends defined with -drive if=scsi are meant to be picked up
>> by machine initialization code: a suitable frontend gets created and
>> wired up automatically.
>>
>> if=scsi drives not picked up that way can still be used with -device
>> as if they had if=none, but that's unclean and best avoided.  Unused
>> ones produce an "Orphaned drive without device" warning.
>>
>> A few machine types default to if=scsi, even though they don't
>> actually have a SCSI HBA.  This makes no sense.  Change their default
>> to if=none.  Affected machines:
>>
>> * aarch64/arm: realview-pbx-a9 vexpress-a9 vexpress-a15 xilinx-zynq-a9
>>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
>> Cc: Alistair Francis <alistair.francis@xilinx.com>
>> Cc: qemu-arm@nongnu.org
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/arm/realview.c    | 1 -
>>  hw/arm/vexpress.c    | 1 -
>>  hw/arm/xilinx_zynq.c | 1 -
>>  3 files changed, 3 deletions(-)
>>
>> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
>> index 8eafcca..8c11c7a 100644
>> --- a/hw/arm/realview.c
>> +++ b/hw/arm/realview.c
>> @@ -443,7 +443,6 @@ static void realview_pbx_a9_class_init(ObjectClass *oc, void *data)
>>
>>      mc->desc = "ARM RealView Platform Baseboard Explore for Cortex-A9";
>>      mc->init = realview_pbx_a9_init;
>> -    mc->block_default_type = IF_SCSI;
>>      mc->max_cpus = 4;
>>  }
>>
>> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
>> index 58760f4..5756470 100644
>> --- a/hw/arm/vexpress.c
>> +++ b/hw/arm/vexpress.c
>> @@ -751,7 +751,6 @@ static void vexpress_class_init(ObjectClass *oc, void *data)
>>
>>      mc->desc = "ARM Versatile Express";
>>      mc->init = vexpress_common_init;
>> -    mc->block_default_type = IF_SCSI;
>>      mc->max_cpus = 4;
>>  }
>>
>> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
>> index 7dac20d..3985356 100644
>> --- a/hw/arm/xilinx_zynq.c
>> +++ b/hw/arm/xilinx_zynq.c
>> @@ -323,7 +323,6 @@ static void zynq_machine_init(MachineClass *mc)
>>  {
>>      mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9";
>>      mc->init = zynq_init;
>> -    mc->block_default_type = IF_SCSI;
>>      mc->max_cpus = 1;
>>      mc->no_sdcard = 1;
>>  }
>
> Right, that looks like old copy-n-paste bugs from other machine types.
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Yeah, I agree. This looks good to me.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

>
>

[Qemu-devel] [PATCH v2 5/8] hw/arm/highbank: Default -drive to if=ide instead of if=scsi

[Markus Armbruster]

These machines have no onboard SCSI HBA, and no way to plug one.
-drive if=scsi therefore cannot work.  They do have an onboard IDE
controller (sysbus-ahci), but fail to honor if=ide.

Change their default to if=ide, and add a TODO comment on what needs
to be done to actually honor -drive if=ide.

Cc: Rob Herring <robh@kernel.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/highbank.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index 80e5fd4..0a4508c 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -363,6 +363,8 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
         sysbus_connect_irq(SYS_BUS_DEVICE(dev), 2, pic[82]);
     }
 
+    /* TODO create and connect IDE devices for ide_drive_get() */
+
     highbank_binfo.ram_size = ram_size;
     highbank_binfo.kernel_filename = kernel_filename;
     highbank_binfo.kernel_cmdline = kernel_cmdline;
@@ -405,7 +407,8 @@ static void highbank_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "Calxeda Highbank (ECX-1000)";
     mc->init = highbank_init;
-    mc->block_default_type = IF_SCSI;
+    mc->block_default_type = IF_IDE;
+    mc->units_per_default_bus = 1;
     mc->max_cpus = 4;
 }
 
@@ -421,7 +424,8 @@ static void midway_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "Calxeda Midway (ECX-2000)";
     mc->init = midway_init;
-    mc->block_default_type = IF_SCSI;
+    mc->block_default_type = IF_IDE;
+    mc->units_per_default_bus = 1;
     mc->max_cpus = 4;
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 5/8] hw/arm/highbank: Default -drive to if=ide instead of if=scsi

[Thomas Huth]

On 26.01.2017 16:09, Markus Armbruster wrote:
> These machines have no onboard SCSI HBA, and no way to plug one.
> -drive if=scsi therefore cannot work.  They do have an onboard IDE
> controller (sysbus-ahci), but fail to honor if=ide.
> 
> Change their default to if=ide, and add a TODO comment on what needs
> to be done to actually honor -drive if=ide.
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/arm/highbank.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index 80e5fd4..0a4508c 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -363,6 +363,8 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
>          sysbus_connect_irq(SYS_BUS_DEVICE(dev), 2, pic[82]);
>      }
>  
> +    /* TODO create and connect IDE devices for ide_drive_get() */
> +
>      highbank_binfo.ram_size = ram_size;
>      highbank_binfo.kernel_filename = kernel_filename;
>      highbank_binfo.kernel_cmdline = kernel_cmdline;
> @@ -405,7 +407,8 @@ static void highbank_class_init(ObjectClass *oc, void *data)
>  
>      mc->desc = "Calxeda Highbank (ECX-1000)";
>      mc->init = highbank_init;
> -    mc->block_default_type = IF_SCSI;
> +    mc->block_default_type = IF_IDE;
> +    mc->units_per_default_bus = 1;
>      mc->max_cpus = 4;
>  }
>  
> @@ -421,7 +424,8 @@ static void midway_class_init(ObjectClass *oc, void *data)
>  
>      mc->desc = "Calxeda Midway (ECX-2000)";
>      mc->init = midway_init;
> -    mc->block_default_type = IF_SCSI;
> +    mc->block_default_type = IF_IDE;
> +    mc->units_per_default_bus = 1;
>      mc->max_cpus = 4;
>  }
>  
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

[Qemu-devel] [PATCH v2 6/8] blockdev: Improve message for orphaned -drive

[Markus Armbruster]

We warn when a -drive isn't supported by the machine type (commit
a66c9dc):

    $ qemu-system-x86_64 -S -display none -drive if=mtd
    Warning: Orphaned drive without device: id=mtd0,file=,if=mtd,bus=0,unit=0

Improve this to point to the offending bit of configuration:

    qemu-system-x86_64: -drive if=mtd: warning: machine type does not support this drive

Especially nice when it's hidden behind -readconfig foo.cfg:

    qemu-system-x86_64:foo.cfg:140: warning: machine type does not support this drive

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 245e1e1..a597a66 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -231,6 +231,7 @@ bool drive_check_orphaned(void)
 {
     BlockBackend *blk;
     DriveInfo *dinfo;
+    Location loc;
     bool rs = false;
 
     for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
@@ -239,10 +240,10 @@ 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) {
-            fprintf(stderr, "Warning: Orphaned drive without device: "
-                    "id=%s,file=%s,if=%s,bus=%d,unit=%d\n",
-                    blk_name(blk), blk_bs(blk) ? blk_bs(blk)->filename : "",
-                    if_name[dinfo->type], dinfo->bus, dinfo->unit);
+            loc_push_none(&loc);
+            qemu_opts_loc_restore(dinfo->opts);
+            error_report("warning: machine type does not support this drive");
+            loc_pop(&loc);
             rs = true;
         }
     }
-- 
2.7.4

[Qemu-devel] [PATCH v2 7/8] blockdev: Make orphaned -drive fatal

[Markus Armbruster]

Block backends defined with "-drive if=T" with T other than "none" are
meant to be picked up by machine initialization code: a suitable
frontend gets created and wired up automatically.

If machine initialization code doesn't comply, the block backend
remains unused.  This triggers a warning since commit a66c9dc, v2.2.0.
Drives created by default are excempted; use -nodefaults to get rid of
them.

Turn this warning into an error.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c                | 14 +++++++-------
 include/sysemu/blockdev.h |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index a597a66..ec9c114 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -227,28 +227,28 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
     return NULL;
 }
 
-bool drive_check_orphaned(void)
+void drive_check_orphaned(void)
 {
     BlockBackend *blk;
     DriveInfo *dinfo;
     Location loc;
-    bool rs = false;
+    bool orphans = false;
 
     for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
         dinfo = blk_legacy_dinfo(blk);
-        /* If dinfo->bdrv->dev is NULL, it has no device attached. */
-        /* Unless this is a default drive, this may be an oversight. */
         if (!blk_get_attached_dev(blk) && !dinfo->is_default &&
             dinfo->type != IF_NONE) {
             loc_push_none(&loc);
             qemu_opts_loc_restore(dinfo->opts);
-            error_report("warning: machine type does not support this drive");
+            error_report("machine type does not support this drive");
             loc_pop(&loc);
-            rs = true;
+            orphans = true;
         }
     }
 
-    return rs;
+    if (orphans) {
+        exit(1);
+    }
 }
 
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index)
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 351a039..ac22f2a 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -48,7 +48,7 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo);
 void override_max_devs(BlockInterfaceType type, int max_devs);
 
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
-bool drive_check_orphaned(void);
+void drive_check_orphaned(void);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);
 int drive_get_max_devs(BlockInterfaceType type);
-- 
2.7.4

Re: [Qemu-devel] [Qemu-block] [PATCH v2 7/8] blockdev: Make orphaned -drive fatal

[John Snow]

On 01/26/2017 10:09 AM, Markus Armbruster wrote:
> Block backends defined with "-drive if=T" with T other than "none" are
> meant to be picked up by machine initialization code: a suitable
> frontend gets created and wired up automatically.
> 
> If machine initialization code doesn't comply, the block backend
> remains unused.  This triggers a warning since commit a66c9dc, v2.2.0.
> Drives created by default are excempted; use -nodefaults to get rid of
> them.
> 
> Turn this warning into an error.
> 

"Exempted," oddly enough.
</eblake>

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  blockdev.c                | 14 +++++++-------
>  include/sysemu/blockdev.h |  2 +-
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index a597a66..ec9c114 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -227,28 +227,28 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
>      return NULL;
>  }
>  
> -bool drive_check_orphaned(void)
> +void drive_check_orphaned(void)
>  {
>      BlockBackend *blk;
>      DriveInfo *dinfo;
>      Location loc;
> -    bool rs = false;
> +    bool orphans = false;
>  
>      for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>          dinfo = blk_legacy_dinfo(blk);
> -        /* If dinfo->bdrv->dev is NULL, it has no device attached. */
> -        /* Unless this is a default drive, this may be an oversight. */
>          if (!blk_get_attached_dev(blk) && !dinfo->is_default &&
>              dinfo->type != IF_NONE) {
>              loc_push_none(&loc);
>              qemu_opts_loc_restore(dinfo->opts);
> -            error_report("warning: machine type does not support this drive");
> +            error_report("machine type does not support this drive");
>              loc_pop(&loc);
> -            rs = true;
> +            orphans = true;
>          }
>      }
>  
> -    return rs;
> +    if (orphans) {
> +        exit(1);
> +    }

Hardcore. Why not just return a status code and allow vl.c to exit? It
only has the one caller. (Unless you added more and I didn't read
because I'm lazy.)

You could even add an Error **arg if you wanted to; but vl.c has exits
all over the dang place, so maybe that's not really interesting.

--js

>  }
>  
>  DriveInfo *drive_get_by_index(BlockInterfaceType type, int index)
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 351a039..ac22f2a 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -48,7 +48,7 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo);
>  void override_max_devs(BlockInterfaceType type, int max_devs);
>  
>  DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
> -bool drive_check_orphaned(void);
> +void drive_check_orphaned(void);
>  DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
>  int drive_get_max_bus(BlockInterfaceType type);
>  int drive_get_max_devs(BlockInterfaceType type);
> 

Re: [Qemu-devel] [Qemu-block] [PATCH v2 7/8] blockdev: Make orphaned -drive fatal

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 01/26/2017 10:09 AM, Markus Armbruster wrote:
>> Block backends defined with "-drive if=T" with T other than "none" are
>> meant to be picked up by machine initialization code: a suitable
>> frontend gets created and wired up automatically.
>> 
>> If machine initialization code doesn't comply, the block backend
>> remains unused.  This triggers a warning since commit a66c9dc, v2.2.0.
>> Drives created by default are excempted; use -nodefaults to get rid of
>> them.
>> 
>> Turn this warning into an error.
>> 
>
> "Exempted," oddly enough.
> </eblake>

Thanks!

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  blockdev.c                | 14 +++++++-------
>>  include/sysemu/blockdev.h |  2 +-
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/blockdev.c b/blockdev.c
>> index a597a66..ec9c114 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -227,28 +227,28 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
>>      return NULL;
>>  }
>>  
>> -bool drive_check_orphaned(void)
>> +void drive_check_orphaned(void)
>>  {
>>      BlockBackend *blk;
>>      DriveInfo *dinfo;
>>      Location loc;
>> -    bool rs = false;
>> +    bool orphans = false;
>>  
>>      for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>>          dinfo = blk_legacy_dinfo(blk);
>> -        /* If dinfo->bdrv->dev is NULL, it has no device attached. */
>> -        /* Unless this is a default drive, this may be an oversight. */
>>          if (!blk_get_attached_dev(blk) && !dinfo->is_default &&
>>              dinfo->type != IF_NONE) {
>>              loc_push_none(&loc);
>>              qemu_opts_loc_restore(dinfo->opts);
>> -            error_report("warning: machine type does not support this drive");
>> +            error_report("machine type does not support this drive");
>>              loc_pop(&loc);
>> -            rs = true;
>> +            orphans = true;
>>          }
>>      }
>>  
>> -    return rs;
>> +    if (orphans) {
>> +        exit(1);
>> +    }
>
> Hardcore. Why not just return a status code and allow vl.c to exit? It
> only has the one caller. (Unless you added more and I didn't read
> because I'm lazy.)
>
> You could even add an Error **arg if you wanted to; but vl.c has exits
> all over the dang place, so maybe that's not really interesting.

My excuse it that checking for orphans makes sense only during initial
startup, and there, exit(1) on error is what we do.  Doing it right away
is simplest.

I'm fine with leaving the exit() to the caller.  I would prefer to leave
the printing to the caller as well then.

Anyone else got a preference?

Re: [Qemu-devel] [Qemu-block] [PATCH v2 7/8] blockdev: Make orphaned -drive fatal

[John Snow]

On 01/31/2017 09:37 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 01/26/2017 10:09 AM, Markus Armbruster wrote:
>>> Block backends defined with "-drive if=T" with T other than "none" are
>>> meant to be picked up by machine initialization code: a suitable
>>> frontend gets created and wired up automatically.
>>>
>>> If machine initialization code doesn't comply, the block backend
>>> remains unused.  This triggers a warning since commit a66c9dc, v2.2.0.
>>> Drives created by default are excempted; use -nodefaults to get rid of
>>> them.
>>>
>>> Turn this warning into an error.
>>>
>>
>> "Exempted," oddly enough.
>> </eblake>
> 
> Thanks!
> 
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  blockdev.c                | 14 +++++++-------
>>>  include/sysemu/blockdev.h |  2 +-
>>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index a597a66..ec9c114 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -227,28 +227,28 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
>>>      return NULL;
>>>  }
>>>  
>>> -bool drive_check_orphaned(void)
>>> +void drive_check_orphaned(void)
>>>  {
>>>      BlockBackend *blk;
>>>      DriveInfo *dinfo;
>>>      Location loc;
>>> -    bool rs = false;
>>> +    bool orphans = false;
>>>  
>>>      for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>>>          dinfo = blk_legacy_dinfo(blk);
>>> -        /* If dinfo->bdrv->dev is NULL, it has no device attached. */
>>> -        /* Unless this is a default drive, this may be an oversight. */
>>>          if (!blk_get_attached_dev(blk) && !dinfo->is_default &&
>>>              dinfo->type != IF_NONE) {
>>>              loc_push_none(&loc);
>>>              qemu_opts_loc_restore(dinfo->opts);
>>> -            error_report("warning: machine type does not support this drive");
>>> +            error_report("machine type does not support this drive");
>>>              loc_pop(&loc);
>>> -            rs = true;
>>> +            orphans = true;
>>>          }
>>>      }
>>>  
>>> -    return rs;
>>> +    if (orphans) {
>>> +        exit(1);
>>> +    }
>>
>> Hardcore. Why not just return a status code and allow vl.c to exit? It
>> only has the one caller. (Unless you added more and I didn't read
>> because I'm lazy.)
>>
>> You could even add an Error **arg if you wanted to; but vl.c has exits
>> all over the dang place, so maybe that's not really interesting.
> 
> My excuse it that checking for orphans makes sense only during initial
> startup, and there, exit(1) on error is what we do.  Doing it right away
> is simplest.
> 
> I'm fine with leaving the exit() to the caller.  I would prefer to leave
> the printing to the caller as well then.
> 
> Anyone else got a preference?
> 

I guess my preference here was just simply to keep the exit() out of
blockdev.c to discourage code-by-example naughtiness in the future.

As many as I can keep or cram into vl.c, the better, I think?

Well, that's my story.

--js

Re: [Qemu-devel] [Qemu-block] [PATCH v2 7/8] blockdev: Make orphaned -drive fatal

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 01/31/2017 09:37 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> On 01/26/2017 10:09 AM, Markus Armbruster wrote:
>>>> Block backends defined with "-drive if=T" with T other than "none" are
>>>> meant to be picked up by machine initialization code: a suitable
>>>> frontend gets created and wired up automatically.
>>>>
>>>> If machine initialization code doesn't comply, the block backend
>>>> remains unused.  This triggers a warning since commit a66c9dc, v2.2.0.
>>>> Drives created by default are excempted; use -nodefaults to get rid of
>>>> them.
>>>>
>>>> Turn this warning into an error.
>>>>
>>>
>>> "Exempted," oddly enough.
>>> </eblake>
>> 
>> Thanks!
>> 
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>  blockdev.c                | 14 +++++++-------
>>>>  include/sysemu/blockdev.h |  2 +-
>>>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index a597a66..ec9c114 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -227,28 +227,28 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
>>>>      return NULL;
>>>>  }
>>>>  
>>>> -bool drive_check_orphaned(void)
>>>> +void drive_check_orphaned(void)
>>>>  {
>>>>      BlockBackend *blk;
>>>>      DriveInfo *dinfo;
>>>>      Location loc;
>>>> -    bool rs = false;
>>>> +    bool orphans = false;
>>>>  
>>>>      for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>>>>          dinfo = blk_legacy_dinfo(blk);
>>>> -        /* If dinfo->bdrv->dev is NULL, it has no device attached. */
>>>> -        /* Unless this is a default drive, this may be an oversight. */
>>>>          if (!blk_get_attached_dev(blk) && !dinfo->is_default &&
>>>>              dinfo->type != IF_NONE) {
>>>>              loc_push_none(&loc);
>>>>              qemu_opts_loc_restore(dinfo->opts);
>>>> -            error_report("warning: machine type does not support this drive");
>>>> +            error_report("machine type does not support this drive");
>>>>              loc_pop(&loc);
>>>> -            rs = true;
>>>> +            orphans = true;
>>>>          }
>>>>      }
>>>>  
>>>> -    return rs;
>>>> +    if (orphans) {
>>>> +        exit(1);
>>>> +    }
>>>
>>> Hardcore. Why not just return a status code and allow vl.c to exit? It
>>> only has the one caller. (Unless you added more and I didn't read
>>> because I'm lazy.)
>>>
>>> You could even add an Error **arg if you wanted to; but vl.c has exits
>>> all over the dang place, so maybe that's not really interesting.
>> 
>> My excuse it that checking for orphans makes sense only during initial
>> startup, and there, exit(1) on error is what we do.  Doing it right away
>> is simplest.
>> 
>> I'm fine with leaving the exit() to the caller.  I would prefer to leave
>> the printing to the caller as well then.

Returning an Error makes reporting all offending -drive too bothersome
to be worthwhile.  We'd report just the first one then.  Pity.

>> Anyone else got a preference?
>> 
>
> I guess my preference here was just simply to keep the exit() out of
> blockdev.c to discourage code-by-example naughtiness in the future.

Understand.  Of course, the error handling rules are a good deal more
complex than "don't exit()".

During initial startup, including command line processing, we exit() on
error.  This is generally wrong elsewhere.  exit() right away saves us
the error-prone busy-work of propagating errors and cleaning up along
the way.  Half-hearted error handling like "I don't dare to exit(), so I
propagate, but I can't be bothered to clean up, because I know we'll
exit()" is the worst, in my opinion.

For reporting errors during startup, error_report() is appropriate.
fprintf() is bad, because it leads to inferior error messages (no
location, no timestamp).

In HMP commands, error_report() is appopriate.  monitor_printf() works,
but is bad style, because it makes errors less obvious in the code.

In code that's used for both by HMP and startup, error_report() is fine,
but you can't exit().

When error_report() is appropriate, calling it right away saves us the
propagating.  Occasionally permits better error messages, because it's
less of a straitjacket.

In QMP commands, you need to error_setg().  error_report(),
monitor_printf, fprintf() and so forth are all wrong.

In code that doesn't (want to) know in what context (startup, HMP, QMP,
other) it is used, you generally need to error_setg().  However, there
are situations where that is plainly impossible, e.g. in a callback for
a guest device register access.  error_report() may be the best you can
do there.  abort() or its fancy cousin hw_error() is still used in many
places.

My point is: when the context is clear, what to do is fairly obvious.
Sadly, we tend to mix code meant for different contexts willy-nilly,
which can make the context needlessly hard to see.

One way to deal with that is to minimize relying on context.  Minimizing
assumptions is good.  Complicating / boiler-plating the code is bad.
Tradeoff.

When a function could conceivably be useful in different contexts,
keeping it unaware of the context can be worth the boilerplate, even
when it's currently used only in one context.

But when it's clearly useful in just one context, I tend to balk.

drive_check_orphaned() makes sense only during startup.  Not relying on
that isn't so bad, because there's nothing to clean up.  Would be easier
to decide if it was bad ;)

> As many as I can keep or cram into vl.c, the better, I think?

Only 142 out of 1000+ exit() are in vl.c.  You got work to do!

> Well, that's my story.
>
> --js

[Qemu-devel] [PATCH v2 8/8] hw: Drop superfluous special checks for orphaned -drive

[Markus Armbruster]

We've traditionally rejected orphans here and there, but not
systematically.  For instance, the sun4m machines have an onboard SCSI
HBA (bus=0), and have always rejected bus>0.  Other machines with an
onboard SCSI HBA don't.

Commit a66c9dc made all orphans trigger a warning, and the previous
commit turned this into an error.  The checks "here and there" are now
redundant.  Drop them.

Note that the one in mips_jazz.c was wrong: it rejected bus > MAX_FD,
but MAX_FD is the number of floppy drives per bus.

Error messages change from

    $ qemu-system-x86_64 -drive if=ide,bus=2
    qemu-system-x86_64: Too many IDE buses defined (3 > 2)
    $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
    qemu: too many floppy drives
    $ qemu-system-sparc -M LX -drive if=scsi,bus=1
    qemu: too many SCSI bus

to

    $ qemu-system-x86_64 -drive if=ide,bus=2
    qemu-system-x86_64: -drive if=ide,bus=2: machine type does not support this drive
    $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
    qemu-system-mips64: -drive if=floppy,bus=2,id=fd1: machine type does not support this drive
    $ qemu-system-sparc -M LX -drive if=scsi,bus=1
    qemu-system-sparc: -drive if=scsi,bus=1: machine type does not support this drive

Cc: John Snow <jsnow@redhat.com>
Cc: "Hervé Poussineau" <hpoussin@reactos.org>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c       | 17 -----------------
 hw/mips/mips_jazz.c |  4 ----
 hw/sparc/sun4m.c    |  5 -----
 3 files changed, 26 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 43709e5..cfa5de6 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2840,23 +2840,6 @@ const VMStateDescription vmstate_ide_bus = {
 void ide_drive_get(DriveInfo **hd, int n)
 {
     int i;
-    int highest_bus = drive_get_max_bus(IF_IDE) + 1;
-    int max_devs = drive_get_max_devs(IF_IDE);
-    int n_buses = max_devs ? (n / max_devs) : n;
-
-    /*
-     * Note: The number of actual buses available is not known.
-     * We compute this based on the size of the DriveInfo* array, n.
-     * If it is less than max_devs * <num_real_buses>,
-     * We will stop looking for drives prematurely instead of overfilling
-     * the array.
-     */
-
-    if (highest_bus > n_buses) {
-        error_report("Too many IDE buses defined (%d > %d)",
-                     highest_bus, n_buses);
-        exit(1);
-    }
 
     for (i = 0; i < n; i++) {
         hd[i] = drive_get_by_index(IF_IDE, i);
diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
index 73f6c9f..1cef581 100644
--- a/hw/mips/mips_jazz.c
+++ b/hw/mips/mips_jazz.c
@@ -291,10 +291,6 @@ static void mips_jazz_init(MachineState *machine,
              qdev_get_gpio_in(rc4030, 5), &esp_reset, &dma_enable);
 
     /* Floppy */
-    if (drive_get_max_bus(IF_FLOPPY) >= MAX_FD) {
-        fprintf(stderr, "qemu: too many floppy drives\n");
-        exit(1);
-    }
     for (n = 0; n < MAX_FD; n++) {
         fds[n] = drive_get(IF_FLOPPY, 0, n);
     }
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index f5b6efd..61416a6 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -989,11 +989,6 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
     slavio_misc_init(hwdef->slavio_base, hwdef->aux1_base, hwdef->aux2_base,
                      slavio_irq[30], fdc_tc);
 
-    if (drive_get_max_bus(IF_SCSI) > 0) {
-        fprintf(stderr, "qemu: too many SCSI bus\n");
-        exit(1);
-    }
-
     esp_init(hwdef->esp_base, 2,
              espdma_memory_read, espdma_memory_write,
              espdma, espdma_irq, &esp_reset, &dma_enable);
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 8/8] hw: Drop superfluous special checks for orphaned -drive

[John Snow]

On 01/26/2017 10:09 AM, Markus Armbruster wrote:
> We've traditionally rejected orphans here and there, but not
> systematically.  For instance, the sun4m machines have an onboard SCSI
> HBA (bus=0), and have always rejected bus>0.  Other machines with an
> onboard SCSI HBA don't.
> 
> Commit a66c9dc made all orphans trigger a warning, and the previous
> commit turned this into an error.  The checks "here and there" are now
> redundant.  Drop them.
> 
> Note that the one in mips_jazz.c was wrong: it rejected bus > MAX_FD,
> but MAX_FD is the number of floppy drives per bus.
> 
> Error messages change from
> 
>     $ qemu-system-x86_64 -drive if=ide,bus=2
>     qemu-system-x86_64: Too many IDE buses defined (3 > 2)
>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>     qemu: too many floppy drives
>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>     qemu: too many SCSI bus
> 
> to
> 
>     $ qemu-system-x86_64 -drive if=ide,bus=2
>     qemu-system-x86_64: -drive if=ide,bus=2: machine type does not support this drive
>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>     qemu-system-mips64: -drive if=floppy,bus=2,id=fd1: machine type does not support this drive
>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>     qemu-system-sparc: -drive if=scsi,bus=1: machine type does not support this drive
> 

Hm, that's a lot less helpful, isn't it? Can we augment with hints?

Re: [Qemu-devel] [PATCH v2 8/8] hw: Drop superfluous special checks for orphaned -drive

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 01/26/2017 10:09 AM, Markus Armbruster wrote:
>> We've traditionally rejected orphans here and there, but not
>> systematically.  For instance, the sun4m machines have an onboard SCSI
>> HBA (bus=0), and have always rejected bus>0.  Other machines with an
>> onboard SCSI HBA don't.
>> 
>> Commit a66c9dc made all orphans trigger a warning, and the previous
>> commit turned this into an error.  The checks "here and there" are now
>> redundant.  Drop them.
>> 
>> Note that the one in mips_jazz.c was wrong: it rejected bus > MAX_FD,
>> but MAX_FD is the number of floppy drives per bus.
>> 
>> Error messages change from
>> 
>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>>     qemu-system-x86_64: Too many IDE buses defined (3 > 2)
>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>>     qemu: too many floppy drives
>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>>     qemu: too many SCSI bus
>> 
>> to
>> 
>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>>     qemu-system-x86_64: -drive if=ide,bus=2: machine type does not support this drive
>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>>     qemu-system-mips64: -drive if=floppy,bus=2,id=fd1: machine type does not support this drive
>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>>     qemu-system-sparc: -drive if=scsi,bus=1: machine type does not support this drive
>> 
>
> Hm, that's a lot less helpful, isn't it? Can we augment with hints?

The message itself may be less specific, but it now comes with a precise
location.  Personally, I'd even find

    qemu-system-sparc: -drive if=scsi,bus=1: *mumble* *mumble*

more helpful than

    qemu: too many SCSI bus

because the former tells me *which* of the options is bad.  We tend to
have lots and lots of them.

The deleted special case errors cover only a minority of "orphan"
-drive.  If these cases need improvement, then so will the general case.
If you can come up with a hint that makes the general case message more
useful, I'm more than happy to squash it into PATCH 6.

Re: [Qemu-devel] [PATCH v2 8/8] hw: Drop superfluous special checks for orphaned -drive

[John Snow]

On 01/27/2017 06:51 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 01/26/2017 10:09 AM, Markus Armbruster wrote:
>>> We've traditionally rejected orphans here and there, but not
>>> systematically.  For instance, the sun4m machines have an onboard SCSI
>>> HBA (bus=0), and have always rejected bus>0.  Other machines with an
>>> onboard SCSI HBA don't.
>>>
>>> Commit a66c9dc made all orphans trigger a warning, and the previous
>>> commit turned this into an error.  The checks "here and there" are now
>>> redundant.  Drop them.
>>>
>>> Note that the one in mips_jazz.c was wrong: it rejected bus > MAX_FD,
>>> but MAX_FD is the number of floppy drives per bus.
>>>
>>> Error messages change from
>>>
>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>>>     qemu-system-x86_64: Too many IDE buses defined (3 > 2)
>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>>>     qemu: too many floppy drives
>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>>>     qemu: too many SCSI bus
>>>
>>> to
>>>
>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>>>     qemu-system-x86_64: -drive if=ide,bus=2: machine type does not support this drive
>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>>>     qemu-system-mips64: -drive if=floppy,bus=2,id=fd1: machine type does not support this drive
>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>>>     qemu-system-sparc: -drive if=scsi,bus=1: machine type does not support this drive
>>>
>>
>> Hm, that's a lot less helpful, isn't it? Can we augment with hints?
> 
> The message itself may be less specific, but it now comes with a precise
> location.  Personally, I'd even find
> 
>     qemu-system-sparc: -drive if=scsi,bus=1: *mumble* *mumble*
> 
> more helpful than
> 
>     qemu: too many SCSI bus
> 
> because the former tells me *which* of the options is bad.  We tend to
> have lots and lots of them.
> 
> The deleted special case errors cover only a minority of "orphan"
> -drive.  If these cases need improvement, then so will the general case.
> If you can come up with a hint that makes the general case message more
> useful, I'm more than happy to squash it into PATCH 6.
> 

The old error had "why" and the new error has "where" but neither has
both. I would suggest that from the "why" you can divine the "where,"
but the opposite is not as easily true.

The new error even suggests information I think is wrong and misleading:
We do support SCSI! (Just not this many of them.)

No suggestions for how or where to append the hints. This is not the
hill I am preparing to die on.

--js

Re: [Qemu-devel] [PATCH v2 8/8] hw: Drop superfluous special checks for orphaned -drive

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 01/27/2017 06:51 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> On 01/26/2017 10:09 AM, Markus Armbruster wrote:
>>>> We've traditionally rejected orphans here and there, but not
>>>> systematically.  For instance, the sun4m machines have an onboard SCSI
>>>> HBA (bus=0), and have always rejected bus>0.  Other machines with an
>>>> onboard SCSI HBA don't.
>>>>
>>>> Commit a66c9dc made all orphans trigger a warning, and the previous
>>>> commit turned this into an error.  The checks "here and there" are now
>>>> redundant.  Drop them.
>>>>
>>>> Note that the one in mips_jazz.c was wrong: it rejected bus > MAX_FD,
>>>> but MAX_FD is the number of floppy drives per bus.
>>>>
>>>> Error messages change from
>>>>
>>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>>>>     qemu-system-x86_64: Too many IDE buses defined (3 > 2)
>>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>>>>     qemu: too many floppy drives
>>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>>>>     qemu: too many SCSI bus
>>>>
>>>> to
>>>>
>>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>>>>     qemu-system-x86_64: -drive if=ide,bus=2: machine type does not support this drive
>>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>>>>     qemu-system-mips64: -drive if=floppy,bus=2,id=fd1: machine type does not support this drive
>>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>>>>     qemu-system-sparc: -drive if=scsi,bus=1: machine type does not support this drive
>>>>
>>>
>>> Hm, that's a lot less helpful, isn't it? Can we augment with hints?
>> 
>> The message itself may be less specific, but it now comes with a precise
>> location.  Personally, I'd even find
>> 
>>     qemu-system-sparc: -drive if=scsi,bus=1: *mumble* *mumble*
>> 
>> more helpful than
>> 
>>     qemu: too many SCSI bus
>> 
>> because the former tells me *which* of the options is bad.  We tend to
>> have lots and lots of them.
>> 
>> The deleted special case errors cover only a minority of "orphan"
>> -drive.  If these cases need improvement, then so will the general case.
>> If you can come up with a hint that makes the general case message more
>> useful, I'm more than happy to squash it into PATCH 6.
>> 
>
> The old error had "why" and the new error has "where" but neither has
> both. I would suggest that from the "why" you can divine the "where,"
> but the opposite is not as easily true.

Some users will be able to divine more easily than others.  Consider my
"too many floppy drives" example.  There's just one, and the machine
actually supports two.  The user has to make the connection to "bus=2"
somehow.  Now, anybody crazy enough to mess with bus= can probably be
expected to figure this out, but still, the deleted error messages
aren't exactly wonderful.

> The new error even suggests information I think is wrong and misleading:
> We do support SCSI! (Just not this many of them.)

Well, the error doesn't say "machine doesn't support SCSI", only
"doesn't support this particular -drive".  Perhaps it could be worded
more clearly.  Ideas?

> No suggestions for how or where to append the hints. This is not the
> hill I am preparing to die on.

Plenty of hills around...

Re: [Qemu-devel] [PATCH v2 8/8] hw: Drop superfluous special checks for orphaned -drive

[John Snow]

On 01/27/2017 11:04 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 01/27/2017 06:51 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> On 01/26/2017 10:09 AM, Markus Armbruster wrote:
>>>>> We've traditionally rejected orphans here and there, but not
>>>>> systematically.  For instance, the sun4m machines have an onboard SCSI
>>>>> HBA (bus=0), and have always rejected bus>0.  Other machines with an
>>>>> onboard SCSI HBA don't.
>>>>>
>>>>> Commit a66c9dc made all orphans trigger a warning, and the previous
>>>>> commit turned this into an error.  The checks "here and there" are now
>>>>> redundant.  Drop them.
>>>>>
>>>>> Note that the one in mips_jazz.c was wrong: it rejected bus > MAX_FD,
>>>>> but MAX_FD is the number of floppy drives per bus.
>>>>>
>>>>> Error messages change from
>>>>>
>>>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>>>>>     qemu-system-x86_64: Too many IDE buses defined (3 > 2)
>>>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>>>>>     qemu: too many floppy drives
>>>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>>>>>     qemu: too many SCSI bus
>>>>>
>>>>> to
>>>>>
>>>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>>>>>     qemu-system-x86_64: -drive if=ide,bus=2: machine type does not support this drive
>>>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>>>>>     qemu-system-mips64: -drive if=floppy,bus=2,id=fd1: machine type does not support this drive
>>>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>>>>>     qemu-system-sparc: -drive if=scsi,bus=1: machine type does not support this drive
>>>>>
>>>>
>>>> Hm, that's a lot less helpful, isn't it? Can we augment with hints?
>>>
>>> The message itself may be less specific, but it now comes with a precise
>>> location.  Personally, I'd even find
>>>
>>>     qemu-system-sparc: -drive if=scsi,bus=1: *mumble* *mumble*
>>>
>>> more helpful than
>>>
>>>     qemu: too many SCSI bus
>>>
>>> because the former tells me *which* of the options is bad.  We tend to
>>> have lots and lots of them.
>>>
>>> The deleted special case errors cover only a minority of "orphan"
>>> -drive.  If these cases need improvement, then so will the general case.
>>> If you can come up with a hint that makes the general case message more
>>> useful, I'm more than happy to squash it into PATCH 6.
>>>
>>
>> The old error had "why" and the new error has "where" but neither has
>> both. I would suggest that from the "why" you can divine the "where,"
>> but the opposite is not as easily true.
> 
> Some users will be able to divine more easily than others.  Consider my
> "too many floppy drives" example.  There's just one, and the machine
> actually supports two.  The user has to make the connection to "bus=2"
> somehow.  Now, anybody crazy enough to mess with bus= can probably be
> expected to figure this out, but still, the deleted error messages
> aren't exactly wonderful.
> 
>> The new error even suggests information I think is wrong and misleading:
>> We do support SCSI! (Just not this many of them.)
> 
> Well, the error doesn't say "machine doesn't support SCSI", only
> "doesn't support this particular -drive".  Perhaps it could be worded
> more clearly.  Ideas?
> 

Ah, I see what you mean now. I interpreted "this drive" to mean SCSI,
not this SCSI *instance*. If it can be made clearer that QEMU is simply
unable to instantiate this particular instance, that'd be fine.

Instead of "Machine type does not support this drive,"

how about

"Machine type cannot instantiate this drive instance"

Or ... follow your own best judgement. This is really YOUR wheelhouse.
My example is a little wordy.

>> No suggestions for how or where to append the hints. This is not the
>> hill I am preparing to die on.
> 
> Plenty of hills around...
> 

Some of them have flowers.

Re: [Qemu-devel] [PATCH v2 8/8] hw: Drop superfluous special checks for orphaned -drive

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 01/27/2017 11:04 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> On 01/27/2017 06:51 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> On 01/26/2017 10:09 AM, Markus Armbruster wrote:
>>>>>> We've traditionally rejected orphans here and there, but not
>>>>>> systematically.  For instance, the sun4m machines have an onboard SCSI
>>>>>> HBA (bus=0), and have always rejected bus>0.  Other machines with an
>>>>>> onboard SCSI HBA don't.
>>>>>>
>>>>>> Commit a66c9dc made all orphans trigger a warning, and the previous
>>>>>> commit turned this into an error.  The checks "here and there" are now
>>>>>> redundant.  Drop them.
>>>>>>
>>>>>> Note that the one in mips_jazz.c was wrong: it rejected bus > MAX_FD,
>>>>>> but MAX_FD is the number of floppy drives per bus.
>>>>>>
>>>>>> Error messages change from
>>>>>>
>>>>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>>>>>>     qemu-system-x86_64: Too many IDE buses defined (3 > 2)
>>>>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>>>>>>     qemu: too many floppy drives
>>>>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>>>>>>     qemu: too many SCSI bus
>>>>>>
>>>>>> to
>>>>>>
>>>>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>>>>>>     qemu-system-x86_64: -drive if=ide,bus=2: machine type does not support this drive
>>>>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>>>>>>     qemu-system-mips64: -drive if=floppy,bus=2,id=fd1: machine type does not support this drive
>>>>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>>>>>>     qemu-system-sparc: -drive if=scsi,bus=1: machine type does not support this drive
>>>>>>
>>>>>
>>>>> Hm, that's a lot less helpful, isn't it? Can we augment with hints?
>>>>
>>>> The message itself may be less specific, but it now comes with a precise
>>>> location.  Personally, I'd even find
>>>>
>>>>     qemu-system-sparc: -drive if=scsi,bus=1: *mumble* *mumble*
>>>>
>>>> more helpful than
>>>>
>>>>     qemu: too many SCSI bus
>>>>
>>>> because the former tells me *which* of the options is bad.  We tend to
>>>> have lots and lots of them.
>>>>
>>>> The deleted special case errors cover only a minority of "orphan"
>>>> -drive.  If these cases need improvement, then so will the general case.
>>>> If you can come up with a hint that makes the general case message more
>>>> useful, I'm more than happy to squash it into PATCH 6.
>>>>
>>>
>>> The old error had "why" and the new error has "where" but neither has
>>> both. I would suggest that from the "why" you can divine the "where,"
>>> but the opposite is not as easily true.
>> 
>> Some users will be able to divine more easily than others.  Consider my
>> "too many floppy drives" example.  There's just one, and the machine
>> actually supports two.  The user has to make the connection to "bus=2"
>> somehow.  Now, anybody crazy enough to mess with bus= can probably be
>> expected to figure this out, but still, the deleted error messages
>> aren't exactly wonderful.
>> 
>>> The new error even suggests information I think is wrong and misleading:
>>> We do support SCSI! (Just not this many of them.)
>> 
>> Well, the error doesn't say "machine doesn't support SCSI", only
>> "doesn't support this particular -drive".  Perhaps it could be worded
>> more clearly.  Ideas?
>> 
>
> Ah, I see what you mean now. I interpreted "this drive" to mean SCSI,
> not this SCSI *instance*.

Uh, I would've written "does not support if=scsi" then.  But I see where
you come from.

>                           If it can be made clearer that QEMU is simply
> unable to instantiate this particular instance, that'd be fine.
>
> Instead of "Machine type does not support this drive,"
>
> how about
>
> "Machine type cannot instantiate this drive instance"

Hmm.

> Or ... follow your own best judgement. This is really YOUR wheelhouse.
> My example is a little wordy.

All I can come up with is even wordier: "Machine type doesn't support
this combination of if, bus, unit", or "if=scsi,bus=%d,unit=%d not
supported with this machine type".  Could also be confusing when the
user specified index instead of bus, unit.

Good error messages are hard...

Re: [Qemu-devel] [PATCH v2 8/8] hw: Drop superfluous special checks for orphaned -drive

[John Snow]

On 01/30/2017 03:10 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 01/27/2017 11:04 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> On 01/27/2017 06:51 AM, Markus Armbruster wrote:
>>>>> John Snow <jsnow@redhat.com> writes:
>>>>>
>>>>>> On 01/26/2017 10:09 AM, Markus Armbruster wrote:
>>>>>>> We've traditionally rejected orphans here and there, but not
>>>>>>> systematically.  For instance, the sun4m machines have an onboard SCSI
>>>>>>> HBA (bus=0), and have always rejected bus>0.  Other machines with an
>>>>>>> onboard SCSI HBA don't.
>>>>>>>
>>>>>>> Commit a66c9dc made all orphans trigger a warning, and the previous
>>>>>>> commit turned this into an error.  The checks "here and there" are now
>>>>>>> redundant.  Drop them.
>>>>>>>
>>>>>>> Note that the one in mips_jazz.c was wrong: it rejected bus > MAX_FD,
>>>>>>> but MAX_FD is the number of floppy drives per bus.
>>>>>>>
>>>>>>> Error messages change from
>>>>>>>
>>>>>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>>>>>>>     qemu-system-x86_64: Too many IDE buses defined (3 > 2)
>>>>>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>>>>>>>     qemu: too many floppy drives
>>>>>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>>>>>>>     qemu: too many SCSI bus
>>>>>>>
>>>>>>> to
>>>>>>>
>>>>>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>>>>>>>     qemu-system-x86_64: -drive if=ide,bus=2: machine type does not support this drive
>>>>>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>>>>>>>     qemu-system-mips64: -drive if=floppy,bus=2,id=fd1: machine type does not support this drive
>>>>>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>>>>>>>     qemu-system-sparc: -drive if=scsi,bus=1: machine type does not support this drive
>>>>>>>
>>>>>>
>>>>>> Hm, that's a lot less helpful, isn't it? Can we augment with hints?
>>>>>
>>>>> The message itself may be less specific, but it now comes with a precise
>>>>> location.  Personally, I'd even find
>>>>>
>>>>>     qemu-system-sparc: -drive if=scsi,bus=1: *mumble* *mumble*
>>>>>
>>>>> more helpful than
>>>>>
>>>>>     qemu: too many SCSI bus
>>>>>
>>>>> because the former tells me *which* of the options is bad.  We tend to
>>>>> have lots and lots of them.
>>>>>
>>>>> The deleted special case errors cover only a minority of "orphan"
>>>>> -drive.  If these cases need improvement, then so will the general case.
>>>>> If you can come up with a hint that makes the general case message more
>>>>> useful, I'm more than happy to squash it into PATCH 6.
>>>>>
>>>>
>>>> The old error had "why" and the new error has "where" but neither has
>>>> both. I would suggest that from the "why" you can divine the "where,"
>>>> but the opposite is not as easily true.
>>>
>>> Some users will be able to divine more easily than others.  Consider my
>>> "too many floppy drives" example.  There's just one, and the machine
>>> actually supports two.  The user has to make the connection to "bus=2"
>>> somehow.  Now, anybody crazy enough to mess with bus= can probably be
>>> expected to figure this out, but still, the deleted error messages
>>> aren't exactly wonderful.
>>>
>>>> The new error even suggests information I think is wrong and misleading:
>>>> We do support SCSI! (Just not this many of them.)
>>>
>>> Well, the error doesn't say "machine doesn't support SCSI", only
>>> "doesn't support this particular -drive".  Perhaps it could be worded
>>> more clearly.  Ideas?
>>>
>>
>> Ah, I see what you mean now. I interpreted "this drive" to mean SCSI,
>> not this SCSI *instance*.
> 
> Uh, I would've written "does not support if=scsi" then.  But I see where
> you come from.
> 
>>                           If it can be made clearer that QEMU is simply
>> unable to instantiate this particular instance, that'd be fine.
>>
>> Instead of "Machine type does not support this drive,"
>>
>> how about
>>
>> "Machine type cannot instantiate this drive instance"
> 
> Hmm.
> 
>> Or ... follow your own best judgement. This is really YOUR wheelhouse.
>> My example is a little wordy.
> 
> All I can come up with is even wordier: "Machine type doesn't support
> this combination of if, bus, unit", or "if=scsi,bus=%d,unit=%d not
> supported with this machine type".  Could also be confusing when the
> user specified index instead of bus, unit.
> 
> Good error messages are hard...
> 

Yes, so just follow your own instincts. Just offering a non-blocking
comment.

--js

Re: [Qemu-devel] [PATCH v2 8/8] hw: Drop superfluous special checks for orphaned -drive

[Fam Zheng]

On Mon, 01/30 09:10, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
> > On 01/27/2017 11:04 AM, Markus Armbruster wrote:
> >> John Snow <jsnow@redhat.com> writes:
> >> 
> >>> On 01/27/2017 06:51 AM, Markus Armbruster wrote:
> >>>> John Snow <jsnow@redhat.com> writes:
> >>>>
> >>>>> On 01/26/2017 10:09 AM, Markus Armbruster wrote:
> >>>>>> We've traditionally rejected orphans here and there, but not
> >>>>>> systematically.  For instance, the sun4m machines have an onboard SCSI
> >>>>>> HBA (bus=0), and have always rejected bus>0.  Other machines with an
> >>>>>> onboard SCSI HBA don't.
> >>>>>>
> >>>>>> Commit a66c9dc made all orphans trigger a warning, and the previous
> >>>>>> commit turned this into an error.  The checks "here and there" are now
> >>>>>> redundant.  Drop them.
> >>>>>>
> >>>>>> Note that the one in mips_jazz.c was wrong: it rejected bus > MAX_FD,
> >>>>>> but MAX_FD is the number of floppy drives per bus.
> >>>>>>
> >>>>>> Error messages change from
> >>>>>>
> >>>>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
> >>>>>>     qemu-system-x86_64: Too many IDE buses defined (3 > 2)
> >>>>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
> >>>>>>     qemu: too many floppy drives
> >>>>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
> >>>>>>     qemu: too many SCSI bus
> >>>>>>
> >>>>>> to
> >>>>>>
> >>>>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
> >>>>>>     qemu-system-x86_64: -drive if=ide,bus=2: machine type does not support this drive
> >>>>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
> >>>>>>     qemu-system-mips64: -drive if=floppy,bus=2,id=fd1: machine type does not support this drive
> >>>>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
> >>>>>>     qemu-system-sparc: -drive if=scsi,bus=1: machine type does not support this drive
> >>>>>>
> >>>>>
> >>>>> Hm, that's a lot less helpful, isn't it? Can we augment with hints?
> >>>>
> >>>> The message itself may be less specific, but it now comes with a precise
> >>>> location.  Personally, I'd even find
> >>>>
> >>>>     qemu-system-sparc: -drive if=scsi,bus=1: *mumble* *mumble*
> >>>>
> >>>> more helpful than
> >>>>
> >>>>     qemu: too many SCSI bus
> >>>>
> >>>> because the former tells me *which* of the options is bad.  We tend to
> >>>> have lots and lots of them.
> >>>>
> >>>> The deleted special case errors cover only a minority of "orphan"
> >>>> -drive.  If these cases need improvement, then so will the general case.
> >>>> If you can come up with a hint that makes the general case message more
> >>>> useful, I'm more than happy to squash it into PATCH 6.
> >>>>
> >>>
> >>> The old error had "why" and the new error has "where" but neither has
> >>> both. I would suggest that from the "why" you can divine the "where,"
> >>> but the opposite is not as easily true.
> >> 
> >> Some users will be able to divine more easily than others.  Consider my
> >> "too many floppy drives" example.  There's just one, and the machine
> >> actually supports two.  The user has to make the connection to "bus=2"
> >> somehow.  Now, anybody crazy enough to mess with bus= can probably be
> >> expected to figure this out, but still, the deleted error messages
> >> aren't exactly wonderful.
> >> 
> >>> The new error even suggests information I think is wrong and misleading:
> >>> We do support SCSI! (Just not this many of them.)
> >> 
> >> Well, the error doesn't say "machine doesn't support SCSI", only
> >> "doesn't support this particular -drive".  Perhaps it could be worded
> >> more clearly.  Ideas?
> >> 
> >
> > Ah, I see what you mean now. I interpreted "this drive" to mean SCSI,
> > not this SCSI *instance*.
> 
> Uh, I would've written "does not support if=scsi" then.  But I see where
> you come from.
> 
> >                           If it can be made clearer that QEMU is simply
> > unable to instantiate this particular instance, that'd be fine.
> >
> > Instead of "Machine type does not support this drive,"
> >
> > how about
> >
> > "Machine type cannot instantiate this drive instance"
> 
> Hmm.
> 
> > Or ... follow your own best judgement. This is really YOUR wheelhouse.
> > My example is a little wordy.
> 
> All I can come up with is even wordier: "Machine type doesn't support
> this combination of if, bus, unit", or "if=scsi,bus=%d,unit=%d not
> supported with this machine type".  Could also be confusing when the
> user specified index instead of bus, unit.
> 
> Good error messages are hard...

I guess the painpoint is "okay, what the heck does the machine support then?" -
that "3 > 2" is the good part of the old message.

Do we have a user documentation for this? Or, can we give a hint how to figure
that out?

Fam

Re: [Qemu-devel] [PATCH v2 8/8] hw: Drop superfluous special checks for orphaned -drive

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Mon, 01/30 09:10, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>> > On 01/27/2017 11:04 AM, Markus Armbruster wrote:
>> >> John Snow <jsnow@redhat.com> writes:
>> >> 
>> >>> On 01/27/2017 06:51 AM, Markus Armbruster wrote:
>> >>>> John Snow <jsnow@redhat.com> writes:
>> >>>>
>> >>>>> On 01/26/2017 10:09 AM, Markus Armbruster wrote:
>> >>>>>> We've traditionally rejected orphans here and there, but not
>> >>>>>> systematically.  For instance, the sun4m machines have an onboard SCSI
>> >>>>>> HBA (bus=0), and have always rejected bus>0.  Other machines with an
>> >>>>>> onboard SCSI HBA don't.
>> >>>>>>
>> >>>>>> Commit a66c9dc made all orphans trigger a warning, and the previous
>> >>>>>> commit turned this into an error.  The checks "here and there" are now
>> >>>>>> redundant.  Drop them.
>> >>>>>>
>> >>>>>> Note that the one in mips_jazz.c was wrong: it rejected bus > MAX_FD,
>> >>>>>> but MAX_FD is the number of floppy drives per bus.
>> >>>>>>
>> >>>>>> Error messages change from
>> >>>>>>
>> >>>>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>> >>>>>>     qemu-system-x86_64: Too many IDE buses defined (3 > 2)
>> >>>>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>> >>>>>>     qemu: too many floppy drives
>> >>>>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>> >>>>>>     qemu: too many SCSI bus
>> >>>>>>
>> >>>>>> to
>> >>>>>>
>> >>>>>>     $ qemu-system-x86_64 -drive if=ide,bus=2
>> >>>>>>     qemu-system-x86_64: -drive if=ide,bus=2: machine type does not support this drive
>> >>>>>>     $ qemu-system-mips64 -M magnum,accel=qtest -drive if=floppy,bus=2,id=fd1
>> >>>>>>     qemu-system-mips64: -drive if=floppy,bus=2,id=fd1: machine type does not support this drive
>> >>>>>>     $ qemu-system-sparc -M LX -drive if=scsi,bus=1
>> >>>>>>     qemu-system-sparc: -drive if=scsi,bus=1: machine type does not support this drive
>> >>>>>>
>> >>>>>
>> >>>>> Hm, that's a lot less helpful, isn't it? Can we augment with hints?
>> >>>>
>> >>>> The message itself may be less specific, but it now comes with a precise
>> >>>> location.  Personally, I'd even find
>> >>>>
>> >>>>     qemu-system-sparc: -drive if=scsi,bus=1: *mumble* *mumble*
>> >>>>
>> >>>> more helpful than
>> >>>>
>> >>>>     qemu: too many SCSI bus
>> >>>>
>> >>>> because the former tells me *which* of the options is bad.  We tend to
>> >>>> have lots and lots of them.
>> >>>>
>> >>>> The deleted special case errors cover only a minority of "orphan"
>> >>>> -drive.  If these cases need improvement, then so will the general case.
>> >>>> If you can come up with a hint that makes the general case message more
>> >>>> useful, I'm more than happy to squash it into PATCH 6.
>> >>>>
>> >>>
>> >>> The old error had "why" and the new error has "where" but neither has
>> >>> both. I would suggest that from the "why" you can divine the "where,"
>> >>> but the opposite is not as easily true.
>> >> 
>> >> Some users will be able to divine more easily than others.  Consider my
>> >> "too many floppy drives" example.  There's just one, and the machine
>> >> actually supports two.  The user has to make the connection to "bus=2"
>> >> somehow.  Now, anybody crazy enough to mess with bus= can probably be
>> >> expected to figure this out, but still, the deleted error messages
>> >> aren't exactly wonderful.
>> >> 
>> >>> The new error even suggests information I think is wrong and misleading:
>> >>> We do support SCSI! (Just not this many of them.)
>> >> 
>> >> Well, the error doesn't say "machine doesn't support SCSI", only
>> >> "doesn't support this particular -drive".  Perhaps it could be worded
>> >> more clearly.  Ideas?
>> >> 
>> >
>> > Ah, I see what you mean now. I interpreted "this drive" to mean SCSI,
>> > not this SCSI *instance*.
>> 
>> Uh, I would've written "does not support if=scsi" then.  But I see where
>> you come from.
>> 
>> >                           If it can be made clearer that QEMU is simply
>> > unable to instantiate this particular instance, that'd be fine.
>> >
>> > Instead of "Machine type does not support this drive,"
>> >
>> > how about
>> >
>> > "Machine type cannot instantiate this drive instance"
>> 
>> Hmm.
>> 
>> > Or ... follow your own best judgement. This is really YOUR wheelhouse.
>> > My example is a little wordy.
>> 
>> All I can come up with is even wordier: "Machine type doesn't support
>> this combination of if, bus, unit", or "if=scsi,bus=%d,unit=%d not
>> supported with this machine type".  Could also be confusing when the
>> user specified index instead of bus, unit.
>> 
>> Good error messages are hard...
>
> I guess the painpoint is "okay, what the heck does the machine support then?" -

At the place where we can reliably detect orphaned drives regardless of
what the machine initialization code does, we have no idea.

> that "3 > 2" is the good part of the old message.

At the places that actually adopt drives, we know.  But there are many
of them.  Just three check for orphans.  One of them gets it right
(ide/core.c), one of them gets it wrong (mips_jazz.c), and one of them
sets a problematic example (sun4m.c): if copied to a machine that lets
users configure additional SCSI HBAs, it would break if=scsi for those.
Dampens my enthusiasm for improving the error message by adding similar
checks to all the places that adopt drives.

We could make machines declare what they support.  Better, I think.  So
if you have a burning desire to leave your mark in git-blame for every
machine...

> Do we have a user documentation for this? Or, can we give a hint how to figure
> that out?

Try it out:

    for i in ide scsi floppy pflash mtd sd virtio xen
    do
        for ((b=0; b<4; b++))
        do
            for ((u=0; u<10; u++))
            do
                echo q | qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -drive if=$i,bus=$b,unit=$u,file=tmp.qcow2 >/dev/null
            done
        done
    done

My upper bounds for bus and unit are arbitrary.

Re: [Qemu-devel] [PATCH v2 8/8] hw: Drop superfluous special checks for orphaned -drive

[Fam Zheng]

On Fri, 02/03 14:35, Markus Armbruster wrote:
> > I guess the painpoint is "okay, what the heck does the machine support then?" -
> 
> At the place where we can reliably detect orphaned drives regardless of
> what the machine initialization code does, we have no idea.
> 
> > that "3 > 2" is the good part of the old message.
> 
> At the places that actually adopt drives, we know.  But there are many
> of them.  Just three check for orphans.  One of them gets it right
> (ide/core.c), one of them gets it wrong (mips_jazz.c), and one of them
> sets a problematic example (sun4m.c): if copied to a machine that lets
> users configure additional SCSI HBAs, it would break if=scsi for those.
> Dampens my enthusiasm for improving the error message by adding similar
> checks to all the places that adopt drives.
> 
> We could make machines declare what they support.  Better, I think.  So
> if you have a burning desire to leave your mark in git-blame for every
> machine...

Could you please remind me the situation about if=scsi across all machines? If
it is simply legacy and it's recommended to use "-drive if=none" and "-device
...", I think saying this in the error message is enough; or if "if=scsi" is a
valid way for some machines, then maybe we can declare support limits found with
your script?

Fam

> 
> > Do we have a user documentation for this? Or, can we give a hint how to figure
> > that out?
> 
> Try it out:
> 
>     for i in ide scsi floppy pflash mtd sd virtio xen
>     do
>         for ((b=0; b<4; b++))
>         do
>             for ((u=0; u<10; u++))
>             do
>                 echo q | qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -drive if=$i,bus=$b,unit=$u,file=tmp.qcow2 >/dev/null
>             done
>         done
>     done
> 
> My upper bounds for bus and unit are arbitrary.

Re: [Qemu-devel] [PATCH v2 8/8] hw: Drop superfluous special checks for orphaned -drive

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Fri, 02/03 14:35, Markus Armbruster wrote:
>> > I guess the painpoint is "okay, what the heck does the machine support then?" -
>> 
>> At the place where we can reliably detect orphaned drives regardless of
>> what the machine initialization code does, we have no idea.
>> 
>> > that "3 > 2" is the good part of the old message.
>> 
>> At the places that actually adopt drives, we know.  But there are many
>> of them.  Just three check for orphans.  One of them gets it right
>> (ide/core.c), one of them gets it wrong (mips_jazz.c), and one of them
>> sets a problematic example (sun4m.c): if copied to a machine that lets
>> users configure additional SCSI HBAs, it would break if=scsi for those.
>> Dampens my enthusiasm for improving the error message by adding similar
>> checks to all the places that adopt drives.
>> 
>> We could make machines declare what they support.  Better, I think.  So
>> if you have a burning desire to leave your mark in git-blame for every
>> machine...
>
> Could you please remind me the situation about if=scsi across all machines? If
> it is simply legacy and it's recommended to use "-drive if=none" and "-device
> ...", I think saying this in the error message is enough; or if "if=scsi" is a
> valid way for some machines, then maybe we can declare support limits found with
> your script?

Short story: once my followup series "[PATCH 0/3] hw: Deprecate unwanted
use -drive if=scsi" is in, if=scsi is deprecated except for machine
types magnum pica61 LX SPARCClassic SPARCbook SS-10 SS-20 SS-4 SS-5
SS-600MP Voyager realview-eb realview-eb-mpcore versatileab versatilepb
pseries-*.  Documenting what they support would be feasible.

Long story: it's... complicated.

Ideally, -drive if=T,... (T!=none) would be sugar for a -drive / -device
combo.  It actually is for if=virtio: drive_new() desugars it.

All the others are implemented by machine-specific ad hoc code,
i.e. machine initialization creates devices however it sees fit.

For modern devices, what's done is often pretty much equivalent to a
-device.  For instance, a PC machine type's action for
if=ide,media=disk,bus=B,unit=U is pretty much equivalent to -device
ide-hd,bus=ide.B,unit=U.  See also docs/qdev-device-use.txt.

Desugaring details depend on the machine, and are code, not data.  Data
would be easier to find and document.

For non-qdevified devices, we're even farther from a clean desugaring,
simply because these are not available with -device, but can only be
created by code.  For instance, some machine types implement if=sd with
the non-qdevified hw/sd/sd.c; grep for sd_init().

So, we're not yet ready for deprecating if!=none.

But you asked for if=scsi specifically.

Only a few machines have a SCSI HBA onboard, and therefore adopt if=scsi
drives: magnum pica61 LX SPARCClassic SPARCbook SS-10 SS-20 SS-4 SS-5
SS-600MP Voyager.  We're not deprecating this usage.

A few more create SCSI HBAs on demand: realview-eb realview-eb-mpcore
versatileab versatilepb pseries-* pc*.  My followup series deprecates it
for pc* only.

Finally, there's some special SCSI magic: most SCSI HBAs adopt if=scsi
drives independend of machine initialization.  My followup series
deprecates that.