Re: [Qemu-devel] hw/arm/virt: vmstate-static-checker.py results

[Dr. David Alan Gilbert]

* Peter Maydell (address@hidden) wrote:
> On 18 August 2016 at 15:00, Andrew Jones <address@hidden> wrote:
> > We've recently started versioning mach-virt, v2.6 was the first versioned
> > release. As an effort to try and make sure we're doing things right, I
> > tried the vmstate-static-checker.py script. I compared a 2.6 machine
> > from a QEMU built from the v2.6.0 tag with a 2.6 machine from a QEMU
> > built from today's latest pull (5844365fe8). I see lots of errors. I have
> > no experience in this area, so I can't even state whether they're truly
> > a concern or not. I can say a few things;
> >
> >  1) Most of the errors look like the same problem. Something is wrong
> >     with xilinx_spi state, which shows up everywhere. Here's an example
> >
> > Section "en25q64", Description "xilinx_spi": expected field 
> > "nonvolatile_cfg", got "cur_addr"; skipping rest
> 
> Well, something here is weird, because en25q64 and nonvolatile_cfg
> aren't part of xilinx_spi at all, they're in hw/block/m25p80.c.

Hmm, except there are two separate things with the name "xilinx_spi";
     vmstate_xilinx_spi in hw/ssi/xilinx_spi.c
which is the state for the "xlnx.xps-spi" (aka TYPE_XILINX_SPI) object.

and for added confusion:
      vmstate_m25p80 in hw/block/m25p80.c
which is the state for the "m25p80-generic" (aka TYPE_M25P80) object.
     also calls itself "xilinx_spi".

These went in a pair of Peter Crosthwaite commits at about the same time 4.5 
years
ago; I'm guessing it was just a copy-paste.

I think my preference would be to update the name for the m25p80 so it's
not got the clash; but it seems m25p80 contains definitions of about a zillion
flash devices all derived from the m25p80, so I think I'd have to try one of
them to see if the xilinx_spi name finds it's way onto the migration stream;
I suspect it doesn't.

Dave

> However we don't care about migration compatibility in the Xilinx
> boards at all, so the simple fix is just not to try to test them.
> Similarly, aspeed and imx are boards where we're not trying to
> preserve migration compat.
> 
> >  2) Several of the remaining problems are also present on a check of the
> >     x86_64 pc-i440fx-2.6 machine type. To be precise
> >
> > Section "am53c974", Description "esp": expected field "cmdlen", got 
> > "cmdbuf"; skipping rest
> > Section "dc390", Description "esp": expected field "cmdlen", got "cmdbuf"; 
> > skipping rest
> > Section "e1000-82544gc", Description "e1000": expected field "tx.ipcss", 
> > got "tx.props.ipcss"; skipping rest
> > Section "e1000-82545em", Description "e1000": expected field "tx.ipcss", 
> > got "tx.props.ipcss"; skipping rest
> > Section "e1000", Description "e1000": expected field "tx.ipcss", got 
> > "tx.props.ipcss"; skipping rest
> > Section "esp", Description "esp": expected field "cmdlen", got "cmdbuf"; 
> > skipping rest
> > Section "rtl8139", Description "rtl8139": expected field "tally_counters", 
> > got "tally_counters.TxOk"; skipping rest
> 
> Looking at just the e1000 for an example, this is a false positive
> in your checker. In commit 093454e2 the struct we're putting the
> ipcss/ipcso/etc fields was moved, so:
> 
> -        VMSTATE_UINT8(tx.ipcss, E1000State),
> -        VMSTATE_UINT8(tx.ipcso, E1000State),
> -        VMSTATE_UINT16(tx.ipcse, E1000State),
> -        VMSTATE_UINT8(tx.tucss, E1000State),
> -        VMSTATE_UINT8(tx.tucso, E1000State),
> -        VMSTATE_UINT16(tx.tucse, E1000State),
> -        VMSTATE_UINT32(tx.paylen, E1000State),
> -        VMSTATE_UINT8(tx.hdr_len, E1000State),
> -        VMSTATE_UINT16(tx.mss, E1000State),
> +        VMSTATE_UINT8(tx.props.ipcss, E1000State),
> +        VMSTATE_UINT8(tx.props.ipcso, E1000State),
> +        VMSTATE_UINT16(tx.props.ipcse, E1000State),
> +        VMSTATE_UINT8(tx.props.tucss, E1000State),
> +        VMSTATE_UINT8(tx.props.tucso, E1000State),
> +        VMSTATE_UINT16(tx.props.tucse, E1000State),
> +        VMSTATE_UINT32(tx.props.paylen, E1000State),
> +        VMSTATE_UINT8(tx.props.hdr_len, E1000State),
> +        VMSTATE_UINT16(tx.props.mss, E1000State),
> 
> but the on-the-wire format doesn't include the names of the C struct
> fields so this isn't a migration break.
> 
> >     x86 only has three additional messages, which look harmless to me
> >
> > Section "apic-common" does not exist in dest
> > Section "apic" does not exist in dest
> > Section "kvm-apic" does not exist in dest
> >
> >  3) I analyzed one error I saw, and see it should be fine, as the device
> >     simply went from unmigratable to migratable (for TCG anyway)
> >
> > Section "arm-gicv3-common" Section "arm-gicv3-common" Description 
> > "arm_gicv3": minimum version error: 0 < 1
> 
> Yep, that should be fine.
> 
> thanks
> -- PMM
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK