[Qemu-devel] [PATCH 0/3] hw/sd/pxa2xx_mmci: convert to sysbus and vmstate

[Qemu-devel] [PATCH 0/3] hw/sd/pxa2xx_mmci: convert to sysbus and vmstate

[Peter Maydell]

This patchset updates the ancient pxa2xx_mmci device to something
resembling modern standards for devices. In particular it makes
it a proper sysbus device and switches to VMStateDescription structs.

The major issue I have with this is in patch 1:
I wanted the device to have a property so its users can set
the BlockBackend* it should use. Unfortunately, DEFINE_PROP_DRIVE()
is no good here, because setting a drive property results in a
call to blk_attach_dev() which attaches the BlockBackend to this
device. That then means that the call in sd_init() to attach the
BlockBackend to the SD card object aborts. I needed a way to
have a drive property which didn't mean "and this device claims
the drive", and the best I could come up with was to use a
pointer property. Suggestions for better approaches welcome.
(The other SD controller devices are either also ancient non-QOM
devices, or use drive_get_next() in the init function...)

There are clearly further cleanup opportunities for this device,
like making the sd callbacks into sysbus gpio input lines rather
than having an ad-hoc pxa2xx_mmci_handlers() function to set them,
but one thing at a time.

Peter Maydell (3):
  hw/sd/pxa2xx_mmci: convert to SysBusDevice object
  hw/sd/pxa2xx_mmci: Convert to VMStateDescription
  hw/sd/pxa2xx_mmci: Add reset function

 hw/sd/pxa2xx_mmci.c | 261 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 159 insertions(+), 102 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Peter Maydell]

Convert the pxa2xx_mmci device to be a sysbus device.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/pxa2xx_mmci.c | 96 +++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 79 insertions(+), 17 deletions(-)

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index d1fe6d5..5b676c7 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -11,16 +11,23 @@
  */
 
 #include "hw/hw.h"
+#include "hw/sysbus.h"
 #include "hw/arm/pxa.h"
 #include "hw/sd.h"
 #include "hw/qdev.h"
 
-struct PXA2xxMMCIState {
+#define TYPE_PXA2XX_MMCI "pxa2xx-mmci"
+#define PXA2XX_MMCI(obj) OBJECT_CHECK(PXA2xxMMCIState, (obj), TYPE_PXA2XX_MMCI)
+
+typedef struct PXA2xxMMCIState {
+    SysBusDevice parent_obj;
+
     MemoryRegion iomem;
     qemu_irq irq;
     qemu_irq rx_dma;
     qemu_irq tx_dma;
 
+    void *blk; /* Should be a BlockBackend* */
     SDState *card;
 
     uint32_t status;
@@ -48,7 +55,7 @@ struct PXA2xxMMCIState {
     int resp_len;
 
     int cmdreq;
-};
+} PXA2xxMMCIState;
 
 #define MMC_STRPCL	0x00	/* MMC Clock Start/Stop register */
 #define MMC_STAT	0x04	/* MMC Status register */
@@ -474,31 +481,86 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
                 BlockBackend *blk, qemu_irq irq,
                 qemu_irq rx_dma, qemu_irq tx_dma)
 {
-    PXA2xxMMCIState *s;
+    DeviceState *dev;
+    SysBusDevice *sbd;
+
+    dev = qdev_create(NULL, TYPE_PXA2XX_MMCI);
+    qdev_prop_set_ptr(dev, "drive", blk);
+    qdev_init_nofail(dev);
+    sbd = SYS_BUS_DEVICE(dev);
+    sysbus_mmio_map(sbd, 0, base);
+    sysbus_connect_irq(sbd, 0, irq);
+    sysbus_connect_irq(sbd, 1, rx_dma);
+    sysbus_connect_irq(sbd, 2, tx_dma);
+    return PXA2XX_MMCI(dev);
+}
 
-    s = (PXA2xxMMCIState *) g_malloc0(sizeof(PXA2xxMMCIState));
-    s->irq = irq;
-    s->rx_dma = rx_dma;
-    s->tx_dma = tx_dma;
+void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
+                qemu_irq coverswitch)
+{
+    sd_set_cb(s->card, readonly, coverswitch);
+}
+
+static void pxa2xx_mmci_instance_init(Object *obj)
+{
+    PXA2xxMMCIState *s = PXA2XX_MMCI(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
-    memory_region_init_io(&s->iomem, NULL, &pxa2xx_mmci_ops, s,
+    memory_region_init_io(&s->iomem, obj, &pxa2xx_mmci_ops, s,
                           "pxa2xx-mmci", 0x00100000);
-    memory_region_add_subregion(sysmem, base, &s->iomem);
+    sysbus_init_mmio(sbd, &s->iomem);
+    sysbus_init_irq(sbd, &s->irq);
+    sysbus_init_irq(sbd, &s->rx_dma);
+    sysbus_init_irq(sbd, &s->tx_dma);
+
+    register_savevm(NULL, "pxa2xx_mmci", 0, 0,
+                    pxa2xx_mmci_save, pxa2xx_mmci_load, s);
+}
+
+static void pxa2xx_mmci_realize(DeviceState *dev, Error **errp)
+{
+    PXA2xxMMCIState *s = PXA2XX_MMCI(dev);
 
     /* Instantiate the actual storage */
-    s->card = sd_init(blk, false);
+    s->card = sd_init(s->blk, false);
     if (s->card == NULL) {
-        exit(1);
+        error_setg(errp, "Could not initialize SD card with specified drive");
+        return;
     }
+}
 
-    register_savevm(NULL, "pxa2xx_mmci", 0, 0,
-                    pxa2xx_mmci_save, pxa2xx_mmci_load, s);
+static Property pxa2xx_mmci_properties[] = {
+    /* Note: pointer property 'drive' may remain NULL, thus no need
+     * for dc->cannot_instantiate_with_device_add_yet = true;
+     * Unfortunately this can't be a DEFINE_PROP_DRIVE, because
+     * setting a 'drive' property results in a call to blk_attach_dev()
+     * attaching the BlockBackend to this device; that then means that
+     * the call in sd_init() to blk_attach_dev_nofail() which tries to
+     * attach the BlockBackend to the SD card object aborts.
+     */
+    DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk),
+    DEFINE_PROP_END_OF_LIST(),
+};
 
-    return s;
+static void pxa2xx_mmci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = pxa2xx_mmci_realize;
+    dc->props = pxa2xx_mmci_properties;
 }
 
-void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
-                qemu_irq coverswitch)
+static const TypeInfo pxa2xx_mmci_info = {
+    .name = TYPE_PXA2XX_MMCI,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(PXA2xxMMCIState),
+    .class_init = pxa2xx_mmci_class_init,
+    .instance_init = pxa2xx_mmci_instance_init,
+};
+
+static void pxa2xx_mmci_register_types(void)
 {
-    sd_set_cb(s->card, readonly, coverswitch);
+    type_register_static(&pxa2xx_mmci_info);
 }
+
+type_init(pxa2xx_mmci_register_types)
-- 
1.9.1

[Qemu-devel] [PATCH 2/3] hw/sd/pxa2xx_mmci: Convert to VMStateDescription

[Peter Maydell]

Convert the pxa2xx_mmci device from manual save/load
functions to a VMStateDescription structure.

This is a migration compatibility break.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/pxa2xx_mmci.c | 149 ++++++++++++++++++++--------------------------------
 1 file changed, 57 insertions(+), 92 deletions(-)

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 5b676c7..ea42434 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -36,27 +36,72 @@ typedef struct PXA2xxMMCIState {
     uint32_t cmdat;
     uint32_t resp_tout;
     uint32_t read_tout;
-    int blklen;
-    int numblk;
+    int32_t blklen;
+    int32_t numblk;
     uint32_t intmask;
     uint32_t intreq;
-    int cmd;
+    int32_t cmd;
     uint32_t arg;
 
-    int active;
-    int bytesleft;
+    int32_t active;
+    int32_t bytesleft;
     uint8_t tx_fifo[64];
-    int tx_start;
-    int tx_len;
+    uint32_t tx_start;
+    uint32_t tx_len;
     uint8_t rx_fifo[32];
-    int rx_start;
-    int rx_len;
+    uint32_t rx_start;
+    uint32_t rx_len;
     uint16_t resp_fifo[9];
-    int resp_len;
+    uint32_t resp_len;
 
-    int cmdreq;
+    int32_t cmdreq;
 } PXA2xxMMCIState;
 
