[Qemu-devel] [PATCH 0/1] vmstatification of 0323 [for 2.8?]

[Qemu-devel] [PATCH 0/1] vmstatification of 0323 [for 2.8?]

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Hi,
  Chipping away at register_vmstate's I thought I'd tackle one of the
small devices;  does anyone care about the migration state of this?

  Note I've bumped the version number on it and haven't got anything
to test with;  Peter does the bump matter to you, and do you have
a test for it?

  I also intend to look at hw/dma/rc4030, hw/input/tsc2005, hw/input/tsc210x and
hw/sd/ssi-sd.c  that all look simple, although again I've got nothing to test
them with.

Dave


Dr. David Alan Gilbert (1):
  vmstateify ssd0323 display

 hw/display/ssd0323.c | 83 ++++++++++++++++++++--------------------------------
 1 file changed, 31 insertions(+), 52 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/1] vmstateify ssd0323 display

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

barely tested.

Note:
   a) Bumps version number because we now use the VMSTATE_SSI_SLAVE that
only uses a byte rather than a 32bit (for saving a bool 'cs').
   b) Some of the int's in it's struct should probably become int32_t.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/display/ssd0323.c | 83 ++++++++++++++++++++--------------------------------
 1 file changed, 31 insertions(+), 52 deletions(-)

diff --git a/hw/display/ssd0323.c b/hw/display/ssd0323.c
index 6d1faf4..013e915 100644
--- a/hw/display/ssd0323.c
+++ b/hw/display/ssd0323.c
@@ -48,9 +48,9 @@ typedef struct {
     SSISlave ssidev;
     QemuConsole *con;
 
-    int cmd_len;
+    uint32_t cmd_len;
     int cmd;
-    int cmd_data[8];
+    int32_t cmd_data[8];
     int row;
     int row_start;
     int row_end;
@@ -279,83 +279,63 @@ static void ssd0323_cd(void *opaque, int n, int level)
     s->mode = level ? SSD0323_DATA : SSD0323_CMD;
 }
 
