[Qemu-devel] [PATCH 0/3] Drop default SD card creation

[Qemu-devel] [PATCH 0/3] Drop default SD card creation

[Peter Maydell]

As suggested in the recent discussion on Marcks' patchset to suppress
unused default drives, this patchset cleans up the omap and pxa2xx
SD card controllers to behave like the other controllers:
 * the init function looks for the next IF_SD drive
 * if there isn't one, we start up as a controller with no card
   present

This then allows us to drop the QEMUMachine no_sdcard flag and
the vl.c code which creates a dummy IF_SD drive.

Not intended for 1.2, obviously.

Peter Maydell (3):
  omap: Get BlockDriverState* in mmc controller init, not board init
  pxa2xx:  Get BlockDriverState* in mmc controller init, not board init
  Drop default SD card creation

 hw/boards.h      |    3 +--
 hw/omap.h        |    3 +--
 hw/omap1.c       |    8 +-------
 hw/omap2.c       |    8 +-------
 hw/omap_mmc.c    |   12 ++++++++----
 hw/pxa.h         |    2 +-
 hw/pxa2xx.c      |   16 ++--------------
 hw/pxa2xx_mmci.c |    7 +++++--
 hw/s390-virtio.c |    1 -
 hw/xilinx_zynq.c |    1 -
 vl.c             |    7 -------
 11 files changed, 20 insertions(+), 48 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/3] omap: Get BlockDriverState* in mmc controller init, not board init

[Peter Maydell]

Instead of getting the BlockDriverState* in the omap board init
and passing it to the mmc controller's init function, have the
mmc controller get the next IF_SD device and use it if present.
This brings us into line with other SD controller models and
means that we correctly emulate an SD controller with no card
present if the user didn't ask for an SD card.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/omap.h     |    3 +--
 hw/omap1.c    |    8 +-------
 hw/omap2.c    |    8 +-------
 hw/omap_mmc.c |   12 ++++++++----
 4 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/hw/omap.h b/hw/omap.h
index 413851b..de5ec8c 100644
--- a/hw/omap.h
+++ b/hw/omap.h
@@ -754,10 +754,9 @@ void omap_rfbi_attach(struct omap_dss_s *s, int cs, struct rfbi_chip_s *chip);
 struct omap_mmc_s;
 struct omap_mmc_s *omap_mmc_init(target_phys_addr_t base,
                 MemoryRegion *sysmem,
-                BlockDriverState *bd,
                 qemu_irq irq, qemu_irq dma[], omap_clk clk);
 struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta,
-                BlockDriverState *bd, qemu_irq irq, qemu_irq dma[],
+                qemu_irq irq, qemu_irq dma[],
                 omap_clk fclk, omap_clk iclk);
 void omap_mmc_reset(struct omap_mmc_s *s);
 void omap_mmc_handlers(struct omap_mmc_s *s, qemu_irq ro, qemu_irq cover);