+static bool pxa2xx_mmci_vmstate_validate(void *opaque, int version_id)
+{
+    PXA2xxMMCIState *s = opaque;
+
+    return s->tx_start < sizeof(s->tx_fifo)
+        && s->rx_start < sizeof(s->rx_fifo)
+        && s->tx_len <= sizeof(s->tx_fifo)
+        && s->rx_len <= sizeof(s->rx_fifo)
+        && s->resp_len <= sizeof(s->resp_fifo);
+}
+
+
+static const VMStateDescription vmstate_pxa2xx_mmci = {
+    .name = "pxa2xx-mmci",
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(status, PXA2xxMMCIState),
+        VMSTATE_UINT32(clkrt, PXA2xxMMCIState),
+        VMSTATE_UINT32(spi, PXA2xxMMCIState),
+        VMSTATE_UINT32(cmdat, PXA2xxMMCIState),
+        VMSTATE_UINT32(resp_tout, PXA2xxMMCIState),
+        VMSTATE_UINT32(read_tout, PXA2xxMMCIState),
+        VMSTATE_INT32(blklen, PXA2xxMMCIState),
+        VMSTATE_INT32(numblk, PXA2xxMMCIState),
+        VMSTATE_UINT32(intmask, PXA2xxMMCIState),
+        VMSTATE_UINT32(intreq, PXA2xxMMCIState),
+        VMSTATE_INT32(cmd, PXA2xxMMCIState),
+        VMSTATE_UINT32(arg, PXA2xxMMCIState),
+        VMSTATE_INT32(cmdreq, PXA2xxMMCIState),
+        VMSTATE_INT32(active, PXA2xxMMCIState),
+        VMSTATE_INT32(bytesleft, PXA2xxMMCIState),
+        VMSTATE_UINT32(tx_start, PXA2xxMMCIState),
+        VMSTATE_UINT32(tx_len, PXA2xxMMCIState),
+        VMSTATE_UINT32(rx_start, PXA2xxMMCIState),
+        VMSTATE_UINT32(rx_len, PXA2xxMMCIState),
+        VMSTATE_UINT32(resp_len, PXA2xxMMCIState),
+        VMSTATE_VALIDATE("fifo size incorrect", pxa2xx_mmci_vmstate_validate),
+        VMSTATE_UINT8_ARRAY(tx_fifo, PXA2xxMMCIState, 64),
+        VMSTATE_UINT8_ARRAY(rx_fifo, PXA2xxMMCIState, 32),
+        VMSTATE_UINT16_ARRAY(resp_fifo, PXA2xxMMCIState, 9),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 #define MMC_STRPCL	0x00	/* MMC Clock Start/Stop register */
 #define MMC_STAT	0x04	/* MMC Status register */
 #define MMC_CLKRT	0x08	/* MMC Clock Rate register */
@@ -398,84 +443,6 @@ static const MemoryRegionOps pxa2xx_mmci_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static void pxa2xx_mmci_save(QEMUFile *f, void *opaque)
-{
-    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
-    int i;
-
-    qemu_put_be32s(f, &s->status);
-    qemu_put_be32s(f, &s->clkrt);
-    qemu_put_be32s(f, &s->spi);
-    qemu_put_be32s(f, &s->cmdat);
-    qemu_put_be32s(f, &s->resp_tout);
-    qemu_put_be32s(f, &s->read_tout);
-    qemu_put_be32(f, s->blklen);
-    qemu_put_be32(f, s->numblk);
-    qemu_put_be32s(f, &s->intmask);
-    qemu_put_be32s(f, &s->intreq);
-    qemu_put_be32(f, s->cmd);
-    qemu_put_be32s(f, &s->arg);
-    qemu_put_be32(f, s->cmdreq);
-    qemu_put_be32(f, s->active);
-    qemu_put_be32(f, s->bytesleft);
-
-    qemu_put_byte(f, s->tx_len);
-    for (i = 0; i < s->tx_len; i ++)
-        qemu_put_byte(f, s->tx_fifo[(s->tx_start + i) & 63]);
-
-    qemu_put_byte(f, s->rx_len);
-    for (i = 0; i < s->rx_len; i ++)
-        qemu_put_byte(f, s->rx_fifo[(s->rx_start + i) & 31]);
-
-    qemu_put_byte(f, s->resp_len);
-    for (i = s->resp_len; i < 9; i ++)
-        qemu_put_be16s(f, &s->resp_fifo[i]);
-}
-
-static int pxa2xx_mmci_load(QEMUFile *f, void *opaque, int version_id)
-{
-    PXA2xxMMCIState *s = (PXA2xxMMCIState *) opaque;
-    int i;
-
-    qemu_get_be32s(f, &s->status);
-    qemu_get_be32s(f, &s->clkrt);
-    qemu_get_be32s(f, &s->spi);
-    qemu_get_be32s(f, &s->cmdat);
-    qemu_get_be32s(f, &s->resp_tout);
-    qemu_get_be32s(f, &s->read_tout);
-    s->blklen = qemu_get_be32(f);
-    s->numblk = qemu_get_be32(f);
-    qemu_get_be32s(f, &s->intmask);
-    qemu_get_be32s(f, &s->intreq);
-    s->cmd = qemu_get_be32(f);
-    qemu_get_be32s(f, &s->arg);
-    s->cmdreq = qemu_get_be32(f);
-    s->active = qemu_get_be32(f);
-    s->bytesleft = qemu_get_be32(f);
-
-    s->tx_len = qemu_get_byte(f);
-    s->tx_start = 0;
-    if (s->tx_len >= sizeof(s->tx_fifo) || s->tx_len < 0)
-        return -EINVAL;
-    for (i = 0; i < s->tx_len; i ++)
-        s->tx_fifo[i] = qemu_get_byte(f);
-
-    s->rx_len = qemu_get_byte(f);
-    s->rx_start = 0;
-    if (s->rx_len >= sizeof(s->rx_fifo) || s->rx_len < 0)
-        return -EINVAL;
-    for (i = 0; i < s->rx_len; i ++)
-        s->rx_fifo[i] = qemu_get_byte(f);
-
-    s->resp_len = qemu_get_byte(f);
-    if (s->resp_len > 9 || s->resp_len < 0)
-        return -EINVAL;
-    for (i = s->resp_len; i < 9; i ++)
-         qemu_get_be16s(f, &s->resp_fifo[i]);
-
-    return 0;
-}
-
 PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
                 hwaddr base,
                 BlockBackend *blk, qemu_irq irq,
@@ -512,9 +479,6 @@ static void pxa2xx_mmci_instance_init(Object *obj)
     sysbus_init_irq(sbd, &s->irq);
     sysbus_init_irq(sbd, &s->rx_dma);
     sysbus_init_irq(sbd, &s->tx_dma);
-
-    register_savevm(NULL, "pxa2xx_mmci", 0, 0,
-                    pxa2xx_mmci_save, pxa2xx_mmci_load, s);
 }
 
 static void pxa2xx_mmci_realize(DeviceState *dev, Error **errp)
@@ -548,6 +512,7 @@ static void pxa2xx_mmci_class_init(ObjectClass *klass, void *data)
 
     dc->realize = pxa2xx_mmci_realize;
     dc->props = pxa2xx_mmci_properties;
+    dc->vmsd = &vmstate_pxa2xx_mmci;
 }
 
 static const TypeInfo pxa2xx_mmci_info = {
-- 
1.9.1

[Qemu-devel] [PATCH 3/3] hw/sd/pxa2xx_mmci: Add reset function

[Peter Maydell]

Add a reset function to the pxa2xx_mmci device; previously it had
no handling for system reset at all.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/pxa2xx_mmci.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index ea42434..0dec8c8 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -468,6 +468,35 @@ void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
     sd_set_cb(s->card, readonly, coverswitch);
 }
 
