Patchwork [V2,10/10] hw/sd.c: introduce SD card "image" property and allow SD hot-insert

login
register
mail settings
Submitter Mitsyanko Igor
Date April 5, 2012, 3:48 p.m.
Message ID <1333640913-16028-11-git-send-email-i.mitsyanko@samsung.com>
Download mbox | patch
Permalink /patch/151226/
State New
Headers show

Comments

Mitsyanko Igor - April 5, 2012, 3:48 p.m.
New SD card "image" property can be used to:
- change image file attached to virtual SD card
- hot-insert new image file into newly initialized BlockDriverState (this was not
possible before).

Example usage:
./qom-set /machine/milkymist/milkymist-memcard/card.image /home/me/mynewcard.img
this will attach image file /home/me/mynewcard.img to virtual SD card connected to
milkymist-memcard host controller device. If virtual card was already attached to
some other image file, eject event is triggered before attaching new file.

Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
---
 hw/sd.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)
Luiz Capitulino - April 9, 2012, 12:42 p.m.
On Thu, 05 Apr 2012 19:02:23 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 05/04/2012 17:48, Igor Mitsyanko ha scritto:
> > New SD card "image" property can be used to:
> > - change image file attached to virtual SD card
> > - hot-insert new image file into newly initialized BlockDriverState (this was not
> > possible before).
> > 
> > Example usage:
> > ./qom-set /machine/milkymist/milkymist-memcard/card.image /home/me/mynewcard.img
> > this will attach image file /home/me/mynewcard.img to virtual SD card connected to
> > milkymist-memcard host controller device. If virtual card was already attached to
> > some other image file, eject event is triggered before attaching new file.
> > 
> > Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
> > ---
> >  hw/sd.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 44 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/sd.c b/hw/sd.c
> > index 8ffaa17..3e75405 100644
> > --- a/hw/sd.c
> > +++ b/hw/sd.c
> > @@ -1816,6 +1816,48 @@ static void sd_set_spimode(Object *obj, Visitor *v, void *opaque,
> >      }
> >  }
> >  
> > +static void sd_set_image_path(Object *obj, Visitor *v, void *opaque,
> > +                         const char *name, Error **errp)
> > +{
> > +    SDState *sd = SD_CARD(obj);
> > +    char *new_image;
> > +
> > +    visit_type_str(v, &new_image, "file", errp);
> 
> Please use the name argument instead of "file".
> 
> > +
> > +    if (error_is_set(errp)) {
> > +        return;
> > +    }
> > +
> > +    if (sd->bdrv) {
> > +        qmp_change_blockdev(bdrv_get_device_name(sd->bdrv), new_image,
> > +                false, NULL, errp);
> > +    } else {
> > +        DriveInfo *di;
> > +        QemuOpts *opts = drive_add(IF_SD, sd->if_idx, new_image, "");
> 
> I think this should simply take a drive name and pass it to
> bdrv_get_device_name.  The drive_add/drive_init should be done
> separately with the drive_add monitor command, like
> 
> drive_add 0 file=foo.img,if=none,id=bar
> 
> With the upcoming support for static properties in objects that are not
> devices, you can just add a drive property to the SD class.
> 
> There is a problem here however.  QOM commands are HMP only, and
> drive_add is QMP only.

I think you meant QOM commands are QMP only, and drive_add is HMP only?

>  I think that blocking drive_add in QMP is
> perfect being the enemy of good.  Alternatively, however, adding the QOM
> property commands to HMP would also make sense.

We've agreed on converting all existing HMP-only commands, such as drive_add,
to QMP as they exist today. No problem adding drive_add to QMP then.
Mitsyanko Igor - April 9, 2012, 2:30 p.m.
On 04/05/2012 09:02 PM, Paolo Bonzini wrote:
> Il 05/04/2012 17:48, Igor Mitsyanko ha scritto:
>> New SD card "image" property can be used to:
>> - change image file attached to virtual SD card
>> - hot-insert new image file into newly initialized BlockDriverState (this was not
>> possible before).
>>
>> Example usage:
>> ./qom-set /machine/milkymist/milkymist-memcard/card.image /home/me/mynewcard.img
>> this will attach image file /home/me/mynewcard.img to virtual SD card connected to
>> milkymist-memcard host controller device. If virtual card was already attached to
>> some other image file, eject event is triggered before attaching new file.
>>
>> Signed-off-by: Igor Mitsyanko<i.mitsyanko@samsung.com>
>> ---
>>   hw/sd.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 44 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/sd.c b/hw/sd.c
>> index 8ffaa17..3e75405 100644
>> --- a/hw/sd.c
>> +++ b/hw/sd.c
>> @@ -1816,6 +1816,48 @@ static void sd_set_spimode(Object *obj, Visitor *v, void *opaque,
>>       }
>>   }
>>
>> +static void sd_set_image_path(Object *obj, Visitor *v, void *opaque,
>> +                         const char *name, Error **errp)
>> +{
>> +    SDState *sd = SD_CARD(obj);
>> +    char *new_image;
>> +
>> +    visit_type_str(v,&new_image, "file", errp);
> Please use the name argument instead of "file".
>
OK
>> +
>> +    if (error_is_set(errp)) {
>> +        return;
>> +    }
>> +
>> +    if (sd->bdrv) {
>> +        qmp_change_blockdev(bdrv_get_device_name(sd->bdrv), new_image,
>> +                false, NULL, errp);
>> +    } else {
>> +        DriveInfo *di;
>> +        QemuOpts *opts = drive_add(IF_SD, sd->if_idx, new_image, "");
> I think this should simply take a drive name and pass it to
> bdrv_get_device_name.
Did you mean bdrv_find()? I can't use it here, BlockDriverState doesn't 
exist yet, I need to create
it first with drive_add.

>   The drive_add/drive_init should be done
> separately with the drive_add monitor command, like
>
> drive_add 0 file=foo.img,if=none,id=bar
>
So you're saying we need to use two commands to insert a new virtual SD 
card:
  first we need to create a BlockDriverState with specified device_name
  and then with second command we associate SD card with previously created
BlockDriverState?
I kind of like another approach, when we can just set filename as SD 
card's property and
BlockDriverState will be automatically created and connected to this SD 
card's state. And when we
want to eject card from virtual slot, we set SDCard::eject property to 
true and then associated
BlockDriverState is dereferenced and freed automatically.
> With the upcoming support for static properties in objects that are not
> devices, you can just add a drive property to the SD class.
>
> There is a problem here however.  QOM commands are HMP only, and
> drive_add is QMP only.  I think that blocking drive_add in QMP is
> perfect being the enemy of good.  Alternatively, however, adding the QOM
> property commands to HMP would also make sense.
I don't understand, is it some kind of conceptual restriction? We just 
call visit_type_str() here, it
could be called by object_property_set_str(), or from QMP socket, or 
from monitor for all we
care, what's the difference?
Paolo Bonzini - April 10, 2012, 6:33 a.m.
Il 09/04/2012 16:30, Igor Mitsyanko ha scritto:
>>> +
>>> +    if (error_is_set(errp)) {
>>> +        return;
>>> +    }
>>> +
>>> +    if (sd->bdrv) {
>>> +        qmp_change_blockdev(bdrv_get_device_name(sd->bdrv), new_image,
>>> +                false, NULL, errp);
>>> +    } else {
>>> +        DriveInfo *di;
>>> +        QemuOpts *opts = drive_add(IF_SD, sd->if_idx, new_image, "");
>> I think this should simply take a drive name and pass it to
>> bdrv_get_device_name.
> Did you mean bdrv_find()? I can't use it here, BlockDriverState doesn't
> exist yet, I need to create it first with drive_add.

Yes, that's the point. :)

>>   The drive_add/drive_init should be done
>> separately with the drive_add monitor command, like
>>
>> drive_add 0 file=foo.img,if=none,id=bar
>>
> So you're saying we need to use two commands to insert a new virtual SD
> card: first we need to create a BlockDriverState with specified device_name
> and then with second command we associate SD card with previously created
> BlockDriverState?
> I kind of like another approach, when we can just set filename as SD
> card's property and BlockDriverState will be automatically created and
> connected to this SD card's state.

But then you cannot set any property like the format, the caching mode,
etc.  Especially with the default of cache=writethrough it may be *very*
slow.

> And when we want to eject card from virtual slot, we set SDCard::eject
> property to true and then associated BlockDriverState is dereferenced
> and freed automatically.

That's possible, though I'm not sure why the normal eject/change
commands cannot work here.

>> There is a problem here however.  QOM commands are HMP only, and
>> drive_add is QMP only.  I think that blocking drive_add in QMP is
>> perfect being the enemy of good.  Alternatively, however, adding the QOM
>> property commands to HMP would also make sense.
> I don't understand, is it some kind of conceptual restriction?

No, it's just that nobody implemented them.

Paolo
Paolo Bonzini - April 10, 2012, 6:39 a.m.
Il 09/04/2012 14:42, Luiz Capitulino ha scritto:
>> > 
>> > There is a problem here however.  QOM commands are HMP only, and
>> > drive_add is QMP only.
> I think you meant QOM commands are QMP only, and drive_add is HMP only?

Yes.

>> >  I think that blocking drive_add in QMP is
>> > perfect being the enemy of good.  Alternatively, however, adding the QOM
>> > property commands to HMP would also make sense.
> We've agreed on converting all existing HMP-only commands, such as drive_add,
> to QMP as they exist today. No problem adding drive_add to QMP then.

Cool, thanks!

Paolo

Patch

diff --git a/hw/sd.c b/hw/sd.c
index 8ffaa17..3e75405 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -1816,6 +1816,48 @@  static void sd_set_spimode(Object *obj, Visitor *v, void *opaque,
     }
 }
 
+static void sd_set_image_path(Object *obj, Visitor *v, void *opaque,
+                         const char *name, Error **errp)
+{
+    SDState *sd = SD_CARD(obj);
+    char *new_image;
+
+    visit_type_str(v, &new_image, "file", errp);
+
+    if (error_is_set(errp)) {
+        return;
+    }
+
+    if (sd->bdrv) {
+        qmp_change_blockdev(bdrv_get_device_name(sd->bdrv), new_image,
+                false, NULL, errp);
+    } else {
+        DriveInfo *di;
+        QemuOpts *opts = drive_add(IF_SD, sd->if_idx, new_image, "");
+
+        if (!opts) {
+            error_set(errp, QERR_OPEN_FILE_FAILED, new_image);
+            return;
+        }
+
+        di = drive_init(opts, 0);
+        if (!di) {
+            error_set(errp, QERR_OPEN_FILE_FAILED, new_image);
+            return;
+        }
+
+        sd_reset(sd, di->bdrv);
+        if (bdrv_attach_dev(sd->bdrv, sd) < 0) {
+            drive_put_ref(di);
+            error_set(errp, QERR_OPEN_FILE_FAILED, new_image);
+            return;
+        }
+        bdrv_set_dev_ops(sd->bdrv, &sd_block_ops, sd);
+        qemu_set_irq(sd->inserted_cb, bdrv_is_inserted(sd->bdrv));
+        qemu_set_irq(sd->readonly_cb, sd->wp_switch);
+    }
+}
+
 static void sd_initfn(Object *obj)
 {
     SDState *sd = SD_CARD(obj);
@@ -1826,6 +1868,8 @@  static void sd_initfn(Object *obj)
             NULL, NULL, NULL);
     object_property_add(obj, "spi-mode", "boolean", sd_is_spi, sd_set_spimode,
             NULL, NULL, NULL);
+    object_property_add(OBJECT(sd), "image", "string",
+            NULL, sd_set_image_path, NULL, NULL, NULL);
 }
 
 static TypeInfo sd_type_info = {