From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44388) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a4rQy-0000IC-3a for qemu-devel@nongnu.org; Fri, 04 Dec 2015 09:34:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a4rQt-00074b-Pl for qemu-devel@nongnu.org; Fri, 04 Dec 2015 09:34:04 -0500 Received: from mail-wm0-x22a.google.com ([2a00:1450:400c:c09::22a]:38191) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a4rQt-00074G-J7 for qemu-devel@nongnu.org; Fri, 04 Dec 2015 09:33:59 -0500 Received: by wmec201 with SMTP id c201so67597212wme.1 for ; Fri, 04 Dec 2015 06:33:58 -0800 (PST) Sender: Paolo Bonzini References: <1449238067-29794-1-git-send-email-armbru@redhat.com> From: Paolo Bonzini Message-ID: <5661A453.5090809@redhat.com> Date: Fri, 4 Dec 2015 15:33:55 +0100 MIME-Version: 1.0 In-Reply-To: <1449238067-29794-1-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] sdhci: Make device "sdhci-pci" unavailable with -device again List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, kevin@koconnor.net, stefanha@redhat.com 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 > --- > 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 = { >