+static void pxa2xx_mmci_reset(DeviceState *d)
+{
+    PXA2xxMMCIState *s = PXA2XX_MMCI(d);
+
+    s->status = 0;
+    s->clkrt = 0;
+    s->spi = 0;
+    s->cmdat = 0;
+    s->resp_tout = 0;
+    s->read_tout = 0;
+    s->blklen = 0;
+    s->numblk = 0;
+    s->intmask = 0;
+    s->intreq = 0;
+    s->cmd = 0;
+    s->arg = 0;
+    s->active = 0;
+    s->bytesleft = 0;
+    s->tx_start = 0;
+    s->tx_len = 0;
+    s->rx_start = 0;
+    s->rx_len = 0;
+    s->resp_len = 0;
+    s->cmdreq = 0;
+    memset(s->tx_fifo, 0, sizeof(s->tx_fifo));
+    memset(s->rx_fifo, 0, sizeof(s->rx_fifo));
+    memset(s->resp_fifo, 0, sizeof(s->resp_fifo));
+}
+
 static void pxa2xx_mmci_instance_init(Object *obj)
 {
     PXA2xxMMCIState *s = PXA2XX_MMCI(obj);
@@ -513,6 +542,7 @@ static void pxa2xx_mmci_class_init(ObjectClass *klass, void *data)
     dc->realize = pxa2xx_mmci_realize;
     dc->props = pxa2xx_mmci_properties;
     dc->vmsd = &vmstate_pxa2xx_mmci;
+    dc->reset = pxa2xx_mmci_reset;
 }
 
 static const TypeInfo pxa2xx_mmci_info = {
-- 
1.9.1

Re: [Qemu-devel] [PATCH 0/3] hw/sd/pxa2xx_mmci: convert to sysbus and vmstate

[Peter Maydell]

On 11 August 2015 at 15:15, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset updates the ancient pxa2xx_mmci device to something
> resembling modern standards for devices. In particular it makes
> it a proper sysbus device and switches to VMStateDescription structs.
>
> The major issue I have with this is in patch 1:
> I wanted the device to have a property so its users can set
> the BlockBackend* it should use. Unfortunately, DEFINE_PROP_DRIVE()
> is no good here, because setting a drive property results in a
> call to blk_attach_dev() which attaches the BlockBackend to this
> device. That then means that the call in sd_init() to attach the
> BlockBackend to the SD card object aborts. I needed a way to
> have a drive property which didn't mean "and this device claims
> the drive", and the best I could come up with was to use a
> pointer property. Suggestions for better approaches welcome.
> (The other SD controller devices are either also ancient non-QOM
> devices, or use drive_get_next() in the init function...)

Ping!

No opinions on this (or review of the patches)?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> Convert the pxa2xx_mmci device to be a sysbus device.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/sd/pxa2xx_mmci.c | 96 +++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 79 insertions(+), 17 deletions(-)
>
> diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
> index d1fe6d5..5b676c7 100644
> --- a/hw/sd/pxa2xx_mmci.c
> +++ b/hw/sd/pxa2xx_mmci.c
> @@ -11,16 +11,23 @@
>   */
>  
>  #include "hw/hw.h"
> +#include "hw/sysbus.h"
>  #include "hw/arm/pxa.h"
>  #include "hw/sd.h"
>  #include "hw/qdev.h"
>  
> -struct PXA2xxMMCIState {
> +#define TYPE_PXA2XX_MMCI "pxa2xx-mmci"
> +#define PXA2XX_MMCI(obj) OBJECT_CHECK(PXA2xxMMCIState, (obj), TYPE_PXA2XX_MMCI)
> +
> +typedef struct PXA2xxMMCIState {
> +    SysBusDevice parent_obj;
> +
>      MemoryRegion iomem;
>      qemu_irq irq;
>      qemu_irq rx_dma;
>      qemu_irq tx_dma;
>  
> +    void *blk; /* Should be a BlockBackend* */
>      SDState *card;
>  
>      uint32_t status;
> @@ -48,7 +55,7 @@ struct PXA2xxMMCIState {
>      int resp_len;
>  
>      int cmdreq;
> -};
> +} PXA2xxMMCIState;
>  
>  #define MMC_STRPCL	0x00	/* MMC Clock Start/Stop register */
>  #define MMC_STAT	0x04	/* MMC Status register */
> @@ -474,31 +481,86 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
>                  BlockBackend *blk, qemu_irq irq,
>                  qemu_irq rx_dma, qemu_irq tx_dma)
>  {
> -    PXA2xxMMCIState *s;
> +    DeviceState *dev;
> +    SysBusDevice *sbd;
> +
> +    dev = qdev_create(NULL, TYPE_PXA2XX_MMCI);
> +    qdev_prop_set_ptr(dev, "drive", blk);
> +    qdev_init_nofail(dev);
> +    sbd = SYS_BUS_DEVICE(dev);
> +    sysbus_mmio_map(sbd, 0, base);
> +    sysbus_connect_irq(sbd, 0, irq);
> +    sysbus_connect_irq(sbd, 1, rx_dma);
> +    sysbus_connect_irq(sbd, 2, tx_dma);
> +    return PXA2XX_MMCI(dev);
> +}
>  
> -    s = (PXA2xxMMCIState *) g_malloc0(sizeof(PXA2xxMMCIState));
> -    s->irq = irq;
> -    s->rx_dma = rx_dma;
> -    s->tx_dma = tx_dma;
> +void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
> +                qemu_irq coverswitch)
> +{
> +    sd_set_cb(s->card, readonly, coverswitch);
> +}
> +
> +static void pxa2xx_mmci_instance_init(Object *obj)
> +{
> +    PXA2xxMMCIState *s = PXA2XX_MMCI(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>  
> -    memory_region_init_io(&s->iomem, NULL, &pxa2xx_mmci_ops, s,
> +    memory_region_init_io(&s->iomem, obj, &pxa2xx_mmci_ops, s,
>                            "pxa2xx-mmci", 0x00100000);
> -    memory_region_add_subregion(sysmem, base, &s->iomem);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +    sysbus_init_irq(sbd, &s->irq);
> +    sysbus_init_irq(sbd, &s->rx_dma);
> +    sysbus_init_irq(sbd, &s->tx_dma);
> +
> +    register_savevm(NULL, "pxa2xx_mmci", 0, 0,
> +                    pxa2xx_mmci_save, pxa2xx_mmci_load, s);
> +}
> +
> +static void pxa2xx_mmci_realize(DeviceState *dev, Error **errp)
> +{
> +    PXA2xxMMCIState *s = PXA2XX_MMCI(dev);
>  
>      /* Instantiate the actual storage */
> -    s->card = sd_init(blk, false);
> +    s->card = sd_init(s->blk, false);
>      if (s->card == NULL) {
> -        exit(1);
> +        error_setg(errp, "Could not initialize SD card with specified drive");
> +        return;
>      }
> +}
>  
> -    register_savevm(NULL, "pxa2xx_mmci", 0, 0,
> -                    pxa2xx_mmci_save, pxa2xx_mmci_load, s);
> +static Property pxa2xx_mmci_properties[] = {
> +    /* Note: pointer property 'drive' may remain NULL, thus no need
> +     * for dc->cannot_instantiate_with_device_add_yet = true;
> +     * Unfortunately this can't be a DEFINE_PROP_DRIVE, because
> +     * setting a 'drive' property results in a call to blk_attach_dev()
> +     * attaching the BlockBackend to this device; that then means that
> +     * the call in sd_init() to blk_attach_dev_nofail() which tries to
> +     * attach the BlockBackend to the SD card object aborts.
> +     */
> +    DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk),
> +    DEFINE_PROP_END_OF_LIST(),
> +};

As far as I can tell, this problem is an artifact of our interface to
the common sd-card code, namely sd_init().  sd_init() was made for the
pre-qdev world: it creates and completely initializes the common
SDState.

In qdev, we do this in three separate steps: create, set properties,
realize.  Split up sd_init(), and the problem should go away.

How?  Well, code common to several related qdevs exists elsewhere.  A
simple example is serial.c.  The serial qdevs embed a SerialState in
their device state, have properties pointing into that sub-state, and
call serial_realize_core() to realize that sub-state.

We also use composition, i.e. do the common part as full-blown qdev,
then wrap specialization qdevs around it.  I doubt you need to be that
fancy here.

>  
> -    return s;
> +static void pxa2xx_mmci_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = pxa2xx_mmci_realize;
> +    dc->props = pxa2xx_mmci_properties;
>  }
>  
> -void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
> -                qemu_irq coverswitch)
> +static const TypeInfo pxa2xx_mmci_info = {
> +    .name = TYPE_PXA2XX_MMCI,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(PXA2xxMMCIState),
> +    .class_init = pxa2xx_mmci_class_init,
> +    .instance_init = pxa2xx_mmci_instance_init,
> +};
> +
> +static void pxa2xx_mmci_register_types(void)
>  {
> -    sd_set_cb(s->card, readonly, coverswitch);
> +    type_register_static(&pxa2xx_mmci_info);
>  }
> +
> +type_init(pxa2xx_mmci_register_types)

