qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2] hw/mips: gt64xxx_pci: Add VMStateDescription


From: Aurelien Jarno
Subject: Re: [Qemu-devel] [PATCH v2] hw/mips: gt64xxx_pci: Add VMStateDescription
Date: Thu, 19 Jun 2014 17:51:18 +0200
User-agent: Mutt/1.5.21 (2010-09-15)

On Thu, Jun 19, 2014 at 04:46:03PM +0100, James Hogan wrote:
> On 19/06/14 16:25, Aurelien Jarno wrote:
> > On Wed, Jun 18, 2014 at 12:10:02AM +0100, James Hogan wrote:
> >> From: Sanjay Lal <address@hidden>
> >>
> >> Add VMStateDescription for GT64120 PCI emulation used by the Malta
> >> platform, to allow it to work with savevm/loadvm and live migration.
> >>
> >> Signed-off-by: Sanjay Lal <address@hidden>
> >> address@hidden: Convert to VMState]
> >> Signed-off-by: James Hogan <address@hidden>
> >> Cc: Aurelien Jarno <address@hidden>
> >> ---
> >> This is based on "[Patch 03/12] KVM/MIPS: Add save/restore state APIs
> >> for saving/restoring KVM guests."[1].
> >>
> >> Changes in v2:
> >>  - Expand commit message
> >>  - Convert to VMState (Peter Maydell)
> >>
> >> [1] https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg00195.html
> >> ---
> >>  hw/mips/gt64xxx_pci.c | 152 
> >> ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 152 insertions(+)
> >>
> >> diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c
> >> index 6398514c99d2..76690ecd6ca4 100644
> >> --- a/hw/mips/gt64xxx_pci.c
> >> +++ b/hw/mips/gt64xxx_pci.c
> >> @@ -312,6 +312,156 @@ static void gt64120_pci_mapping(GT64120State *s)
> >>      }
> >>  }
> >>  
> >> +static int gt64120_post_load(void *opaque, int version_id)
> >> +{
> >> +    GT64120State *s = opaque;
> >> +
> >> +    gt64120_isd_mapping(s);
> >> +    gt64120_pci_mapping(s);
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static const VMStateDescription vmstate_gt64120 = {
> >> +    .name = "gt64120",
> >> +    .version_id = 1,
> >> +    .minimum_version_id = 1,
> >> +    .post_load = gt64120_post_load,
> >> +    .fields = (VMStateField[]) {
> >> +        /* CPU Configuration */
> >> +        VMSTATE_UINT32(regs[GT_CPU], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_MULTI], GT64120State),
> >> +        /* CPU Address decode */
> >> +        VMSTATE_UINT32(regs[GT_SCS10LD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SCS10HD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SCS32LD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SCS32HD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CS20LD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CS20HD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CS3BOOTLD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CS3BOOTHD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0IOLD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0IOHD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0M0LD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0M0HD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_ISD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0M1LD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0M1HD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1IOLD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1IOHD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1M0LD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1M0HD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1M1LD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1M1HD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SCS10AR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SCS32AR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CS20R], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CS3BOOTR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0IOREMAP], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0M0REMAP], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0M1REMAP], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1IOREMAP], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1M0REMAP], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1M1REMAP], GT64120State),
> >> +        /* CPU Error Report */
> >> +        VMSTATE_UINT32(regs[GT_CPUERR_ADDRLO], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CPUERR_ADDRHI], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CPUERR_DATALO], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CPUERR_DATAHI], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CPUERR_PARITY], GT64120State),
> >> +        /* CPU Sync Barrier */
> >> +        VMSTATE_UINT32(regs[GT_PCI0SYNC], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1SYNC], GT64120State),
> >> +        /* SDRAM and Device Address Decode */
> >> +        VMSTATE_UINT32(regs[GT_SCS0LD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SCS0HD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SCS1LD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SCS1HD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SCS2LD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SCS2HD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SCS3LD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SCS3HD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CS0LD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CS0HD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CS1LD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CS1HD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CS2LD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CS2HD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CS3LD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_CS3HD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_BOOTLD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_BOOTHD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_ADERR], GT64120State),
> >> +        /* SDRAM Configuration */
> >> +        VMSTATE_UINT32(regs[GT_SDRAM_CFG], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SDRAM_OPMODE], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SDRAM_BM], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SDRAM_ADDRDECODE], GT64120State),
> >> +        /* SDRAM Parameters */
> >> +        VMSTATE_UINT32(regs[GT_SDRAM_B0], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SDRAM_B1], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SDRAM_B2], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_SDRAM_B3], GT64120State),
> >> +        /* ECC */
> >> +        VMSTATE_UINT32(regs[GT_ECC_ERRDATALO], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_ECC_ERRDATAHI], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_ECC_MEM], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_ECC_CALC], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_ECC_ERRADDR], GT64120State),
> >> +        /* Device Parameters */
> >> +        VMSTATE_UINT32(regs[GT_DEV_B0], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_DEV_B1], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_DEV_B2], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_DEV_B3], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_DEV_BOOT], GT64120State),
> >> +        /* DMA registers are all zeroed at reset */
> >> +        /* Timer/Counter */
> >> +        VMSTATE_UINT32(regs[GT_TC0], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_TC1], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_TC2], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_TC3], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_TC_CONTROL], GT64120State),
> >> +        /* PCI Internal */
> >> +        VMSTATE_UINT32(regs[GT_PCI0_CMD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_CMD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_TOR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_BS_SCS10], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_BS_SCS32], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_BS_CS20], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_BS_CS3BT], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_IACK], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_IACK], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_BARE], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_PREFMBR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_SCS10_BAR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_SCS32_BAR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_CS20_BAR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_CS3BT_BAR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_SSCS10_BAR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_SSCS32_BAR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_SCS3BT_BAR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_CMD], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_TOR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_BS_SCS10], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_BS_SCS32], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_BS_CS20], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_BS_CS3BT], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_BARE], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_PREFMBR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_SCS10_BAR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_SCS32_BAR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_CS20_BAR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_CS3BT_BAR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_SSCS10_BAR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_SSCS32_BAR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_SCS3BT_BAR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_CFGADDR], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI1_CFGDATA], GT64120State),
> >> +        VMSTATE_UINT32(regs[GT_PCI0_CFGADDR], GT64120State),
> >> +        VMSTATE_END_OF_LIST()
> >> +    }
> >> +};
> >> +
> > 
> > Disclaimer: my knowledge of VMSTATE is quite limited.
> > 
> > This looks like we are basically saving all registers (which is
> > correct), so I do wonder if we shouldn't use a VMSTATE_ARRAY function,
> > for example VMSTATE_VARRAY_UINT32.
> 
> Yeh, I wondered that too as the device I copied did that for some parts.
> However it appeared that the GT64xxx register address space is fairly
> sparse. It only saves 118 of the 154 registers #defined, out of an array
> of GT_REGS=1024 elements) so I went for a 1:1 conversion of the original
> patch. TBH I haven't looked deeply enough yet to know whether the other
> registers should be saved.
> 
> Should be easy enough to convert to save the whole array if you think
> that'd be better/safer though.

If we latter realize we need to save another register (because we forgot
one, or because we emulate one more functionality), we have to change
the format and care about compatibility. Saving the whole set of
registers is better from that point of view. It means using extra
memory, but I doubt that 4kB is that much.

That said there might be some drawbacks in doing that, and I don't
really have enough experience to judge about that. It would be nice if
some more people can give their opinion there.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net



reply via email to

[Prev in Thread] Current Thread [Next in Thread]