[Qemu-devel] [PATCH for-2.5 0/2] sd: Avoid ABI mistake, add a FIXME

[Qemu-devel] [PATCH for-2.5 0/2] sd: Avoid ABI mistake, add a FIXME

[Markus Armbruster]

Markus Armbruster (2):
  sdhci: Sanitize "sdhci-pci" properties for future qomification
  sd: Mark brittle abuse of blk_attach_dev() FIXME

 hw/sd/sd.c            | 1 +
 hw/sd/sdhci.c         | 9 +++++++--
 include/hw/sd/sdhci.h | 2 +-
 3 files changed, 9 insertions(+), 3 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 1/2] sdhci: Sanitize "sdhci-pci" properties for future qomification

[Markus Armbruster]

We currently fuse controller and card into a single device model, but
we intend qomify things properly and separate the two.  The properties
that really belong to the card would then have to somehow pass-through
to the card's properties.  To avoid that complication, either mark
them experimental or drop them.

Properties "capareg", "maxcurr" and the usual PCI device properties
belong to the controller.  Property "drive" belongs to the card;
rename it to "x-drive".  Properties "logical_block_size",
"physical_block_size", "min_io_size", "opt_io_size",
"discard_granularity" belong to the card, but have no effect; drop
them.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/sd/sdhci.c         | 9 +++++++--
 include/hw/sd/sdhci.h | 2 +-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index d70d1a6..8612760 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1221,7 +1221,12 @@ const VMStateDescription sdhci_vmstate = {
 /* Capabilities registers provide information on supported features of this
  * specific host controller implementation */
 static Property sdhci_pci_properties[] = {
-    DEFINE_BLOCK_PROPERTIES(SDHCIState, conf),
+    /*
+     * We currently fuse controller and card into a single device
+     * model, but we intend to separate them.  For that purpose, the
+     * properties that belong to the card are marked as experimental.
+     */
+    DEFINE_PROP_DRIVE("x-drive", SDHCIState, blk),
     DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
             SDHC_CAPAB_REG_DEFAULT),
     DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
@@ -1233,7 +1238,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, s->conf.blk);
+    sdhci_initfn(s, s->blk);
     s->buf_maxsz = sdhci_get_fifolen(s);
     s->fifo_buffer = g_malloc0(s->buf_maxsz);
     s->irq = pci_allocate_irq(dev);
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 9b3031f..e78d938 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -39,7 +39,7 @@ typedef struct SDHCIState {
     };
     SDState *card;
     MemoryRegion iomem;
-    BlockConf conf;
+    BlockBackend *blk;
 
     QEMUTimer *insert_timer;       /* timer for 'changing' sd card. */
     QEMUTimer *transfer_timer;
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 2/2] sd: Mark brittle abuse of blk_attach_dev() FIXME

[Markus Armbruster]

blk_attach_dev() fails here only when we're working for device
"sdhci-pci" (which already attached the backend), and then we don't
want to attach a second time.  If we ever create another failure mode,
we're setting up ourselves to using the same backend from multiple
frontends, which is likely to end in tears.  Can't clean this up this
close to the release, so mark it FIXME.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index ce4d44b..1a9935c 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 dangerously brittle */
         blk_attach_dev(sd->blk, sd);
         blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
     }
-- 
2.4.3

Re: [Qemu-devel] [PATCH for-2.5 0/2] sd: Avoid ABI mistake, add a FIXME

[Peter Maydell]

On 7 December 2015 at 15:55, Markus Armbruster <armbru@redhat.com> wrote:
> Markus Armbruster (2):
>   sdhci: Sanitize "sdhci-pci" properties for future qomification
>   sd: Mark brittle abuse of blk_attach_dev() FIXME
>
>  hw/sd/sd.c            | 1 +
>  hw/sd/sdhci.c         | 9 +++++++--
>  include/hw/sd/sdhci.h | 2 +-
>  3 files changed, 9 insertions(+), 3 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

These patches look good to me and like the best compromise we
have for 2.5.

thanks
-- PMM