[Qemu-devel] [PATCH for-2.3 0/3] Contain drive_get() misuse

[Qemu-devel] [PATCH for-2.3 0/3] Contain drive_get() misuse

[Markus Armbruster]

Drives defined with if!=none are for board initialization to wire up.
Board code calls drive_get() or similar to find them, and creates
devices with their qdev drive properties set accordingly.

Except a few devices go on a fishing expedition for a suitable backend
instead of exposing a drive property for board code to set: they call
driver_get() or drive_get_next() in their realize() or init() method.
Wrong.

We can't fix this in time for the release, so do the next best thing:
contain the mistakes as far as possible so they don't become ABI:

* Mark them all with suitable FIXME comments [PATCH 1]

* sdhci-pci is new, set cannot_instantiate_with_device_add_yet to make
  it unavailable with -device [PATCH 2]

* A few more aren't currently available with -device, set
  cannot_instantiate_with_device_add_yet to ensure they stay
  unavailable [PATCH 3]

* Left alone: m25p80-generic and its derivatives, ssi-sd, pc87312

Markus Armbruster (3):
  hw: Mark devices misusing drive_get(), drive_get_next() FIXME
  sdhci: Make device "sdhci-pci" unavailable with -device
  sysbus: Contain drive_get_next() misuse

 hw/arm/spitz.c            | 3 +++
 hw/block/m25p80.c         | 1 +
 hw/isa/pc87312.c          | 2 ++
 hw/sd/milkymist-memcard.c | 3 +++
 hw/sd/pl181.c             | 3 +++
 hw/sd/sdhci.c             | 6 ++++++
 hw/sd/ssi-sd.c            | 1 +
 7 files changed, 19 insertions(+)

-- 
1.9.3

[Qemu-devel] [PATCH for-2.3 1/3] hw: Mark devices misusing drive_get(), drive_get_next() FIXME

[Markus Armbruster]

Drives defined with if!=none are for board initialization to wire up.
Board code calls drive_get() or similar to find them, and creates
devices with their qdev drive properties set accordingly.

Except a few devices go on a fishing expedition for a suitable backend
instead of exposing a drive property for board code to set: they call
driver_get() or drive_get_next() in their realize() or init() method.
Wrong.  Mark them with suitable FIXME comments.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/spitz.c            | 1 +
 hw/block/m25p80.c         | 1 +
 hw/isa/pc87312.c          | 2 ++
 hw/sd/milkymist-memcard.c | 1 +
 hw/sd/pl181.c             | 1 +
 hw/sd/sdhci.c             | 1 +
 hw/sd/ssi-sd.c            | 1 +
 7 files changed, 8 insertions(+)

diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index a16831c..da02932 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -168,6 +168,7 @@ static int sl_nand_init(SysBusDevice *dev)
     DriveInfo *nand;
 
     s->ctl = 0;
+    /* FIXME use a qdev drive property instead of drive_get() */
     nand = drive_get(IF_MTD, 0, 0);
     s->nand = nand_init(nand ? blk_by_legacy_dinfo(nand) : NULL,
                         s->manf_id, s->chip_id);
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index ff1106b..afe243b 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -623,6 +623,7 @@ static int m25p80_init(SSISlave *ss)
     s->dirty_page = -1;
     s->storage = blk_blockalign(s->blk, s->size);
 
