From patchwork Thu Jul 30 18:04:29 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin O'Connor X-Patchwork-Id: 502240 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 1139C1402B9 for ; Fri, 31 Jul 2015 04:05:01 +1000 (AEST) Received: from localhost ([::1]:41334 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKsCQ-0004iB-Kn for incoming@patchwork.ozlabs.org; Thu, 30 Jul 2015 14:04:58 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54731) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKsC5-0004F1-15 for qemu-devel@nongnu.org; Thu, 30 Jul 2015 14:04:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKsC0-0003iq-Uw for qemu-devel@nongnu.org; Thu, 30 Jul 2015 14:04:36 -0400 Received: from mail-qg0-f54.google.com ([209.85.192.54]:36037) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKsC0-0003ie-QN for qemu-devel@nongnu.org; Thu, 30 Jul 2015 14:04:32 -0400 Received: by qgeh16 with SMTP id h16so29605888qge.3 for ; Thu, 30 Jul 2015 11:04:31 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=GHytr1WftaZ2Qu3G2u5uYPPW85Cj11tbh4KSQW+VL+k=; b=GhztLgCsrd80ITj+gBcxEQBUzbUCgqqT2KOaw9bRTlssKzmplx6k1QLPREb/l/7Zad UlI3JSpLsO67Ow4r8PZa/hIrNBMpNhzLzwr2Pm3uZ7PP/OkyOAJQf5hXAMmKiJZHnqVS YEjfC+i3WAdNU2HMaHSVm22HTgX6qeJwc/tziPWxzpn8ZHBPij/7poNW9ogduvkdXPBP s9wUND+VzCddcg/yT/ny/GWgY7zreXOxqR3eFbJMXumN+L/5qu7q8F1M8PWp6kpgLFrD Czxuc9dJku5y1rfMROAPhj72u2AuuGhzgeIj3H3Il9/OFEWqc4R7WDDpO2eWlmoKnij7 4GFw== X-Gm-Message-State: ALoCoQlJRkxRIWUu1z42baO/2UdpXrxDZJ9Riypc2S9tnj53N/Ps3/U5vr0aBrN/CGdujEuLpVZk X-Received: by 10.140.85.208 with SMTP id n74mr71466586qgd.67.1438279471621; Thu, 30 Jul 2015 11:04:31 -0700 (PDT) Received: from localhost (209-122-232-221.c3-0.avec-ubr1.nyr-avec.ny.cable.rcn.com. [209.122.232.221]) by smtp.gmail.com with ESMTPSA id f71sm843320qhe.7.2015.07.30.11.04.30 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 30 Jul 2015 11:04:30 -0700 (PDT) Date: Thu, 30 Jul 2015 14:04:29 -0400 From: Kevin O'Connor To: Stefan Hajnoczi Message-ID: <20150730180429.GA11818@morn.localdomain> References: <1438100563-14453-1-git-send-email-kevin@koconnor.net> <20150729090103.GC10617@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150729090103.GC10617@stefanha-thinkpad.redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.85.192.54 Cc: Paolo Bonzini , qemu-devel@nongnu.org, Markus Armbruster Subject: Re: [Qemu-devel] [PATCH] sdhci: Pass drive parameter to sdhci-pci via qdev property X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Wed, Jul 29, 2015 at 10:01:03AM +0100, Stefan Hajnoczi wrote: > On Tue, Jul 28, 2015 at 12:22:43PM -0400, Kevin O'Connor wrote: > > Commit 19109131 disabled the sdhci-pci support because it used > > drive_get_next(). This patch reenables sdhci-pci and changes it to > > pass the drive via a qdev property - for example: > > -device sdhci-pci,drive=drive0 -drive id=drive0,if=sd,file=myimage > > > > Signed-off-by: Kevin O'Connor > > --- > > > > This patch only changes the SDHCI PCI code - the sysbus code continues > > to use drive_get_next(). > > > > The call to blk_detach_dev() is suspicious - I added it because > > sd.c:sd_init() calls blk_attach_dev_nofail() and that causes a crash > > if the drive is already attached to the PCI device. It's not clear to > > me how this should be wired up though. > > Ugly bits: > > hw/core/qdev-properties-system.c:release_drive() will call > blk_detach_dev(*ptr, dev) and assert(blk->dev == dev) will fail. Thanks for reviewing and catching the above. > The SD card (SDState) isn't a DeviceState, it's a plain old C struct. > So it doesn't have the qdev machinery for its own drive property. > > There is no detach call paired with sd_init() attach, so that's ugly > too. > > A solution: > > Add an sd_init() flag argument that skips the attach/set_dev_ops calls. > Make sd_cardchange() externally visible and then call blk_set_dev_ops() > from sdhci.c instead. > > That way, existing users can rely on the semi-broken but convenient > sd_init() behavior. sdhci can forward the .change_media_cb() to > sd_cardchange() keep itself as the blk->dev pointer. I can do that. However, another solution seems to be to just change the blk_attach_dev_nofail() call in sd.c:sd_init() to blk_attach_dev() and ignore any errors. (Example patch below.) I'm confused by what blk_attach_dev() attempts to accomplish. The BlockBackend->dev field is only ever used as a boolean flag. (There are four users of it - blk_dev_has_removable_media, drive_check_orphaned, hmp_drive_del, pci_piix3_xen_ide_unplug - the first three use it only as a non-NULL check and the fourth only uses it to make blk_detach_dev() succeed.) To the SD code, it doesn't matter if it sets the "attached flag" or if something else has already set that flag. (Is it even important to set the flag at all?) Thoughts? -Kevin diff --git a/hw/sd/sd.c b/hw/sd/sd.c index a1ff465..29cd22c 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -493,7 +493,7 @@ SDState *sd_init(BlockBackend *blk, bool is_spi) sd->blk = blk; sd_reset(sd); if (sd->blk) { - blk_attach_dev_nofail(sd->blk, sd); + blk_attach_dev(sd->blk, sd); blk_set_dev_ops(sd->blk, &sd_block_ops, sd); } vmstate_register(NULL, -1, &sd_vmstate, sd); diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index e63367b..6e01de7 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -1142,13 +1142,9 @@ static inline unsigned int sdhci_get_fifolen(SDHCIState *s) } } -static void sdhci_initfn(SDHCIState *s) +static void sdhci_initfn(SDHCIState *s, BlockBackend *blk) { - 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); + s->card = sd_init(blk, false); if (s->card == NULL) { exit(1); } @@ -1214,7 +1210,8 @@ const VMStateDescription sdhci_vmstate = { /* Capabilities registers provide information on supported features of this * specific host controller implementation */ -static Property sdhci_properties[] = { +static Property sdhci_pci_properties[] = { + DEFINE_BLOCK_PROPERTIES(SDHCIState, conf), DEFINE_PROP_UINT32("capareg", SDHCIState, capareg, SDHC_CAPAB_REG_DEFAULT), DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0), @@ -1226,7 +1223,7 @@ static void sdhci_pci_realize(PCIDevice *dev, Error **errp) SDHCIState *s = PCI_SDHCI(dev); dev->config[PCI_CLASS_PROG] = 0x01; /* Standard Host supported DMA */ dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin A */ - sdhci_initfn(s); + sdhci_initfn(s, s->conf.blk); s->buf_maxsz = sdhci_get_fifolen(s); s->fifo_buffer = g_malloc0(s->buf_maxsz); s->irq = pci_allocate_irq(dev); @@ -1253,9 +1250,7 @@ static void sdhci_pci_class_init(ObjectClass *klass, void *data) k->class_id = PCI_CLASS_SYSTEM_SDHCI; 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; + dc->props = sdhci_pci_properties; } static const TypeInfo sdhci_pci_info = { @@ -1265,10 +1260,21 @@ static const TypeInfo sdhci_pci_info = { .class_init = sdhci_pci_class_init, }; +static Property sdhci_sysbus_properties[] = { + DEFINE_PROP_UINT32("capareg", SDHCIState, capareg, + SDHC_CAPAB_REG_DEFAULT), + DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0), + DEFINE_PROP_END_OF_LIST(), +}; + static void sdhci_sysbus_init(Object *obj) { SDHCIState *s = SYSBUS_SDHCI(obj); - sdhci_initfn(s); + DriveInfo *di; + + /* FIXME use a qdev drive property instead of drive_get_next() */ + di = drive_get_next(IF_SD); + sdhci_initfn(s, di ? blk_by_legacy_dinfo(di) : NULL); } static void sdhci_sysbus_finalize(Object *obj) @@ -1295,7 +1301,7 @@ static void sdhci_sysbus_class_init(ObjectClass *klass, void *data) DeviceClass *dc = DEVICE_CLASS(klass); dc->vmsd = &sdhci_vmstate; - dc->props = sdhci_properties; + dc->props = sdhci_sysbus_properties; dc->realize = sdhci_sysbus_realize; /* Reason: instance_init() method uses drive_get_next() */ dc->cannot_instantiate_with_device_add_yet = true; diff --git a/hw/sd/sdhci.h b/hw/sd/sdhci.h index 3352d23..e2de92d 100644 --- a/hw/sd/sdhci.h +++ b/hw/sd/sdhci.h @@ -26,6 +26,7 @@ #define SDHCI_H #include "qemu-common.h" +#include "hw/block/block.h" #include "hw/pci/pci.h" #include "hw/sysbus.h" #include "hw/sd.h" @@ -239,6 +240,7 @@ typedef struct SDHCIState { }; SDState *card; MemoryRegion iomem; + BlockConf conf; QEMUTimer *insert_timer; /* timer for 'changing' sd card. */ QEMUTimer *transfer_timer;