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 <address@hidden>
> ---
>  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 = {
>