Sorry for taking so long to review.

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Peter Maydell]

On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> Convert the pxa2xx_mmci device to be a sysbus device.

>> +static Property pxa2xx_mmci_properties[] = {
>> +    /* Note: pointer property 'drive' may remain NULL, thus no need
>> +     * for dc->cannot_instantiate_with_device_add_yet = true;
>> +     * Unfortunately this can't be a DEFINE_PROP_DRIVE, because
>> +     * setting a 'drive' property results in a call to blk_attach_dev()
>> +     * attaching the BlockBackend to this device; that then means that
>> +     * the call in sd_init() to blk_attach_dev_nofail() which tries to
>> +     * attach the BlockBackend to the SD card object aborts.
>> +     */
>> +    DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>
> As far as I can tell, this problem is an artifact of our interface to
> the common sd-card code, namely sd_init().  sd_init() was made for the
> pre-qdev world: it creates and completely initializes the common
> SDState.
>
> In qdev, we do this in three separate steps: create, set properties,
> realize.  Split up sd_init(), and the problem should go away.

Yes, but I don't really want to gate QOMification of mmc
controller devices on the more complicated problem of
QOMifying sd.c itself, especially since we already have several
QOMified mmc controllers...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> Convert the pxa2xx_mmci device to be a sysbus device.
>
>>> +static Property pxa2xx_mmci_properties[] = {
>>> +    /* Note: pointer property 'drive' may remain NULL, thus no need
>>> +     * for dc->cannot_instantiate_with_device_add_yet = true;
>>> +     * Unfortunately this can't be a DEFINE_PROP_DRIVE, because
>>> +     * setting a 'drive' property results in a call to blk_attach_dev()
>>> +     * attaching the BlockBackend to this device; that then means that
>>> +     * the call in sd_init() to blk_attach_dev_nofail() which tries to
>>> +     * attach the BlockBackend to the SD card object aborts.
>>> +     */
>>> +    DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk),
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>>
>> As far as I can tell, this problem is an artifact of our interface to
>> the common sd-card code, namely sd_init().  sd_init() was made for the
>> pre-qdev world: it creates and completely initializes the common
>> SDState.
>>
>> In qdev, we do this in three separate steps: create, set properties,
>> realize.  Split up sd_init(), and the problem should go away.
>
> Yes, but I don't really want to gate QOMification of mmc
> controller devices on the more complicated problem of
> QOMifying sd.c itself, especially since we already have several
> QOMified mmc controllers...

Is serial.c QOMified?  I don't think so, it's merely structured in a
QOM-friendly way: typedef SerialState, realize helper
serial_realize_core(), unrealize helper serial_exit_core().  If
SerialState had more properties, we'd also need a macro to define them.

I believe sd.c could be made like that, with not much effort.

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Peter Crosthwaite]

On Mon, Sep 7, 2015 at 9:42 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> Convert the pxa2xx_mmci device to be a sysbus device.
>
>>> +static Property pxa2xx_mmci_properties[] = {
>>> +    /* Note: pointer property 'drive' may remain NULL, thus no need
>>> +     * for dc->cannot_instantiate_with_device_add_yet = true;
>>> +     * Unfortunately this can't be a DEFINE_PROP_DRIVE, because
>>> +     * setting a 'drive' property results in a call to blk_attach_dev()
>>> +     * attaching the BlockBackend to this device; that then means that
>>> +     * the call in sd_init() to blk_attach_dev_nofail() which tries to
>>> +     * attach the BlockBackend to the SD card object aborts.
>>> +     */
>>> +    DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk),
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>>
>> As far as I can tell, this problem is an artifact of our interface to
>> the common sd-card code, namely sd_init().  sd_init() was made for the
>> pre-qdev world: it creates and completely initializes the common
>> SDState.
>>
>> In qdev, we do this in three separate steps: create, set properties,
>> realize.  Split up sd_init(), and the problem should go away.
>
> Yes, but I don't really want to gate QOMification of mmc
> controller devices on the more complicated problem of
> QOMifying sd.c itself, especially since we already have several
> QOMified mmc controllers...
>

+1. We have QOMified SDHCI already and I would assume that the SD
QOMification will be made easier if the attached controllers are
QOMified in their own right.

Regards,
Peter

> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Peter Maydell]

On 7 September 2015 at 17:57, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote:
>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>
>>>> Convert the pxa2xx_mmci device to be a sysbus device.
>>
>>>> +static Property pxa2xx_mmci_properties[] = {
>>>> +    /* Note: pointer property 'drive' may remain NULL, thus no need
>>>> +     * for dc->cannot_instantiate_with_device_add_yet = true;
>>>> +     * Unfortunately this can't be a DEFINE_PROP_DRIVE, because
>>>> +     * setting a 'drive' property results in a call to blk_attach_dev()
>>>> +     * attaching the BlockBackend to this device; that then means that
>>>> +     * the call in sd_init() to blk_attach_dev_nofail() which tries to
>>>> +     * attach the BlockBackend to the SD card object aborts.
>>>> +     */
>>>> +    DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk),
>>>> +    DEFINE_PROP_END_OF_LIST(),
>>>> +};
>>>
>>> As far as I can tell, this problem is an artifact of our interface to
>>> the common sd-card code, namely sd_init().  sd_init() was made for the
>>> pre-qdev world: it creates and completely initializes the common
>>> SDState.
>>>
>>> In qdev, we do this in three separate steps: create, set properties,
>>> realize.  Split up sd_init(), and the problem should go away.
>>
>> Yes, but I don't really want to gate QOMification of mmc
>> controller devices on the more complicated problem of
>> QOMifying sd.c itself, especially since we already have several
>> QOMified mmc controllers...
>
> Is serial.c QOMified?  I don't think so, it's merely structured in a
> QOM-friendly way: typedef SerialState, realize helper
> serial_realize_core(), unrealize helper serial_exit_core().  If
> SerialState had more properties, we'd also need a macro to define them.

It looks like since we had this conversation the problem has been
dealt with in commit 5ec911c30ff433 by simply turning the sd_init() call
to blk_attach_dev_nofail() into a call to blk_attach_dev() which ignores
its error return. So I should be able to do this with a DEFINE_PROP_DRIVE
now I think...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 September 2015 at 17:57, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>
>>>>> Convert the pxa2xx_mmci device to be a sysbus device.
>>>
>>>>> +static Property pxa2xx_mmci_properties[] = {
>>>>> +    /* Note: pointer property 'drive' may remain NULL, thus no need
>>>>> +     * for dc->cannot_instantiate_with_device_add_yet = true;
>>>>> +     * Unfortunately this can't be a DEFINE_PROP_DRIVE, because
>>>>> +     * setting a 'drive' property results in a call to blk_attach_dev()
>>>>> +     * attaching the BlockBackend to this device; that then means that
>>>>> +     * the call in sd_init() to blk_attach_dev_nofail() which tries to
>>>>> +     * attach the BlockBackend to the SD card object aborts.
>>>>> +     */
>>>>> +    DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk),
>>>>> +    DEFINE_PROP_END_OF_LIST(),
>>>>> +};
>>>>
>>>> As far as I can tell, this problem is an artifact of our interface to
>>>> the common sd-card code, namely sd_init().  sd_init() was made for the
>>>> pre-qdev world: it creates and completely initializes the common
>>>> SDState.
>>>>
>>>> In qdev, we do this in three separate steps: create, set properties,
>>>> realize.  Split up sd_init(), and the problem should go away.
>>>
>>> Yes, but I don't really want to gate QOMification of mmc
>>> controller devices on the more complicated problem of
>>> QOMifying sd.c itself, especially since we already have several
>>> QOMified mmc controllers...
>>
>> Is serial.c QOMified?  I don't think so, it's merely structured in a
>> QOM-friendly way: typedef SerialState, realize helper
>> serial_realize_core(), unrealize helper serial_exit_core().  If
>> SerialState had more properties, we'd also need a macro to define them.
>
> It looks like since we had this conversation the problem has been
> dealt with in commit 5ec911c30ff433 by simply turning the sd_init() call
> to blk_attach_dev_nofail() into a call to blk_attach_dev() which ignores
> its error return. So I should be able to do this with a DEFINE_PROP_DRIVE
> now I think...

Ignoring the error is intentional according to the comment, but why is
it appropriate?

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Peter Maydell]

On 4 December 2015 at 07:30, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 7 September 2015 at 17:57, Markus Armbruster <armbru@redhat.com> wrote:
>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>
>>>> On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote:
>>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>>
>>>>>> Convert the pxa2xx_mmci device to be a sysbus device.
>>>>
>>>>>> +static Property pxa2xx_mmci_properties[] = {
>>>>>> +    /* Note: pointer property 'drive' may remain NULL, thus no need
>>>>>> +     * for dc->cannot_instantiate_with_device_add_yet = true;
>>>>>> +     * Unfortunately this can't be a DEFINE_PROP_DRIVE, because
>>>>>> +     * setting a 'drive' property results in a call to blk_attach_dev()
>>>>>> +     * attaching the BlockBackend to this device; that then means that
>>>>>> +     * the call in sd_init() to blk_attach_dev_nofail() which tries to
>>>>>> +     * attach the BlockBackend to the SD card object aborts.
>>>>>> +     */
>>>>>> +    DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk),
>>>>>> +    DEFINE_PROP_END_OF_LIST(),
>>>>>> +};
>>>>>
>>>>> As far as I can tell, this problem is an artifact of our interface to
>>>>> the common sd-card code, namely sd_init().  sd_init() was made for the
>>>>> pre-qdev world: it creates and completely initializes the common
>>>>> SDState.
>>>>>
>>>>> In qdev, we do this in three separate steps: create, set properties,
>>>>> realize.  Split up sd_init(), and the problem should go away.
>>>>
>>>> Yes, but I don't really want to gate QOMification of mmc
>>>> controller devices on the more complicated problem of
>>>> QOMifying sd.c itself, especially since we already have several
>>>> QOMified mmc controllers...
>>>
>>> Is serial.c QOMified?  I don't think so, it's merely structured in a
>>> QOM-friendly way: typedef SerialState, realize helper
>>> serial_realize_core(), unrealize helper serial_exit_core().  If
>>> SerialState had more properties, we'd also need a macro to define them.
>>
>> It looks like since we had this conversation the problem has been
>> dealt with in commit 5ec911c30ff433 by simply turning the sd_init() call
>> to blk_attach_dev_nofail() into a call to blk_attach_dev() which ignores
>> its error return. So I should be able to do this with a DEFINE_PROP_DRIVE
>> now I think...
>
> Ignoring the error is intentional according to the comment, but why is
> it appropriate?

That seems like a question to ask the author and reviewer of that
commit :-) [cc'd].

The intention seems to have been to allow sdhci to do the same thing
I want -- take a drive property (which attaches the BlockBackend to
the controller device) and then hand the BlockBackend to sd_init()
without having it blow up.

Incidentally, in an ideal world wouldn't the block/drive properties
be on the SD card object rather than the controller object ? At least,
we seem to have that split for IDE and SCSI disks.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 4 December 2015 at 07:30, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 7 September 2015 at 17:57, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>
>>>>> On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>>>
>>>>>>> Convert the pxa2xx_mmci device to be a sysbus device.
>>>>>
>>>>>>> +static Property pxa2xx_mmci_properties[] = {
>>>>>>> +    /* Note: pointer property 'drive' may remain NULL, thus no need
>>>>>>> +     * for dc->cannot_instantiate_with_device_add_yet = true;
>>>>>>> +     * Unfortunately this can't be a DEFINE_PROP_DRIVE, because
>>>>>>> +     * setting a 'drive' property results in a call to blk_attach_dev()
>>>>>>> +     * attaching the BlockBackend to this device; that then means that
>>>>>>> +     * the call in sd_init() to blk_attach_dev_nofail() which tries to
>>>>>>> +     * attach the BlockBackend to the SD card object aborts.
>>>>>>> +     */
>>>>>>> +    DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk),
>>>>>>> +    DEFINE_PROP_END_OF_LIST(),
>>>>>>> +};
>>>>>>
>>>>>> As far as I can tell, this problem is an artifact of our interface to
>>>>>> the common sd-card code, namely sd_init().  sd_init() was made for the
>>>>>> pre-qdev world: it creates and completely initializes the common
>>>>>> SDState.
>>>>>>
>>>>>> In qdev, we do this in three separate steps: create, set properties,
>>>>>> realize.  Split up sd_init(), and the problem should go away.
>>>>>
>>>>> Yes, but I don't really want to gate QOMification of mmc
>>>>> controller devices on the more complicated problem of
>>>>> QOMifying sd.c itself, especially since we already have several
>>>>> QOMified mmc controllers...
>>>>
>>>> Is serial.c QOMified?  I don't think so, it's merely structured in a
>>>> QOM-friendly way: typedef SerialState, realize helper
>>>> serial_realize_core(), unrealize helper serial_exit_core().  If
>>>> SerialState had more properties, we'd also need a macro to define them.
>>>
>>> It looks like since we had this conversation the problem has been
>>> dealt with in commit 5ec911c30ff433 by simply turning the sd_init() call
>>> to blk_attach_dev_nofail() into a call to blk_attach_dev() which ignores
>>> its error return. So I should be able to do this with a DEFINE_PROP_DRIVE
>>> now I think...
>>
>> Ignoring the error is intentional according to the comment, but why is
>> it appropriate?
>
> That seems like a question to ask the author and reviewer of that
> commit :-) [cc'd].
>
> The intention seems to have been to allow sdhci to do the same thing
> I want -- take a drive property (which attaches the BlockBackend to
> the controller device) and then hand the BlockBackend to sd_init()
> without having it blow up.
>
> Incidentally, in an ideal world wouldn't the block/drive properties
> be on the SD card object rather than the controller object ? At least,
> we seem to have that split for IDE and SCSI disks.

This is really an instance of the common split device pattern: we have a
front end (a.k.a. device model) connected to a backend (or even multiple
backends).  A character device model is connected to a CharDriverState.
A NIC device model is connected to NICPeers.  And a block device is
connected to a BlockBackend.

For qdevified devices, the connection is made with a qdev property.  A
character device model has one defined with DEFINE_PROP_CHR().  A NIC
device model has one defined with DEFINE_PROP_NETDEV(), usually via
DEFINE_NIC_PROPERTIES().  A block device model has one defined with
DEFINE_PROP_DRIVE(), often via DEFINE_BLOCK_PROPERTIES().

Backends commonly can only be connected to one frontend.  The properties
check the applicable constraints, and error out when the user tries to
violate them.  Example: if you try to connect a block backend to
multiple frontends by specifying the same drive= value with each of
them, you get an error.  Good, because without that, you'd likely get
data corruption.

To finally answer your question: the proper owner of the property
connecting the backend is the frontend half of the split device.  So, if
we have separate device models for controller and the block devices
connected to it, the block backend property belongs to the latter, not
the former.  If we have separate device models for SD controller and
card, the block backend property belongs to the card, not the
controller.

Non-qdevified devices need to setup the connection manually, making sure
applicable constraints get checked.

As I explained above, the problem with SD cards is "an artifact of our
interface to the common sd-card code": it serves both non-qdevified and
qdevified code, badly.  We should either qdevify everything, or
restructure the common code to make it serve both qdevified and
non-qdevified code well.  I acknowledge the former is a tall order.  The
latter, however, should be doable, and I pointed to similarly common
code that already does it.

Commit 5ec911c papers over the problem instead, in sd_init().  Works
basically like this:

1. Try to connect the backend, ignoring errors.

2. Configure the backend to taste.

Fine if the backend is already connected to this frontend, and we're
trying to connect it again only because our code is too disorganized to
know.

However, if the backend is actually connected to something else, it's
bound to end in tears.

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Peter Maydell]

On 4 December 2015 at 12:50, Markus Armbruster <armbru@redhat.com> wrote:
> To finally answer your question: the proper owner of the property
> connecting the backend is the frontend half of the split device.  So, if
> we have separate device models for controller and the block devices
> connected to it, the block backend property belongs to the latter, not
> the former.  If we have separate device models for SD controller and
> card, the block backend property belongs to the card, not the
> controller.

I thought this might be your answer, but this is problematic because
we now have a device in the tree (the sdhci pci card) which has the
property on the controller, and so that's now user-facing command
line ABI which we can't change...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 4 December 2015 at 12:50, Markus Armbruster <armbru@redhat.com> wrote:
>> To finally answer your question: the proper owner of the property
>> connecting the backend is the frontend half of the split device.  So, if
>> we have separate device models for controller and the block devices
>> connected to it, the block backend property belongs to the latter, not
>> the former.  If we have separate device models for SD controller and
>> card, the block backend property belongs to the card, not the
>> controller.
>
> I thought this might be your answer, but this is problematic because
> we now have a device in the tree (the sdhci pci card) which has the
> property on the controller, and so that's now user-facing command
> line ABI which we can't change...

As far as I can tell, device sdhci-pci hasn't been in a release, and we
can still keep it out of 2.5:

* Created in commit 224d10f, v2.3.0.

* Made unavailable in commit 1910913, v2.3.0

* Made available in commit 5ec911c, not yet released.

I'll post a patch making it unavailable again right away, to give you
options.

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Paolo Bonzini]

On 04/12/2015 12:00, Peter Maydell wrote:
> On 4 December 2015 at 07:30, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 7 September 2015 at 17:57, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>
>>>>> On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>>>
>>>>>>> Convert the pxa2xx_mmci device to be a sysbus device.
>>>>>
>>>>>>> +static Property pxa2xx_mmci_properties[] = {
>>>>>>> +    /* Note: pointer property 'drive' may remain NULL, thus no need
>>>>>>> +     * for dc->cannot_instantiate_with_device_add_yet = true;
>>>>>>> +     * Unfortunately this can't be a DEFINE_PROP_DRIVE, because
>>>>>>> +     * setting a 'drive' property results in a call to blk_attach_dev()
>>>>>>> +     * attaching the BlockBackend to this device; that then means that
>>>>>>> +     * the call in sd_init() to blk_attach_dev_nofail() which tries to
>>>>>>> +     * attach the BlockBackend to the SD card object aborts.
>>>>>>> +     */
>>>>>>> +    DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk),
>>>>>>> +    DEFINE_PROP_END_OF_LIST(),
>>>>>>> +};
>>>>>>
>>>>>> As far as I can tell, this problem is an artifact of our interface to
>>>>>> the common sd-card code, namely sd_init().  sd_init() was made for the
>>>>>> pre-qdev world: it creates and completely initializes the common
>>>>>> SDState.
>>>>>>
>>>>>> In qdev, we do this in three separate steps: create, set properties,
>>>>>> realize.  Split up sd_init(), and the problem should go away.
>>>>>
>>>>> Yes, but I don't really want to gate QOMification of mmc
>>>>> controller devices on the more complicated problem of
>>>>> QOMifying sd.c itself, especially since we already have several
>>>>> QOMified mmc controllers...
>>>>
>>>> Is serial.c QOMified?  I don't think so, it's merely structured in a
>>>> QOM-friendly way: typedef SerialState, realize helper
>>>> serial_realize_core(), unrealize helper serial_exit_core().  If
>>>> SerialState had more properties, we'd also need a macro to define them.
>>>
>>> It looks like since we had this conversation the problem has been
>>> dealt with in commit 5ec911c30ff433 by simply turning the sd_init() call
>>> to blk_attach_dev_nofail() into a call to blk_attach_dev() which ignores
>>> its error return. So I should be able to do this with a DEFINE_PROP_DRIVE
>>> now I think...
>>
>> Ignoring the error is intentional according to the comment, but why is
>> it appropriate?
> 
> That seems like a question to ask the author and reviewer of that
> commit :-) [cc'd].
> 
> The intention seems to have been to allow sdhci to do the same thing
> I want -- take a drive property (which attaches the BlockBackend to
> the controller device) and then hand the BlockBackend to sd_init()
> without having it blow up.
> 
> Incidentally, in an ideal world wouldn't the block/drive properties
> be on the SD card object rather than the controller object ? At least,
> we seem to have that split for IDE and SCSI disks.

[copying from the other thread]

FWIW, I don't think the SD card will be qdevified because it doesn't
need a bus.  It's similar indeed to SerialState, which was supposed to
be the poster child of QOM embedding and never got QOMified.

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, which is exactly
what this series does.

Paolo

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Kevin O'Connor]

On Fri, Dec 04, 2015 at 12:55:07PM +0000, Peter Maydell wrote:
> On 4 December 2015 at 12:50, Markus Armbruster <armbru@redhat.com> wrote:
> > To finally answer your question: the proper owner of the property
> > connecting the backend is the frontend half of the split device.  So, if
> > we have separate device models for controller and the block devices
> > connected to it, the block backend property belongs to the latter, not
> > the former.  If we have separate device models for SD controller and
> > card, the block backend property belongs to the card, not the
> > controller.
> 
> I thought this might be your answer, but this is problematic because
> we now have a device in the tree (the sdhci pci card) which has the
> property on the controller, and so that's now user-facing command
> line ABI which we can't change...

If by ABI, you mean the command line format:

 -device sdhci-pci,drive=drive0 -drive id=drive0,if=sd,file=myimage

then I don't see why that should change.  As Paolo points out, there
is only one card per controller.  That's how real hardware does it as
well.

Cheers,
-Kevin

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Peter Crosthwaite]

On Fri, Dec 4, 2015 at 8:42 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 04/12/2015 12:00, Peter Maydell wrote:
>> On 4 December 2015 at 07:30, Markus Armbruster <armbru@redhat.com> wrote:
>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>
>>>> On 7 September 2015 at 17:57, Markus Armbruster <armbru@redhat.com> wrote:
>>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>>
>>>>>> On 7 September 2015 at 17:40, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>>>>
>>>>>>>> Convert the pxa2xx_mmci device to be a sysbus device.
>>>>>>
>>>>>>>> +static Property pxa2xx_mmci_properties[] = {
>>>>>>>> +    /* Note: pointer property 'drive' may remain NULL, thus no need
>>>>>>>> +     * for dc->cannot_instantiate_with_device_add_yet = true;
>>>>>>>> +     * Unfortunately this can't be a DEFINE_PROP_DRIVE, because
>>>>>>>> +     * setting a 'drive' property results in a call to blk_attach_dev()
>>>>>>>> +     * attaching the BlockBackend to this device; that then means that
>>>>>>>> +     * the call in sd_init() to blk_attach_dev_nofail() which tries to
>>>>>>>> +     * attach the BlockBackend to the SD card object aborts.
>>>>>>>> +     */
>>>>>>>> +    DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk),
>>>>>>>> +    DEFINE_PROP_END_OF_LIST(),
>>>>>>>> +};
>>>>>>>
>>>>>>> As far as I can tell, this problem is an artifact of our interface to
>>>>>>> the common sd-card code, namely sd_init().  sd_init() was made for the
>>>>>>> pre-qdev world: it creates and completely initializes the common
>>>>>>> SDState.
>>>>>>>
>>>>>>> In qdev, we do this in three separate steps: create, set properties,
>>>>>>> realize.  Split up sd_init(), and the problem should go away.
>>>>>>
>>>>>> Yes, but I don't really want to gate QOMification of mmc
>>>>>> controller devices on the more complicated problem of
>>>>>> QOMifying sd.c itself, especially since we already have several
>>>>>> QOMified mmc controllers...
>>>>>
>>>>> Is serial.c QOMified?  I don't think so, it's merely structured in a
>>>>> QOM-friendly way: typedef SerialState, realize helper
>>>>> serial_realize_core(), unrealize helper serial_exit_core().  If
>>>>> SerialState had more properties, we'd also need a macro to define them.
>>>>
>>>> It looks like since we had this conversation the problem has been
>>>> dealt with in commit 5ec911c30ff433 by simply turning the sd_init() call
>>>> to blk_attach_dev_nofail() into a call to blk_attach_dev() which ignores
>>>> its error return. So I should be able to do this with a DEFINE_PROP_DRIVE
>>>> now I think...
>>>
>>> Ignoring the error is intentional according to the comment, but why is
>>> it appropriate?
>>
>> That seems like a question to ask the author and reviewer of that
>> commit :-) [cc'd].
>>
>> The intention seems to have been to allow sdhci to do the same thing
>> I want -- take a drive property (which attaches the BlockBackend to
>> the controller device) and then hand the BlockBackend to sd_init()
>> without having it blow up.
>>
>> Incidentally, in an ideal world wouldn't the block/drive properties
>> be on the SD card object rather than the controller object ? At least,
>> we seem to have that split for IDE and SCSI disks.
>
> [copying from the other thread]
>
> FWIW, I don't think the SD card will be qdevified because it doesn't
> need a bus.  It's similar indeed to SerialState, which was supposed to
> be the poster child of QOM embedding and never got QOMified.
>

SD is a bus in its own right and should be busified and QOMified IMO.
SDHCI can talk to non-sd cards (SDIO). There is also a range of
incompatible cards that you can talk to - MMC/eMMC/SD(H|S|X)C. I think
anything that couples the controller to an SD card is a bug, the card
and device should be arranged as separate devices.

> A host controller controls exactly one SD card, the SSI bridge is also
> for exactly one SD card, etc.

I think you can RYO chip selects with a GPIO and control multiple SD
cards with one SDHCI.

Regards,
Peter

> 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, which is exactly
> what this series does.
>
> Paolo
>

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Kevin O'Connor]

On Fri, Dec 04, 2015 at 10:50:21AM -0800, Peter Crosthwaite wrote:
> > FWIW, I don't think the SD card will be qdevified because it doesn't
> > need a bus.  It's similar indeed to SerialState, which was supposed to
> > be the poster child of QOM embedding and never got QOMified.
> 
> SD is a bus in its own right and should be busified and QOMified IMO.
> SDHCI can talk to non-sd cards (SDIO). There is also a range of
> incompatible cards that you can talk to - MMC/eMMC/SD(H|S|X)C. I think
> anything that couples the controller to an SD card is a bug, the card
> and device should be arranged as separate devices.
> 
> > A host controller controls exactly one SD card, the SSI bridge is also
> > for exactly one SD card, etc.
> 
> I think you can RYO chip selects with a GPIO and control multiple SD
> cards with one SDHCI.

In practice, the SDHCI controllers are one-to-one with cards.  This is
codified in the sdhci spec as it has a "card present" bit and "port
location" information that is per controller.

I suppose in theory, one could put an SDHCI contoller into SPI
compatibility mode and "hot wire" it into a bus, but qemu doesn't
support that anyway, and it is a lot of complexity for something that
is not done in practice.

-Kevin

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Peter Crosthwaite]

On Fri, Dec 4, 2015 at 11:24 AM, Kevin O'Connor <kevin@koconnor.net> wrote:
> On Fri, Dec 04, 2015 at 10:50:21AM -0800, Peter Crosthwaite wrote:
>> > FWIW, I don't think the SD card will be qdevified because it doesn't
>> > need a bus.  It's similar indeed to SerialState, which was supposed to
>> > be the poster child of QOM embedding and never got QOMified.
>>
>> SD is a bus in its own right and should be busified and QOMified IMO.
>> SDHCI can talk to non-sd cards (SDIO). There is also a range of
>> incompatible cards that you can talk to - MMC/eMMC/SD(H|S|X)C. I think
>> anything that couples the controller to an SD card is a bug, the card
>> and device should be arranged as separate devices.
>>
>> > A host controller controls exactly one SD card, the SSI bridge is also
>> > for exactly one SD card, etc.
>>
>> I think you can RYO chip selects with a GPIO and control multiple SD
>> cards with one SDHCI.
>
> In practice, the SDHCI controllers are one-to-one with cards.  This is
> codified in the sdhci spec as it has a "card present" bit and "port
> location" information that is per controller.
>

But SDHCI is only one SD controller implementation. Other SD
controllers may easily implement the SDIO bus as non 1:1. I also don't
see why a 1:1 bus should have a completely different implementation
that propagates through to UI level issues. SD is a bus like any other
IMO.

> I suppose in theory, one could put an SDHCI contoller into SPI
> compatibility mode and "hot wire" it into a bus, but qemu doesn't
> support that anyway, and it is a lot of complexity for something that
> is not done in practice.
>

Note that the chip selects for SD are on the copper interface as well,
so you can easily implement multi-device SD bus on the level above
SDHCI. SPI mode should not required. Even if SDHCI does have
card-detection CS control logic this can be demultiplexed to multiple
cards by extra logic. We also have precedent of implementers of SDHCI
opting-out of features like power control, and assumptions about how
SDHCI is wired have already led to bugs like this:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg338495.html

We don't need to be implementing the multi CS logic today, but making
UI level decisions based on this 1:1 assumption seems wrong.

Regards,
Peter

> -Kevin

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Kevin O'Connor]

On Sun, Dec 06, 2015 at 04:02:14PM -0800, Peter Crosthwaite wrote:
> On Fri, Dec 4, 2015 at 11:24 AM, Kevin O'Connor <kevin@koconnor.net> wrote:
> > On Fri, Dec 04, 2015 at 10:50:21AM -0800, Peter Crosthwaite wrote:
> >> > FWIW, I don't think the SD card will be qdevified because it doesn't
> >> > need a bus.  It's similar indeed to SerialState, which was supposed to
> >> > be the poster child of QOM embedding and never got QOMified.
> >>
> >> SD is a bus in its own right and should be busified and QOMified IMO.
> >> SDHCI can talk to non-sd cards (SDIO). There is also a range of
> >> incompatible cards that you can talk to - MMC/eMMC/SD(H|S|X)C. I think
> >> anything that couples the controller to an SD card is a bug, the card
> >> and device should be arranged as separate devices.
> >>
> >> > A host controller controls exactly one SD card, the SSI bridge is also
> >> > for exactly one SD card, etc.
> >>
> >> I think you can RYO chip selects with a GPIO and control multiple SD
> >> cards with one SDHCI.
> >
> > In practice, the SDHCI controllers are one-to-one with cards.  This is
> > codified in the sdhci spec as it has a "card present" bit and "port
> > location" information that is per controller.
> >
> 
> But SDHCI is only one SD controller implementation. Other SD
> controllers may easily implement the SDIO bus as non 1:1. I also don't
> see why a 1:1 bus should have a completely different implementation
> that propagates through to UI level issues. SD is a bus like any other
> IMO.
> 
> > I suppose in theory, one could put an SDHCI contoller into SPI
> > compatibility mode and "hot wire" it into a bus, but qemu doesn't
> > support that anyway, and it is a lot of complexity for something that
> > is not done in practice.
> >
> 
> Note that the chip selects for SD are on the copper interface as well,
> so you can easily implement multi-device SD bus on the level above
> SDHCI. SPI mode should not required.

The "card select" line becomes a data line (DAT3) when not in SPI
mode.  That said, the MMC specs do define a convoluted mechanism for
assigning unique ids to each card so that a bus could be implemented.
(Which doesn't apply to SD cards, by my read of the SD specs.)

>Even if SDHCI does have
> card-detection CS control logic this can be demultiplexed to multiple
> cards by extra logic. We also have precedent of implementers of SDHCI
> opting-out of features like power control, and assumptions about how
> SDHCI is wired have already led to bugs like this:
> 
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg338495.html
> 
> We don't need to be implementing the multi CS logic today, but making
> UI level decisions based on this 1:1 assumption seems wrong.

I have no objection to a different internal or external represention
to the SD cards.  However, the immiediate choice seems to be whether
to disable pci-sdhci or not.  It seems unfortunate to me that it would
be disabled.  The QEMU SD code (beyond just the sdhci controller) is
not organized as a bus, and it's not clear to me that there are real
world gains to be had by modeling it as a bus.

My interest in the sd cards is for testing purposes.  Several recent
Intel chipsets include multiple sdhci controllers on them, and several
currently shipping devices use them for both eMMC storage and for
external SD card ports - in particular several of the Chromebooks have
them.  Being able to perform simple sanity checks in QEMU is helpful.

Cheers,
-Kevin

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Markus Armbruster]

"Kevin O'Connor" <kevin@koconnor.net> writes:

> On Sun, Dec 06, 2015 at 04:02:14PM -0800, Peter Crosthwaite wrote:
>> On Fri, Dec 4, 2015 at 11:24 AM, Kevin O'Connor <kevin@koconnor.net> wrote:
>> > On Fri, Dec 04, 2015 at 10:50:21AM -0800, Peter Crosthwaite wrote:
>> >> > FWIW, I don't think the SD card will be qdevified because it doesn't
>> >> > need a bus.  It's similar indeed to SerialState, which was supposed to
>> >> > be the poster child of QOM embedding and never got QOMified.
>> >>
>> >> SD is a bus in its own right and should be busified and QOMified IMO.
>> >> SDHCI can talk to non-sd cards (SDIO). There is also a range of
>> >> incompatible cards that you can talk to - MMC/eMMC/SD(H|S|X)C. I think
>> >> anything that couples the controller to an SD card is a bug, the card
>> >> and device should be arranged as separate devices.
>> >>
>> >> > A host controller controls exactly one SD card, the SSI bridge is also
>> >> > for exactly one SD card, etc.
>> >>
>> >> I think you can RYO chip selects with a GPIO and control multiple SD
>> >> cards with one SDHCI.
>> >
>> > In practice, the SDHCI controllers are one-to-one with cards.  This is
>> > codified in the sdhci spec as it has a "card present" bit and "port
>> > location" information that is per controller.
>> >
>> 
>> But SDHCI is only one SD controller implementation. Other SD
>> controllers may easily implement the SDIO bus as non 1:1. I also don't
>> see why a 1:1 bus should have a completely different implementation
>> that propagates through to UI level issues. SD is a bus like any other
>> IMO.
>> 
>> > I suppose in theory, one could put an SDHCI contoller into SPI
>> > compatibility mode and "hot wire" it into a bus, but qemu doesn't
>> > support that anyway, and it is a lot of complexity for something that
>> > is not done in practice.
>> >
>> 
>> Note that the chip selects for SD are on the copper interface as well,
>> so you can easily implement multi-device SD bus on the level above
>> SDHCI. SPI mode should not required.
>
> The "card select" line becomes a data line (DAT3) when not in SPI
> mode.  That said, the MMC specs do define a convoluted mechanism for
> assigning unique ids to each card so that a bus could be implemented.
> (Which doesn't apply to SD cards, by my read of the SD specs.)
>
>>Even if SDHCI does have
>> card-detection CS control logic this can be demultiplexed to multiple
>> cards by extra logic. We also have precedent of implementers of SDHCI
>> opting-out of features like power control, and assumptions about how
>> SDHCI is wired have already led to bugs like this:
>> 
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg338495.html
>> 
>> We don't need to be implementing the multi CS logic today, but making
>> UI level decisions based on this 1:1 assumption seems wrong.
>
> I have no objection to a different internal or external represention
> to the SD cards.  However, the immiediate choice seems to be whether
> to disable pci-sdhci or not.  It seems unfortunate to me that it would
> be disabled.  The QEMU SD code (beyond just the sdhci controller) is
> not organized as a bus, and it's not clear to me that there are real
> world gains to be had by modeling it as a bus.
>
> My interest in the sd cards is for testing purposes.  Several recent
> Intel chipsets include multiple sdhci controllers on them, and several
> currently shipping devices use them for both eMMC storage and for
> external SD card ports - in particular several of the Chromebooks have
> them.  Being able to perform simple sanity checks in QEMU is helpful.

The device is obviously useful.  However, there is dissent on how it
ought to be modelled.  The modelling is visible at the -device user
interface.  By making the device available there in 2.5, we commit to
the current modelling's user interface before we reach consensus on how
it ought to be modelled.  Options:

(1) Make device "sdhci-pci" unavailable with -device until we reach
    consensus.  This is what we normally do.  Trivial patch is on list.

(2) Mark the properties that belong to the card rather than the
    controller as experimental until we reach consensus, by prefixing
    their name with "x-".  Needs a patch.

(3) Keep it available, commit to the user interface, deal with the
    consequences if and when they arise.

I think (1) is the most prudent, but (2) should work, too.  Having dealt
with consequences of prior modelling mistakes, I dislike 3.

Opinions?

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Paolo Bonzini]

On 07/12/2015 09:50, Markus Armbruster wrote:
> The device is obviously useful.  However, there is dissent on how it
> ought to be modelled.  The modelling is visible at the -device user
> interface.  By making the device available there in 2.5, we commit to
> the current modelling's user interface before we reach consensus on how
> it ought to be modelled.  Options:
> 
> (1) Make device "sdhci-pci" unavailable with -device until we reach
>     consensus.  This is what we normally do.  Trivial patch is on list.
> 
> (2) Mark the properties that belong to the card rather than the
>     controller as experimental until we reach consensus, by prefixing
>     their name with "x-".  Needs a patch.
> 
> (3) Keep it available, commit to the user interface, deal with the
>     consequences if and when they arise.
> 
> I think (1) is the most prudent, but (2) should work, too.  Having dealt
> with consequences of prior modelling mistakes, I dislike 3.

There have been 10 commits in 2 years to sd.c, none of them getting a
step closer to qdev-ification basically.  So there's no interest, which
is basically explained by the fact that quite frankly SDIO is dead.

I don't see any real difference between sdhci-pci and pci-serial.

Paolo

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 07/12/2015 09:50, Markus Armbruster wrote:
>> The device is obviously useful.  However, there is dissent on how it
>> ought to be modelled.  The modelling is visible at the -device user
>> interface.  By making the device available there in 2.5, we commit to
>> the current modelling's user interface before we reach consensus on how
>> it ought to be modelled.  Options:
>> 
>> (1) Make device "sdhci-pci" unavailable with -device until we reach
>>     consensus.  This is what we normally do.  Trivial patch is on list.
>> 
>> (2) Mark the properties that belong to the card rather than the
>>     controller as experimental until we reach consensus, by prefixing
>>     their name with "x-".  Needs a patch.
>> 
>> (3) Keep it available, commit to the user interface, deal with the
>>     consequences if and when they arise.
>> 
>> I think (1) is the most prudent, but (2) should work, too.  Having dealt
>> with consequences of prior modelling mistakes, I dislike 3.
>
> There have been 10 commits in 2 years to sd.c, none of them getting a
> step closer to qdev-ification basically.  So there's no interest, which
> is basically explained by the fact that quite frankly SDIO is dead.

There hasn't been progress towards qdevification because we've offered
neither sticks nor carrots to the donkeys who're supposed to do the
donkey-work.

> I don't see any real difference between sdhci-pci and pci-serial.

The difference is dissent vs. consensus.

I don't have a strong opinion myself, but Peter C. seems to have.

I suppose we could find rough consensus, but finding it under duress of
a release isn't how we normally do it.

Option (2) would make the device available for testing in 2.5 without
breaking off the debate before consensus is reached or it becomes clear
it cannot be reached.

Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Peter Maydell]

On 7 December 2015 at 10:31, Markus Armbruster <armbru@redhat.com> wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 07/12/2015 09:50, Markus Armbruster wrote:
>>> The device is obviously useful.  However, there is dissent on how it
>>> ought to be modelled.  The modelling is visible at the -device user
>>> interface.  By making the device available there in 2.5, we commit to
>>> the current modelling's user interface before we reach consensus on how
>>> it ought to be modelled.  Options:
>>>
>>> (1) Make device "sdhci-pci" unavailable with -device until we reach
>>>     consensus.  This is what we normally do.  Trivial patch is on list.
>>>
>>> (2) Mark the properties that belong to the card rather than the
>>>     controller as experimental until we reach consensus, by prefixing
>>>     their name with "x-".  Needs a patch.
>>>
>>> (3) Keep it available, commit to the user interface, deal with the
>>>     consequences if and when they arise.
>>>
>>> I think (1) is the most prudent, but (2) should work, too.  Having dealt
>>> with consequences of prior modelling mistakes, I dislike 3.
>>
>> There have been 10 commits in 2 years to sd.c, none of them getting a
>> step closer to qdev-ification basically.  So there's no interest, which
>> is basically explained by the fact that quite frankly SDIO is dead.
>
> There hasn't been progress towards qdevification because we've offered
> neither sticks nor carrots to the donkeys who're supposed to do the
> donkey-work.

I will QOMify sd.c for 2.6. (Most of the users are ARM boards anyway.)

>> I don't see any real difference between sdhci-pci and pci-serial.
>
> The difference is dissent vs. consensus.
>
> I don't have a strong opinion myself, but Peter C. seems to have.
>
> I suppose we could find rough consensus, but finding it under duress of
> a release isn't how we normally do it.
>
> Option (2) would make the device available for testing in 2.5 without
> breaking off the debate before consensus is reached or it becomes clear
> it cannot be reached.

I definitely would prefer us not to have to be stuck with a wrongly
modelled sd.c. So I'd prefer option (1) or option (2). Option 2
would allow people to use the device in 2.5 at least.

The release notes will also want a warning that sdhci might end
up with a migration compatibility break in 2.6. (Obviously we'd try
to avoid that but QOMifying sd.c might require it.)

thanks
-- PMM