[Qemu-devel] [PATCH] sdhci: Make device "sdhci-pci" unavailable with -device again

[Qemu-devel] [PATCH] sdhci: Make device "sdhci-pci" unavailable with -device again

[Markus Armbruster]

We made it unavailable in commit 1910913 because its use of
drive_get_next() instead of a property.  Commit 5ec911c replaced
drive_get_next() and made the device available, but the property isn't
quite right, and the code dangerously ignores blk_attach_dev()
failure.  Disable it again before the property becomes ABI, and mark
the dangerous spot FIXME.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index ce4d44b..d0be5ea 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -494,6 +494,7 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
     if (sd->blk) {
         /* Attach dev if not already attached.  (This call ignores an
          * error return code if sd->blk is already attached.) */
+        /* FIXME ignoring blk_attach_dev() failure is wrong and dangerous */
         blk_attach_dev(sd->blk, sd);
         blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
     }
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index d70d1a6..0a338ed 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1261,6 +1261,12 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->vmsd = &sdhci_vmstate;
     dc->props = sdhci_pci_properties;
+    /*
+     * Reason: realize() method uses sd_init(), which ignores
+     * blk_attach_dev() failure (potentially dangerous), and the block
+     * properties really belong to the card, not the controller.
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo sdhci_pci_info = {
-- 
2.4.3

Re: [Qemu-devel] [PATCH] sdhci: Make device "sdhci-pci" unavailable with -device again

[Paolo Bonzini]

On 04/12/2015 15:07, Markus Armbruster wrote:
> We made it unavailable in commit 1910913 because its use of
> drive_get_next() instead of a property.  Commit 5ec911c replaced
> drive_get_next() and made the device available, but the property isn't
> quite right, and the code dangerously ignores blk_attach_dev()
> failure.  Disable it again before the property becomes ABI, and mark
> the dangerous spot FIXME.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/sd/sd.c    | 1 +
>  hw/sd/sdhci.c | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index ce4d44b..d0be5ea 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -494,6 +494,7 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
>      if (sd->blk) {
>          /* Attach dev if not already attached.  (This call ignores an
>           * error return code if sd->blk is already attached.) */
> +        /* FIXME ignoring blk_attach_dev() failure is wrong and dangerous */

No, it's not (it is tricky though) because blk_attach_dev actually will
always  fail here when using the drive= property, and never when using
drive_get_next.

In the drive= case, the successful call (and also the one that will
catch possible mistakes) is from parse_drive in hw/core/qdev-properties-system.c:

$ x86_64-softmmu/qemu-system-x86_64 -drive if=none,driver=null-aio,id=foo -device virtio-blk-pci,drive=foo -device sdhci-pci,drive=foo
qemu-system-x86_64: -device sdhci-pci,drive=foo: Drive 'foo' is already in use by another device

Did you have something else in mind?

Paolo

>          blk_attach_dev(sd->blk, sd);
>          blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
>      }
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index d70d1a6..0a338ed 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1261,6 +1261,12 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data)
>      set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>      dc->vmsd = &sdhci_vmstate;
>      dc->props = sdhci_pci_properties;
> +    /*
> +     * Reason: realize() method uses sd_init(), which ignores
> +     * blk_attach_dev() failure (potentially dangerous), and the block
> +     * properties really belong to the card, not the controller.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo sdhci_pci_info = {
> 

Re: [Qemu-devel] [PATCH] sdhci: Make device "sdhci-pci" unavailable with -device again

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 04/12/2015 15:07, Markus Armbruster wrote:
>> We made it unavailable in commit 1910913 because its use of
>> drive_get_next() instead of a property.  Commit 5ec911c replaced
>> drive_get_next() and made the device available, but the property isn't
>> quite right, and the code dangerously ignores blk_attach_dev()
>> failure.  Disable it again before the property becomes ABI, and mark
>> the dangerous spot FIXME.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/sd/sd.c    | 1 +
>>  hw/sd/sdhci.c | 6 ++++++
>>  2 files changed, 7 insertions(+)
>> 
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index ce4d44b..d0be5ea 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -494,6 +494,7 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
>>      if (sd->blk) {
>>          /* Attach dev if not already attached.  (This call ignores an
>>           * error return code if sd->blk is already attached.) */
>> +        /* FIXME ignoring blk_attach_dev() failure is wrong and dangerous */
>
> No, it's not (it is tricky though) because blk_attach_dev actually will
> always  fail here when using the drive= property, and never when using
> drive_get_next.
>
> In the drive= case, the successful call (and also the one that will
> catch possible mistakes) is from parse_drive in
> hw/core/qdev-properties-system.c:
>
> $ x86_64-softmmu/qemu-system-x86_64 -drive
> if=none,driver=null-aio,id=foo -device virtio-blk-pci,drive=foo
> -device sdhci-pci,drive=foo
> qemu-system-x86_64: -device sdhci-pci,drive=foo: Drive 'foo' is
> already in use by another device
>
> Did you have something else in mind?