+    /* FIXME use a qdev drive property instead of drive_get_next() */
     dinfo = drive_get_next(IF_MTD);
 
     if (dinfo) {
diff --git a/hw/isa/pc87312.c b/hw/isa/pc87312.c
index 40a1106..2849e8d 100644
--- a/hw/isa/pc87312.c
+++ b/hw/isa/pc87312.c
@@ -319,11 +319,13 @@ static void pc87312_realize(DeviceState *dev, Error **errp)
         d = DEVICE(isa);
         qdev_prop_set_uint32(d, "iobase", get_fdc_iobase(s));
         qdev_prop_set_uint32(d, "irq", 6);
+        /* FIXME use a qdev drive property instead of drive_get() */
         drive = drive_get(IF_FLOPPY, 0, 0);
         if (drive != NULL) {
             qdev_prop_set_drive_nofail(d, "driveA",
                                        blk_by_legacy_dinfo(drive));
         }
+        /* FIXME use a qdev drive property instead of drive_get() */
         drive = drive_get(IF_FLOPPY, 0, 1);
         if (drive != NULL) {
             qdev_prop_set_drive_nofail(d, "driveB",
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index 9661eaf..0cc53d9 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -255,6 +255,7 @@ static int milkymist_memcard_init(SysBusDevice *dev)
     DriveInfo *dinfo;
     BlockBackend *blk;
 
+    /* FIXME use a qdev drive property instead of drive_get_next() */
     dinfo = drive_get_next(IF_SD);
     blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
     s->card = sd_init(blk, false);
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index e704b6e..bf37da6 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -490,6 +490,7 @@ static int pl181_init(SysBusDevice *sbd)
     sysbus_init_irq(sbd, &s->irq[0]);
     sysbus_init_irq(sbd, &s->irq[1]);
     qdev_init_gpio_out(dev, s->cardstatus, 2);
+    /* FIXME use a qdev drive property instead of drive_get_next() */
     dinfo = drive_get_next(IF_SD);
     s->card = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, false);
     if (s->card == NULL) {
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 27b914a..f056c52 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1146,6 +1146,7 @@ static void sdhci_initfn(SDHCIState *s)
 {
     DriveInfo *di;
 
+    /* FIXME use a qdev drive property instead of drive_get_next() */
     di = drive_get_next(IF_SD);
     s->card = sd_init(di ? blk_by_legacy_dinfo(di) : NULL, false);
     if (s->card == NULL) {
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index a71fbca..e4b2d4f 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -255,6 +255,7 @@ static int ssi_sd_init(SSISlave *d)
     DriveInfo *dinfo;
 
     s->mode = SSI_SD_CMD;
+    /* FIXME use a qdev drive property instead of drive_get_next() */
     dinfo = drive_get_next(IF_SD);
     s->sd = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, true);
     if (s->sd == NULL) {
-- 
1.9.3

[Qemu-devel] [PATCH for-2.3 2/3] sdhci: Make device "sdhci-pci" unavailable with -device

[Markus Armbruster]

Due to misuse of drive_get_next(), -device sdhci can connect to a
-drive if=sd,...  If you can't see what's wrong here, check out the
FIXME in sdhci_initfn() and the commit that added it.

We can't fix this in time for the release, but since the device is new
in 2.3, we can disable it before this mistake becomes ABI: set
cannot_instantiate_with_device_add_yet.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/sd/sdhci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index f056c52..ab13505 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1254,6 +1254,8 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->vmsd = &sdhci_vmstate;
     dc->props = sdhci_properties;
+    /* Reason: realize() method uses drive_get_next() */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo sdhci_pci_info = {
-- 
1.9.3

Re: [Qemu-devel] [PATCH for-2.3 2/3] sdhci: Make device "sdhci-pci" unavailable with -device

[Peter Maydell]

On 23 March 2015 at 19:09, Markus Armbruster <armbru@redhat.com> wrote:
> Due to misuse of drive_get_next(), -device sdhci can connect to a
> -drive if=sd,...  If you can't see what's wrong here, check out the
> FIXME in sdhci_initfn() and the commit that added it.
>
> We can't fix this in time for the release, but since the device is new
> in 2.3, we can disable it before this mistake becomes ABI: set
> cannot_instantiate_with_device_add_yet.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/sd/sdhci.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index f056c52..ab13505 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1254,6 +1254,8 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>      dc->vmsd = &sdhci_vmstate;
>      dc->props = sdhci_properties;
> +    /* Reason: realize() method uses drive_get_next() */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }

This is basically saying "this device won't work in this
release", right?

-- PMM

Re: [Qemu-devel] [PATCH for-2.3 2/3] sdhci: Make device "sdhci-pci" unavailable with -device

[Markus Armbruster]

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

> On 23 March 2015 at 19:09, Markus Armbruster <armbru@redhat.com> wrote:
>> Due to misuse of drive_get_next(), -device sdhci can connect to a
>> -drive if=sd,...  If you can't see what's wrong here, check out the
>> FIXME in sdhci_initfn() and the commit that added it.
>>
>> We can't fix this in time for the release, but since the device is new
>> in 2.3, we can disable it before this mistake becomes ABI: set
>> cannot_instantiate_with_device_add_yet.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/sd/sdhci.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index f056c52..ab13505 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -1254,6 +1254,8 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
>>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>      dc->vmsd = &sdhci_vmstate;
>>      dc->props = sdhci_properties;
>> +    /* Reason: realize() method uses drive_get_next() */
>> +    dc->cannot_instantiate_with_device_add_yet = true;
>>  }
>
> This is basically saying "this device won't work in this
> release", right?

Yes, because with this patch, you can use it only from code, and code
isn't using it right now.

[Qemu-devel] [PATCH for-2.3 3/3] sysbus: Contain drive_get_next() misuse

[Markus Armbruster]

A number of sysbus devices abuse drive_get_next(): sl-nand,
milkymist-memcard, pl181, generic-sdhci.  If you can't see what's
wrong with their use of drive_get_next(), check out the FIXMEs in
their init() methods and the commit that added them.

We're in luck, though: only machines ppce500 and pseries-* support
-device with sysbus devices, and none of the devices above is
available with these machines.

Set cannot_instantiate_with_device_add_yet to preserve our luck.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/arm/spitz.c            | 2 ++
 hw/sd/milkymist-memcard.c | 2 ++
 hw/sd/pl181.c             | 2 ++
 hw/sd/sdhci.c             | 3 +++
 4 files changed, 9 insertions(+)

diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index da02932..5bf032a 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -1036,6 +1036,8 @@ static void sl_nand_class_init(ObjectClass *klass, void *data)
     k->init = sl_nand_init;
     dc->vmsd = &vmstate_sl_nand_info;
     dc->props = sl_nand_properties;
+    /* Reason: init() method uses drive_get() */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo sl_nand_info = {
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index 0cc53d9..2209ef1 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -297,6 +297,8 @@ static void milkymist_memcard_class_init(ObjectClass *klass, void *data)
     k->init = milkymist_memcard_init;
     dc->reset = milkymist_memcard_reset;
     dc->vmsd = &vmstate_milkymist_memcard;
+    /* Reason: init() method uses drive_get_next() */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo milkymist_memcard_info = {
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index bf37da6..11fcd47 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -508,6 +508,8 @@ static void pl181_class_init(ObjectClass *klass, void *data)
     sdc->init = pl181_init;
     k->vmsd = &vmstate_pl181;
     k->reset = pl181_reset;
+    /* Reason: init() method uses drive_get_next() */
+    k->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo pl181_info = {
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index ab13505..e4c70b5 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1297,12 +1297,15 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &sdhci_vmstate;
     dc->props = sdhci_properties;
     dc->realize = sdhci_sysbus_realize;
+    /* Reason: instance_init() method uses drive_get_next() */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo sdhci_sysbus_info = {
     .name = TYPE_SYSBUS_SDHCI,
     .parent = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(SDHCIState),
+    /* FIXME SysBusDeviceClass init() instead of .instance_init */
     .instance_init = sdhci_sysbus_init,
     .instance_finalize = sdhci_sysbus_finalize,
     .class_init = sdhci_sysbus_class_init,
-- 
1.9.3

Re: [Qemu-devel] [PATCH for-2.3 0/3] Contain drive_get() misuse

[Paolo Bonzini]

On 23/03/2015 20:09, Markus Armbruster wrote:
> Drives defined with if!=none are for board initialization to wire up.
> Board code calls drive_get() or similar to find them, and creates
> devices with their qdev drive properties set accordingly.
> 
> Except a few devices go on a fishing expedition for a suitable backend
> instead of exposing a drive property for board code to set: they call
> driver_get() or drive_get_next() in their realize() or init() method.
> Wrong.
> 
> We can't fix this in time for the release, so do the next best thing:
> contain the mistakes as far as possible so they don't become ABI:
> 
> * Mark them all with suitable FIXME comments [PATCH 1]
> 
> * sdhci-pci is new, set cannot_instantiate_with_device_add_yet to make
>   it unavailable with -device [PATCH 2]
> 
> * A few more aren't currently available with -device, set
>   cannot_instantiate_with_device_add_yet to ensure they stay
>   unavailable [PATCH 3]
> 
> * Left alone: m25p80-generic and its derivatives, ssi-sd, pc87312

Maybe worth documenting as future incompatible changes?  These machines
are not versioned, so it's not the end of the world to make things saner
if somebody comes and qdevifies the SD card.

> Markus Armbruster (3):
>   hw: Mark devices misusing drive_get(), drive_get_next() FIXME
>   sdhci: Make device "sdhci-pci" unavailable with -device
>   sysbus: Contain drive_get_next() misuse
> 
>  hw/arm/spitz.c            | 3 +++
>  hw/block/m25p80.c         | 1 +
>  hw/isa/pc87312.c          | 2 ++
>  hw/sd/milkymist-memcard.c | 3 +++
>  hw/sd/pl181.c             | 3 +++
>  hw/sd/sdhci.c             | 6 ++++++
>  hw/sd/ssi-sd.c            | 1 +
>  7 files changed, 19 insertions(+)
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH for-2.3 0/3] Contain drive_get() misuse

[Markus Armbruster]

Adding Andreas because he's the odd fixer for pc87312.

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 23/03/2015 20:09, Markus Armbruster wrote:
>> Drives defined with if!=none are for board initialization to wire up.
>> Board code calls drive_get() or similar to find them, and creates
>> devices with their qdev drive properties set accordingly.
>> 
>> Except a few devices go on a fishing expedition for a suitable backend
>> instead of exposing a drive property for board code to set: they call
>> driver_get() or drive_get_next() in their realize() or init() method.
>> Wrong.
>> 
>> We can't fix this in time for the release, so do the next best thing:
>> contain the mistakes as far as possible so they don't become ABI:
>> 
>> * Mark them all with suitable FIXME comments [PATCH 1]
>> 
>> * sdhci-pci is new, set cannot_instantiate_with_device_add_yet to make
>>   it unavailable with -device [PATCH 2]
>> 
>> * A few more aren't currently available with -device, set
>>   cannot_instantiate_with_device_add_yet to ensure they stay
>>   unavailable [PATCH 3]
>> 
>> * Left alone: m25p80-generic and its derivatives, ssi-sd, pc87312
>
> Maybe worth documenting as future incompatible changes?  These machines
> are not versioned, so it's not the end of the world to make things saner
> if somebody comes and qdevifies the SD card.

Two questions: what exactly is going to change, and where do we want to
document it?

On the former:

Use of -drive if=floppy with onboard pc87312 (machine "prep") shouldn't
be affected.  Likewise for connecting onboard m25p80-generic derivatives
with if=mtd drives, or onboard ssi-sd with if=sd.

Weird usage similar to the one you caught in time for sdhci-pci (--drive
if=sd --device sdhci-pci) would break.  It's possible when the target
has the device, and the machine type has a suitable bus.

* pc87312

  Depends on CONFIG_PC87312, set in {ppc,ppc64}-softmmu.mak.

  Requires an ISA bus.  I believe "prep" is the only machine providing
  one.  Adding a second one with --device seems unlikely to work (I
  didn't try).  If that's correct, there's nothing to document.

  If Andreas agrees, I can set cannot_instantiate_with_device_add_yet
  for pc87312 now.

* ssi-sd

  Depends on CONFIG_SSI_SD, set in arm-softmmu.mak.

  Requires an SSI bus.  At least the following targets provide one:
  collie, highbank, lm3s6965evb, lm3s811evb, midway, xilinx-zynq-a9.
  Can't exclude use of --device ssi-sd.

  I guess we want to document that --device ssi-sd will at some point
  cease to auto-connect to the next available if=sd drive and require
  the usual drive property instead.  Okay?

* m25p80-generic
 
  Depends on CONFIG_SSI_M25P80, set in
  {arm,microblaze,microblazeel}-softmmu.mak.

  Requires an SSI bus.  At least the ARM targets above provide one.
  Can't exclude use with --device.

  Document just like ssi-sd.

[...]
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH for-2.3 0/3] Contain drive_get() misuse

[Paolo Bonzini]

On 24/03/2015 13:48, Markus Armbruster wrote:
> Use of -drive if=floppy with onboard pc87312 (machine "prep") shouldn't
> be affected.  Likewise for connecting onboard m25p80-generic derivatives
> with if=mtd drives, or onboard ssi-sd with if=sd.

Exactly.

> Weird usage similar to the one you caught in time for sdhci-pci (--drive
> if=sd --device sdhci-pci) would break.  It's possible when the target
> has the device, and the machine type has a suitable bus.
> 
> * pc87312
> 
>   Depends on CONFIG_PC87312, set in {ppc,ppc64}-softmmu.mak.
> 
>   Requires an ISA bus.  I believe "prep" is the only machine providing
>   one.

You can add one with -device i82378.  Actually used in
tests/endianness-test.c, hence I guess supported.

>   If Andreas agrees, I can set cannot_instantiate_with_device_add_yet
>   for pc87312 now.

Could do that, could also decide that "-device i82378 -device pc87312"
is a valid way to add all the legacy crap to a PCI machine.  In which
case supporting "-drive if=floppy" is a weird feature but it's also hard
to call it a bug.

The difference with other devices is that you can only add it once.
It's a big difference.

> * ssi-sd
> 
>   I guess we want to document that --device ssi-sd will at some point
>   cease to auto-connect to the next available if=sd drive and require
>   the usual drive property instead.  Okay?
> 
> * m25p80-generic
>  
>   Document just like ssi-sd.

Ack for these two.  Boards can still hook -drive if={sd,mtd} to them,
but (hypothetical) users would have to switch to -drive if=none.

Paolo

> [...]
>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Thanks!
> 

Re: [Qemu-devel] [PATCH for-2.3 0/3] Contain drive_get() misuse

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 24/03/2015 13:48, Markus Armbruster wrote:
>> Use of -drive if=floppy with onboard pc87312 (machine "prep") shouldn't
>> be affected.  Likewise for connecting onboard m25p80-generic derivatives
>> with if=mtd drives, or onboard ssi-sd with if=sd.
>
> Exactly.
>
>> Weird usage similar to the one you caught in time for sdhci-pci (--drive
>> if=sd --device sdhci-pci) would break.  It's possible when the target
>> has the device, and the machine type has a suitable bus.
>> 
>> * pc87312
>> 
>>   Depends on CONFIG_PC87312, set in {ppc,ppc64}-softmmu.mak.
>> 
>>   Requires an ISA bus.  I believe "prep" is the only machine providing
>>   one.
>
> You can add one with -device i82378.  Actually used in
> tests/endianness-test.c, hence I guess supported.

Point taken.

>>   If Andreas agrees, I can set cannot_instantiate_with_device_add_yet
>>   for pc87312 now.
>
> Could do that, could also decide that "-device i82378 -device pc87312"
> is a valid way to add all the legacy crap to a PCI machine.  In which
> case supporting "-drive if=floppy" is a weird feature but it's also hard
> to call it a bug.
>
> The difference with other devices is that you can only add it once.
> It's a big difference.

$ qemu-system-ppc64 -M bamboo -S -device i82378 -device pc87312 -device pc87312
qemu-system-ppc64: -device pc87312: Property 'isa-parallel.chardev' can't take value 'parallel0', it's in use
Aborted (core dumped)

drive_get() & friends were designed for board code, and never meant for
realize().  They got used it in a few, but that were honest mistakes.
Attempting to promote mistakes into a design of sorts just to avoid the
consequences of reverting the mistakes is not a bright idea.  Especially
not when it complicates our conventions for device models.  People are
confused enough about them as it is, without alternate ways to do stuff
that only work in special circumstances.

Let's not go there.

I really can't see why we should tie ourselves into knots to avoid an
incompatible change here.  I seriously doubt anyone will notice if drop
the mistaken automatic backend pickup so that "-drive if=floppy -device
i82378 -device pc87312" no longer picks up the floppy.

>> * ssi-sd
>> 
>>   I guess we want to document that --device ssi-sd will at some point
>>   cease to auto-connect to the next available if=sd drive and require
>>   the usual drive property instead.  Okay?
>> 
>> * m25p80-generic
>>  
>>   Document just like ssi-sd.
>
> Ack for these two.  Boards can still hook -drive if={sd,mtd} to them,
> but (hypothetical) users would have to switch to -drive if=none.

Want me to add something to
http://wiki.qemu.org/ChangeLog/2.3#Future_incompatible_changes
?

[...]

Re: [Qemu-devel] [PATCH for-2.3 0/3] Contain drive_get() misuse

[Paolo Bonzini]

> I really can't see why we should tie ourselves into knots to avoid an
> incompatible change here.  I seriously doubt anyone will notice if drop
> the mistaken automatic backend pickup so that "-drive if=floppy -device
> i82378 -device pc87312" no longer picks up the floppy.

Ok, it wasn't too hard to convince me.  Let's document the three of them
together.  I'll do it tomorrow if you haven't beaten me to it.

Paolo

> >> * ssi-sd
> >> 
> >>   I guess we want to document that --device ssi-sd will at some point
> >>   cease to auto-connect to the next available if=sd drive and require
> >>   the usual drive property instead.  Okay?
> >> 
> >> * m25p80-generic
> >>  
> >>   Document just like ssi-sd.
> >
> > Ack for these two.  Boards can still hook -drive if={sd,mtd} to them,
> > but (hypothetical) users would have to switch to -drive if=none.
> 
> Want me to add something to
> http://wiki.qemu.org/ChangeLog/2.3#Future_incompatible_changes
> ?
> 
> [...]
> 

Re: [Qemu-devel] [PATCH for-2.3 0/3] Contain drive_get() misuse

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

>> I really can't see why we should tie ourselves into knots to avoid an
>> incompatible change here.  I seriously doubt anyone will notice if drop
>> the mistaken automatic backend pickup so that "-drive if=floppy -device
>> i82378 -device pc87312" no longer picks up the floppy.
>
> Ok, it wasn't too hard to convince me.  Let's document the three of them
> together.  I'll do it tomorrow if you haven't beaten me to it.

I started to do it, then stopped to look for misuse of other kinds of
backends.  Here are some:

* serial_hds[] via qemu_char_get_next_serial()

  device                bus     depends on        flawed function
  "cadence_uart"        sysbus  CONFIG_CADENCE    cadence_uart_realize()
  "digic-uart"          sysbus  CONFIG_DIGIC      digic_uart_realize()
  "etraxfs,serial"      sysbus  CONFIG_ETRAXFS    etraxfs_ser_init()
  "lm32-juart"          sysbus  CONFIG_LM32       m32_juart_init()
  "lm32-uart"           sysbus  CONFIG_LM32       lm32_uart_init()
  "milkymist-uart"      sysbus  CONFIG_MILKYMIST  milkymist_uart_realize()
  "pl011"               sysbus  CONFIG_PL011      pl011_realize()
  "stm32f2xx-usart"     sysbus  CONFIG_STM32F2XX_USART
                                                  stm32f2xx_usart_init()
  "xlnx.xps-uartlite"   sysbus  CONFIG_XILINX     xilinx_uartlite_realize()

* serial_hds[] directly

  device                bus     depends on        flawed function
  "allwinner-a10"       none    CONFIG_ALLWINNER_A10
                                                  aw_a10_realize()
  "pc87312"             isa     CONFIG_PC87312    pc87312_realize()

* parallel_hds[] directly

  device                bus     depends on        flawed function
  "pc87312"             isa     CONFIG_PC87312    pc87312_realize()

I'll prepare a patch tomorrow that marks them all FIXME, just like the
drive_get() misuses.

What else needs to be done?

* "pc87312"

   Old news, just more incompatible change to document.

* "allwinner-a10"

   "-nodefaults -serial null -device allwinner-a10" doesn't explode,
   which means I can't exclude the possibility that this might actually
   do something useful for someone.  I'd say we treat it just like
   "pc87312": leave alone now, document incompatible change.

* Of the sysbus devices only "xlnx.xps-uartlite" seems to be available
  with the machines that support -device for sysbus devices (ppce500 and
  pseries-*).  When I try to -device it there, I get "Device
  xlnx.xps-uartlite is not supported by this machine yet."  I'll ask
  Alex to confirm.  I'll prepare a patch that sets
  cannot_instantiate_with_device_add_yet for the unavailable ones to
  keep them unavailable, just like for the drive_get() abusers.

How could this kind of mistake could look like in NIC device models?
"allwinner-a10"'s instance_init aw_a10_init() provides a clue: it messes
with qemu_check_nic_model():

    if (nd_table[0].used) {
        qemu_check_nic_model(&nd_table[0], TYPE_AW_EMAC);
        qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]);
    }

No other device does that.  Without -nodefaults, this fails:

    -device allwinner-a10: Unsupported NIC model: xgmac

I haven't checked all uses of nd_table[].

Re: [Qemu-devel] [PATCH for-2.3 0/3] Contain drive_get() misuse

[Paolo Bonzini]

On 24/03/2015 21:03, Markus Armbruster wrote:
> * Of the sysbus devices only "xlnx.xps-uartlite" seems to be available
>   with the machines that support -device for sysbus devices (ppce500 and
>   pseries-*).  When I try to -device it there, I get "Device
>   xlnx.xps-uartlite is not supported by this machine yet."  I'll ask
>   Alex to confirm.  I'll prepare a patch that sets
>   cannot_instantiate_with_device_add_yet for the unavailable ones to
>   keep them unavailable, just like for the drive_get() abusers.

Right, only specific sysbus devices can be added with -device, because
there is board-specific code to describe the device in the device tree.

Paolo

Re: [Qemu-devel] [PATCH for-2.3 0/3] Contain drive_get() misuse

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

[...]
> * "allwinner-a10"
>
>    "-nodefaults -serial null -device allwinner-a10" doesn't explode,
>    which means I can't exclude the possibility that this might actually
>    do something useful for someone.  I'd say we treat it just like
>    "pc87312": leave alone now, document incompatible change.
[...]
> How could this kind of mistake could look like in NIC device models?
> "allwinner-a10"'s instance_init aw_a10_init() provides a clue: it messes
> with qemu_check_nic_model():
>
>     if (nd_table[0].used) {
>         qemu_check_nic_model(&nd_table[0], TYPE_AW_EMAC);
>         qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]);
>     }
>
> No other device does that.  Without -nodefaults, this fails:
>
>     -device allwinner-a10: Unsupported NIC model: xgmac

Even better:

    $  qemu-system-arm -S -M highbank -monitor stdio
    QEMU 2.2.90 monitor - type 'help' for more information
    (qemu) device_add allwinner-a10
    Unsupported NIC model: xgmac
    [Exit 1 ]

exit() in monitor command, big no-no.

Machine type doesn't matter.  To get past this, nd_table[0] must have
model=allwinner-emac.  With -net nic,model=allwinner-emac, it has, and
we get to the next round of failures:

* cubieboard

  Board already creates an allwinner-a10, and creating a second one with
  -device or device_add aborts.

* Any other board with an onboard NIC, e.g. highbank

  Refuses to start, as board doesn't support model=allwinner-emac:

      qemu-system-arm: Unsupported NIC model: allwinner-emac

* Any other board without an onboard NIC, e.g. virt

  Warns on startup:

      Warning: requested NIC (anonymous, model allwinner-emac) was not created (not supported by this machine?

  -device / device_add allwinner-a10 succeeds, as long as serial_hds[0]
  is set.  If you suppress it with -nodefaults, -device / device_add
  exit()s (no-no again) with

      Can't create serial device, empty char device

  If the board has an onboard serial (e.g. collie), both the onboard
  serial and the allwinner-a10 get are now connected to it.  Not going
  to work.  We normally catch this error, but bypassing qdev properties
  also bypasses the check.

Actual use of -device / device_add allwinner-a10 seems vanishingly
unlikely.  Thus, setting cannot_instantiate_with_device_add_yet is
unlikely to break anything.  Let's do it to mask the "device_add
exit()s" bug.

> I haven't checked all uses of nd_table[].

I have now, this is the only offender.

Re: [Qemu-devel] [PATCH for-2.3 0/3] Contain drive_get() misuse

[Markus Armbruster]

Superseded by
[PATCH v2 for-2.3 0/5] Contain drive, serial, parallel, net misuse