diff --git a/hw/omap1.c b/hw/omap1.c
index ad60cc4..641e260 100644
--- a/hw/omap1.c
+++ b/hw/omap1.c
@@ -3823,7 +3823,6 @@ struct omap_mpu_state_s *omap310_mpu_init(MemoryRegion *system_memory,
             g_malloc0(sizeof(struct omap_mpu_state_s));
     qemu_irq *cpu_irq;
     qemu_irq dma_irqs[6];
-    DriveInfo *dinfo;
     SysBusDevice *busdev;
 
     if (!core)
@@ -3961,12 +3960,7 @@ struct omap_mpu_state_s *omap310_mpu_init(MemoryRegion *system_memory,
     s->dpll[2] = omap_dpll_init(system_memory, 0xfffed100,
                                 omap_findclk(s, "dpll3"));
 
-    dinfo = drive_get(IF_SD, 0, 0);
-    if (!dinfo) {
-        fprintf(stderr, "qemu: missing SecureDigital device\n");
-        exit(1);
-    }
-    s->mmc = omap_mmc_init(0xfffb7800, system_memory, dinfo->bdrv,
+    s->mmc = omap_mmc_init(0xfffb7800, system_memory,
                            qdev_get_gpio_in(s->ih[1], OMAP_INT_OQN),
                            &s->drq[OMAP_DMA_MMC_TX],
                     omap_findclk(s, "mmc_ck"));
diff --git a/hw/omap2.c b/hw/omap2.c
index 4278dd1..28178dd 100644
--- a/hw/omap2.c
+++ b/hw/omap2.c
@@ -2246,7 +2246,6 @@ struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sysmem,
             g_malloc0(sizeof(struct omap_mpu_state_s));
     qemu_irq *cpu_irq;
     qemu_irq dma_irqs[4];
-    DriveInfo *dinfo;
     int i;
     SysBusDevice *busdev;
     struct omap_target_agent_s *ta;
@@ -2454,12 +2453,7 @@ struct omap_mpu_state_s *omap2420_mpu_init(MemoryRegion *sysmem,
                              qdev_get_gpio_in(s->ih[0], OMAP_INT_24XX_GPMC_IRQ),
                              s->drq[OMAP24XX_DMA_GPMC]);
 
-    dinfo = drive_get(IF_SD, 0, 0);
-    if (!dinfo) {
-        fprintf(stderr, "qemu: missing SecureDigital device\n");
-        exit(1);
-    }
-    s->mmc = omap2_mmc_init(omap_l4tao(s->l4, 9), dinfo->bdrv,
+    s->mmc = omap2_mmc_init(omap_l4tao(s->l4, 9),
                     qdev_get_gpio_in(s->ih[0], OMAP_INT_24XX_MMC_IRQ),
                     &s->drq[OMAP24XX_DMA_MMC1_TX],
                     omap_findclk(s, "mmc_fclk"), omap_findclk(s, "mmc_iclk"));
diff --git a/hw/omap_mmc.c b/hw/omap_mmc.c
index aec0285..eada07d 100644
--- a/hw/omap_mmc.c
+++ b/hw/omap_mmc.c
@@ -19,6 +19,7 @@
 #include "hw.h"
 #include "omap.h"
 #include "sd.h"
+#include "blockdev.h"
 
 struct omap_mmc_s {
     qemu_irq irq;
@@ -574,9 +575,9 @@ static void omap_mmc_cover_cb(void *opaque, int line, int level)
 
 struct omap_mmc_s *omap_mmc_init(target_phys_addr_t base,
                 MemoryRegion *sysmem,
-                BlockDriverState *bd,
                 qemu_irq irq, qemu_irq dma[], omap_clk clk)
 {
+    DriveInfo *dinfo;
     struct omap_mmc_s *s = (struct omap_mmc_s *)
             g_malloc0(sizeof(struct omap_mmc_s));
 
@@ -592,15 +593,17 @@ struct omap_mmc_s *omap_mmc_init(target_phys_addr_t base,
     memory_region_add_subregion(sysmem, base, &s->iomem);
 
     /* Instantiate the storage */
-    s->card = sd_init(bd, 0);
+    dinfo = drive_get_next(IF_SD);
+    s->card = sd_init(dinfo ? dinfo->bdrv : NULL, 0);
 
     return s;
 }
 
 struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta,
-                BlockDriverState *bd, qemu_irq irq, qemu_irq dma[],
+                qemu_irq irq, qemu_irq dma[],
                 omap_clk fclk, omap_clk iclk)
 {
+    DriveInfo *dinfo;
     struct omap_mmc_s *s = (struct omap_mmc_s *)
             g_malloc0(sizeof(struct omap_mmc_s));
 
@@ -617,7 +620,8 @@ struct omap_mmc_s *omap2_mmc_init(struct omap_target_agent_s *ta,
     omap_l4_attach(ta, 0, &s->iomem);
 
     /* Instantiate the storage */
-    s->card = sd_init(bd, 0);
+    dinfo = drive_get_next(IF_SD);
+    s->card = sd_init(dinfo ? dinfo->bdrv : NULL, 0);
 
     s->cdet = qemu_allocate_irqs(omap_mmc_cover_cb, s, 1)[0];
     sd_set_cb(s->card, NULL, s->cdet);
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/3] pxa2xx: Get BlockDriverState* in mmc controller init, not board init

[Peter Maydell]

Instead of getting the BlockDriverState* in the pxa2xx board init
and passing it to the mmc controller's init function, have the
mmc controller get the next IF_SD device and use it if present.
This brings us into line with other SD controller models and
means that we correctly emulate an SD controller with no card
present if the user didn't ask for an SD card.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pxa.h         |    2 +-
 hw/pxa2xx.c      |   16 ++--------------
 hw/pxa2xx_mmci.c |    7 +++++--
 3 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/hw/pxa.h b/hw/pxa.h
index 6a21205..569994b 100644
--- a/hw/pxa.h
+++ b/hw/pxa.h
@@ -87,7 +87,7 @@ void pxa2xx_lcdc_oritentation(void *opaque, int angle);
 typedef struct PXA2xxMMCIState PXA2xxMMCIState;
 PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
                 target_phys_addr_t base,
-                BlockDriverState *bd, qemu_irq irq,
+                qemu_irq irq,
                 qemu_irq rx_dma, qemu_irq tx_dma);
 void pxa2xx_mmci_handlers(PXA2xxMMCIState *s, qemu_irq readonly,
                 qemu_irq coverswitch);
diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
index d5f1420..2c3ef1f 100644
--- a/hw/pxa2xx.c
+++ b/hw/pxa2xx.c
@@ -2006,7 +2006,6 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
 {
     PXA2xxState *s;
     int i;
-    DriveInfo *dinfo;
     s = (PXA2xxState *) g_malloc0(sizeof(PXA2xxState));
 
     if (revision && strncmp(revision, "pxa27", 5)) {
@@ -2047,12 +2046,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
 
     s->gpio = pxa2xx_gpio_init(0x40e00000, &s->cpu->env, s->pic, 121);
 
-    dinfo = drive_get(IF_SD, 0, 0);
-    if (!dinfo) {
-        fprintf(stderr, "qemu: missing SecureDigital device\n");
-        exit(1);
-    }
-    s->mmc = pxa2xx_mmci_init(address_space, 0x41100000, dinfo->bdrv,
+    s->mmc = pxa2xx_mmci_init(address_space, 0x41100000,
                     qdev_get_gpio_in(s->pic, PXA2XX_PIC_MMC),
                     qdev_get_gpio_in(s->dma, PXA2XX_RX_RQ_MMCI),
                     qdev_get_gpio_in(s->dma, PXA2XX_TX_RQ_MMCI));
@@ -2143,7 +2137,6 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
 {
     PXA2xxState *s;
     int i;
-    DriveInfo *dinfo;
 
     s = (PXA2xxState *) g_malloc0(sizeof(PXA2xxState));
 
@@ -2178,12 +2171,7 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
 
     s->gpio = pxa2xx_gpio_init(0x40e00000, &s->cpu->env, s->pic, 85);
 
-    dinfo = drive_get(IF_SD, 0, 0);
-    if (!dinfo) {
-        fprintf(stderr, "qemu: missing SecureDigital device\n");
-        exit(1);
-    }
-    s->mmc = pxa2xx_mmci_init(address_space, 0x41100000, dinfo->bdrv,
+    s->mmc = pxa2xx_mmci_init(address_space, 0x41100000,
                     qdev_get_gpio_in(s->pic, PXA2XX_PIC_MMC),
                     qdev_get_gpio_in(s->dma, PXA2XX_RX_RQ_MMCI),
                     qdev_get_gpio_in(s->dma, PXA2XX_TX_RQ_MMCI));
diff --git a/hw/pxa2xx_mmci.c b/hw/pxa2xx_mmci.c
index b505a4c..f645773 100644
--- a/hw/pxa2xx_mmci.c
+++ b/hw/pxa2xx_mmci.c
@@ -14,6 +14,7 @@
 #include "pxa.h"
 #include "sd.h"
 #include "qdev.h"
+#include "blockdev.h"
 
 struct PXA2xxMMCIState {
     MemoryRegion iomem;
@@ -523,10 +524,11 @@ static int pxa2xx_mmci_load(QEMUFile *f, void *opaque, int version_id)
 
 PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
                 target_phys_addr_t base,
-                BlockDriverState *bd, qemu_irq irq,
+                qemu_irq irq,
                 qemu_irq rx_dma, qemu_irq tx_dma)
 {
     PXA2xxMMCIState *s;
+    DriveInfo *dinfo;
 
     s = (PXA2xxMMCIState *) g_malloc0(sizeof(PXA2xxMMCIState));
     s->irq = irq;
@@ -538,7 +540,8 @@ PXA2xxMMCIState *pxa2xx_mmci_init(MemoryRegion *sysmem,
     memory_region_add_subregion(sysmem, base, &s->iomem);
 
     /* Instantiate the actual storage */
-    s->card = sd_init(bd, 0);
+    dinfo = drive_get_next(IF_SD);
+    s->card = sd_init(dinfo ? dinfo->bdrv : NULL, 0);
 
     register_savevm(NULL, "pxa2xx_mmci", 0, 0,
                     pxa2xx_mmci_save, pxa2xx_mmci_load, s);
-- 
1.7.9.5

[Qemu-devel] [PATCH 3/3] Drop default SD card creation

[Peter Maydell]

Now that all users of IF_SD drives can cope with there being
no drive present (ie "controller exists but there is no card in it")
we can drop the creation of the default IF_SD card in vl.c and
the no_sdcard field in the QEMUMachine struct.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/boards.h      |    3 +--
 hw/s390-virtio.c |    1 -
 hw/xilinx_zynq.c |    1 -
 vl.c             |    7 -------
 4 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/hw/boards.h b/hw/boards.h
index 59c01d0..af4c019 100644
--- a/hw/boards.h
+++ b/hw/boards.h
@@ -23,8 +23,7 @@ typedef struct QEMUMachine {
         no_parallel:1,
         use_virtcon:1,
         no_floppy:1,
-        no_cdrom:1,
-        no_sdcard:1;
+        no_cdrom:1;
     int is_default;
     const char *default_machine_opts;
     GlobalProperty *compat_props;
diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index 47eed35..560393f 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -338,7 +338,6 @@ static QEMUMachine s390_machine = {
     .no_floppy = 1,
     .no_serial = 1,
     .no_parallel = 1,
-    .no_sdcard = 1,
     .use_virtcon = 1,
     .max_cpus = 255,
     .is_default = 1,
diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c
index 7e6c273..b532953 100644
--- a/hw/xilinx_zynq.c
+++ b/hw/xilinx_zynq.c
@@ -146,7 +146,6 @@ static QEMUMachine zynq_machine = {
     .init = zynq_init,
     .use_scsi = 1,
     .max_cpus = 1,
-    .no_sdcard = 1
 };
 
 static void zynq_machine_init(void)
diff --git a/vl.c b/vl.c
index d01256a..ba1953c 100644
--- a/vl.c
+++ b/vl.c
@@ -268,7 +268,6 @@ static int default_virtcon = 1;
 static int default_monitor = 1;
 static int default_floppy = 1;
 static int default_cdrom = 1;
-static int default_sdcard = 1;
 static int default_vga = 1;
 
 static struct {
@@ -3169,7 +3168,6 @@ int main(int argc, char **argv, char **envp)
                 default_net = 0;
                 default_floppy = 0;
                 default_cdrom = 0;
-                default_sdcard = 0;
                 default_vga = 0;
                 break;
             case QEMU_OPTION_xen_domid:
@@ -3343,9 +3341,6 @@ int main(int argc, char **argv, char **envp)
     if (machine->no_cdrom) {
         default_cdrom = 0;
     }
-    if (machine->no_sdcard) {
-        default_sdcard = 0;
-    }
 
     if (display_type == DT_NOGRAPHIC) {
         if (default_parallel)
@@ -3488,8 +3483,6 @@ int main(int argc, char **argv, char **envp)
                   IF_DEFAULT, 2, CDROM_OPTS);
     default_drive(default_floppy, snapshot, machine->use_scsi,
                   IF_FLOPPY, 0, FD_OPTS);
-    default_drive(default_sdcard, snapshot, machine->use_scsi,
-                  IF_SD, 0, SD_OPTS);
 
     register_savevm_live(NULL, "ram", 0, 4, &savevm_ram_handlers, NULL);
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 0/3] Drop default SD card creation

[Markus Armbruster]

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

> As suggested in the recent discussion on Marcks' patchset to suppress
> unused default drives, this patchset cleans up the omap and pxa2xx
> SD card controllers to behave like the other controllers:
>  * the init function looks for the next IF_SD drive
>  * if there isn't one, we start up as a controller with no card
>    present
>
> This then allows us to drop the QEMUMachine no_sdcard flag and
> the vl.c code which creates a dummy IF_SD drive.
>
> Not intended for 1.2, obviously.

Isn't this an incompatible change?  Before, you get an SD card reader
backed by an empty BDS default.  You can load/unload cards in the
monitor.  After, you get an SD card reader that isn't backed by a BDS by
default.  Device models prepared for that can treat it as permanently
empty.

Re: [Qemu-devel] [PATCH 0/3] Drop default SD card creation

[Peter Maydell]

On 16 August 2012 15:11, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> As suggested in the recent discussion on Markus' patchset to suppress
>> unused default drives, this patchset cleans up the omap and pxa2xx
>> SD card controllers to behave like the other controllers:
>>  * the init function looks for the next IF_SD drive
>>  * if there isn't one, we start up as a controller with no card
>>    present

> Isn't this an incompatible change?  Before, you get an SD card reader
> backed by an empty BDS default.  You can load/unload cards in the
> monitor.  After, you get an SD card reader that isn't backed by a BDS by
> default.  Device models prepared for that can treat it as permanently
> empty.

Hmm, yes, but most of our SD controllers already act that way.
We should probably fix them all...

So what's the block layer equivalent of drive_get_next() that always
returns us something we can get a bdrv from?

-- PMM

Re: [Qemu-devel] [PATCH 0/3] Drop default SD card creation

[Paul Brook]

> On 16 August 2012 15:11, Markus Armbruster <armbru@redhat.com> wrote:
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >> As suggested in the recent discussion on Markus' patchset to suppress
> >> unused default drives, this patchset cleans up the omap and pxa2xx
> >> 
> >> SD card controllers to behave like the other controllers:
> >>  * the init function looks for the next IF_SD drive
> >>  * if there isn't one, we start up as a controller with no card
> >>  
> >>    present
> > 
> > Isn't this an incompatible change?  Before, you get an SD card reader
> > backed by an empty BDS default.  You can load/unload cards in the
> > monitor.  After, you get an SD card reader that isn't backed by a BDS by
> > default.  Device models prepared for that can treat it as permanently
> > empty.
> 
> Hmm, yes, but most of our SD controllers already act that way.
> We should probably fix them all...
> 
> So what's the block layer equivalent of drive_get_next() that always
> returns us something we can get a bdrv from?

I think this may be the wrong way to fix this.  SD cards aren't really have 
removable media.  In the same way that a SCSI HDD are generally not removable 
media - you hotplug the whole drive.

Don't we really want a proper QOM device for the SD card, with hotplug 
support.

Paul

Re: [Qemu-devel] [PATCH 0/3] Drop default SD card creation

[Markus Armbruster]

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

> On 16 August 2012 15:11, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> As suggested in the recent discussion on Markus' patchset to suppress
>>> unused default drives, this patchset cleans up the omap and pxa2xx
>>> SD card controllers to behave like the other controllers:
>>>  * the init function looks for the next IF_SD drive
>>>  * if there isn't one, we start up as a controller with no card
>>>    present
>
>> Isn't this an incompatible change?  Before, you get an SD card reader
>> backed by an empty BDS default.  You can load/unload cards in the
>> monitor.  After, you get an SD card reader that isn't backed by a BDS by
>> default.  Device models prepared for that can treat it as permanently
>> empty.
>
> Hmm, yes, but most of our SD controllers already act that way.
> We should probably fix them all...
>
> So what's the block layer equivalent of drive_get_next() that always
> returns us something we can get a bdrv from?

I figure you need a variant of drive_get(type, bus, unit) that creates
and returns a default drive instead of null when

1. the user didn't suppress default drives with -nodefaults, and

2. (type, index) are (IF_FLOPPY, 0), (use_scsi ? IF_SCSI : IF_IDE, 2),
or (IF_SD, 0), where index satisfies drive_index_to_bus_id(type, index)
== bus and drive_index_to_unit_id(type, index) == unit.

Happy coding :)


PS: I hate drive_get_next(), because it makes which device model gets
which drive depend on initialization order.

Re: [Qemu-devel] [PATCH 0/3] Drop default SD card creation

[Markus Armbruster]

Paul Brook <paul@codesourcery.com> writes:

>> On 16 August 2012 15:11, Markus Armbruster <armbru@redhat.com> wrote:
>> > Peter Maydell <peter.maydell@linaro.org> writes:
>> >> As suggested in the recent discussion on Markus' patchset to suppress
>> >> unused default drives, this patchset cleans up the omap and pxa2xx
>> >> 
>> >> SD card controllers to behave like the other controllers:
>> >>  * the init function looks for the next IF_SD drive
>> >>  * if there isn't one, we start up as a controller with no card
>> >>  
>> >>    present
>> > 
>> > Isn't this an incompatible change?  Before, you get an SD card reader
>> > backed by an empty BDS default.  You can load/unload cards in the
>> > monitor.  After, you get an SD card reader that isn't backed by a BDS by
>> > default.  Device models prepared for that can treat it as permanently
>> > empty.
>> 
>> Hmm, yes, but most of our SD controllers already act that way.
>> We should probably fix them all...
>> 
>> So what's the block layer equivalent of drive_get_next() that always
>> returns us something we can get a bdrv from?
>
> I think this may be the wrong way to fix this.  SD cards aren't really have 
> removable media.  In the same way that a SCSI HDD are generally not removable 
> media - you hotplug the whole drive.

If an SD card device doesn't support media change, then the device model
should:

1. Insist on non-null, non-empty BDS on initialization (this ensures we
got media)

2. Not implement BlockDevOps method change_media_cb() (this ensures we
keep it).

> Don't we really want a proper QOM device for the SD card, with hotplug 
> support.

Don't we really want that for any device model?

Re: [Qemu-devel] [PATCH 0/3] Drop default SD card creation

[Peter Maydell]

On 16 August 2012 16:17, Markus Armbruster <armbru@redhat.com> wrote:
> Paul Brook <paul@codesourcery.com> writes:
>> I think this may be the wrong way to fix this.  SD cards aren't really have
>> removable media.  In the same way that a SCSI HDD are generally not removable
>> media - you hotplug the whole drive.
>
> If an SD card device doesn't support media change, then the device model
> should:
>
> 1. Insist on non-null, non-empty BDS on initialization (this ensures we
> got media)

This seems to be trying to draw a distinction that I don't understand.
The SD card *is* the media, it's the physical object you stuff in and
out of the slot on the side of your device.

I guess that that means that "change SD card" should ideally be modelled
as "destroy the sd.c device object and create a new one and reconnect
it to the controller" but we don't really model things quite in the
right way to permit that, so we fake it up at the moment by allowing
the underlying BDS to change its idea of media. This works except
that if the initial state is "no card present" we have a NULL BDS rather
than one which is non-NULL but has no media at the moment.

(I think Paul is suggesting that we should fix our model to
move closer to this idea rather than faking things...)

-- PMM

Re: [Qemu-devel] [PATCH 0/3] Drop default SD card creation

[Markus Armbruster]

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

> On 16 August 2012 16:17, Markus Armbruster <armbru@redhat.com> wrote:
>> Paul Brook <paul@codesourcery.com> writes:
>>> I think this may be the wrong way to fix this.  SD cards aren't really have
>>> removable media.  In the same way that a SCSI HDD are generally not removable
>>> media - you hotplug the whole drive.
>>
>> If an SD card device doesn't support media change, then the device model
>> should:
>>
>> 1. Insist on non-null, non-empty BDS on initialization (this ensures we
>> got media)
>
> This seems to be trying to draw a distinction that I don't understand.
> The SD card *is* the media, it's the physical object you stuff in and
> out of the slot on the side of your device.
>
> I guess that that means that "change SD card" should ideally be modelled
> as "destroy the sd.c device object and create a new one and reconnect
> it to the controller" but we don't really model things quite in the
> right way to permit that, so we fake it up at the moment by allowing
> the underlying BDS to change its idea of media. This works except
> that if the initial state is "no card present" we have a NULL BDS rather
> than one which is non-NULL but has no media at the moment.
>
> (I think Paul is suggesting that we should fix our model to
> move closer to this idea rather than faking things...)

I figure there are two ways to model this, and both can work.

One way is to treat the SD card as a hot-pluggable device.  A card
reader device model provides a connector for the SD card device model.
The SD card device model is backed by a block backend, with
non-removable medium.  Card change is device hot plug.

If the physical board doesn't let you change the card while the machine
runs, and you want to model that faithfully, disable hot plug on that
connector.

Note that we could model floppies and CD-ROMs that way, too.

Another way is to treat the SD card as media.  This is possible because
the guest can talk to the card only through the card reader.  Card
change is block backend media change.

This is how we currently do block device models.

The first way models the SD card as TYPE_DEVICE, the second as
TYPE_MEDIUM.  Except TYPE_MEDIUM doesn't exist, but after block layer
QOMification, it could.

Re: [Qemu-devel] [PATCH 0/3] Drop default SD card creation

[Paul Brook]

> On 16 August 2012 16:17, Markus Armbruster <armbru@redhat.com> wrote:
> > Paul Brook <paul@codesourcery.com> writes:
> >> I think this may be the wrong way to fix this.  SD cards aren't really
> >> have removable media.  In the same way that a SCSI HDD are generally
> >> not removable media - you hotplug the whole drive.
> > 
> > If an SD card device doesn't support media change, then the device model
> > should:
> > 
> > 1. Insist on non-null, non-empty BDS on initialization (this ensures we
> > got media)
> 
> This seems to be trying to draw a distinction that I don't understand.
> The SD card *is* the media, it's the physical object you stuff in and
> out of the slot on the side of your device.

It's the difference between "not present" and "present but empty".

In the case of an SD card the media (i.e. flash) is generally not seperable 
from the SD device - I don't remember if the SD spec even supports removable 
media.  The same is true for most hard disks - the disk platters are an 
integral part of the drive.  In these cases the "present but empty" state does 
not exist.

c.f. cdrom drives where the concept of an "empty" device is clearly very 
different to an absent device.
 
> I guess that that means that "change SD card" should ideally be modelled
> as "destroy the sd.c device object and create a new one and reconnect
> it to the controller" but we don't really model things quite in the
> right way to permit that, so we fake it up at the moment by allowing
> the underlying BDS to change its idea of media. This works except
> that if the initial state is "no card present" we have a NULL BDS rather
> than one which is non-NULL but has no media at the moment.
> 
> (I think Paul is suggesting that we should fix our model to
> move closer to this idea rather than faking things...)

I think we have two options:

A) Model the SD slot and card explicitly as separate objects.  Effectively the 
same way we have a scsi bus with scsi drives connected to it.  Cards can be 
hotplugged.  A card has a block device that is not optional, and not 
removable.

I don't know how well our UI handles this.  It may well require user-visible 
changes.

B) Continue to effectively model just the SD slot, with the card being 
implicit.  The slot should always create/find a [removable] block device.  An 
"empty" block device is modelled as an absent card.  A slot without a block 
device is IMO a bug.

This can create awkwardness because there's no good way to expose card 
specific properties (we don't curently implement any interesting ones).  These 
should really be per-card, i.e. may change when you change the contents.  
However the only thing we have to attach them to is the long-lived slot 
object.  e.g. in some cases data may be either an SD or an SDHC card.  We 
currently make a guess.  The only place to attach a user override is the SD 
slot, and that must be determined at machine creation, not when you associate 
data with the block device.

Paul

Re: [Qemu-devel] [PATCH 0/3] Drop default SD card creation

[Paul Brook]

> One way is to treat the SD card as a hot-pluggable device.  A card
> reader device model provides a connector for the SD card device model.
> The SD card device model is backed by a block backend, with
> non-removable medium.  Card change is device hot plug.
>... 
> Note that we could model floppies and CD-ROMs that way, too.

That's a good point.  e.g. for a cdrom I'm pretty sure there's a bit somewhere 
that tells you whether it's a pressed cd or a cd-r.  Attaching this 
information to a cdrom-disk device (hotplugged into a cdrom-drive) seems to 
make sense.

Paul