-static void ssd0323_save(QEMUFile *f, void *opaque)
+static int ssd0323_post_load(void *opaque, int version_id)
 {
-    SSISlave *ss = SSI_SLAVE(opaque);
     ssd0323_state *s = (ssd0323_state *)opaque;
-    int i;
-
-    qemu_put_be32(f, s->cmd_len);
-    qemu_put_be32(f, s->cmd);
-    for (i = 0; i < 8; i++)
-        qemu_put_be32(f, s->cmd_data[i]);
-    qemu_put_be32(f, s->row);
-    qemu_put_be32(f, s->row_start);
-    qemu_put_be32(f, s->row_end);
-    qemu_put_be32(f, s->col);
-    qemu_put_be32(f, s->col_start);
-    qemu_put_be32(f, s->col_end);
-    qemu_put_be32(f, s->redraw);
-    qemu_put_be32(f, s->remap);
-    qemu_put_be32(f, s->mode);
-    qemu_put_buffer(f, s->framebuffer, sizeof(s->framebuffer));
-
-    qemu_put_be32(f, ss->cs);
-}
-
-static int ssd0323_load(QEMUFile *f, void *opaque, int version_id)
-{
-    SSISlave *ss = SSI_SLAVE(opaque);
-    ssd0323_state *s = (ssd0323_state *)opaque;
-    int i;
 
-    if (version_id != 1)
-        return -EINVAL;
-
-    s->cmd_len = qemu_get_be32(f);
-    if (s->cmd_len < 0 || s->cmd_len > ARRAY_SIZE(s->cmd_data)) {
+    if (s->cmd_len > ARRAY_SIZE(s->cmd_data)) {
         return -EINVAL;
     }
-    s->cmd = qemu_get_be32(f);
-    for (i = 0; i < 8; i++)
-        s->cmd_data[i] = qemu_get_be32(f);
-    s->row = qemu_get_be32(f);
     if (s->row < 0 || s->row >= 80) {
         return -EINVAL;
     }
-    s->row_start = qemu_get_be32(f);
     if (s->row_start < 0 || s->row_start >= 80) {
         return -EINVAL;
     }
-    s->row_end = qemu_get_be32(f);
     if (s->row_end < 0 || s->row_end >= 80) {
         return -EINVAL;
     }
-    s->col = qemu_get_be32(f);
     if (s->col < 0 || s->col >= 64) {
         return -EINVAL;
     }
-    s->col_start = qemu_get_be32(f);
     if (s->col_start < 0 || s->col_start >= 64) {
         return -EINVAL;
     }
-    s->col_end = qemu_get_be32(f);
     if (s->col_end < 0 || s->col_end >= 64) {
         return -EINVAL;
     }
-    s->redraw = qemu_get_be32(f);
-    s->remap = qemu_get_be32(f);
-    s->mode = qemu_get_be32(f);
     if (s->mode != SSD0323_CMD && s->mode != SSD0323_DATA) {
         return -EINVAL;
     }
-    qemu_get_buffer(f, s->framebuffer, sizeof(s->framebuffer));
-
-    ss->cs = qemu_get_be32(f);
 
     return 0;
 }
 
+static const VMStateDescription vmstate_ssd0323 = {
+    .name = "ssd0323_oled",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .post_load = ssd0323_post_load,
+    .fields      = (VMStateField []) {
+        VMSTATE_UINT32(cmd_len, ssd0323_state),
+        VMSTATE_INT32(cmd, ssd0323_state),
+        VMSTATE_INT32_ARRAY(cmd_data, ssd0323_state, 8),
+        VMSTATE_INT32(row, ssd0323_state),
+        VMSTATE_INT32(row_start, ssd0323_state),
+        VMSTATE_INT32(row_end, ssd0323_state),
+        VMSTATE_INT32(col, ssd0323_state),
+        VMSTATE_INT32(col_start, ssd0323_state),
+        VMSTATE_INT32(col_end, ssd0323_state),
+        VMSTATE_INT32(redraw, ssd0323_state),
+        VMSTATE_INT32(remap, ssd0323_state),
+        VMSTATE_UINT32(mode, ssd0323_state),
+        VMSTATE_BUFFER(framebuffer, ssd0323_state),
+        VMSTATE_SSI_SLAVE(ssidev, ssd0323_state),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const GraphicHwOps ssd0323_ops = {
     .invalidate  = ssd0323_invalidate_display,
     .gfx_update  = ssd0323_update_display,
@@ -372,18 +352,17 @@ static void ssd0323_realize(SSISlave *d, Error **errp)
     qemu_console_resize(s->con, 128 * MAGNIFY, 64 * MAGNIFY);
 
     qdev_init_gpio_in(dev, ssd0323_cd, 1);
-
-    register_savevm(dev, "ssd0323_oled", -1, 1,
-                    ssd0323_save, ssd0323_load, s);
 }
 
 static void ssd0323_class_init(ObjectClass *klass, void *data)
 {
+    DeviceClass *dc = DEVICE_CLASS(klass);
     SSISlaveClass *k = SSI_SLAVE_CLASS(klass);
 
     k->realize = ssd0323_realize;
     k->transfer = ssd0323_transfer;
     k->cs_polarity = SSI_CS_HIGH;
+    dc->vmsd = &vmstate_ssd0323;
 }
 
 static const TypeInfo ssd0323_info = {
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/1] vmstatification of 0323 [for 2.8?]

[Peter Maydell]

On 18 July 2016 at 12:07, Dr. David Alan Gilbert (git)
<dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Hi,
>   Chipping away at register_vmstate's I thought I'd tackle one of the
> small devices;  does anyone care about the migration state of this?
>
>   Note I've bumped the version number on it and haven't got anything
> to test with;  Peter does the bump matter to you, and do you have
> a test for it?

This is only used by the stellaris boards. I don't care about
migration compat on those, so version bump is fine.
http://people.linaro.org/~peter.maydell/stellaris.tgz is probably
a working test case for that board (64MB)

>   I also intend to look at hw/dma/rc4030, hw/input/tsc2005, hw/input/tsc210x and
> hw/sd/ssi-sd.c  that all look simple, although again I've got nothing to test
> them with.

http://people.linaro.org/~peter.maydell/n8x0-images.tgz (1.8GB)
should work as a test case for at least one of the tsc devices
(ignore all the chatter about read-only dma registers on stderr,
the image does boot.)

ssi-sd is used by stellaris. rc4030 is only in the mips jazz
board, and I don't have a test image for that.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/1] vmstatification of 0323 [for 2.8?]

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 18 July 2016 at 12:07, Dr. David Alan Gilbert (git)
> <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Hi,
> >   Chipping away at register_vmstate's I thought I'd tackle one of the
> > small devices;  does anyone care about the migration state of this?
> >
> >   Note I've bumped the version number on it and haven't got anything
> > to test with;  Peter does the bump matter to you, and do you have
> > a test for it?
> 
> This is only used by the stellaris boards. I don't care about
> migration compat on those, so version bump is fine.
> http://people.linaro.org/~peter.maydell/stellaris.tgz is probably
> a working test case for that board (64MB)

<snip/paste>

> ssi-sd is used by stellaris. rc4030 is only in the mips jazz
> board, and I don't have a test image for that.
> 

Thanks, the graphics version seems to work on that (the runme);
the sd card demo, isn't as happy:

~/try/arm-softmmu/qemu-system-arm -M lm3s6965evb -serial stdio -kernel  ~/arm/stellaris-test/sd_card.bin -drive file=~/arm/stellaris-test/sdcard.img,if=sd,format=raw


SD Card Example Program
Type 'help' for help.

/> ls
Open
Command returned error code FR_NOT_READY

Yep, and I've mailed the MIPS guys about the rc4030.

> >   I also intend to look at hw/dma/rc4030, hw/input/tsc2005, hw/input/tsc210x and
> > hw/sd/ssi-sd.c  that all look simple, although again I've got nothing to test
> > them with.
> 
> http://people.linaro.org/~peter.maydell/n8x0-images.tgz (1.8GB)
> should work as a test case for at least one of the tsc devices
> (ignore all the chatter about read-only dma registers on stderr,
> the image does boot.)

Thanks, I'll give that a go when I attack the tsc's.

Dave

> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/1] vmstatification of 0323 [for 2.8?]

[Philippe Mathieu-Daudé]

[-- Attachment #1: Type: text/plain, Size: 5469 bytes --]

On 07/18/2016 02:00 PM, Dr. David Alan Gilbert wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> On 18 July 2016 at 12:07, Dr. David Alan Gilbert (git)
>> <dgilbert@redhat.com> wrote:
>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>
>>> Hi,
>>>   Chipping away at register_vmstate's I thought I'd tackle one of the
>>> small devices;  does anyone care about the migration state of this?
>>>
>>>   Note I've bumped the version number on it and haven't got anything
>>> to test with;  Peter does the bump matter to you, and do you have
>>> a test for it?
>>
>> This is only used by the stellaris boards. I don't care about
>> migration compat on those, so version bump is fine.
>> http://people.linaro.org/~peter.maydell/stellaris.tgz is probably
>> a working test case for that board (64MB)

After about 4h reversing this this binary, and reading the SD spec:

    7.2.1 Mode Selection and Initialization (SPI mode)
    The SD Card is powered up in the SD mode. It will enter
    SPI mode if the CS signal is asserted (negative) during
    the reception of the reset command (CMD0). If the card
    recognizes that the SD mode is required it will not
    respond to the command and remain in the SD mode. If SPI
    mode is required, the card will switch to SPI and respond
    with the SPI mode R1 response.
    The only way to return to the SD mode is by entering the
    power cycle. In SPI mode, the SD Card protocol state
    machine in SD mode is not observed. All the SD Card
    commands supported in SPI mode are always available.
    [...]
    If the card indicates an illegal command, the card is
    legacy and does not support CMD8. If the card supports
    CMD8 and can operate on the supplied voltage, the response
    echoes back the supply voltage and the check pattern that
    were set in the command argument.

I realized the binary expect CMD8 to not return, to use the SD card
with Spec v1.x. Which is the very patch Paul Brook attached...
http://lists.nongnu.org/archive/html/qemu-devel/2012-04/msg03790.html

curl
http://lists.nongnu.org/archive/html/qemu-devel/2012-04/txtwo8B9rbn9w.txt

     case 8:	/* CMD8:   SEND_IF_COND */
+        if (sd->spi) {
+            goto bad_cmd;
+        }
         /* Physical Layer Specification Version 2.00 command */
         switch (sd->state) {
         case sd_idle_state:

> 
> <snip/paste>
> 
>> ssi-sd is used by stellaris. rc4030 is only in the mips jazz
>> board, and I don't have a test image for that.
>>
> 
> Thanks, the graphics version seems to work on that (the runme);
> the sd card demo, isn't as happy:
> 
> ~/try/arm-softmmu/qemu-system-arm -M lm3s6965evb -serial stdio -kernel  ~/arm/stellaris-test/sd_card.bin -drive file=~/arm/stellaris-test/sdcard.img,if=sd,format=raw
> 
> 
> SD Card Example Program
> Type 'help' for help.
> 
> /> ls
> Open
> Command returned error code FR_NOT_READY

I tried to bisect this trying every stable releases until v0.11.1 (no
need to share the details I went to...). I quit at v0.9.1 when I got:

    WARNING: "gcc-8" looks like gcc 4.x
    QEMU is known to have problems when compiled with gcc 4.x
    It is recommended that you use gcc 3.x to build QEMU
    To use this compiler anyway, configure with --disable-gcc-check

With the previous 3-lines patch applied (which never hit master, while
8120e7144b4 did...) it works:

$ qemu-system-arm -M lm3s6965evb -serial stdio \
  -kernel sd_card.bin -sd sdcard.img

SD Card Example Program
Type 'help' for help.

/> ls
Open
listing

----A 2012/04/25 17:44        12  README.TXT

   1 File(s),        12 bytes total
   0 Dir(s),      61182K bytes free

/> cat README.TXT
Hello World

$ git rev-list --oneline 8120e7144b4007d26531835c7b6bc2b40a736cd2~..
hw/sd/ssi-sd.c
c3abd91309 hw/sd/ssi-sd: use the SDBus API, connect the SDCard to the bus
8046d44f3c hw/sd/ssi-sd: Reset SD card on controller reset
2ccfd336dc vmstateify ssi-sd
7673bb4cd3 ssi: change ssi_slave_init to be a realize ops
0430891ce1 hw: Clean up includes
8fd06719e7 ssi: Move ssi.h into a separate directory
e3382ef0ea sd.h: Move sd.h to include/hw/sd/
af9e40aa8f hw: Mark devices picking up block backends actively FIXME
4be746345f hw: Convert from BlockDriverState to BlockBackend, mostly
fa1d36df74 block: Eliminate DriveInfo member bdrv, use blk_by_legacy_dinfo()
a9c380db3b ssi-sd: fix buffer overrun on invalid state load
1a7d9ee6dd ssi: Convert legacy SSI_SLAVE -> DEVICE casts
4f8a066b5f blockdev: Remove IF_* check for read-only blockdev_init
6790f59da3 sd: pass bool parameter for sd_init
49ab747f66 hw: move target-independent files to subdirectories

At least 15 commits in 6 years to maintain a non working device :|

> 
> Yep, and I've mailed the MIPS guys about the rc4030.
> 
>>>   I also intend to look at hw/dma/rc4030, hw/input/tsc2005, hw/input/tsc210x and
>>> hw/sd/ssi-sd.c  that all look simple, although again I've got nothing to test
>>> them with.
>>
>> http://people.linaro.org/~peter.maydell/n8x0-images.tgz (1.8GB)
>> should work as a test case for at least one of the tsc devices
>> (ignore all the chatter about read-only dma registers on stderr,
>> the image does boot.)
> 
> Thanks, I'll give that a go when I attack the tsc's.
> 
> Dave
> 
>> thanks
>> -- PMM
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 0/1] vmstatification of 0323 [for 2.8?]

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 656 bytes --]

On 02/06/2018 07:09, Philippe Mathieu-Daudé wrote:
> 
> I realized the binary expect CMD8 to not return, to use the SD card
> with Spec v1.x. Which is the very patch Paul Brook attached...
> http://lists.nongnu.org/archive/html/qemu-devel/2012-04/msg03790.html
> 
> curl
> http://lists.nongnu.org/archive/html/qemu-devel/2012-04/txtwo8B9rbn9w.txt
> 
>      case 8:	/* CMD8:   SEND_IF_COND */
> +        if (sd->spi) {
> +            goto bad_cmd;
> +        }
>          /* Physical Layer Specification Version 2.00 command */
>          switch (sd->state) {
>          case sd_idle_state:

Can you send it with SoB?  Thanks!

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 0/1] vmstatification of 0323 [for 2.8?]

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 18 July 2016 at 12:07, Dr. David Alan Gilbert (git)
> <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Hi,
> >   Chipping away at register_vmstate's I thought I'd tackle one of the
> > small devices;  does anyone care about the migration state of this?
> >
> >   Note I've bumped the version number on it and haven't got anything
> > to test with;  Peter does the bump matter to you, and do you have
> > a test for it?
> 
> This is only used by the stellaris boards. I don't care about
> migration compat on those, so version bump is fine.
> http://people.linaro.org/~peter.maydell/stellaris.tgz is probably
> a working test case for that board (64MB)
> 
> >   I also intend to look at hw/dma/rc4030, hw/input/tsc2005, hw/input/tsc210x and
> > hw/sd/ssi-sd.c  that all look simple, although again I've got nothing to test
> > them with.
> 
> http://people.linaro.org/~peter.maydell/n8x0-images.tgz (1.8GB)
> should work as a test case for at least one of the tsc devices
> (ignore all the chatter about read-only dma registers on stderr,
> the image does boot.)

Hmm the n810 isn't happy for me in many ways:
    a) It boots up to a desktop like thing with a couple of colourful
       characters and a few icons but I can't interact with it at all.
    b) Migration with current upstream fails with a migration format error after
       the sd card on load - it goes if I revert dd26eb43337adf53d22b3fda3591e3837bc08b8c
       - my suspicion is that wpgrps_size isn't being initialised by the time
       of the load; but I've not dug into it.
    c) Even if I revert the commit in (b) reloading from a migration image
       gets me a blank white screen in paused state.

So I think that model is pretty broken anyway.

Dave

> 
> ssi-sd is used by stellaris. rc4030 is only in the mips jazz
> board, and I don't have a test image for that.
> 
> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/1] vmstatification of 0323 [for 2.8?]

[Peter Maydell]

On 19 July 2016 at 18:51, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> Hmm the n810 isn't happy for me in many ways:
>     a) It boots up to a desktop like thing with a couple of colourful
>        characters and a few icons but I can't interact with it at all.
>     b) Migration with current upstream fails with a migration format error after
>        the sd card on load - it goes if I revert dd26eb43337adf53d22b3fda3591e3837bc08b8c
>        - my suspicion is that wpgrps_size isn't being initialised by the time
>        of the load; but I've not dug into it.
>     c) Even if I revert the commit in (b) reloading from a migration image
>        gets me a blank white screen in paused state.
>
> So I think that model is pretty broken anyway.

Well, it's maybe not great but it demonstrates that it's
not totally busted... we should probably try to fix b and c
at least I guess.

thanks
-- PMM