My comment makes two claims: "wrong" and "dangerous".

First "dangerous".  You're making a non-local argument why it's not
actually broken, and you might be right.  If you are, it's just fragile,
not broken.  We could debate whether to call it dangerous or fragile,
but I don't really care.  If you'd prefer to call it fragile, let's
update the comment and the commit message.

Now "wrong".  The qdev property belongs to the SD card (the thing we're
initializing here), not the SD controller sdhci-pci.  Unfortunately, the
SD card still hasn't been qdevified.  But if we permit tacking the
property to the controller now, we're stuck with having it there
forever.  No harm if the SD card never becomes an object in its own
right.  But if it does, it'll end up in the same unhappy place as
usb-storage, where we hackishly jump through hoops to somehow transfer
the backend from the controller to the SCSI device.  This has caused so
much trouble that we replaced the whole thing by usb-bot.  I'm not keen
on repeating the experience.

Re: [Qemu-devel] [PATCH] sdhci: Make device "sdhci-pci" unavailable with -device again

[Paolo Bonzini]

On 04/12/2015 15:59, Markus Armbruster wrote:
> My comment makes two claims: "wrong" and "dangerous".
> 
> First "dangerous".  You're making a non-local argument why it's not
> actually broken, and you might be right.  If you are, it's just fragile,
> not broken.  We could debate whether to call it dangerous or fragile,
> but I don't really care.  If you'd prefer to call it fragile, let's
> update the comment and the commit message.

Indeed I didn't claim it wasn't dangerous. :)  Still, that doesn't
justify disabling device-add.

> Now "wrong".  The qdev property belongs to the SD card (the thing we're
> initializing here), not the SD controller sdhci-pci.  Unfortunately, the
> SD card still hasn't been qdevified.  But if we permit tacking the
> property to the controller now, we're stuck with having it there
> forever.  No harm if the SD card never becomes an object in its own
> right.  But if it does, it'll end up in the same unhappy place as
> usb-storage, where we hackishly jump through hoops to somehow transfer
> the backend from the controller to the SCSI device.  This has caused so
> much trouble that we replaced the whole thing by usb-bot.  I'm not keen
> on repeating the experience.

That's a different kind of wrong, unrelated to blk_attach_dev.

I cannot say that it will never happen, and I'm not keen on having
another usb-storage.  That said, we have another example of automagic
property setting on children, which is virtio-blk-pci.  That one works
well, and I wonder whether lessons taught by virtio-blk-pci could be
applied to cleanup usb-storage.

But perfect is the enemy of good.  sdhci-pci is a useful device for
testing, and I don't see the point of disabling it with no qdevification
in sight for the SD card.

FWIW, I don't think the SD card will be qdevified because it doesn't
need a bus.  A host controller controls exactly one SD card, the SSI
bridge is also for exactly one SD card, etc.  So there's not much to
gain from qdevification of the card itself.  There would be a gain from
qdevification of the OMAP and StrongARM controllers (i.e. drive_get_next
is moved to the board, the problematic blk_attach_dev can disappear, etc.).

Paolo