Re: [Qemu-devel] [PATCHv2 7/8] pseries: Move rtc_offset into RTC device's state structure

[David Gibson]

On Tue, Dec 23, 2014 at 01:30:11AM +0100, Alexander Graf wrote:
> 
> 
> On 23.12.14 01:17, David Gibson wrote:
> > The initial creation of the PAPR RTC qdev class left a wart - the rtc's
> > offset was left in the sPAPREnvironment structure, accessed via a global.
> > 
> > This patch moves it into the RTC device's own state structure, were it
> > belongs.  This requires a small change to the migration stream format.  In
> > order to handle incoming streams from older versions, we also need to
> > retain the rtc_offset field in the sPAPREnvironment structure, so that it
> > can be loaded into via the vmsd, then pushed into the RTC device.
> > 
> > Since we're changing the migration format, this also takes the opportunity
> > to change the rtc offset from a value in seconds to a value in nanoseconds,
> > allowing nanosecond offsets between host and guest rtc time, if desired.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  hw/ppc/spapr.c         | 19 ++++++++++++++++++-
> >  hw/ppc/spapr_rtc.c     | 41 +++++++++++++++++++++++++++++++++++++----
> >  include/hw/ppc/spapr.h |  3 ++-
> >  3 files changed, 57 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 9722b42..3070be0 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -980,10 +980,27 @@ static int spapr_vga_init(PCIBus *pci_bus)
> >      }
> >  }
> >  
> > +static int spapr_post_load(void *opaque, int version_id)
> > +{
> > +    sPAPREnvironment *spapr = (sPAPREnvironment *)opaque;
> > +    int err = 0;
> > +
> > +    /* In earlier versions, there was no seperate qdev for the PAPR
> > +     * RTC, so the RTC offset was stored directly in sPAPREnvironment.
> > +     * So when migrating from those versions, poke the incoming offset
> > +     * value into the RTC device */
> > +    if (version_id < 3) {
> > +        err = spapr_rtc_import_offset(spapr->rtc, spapr->rtc_offset);
> 
> Do you think you could do this via a qom property set rather than an
> explicit function call into the rtc code base instead?

Hrm.  So, I could expose ns_offset as a property, but I'm not sure
it's a good idea.  Apart from this one instance to handle backwards
compat migration, it's a purely internal value - seeting it from the
command line is unlikely to have the desired effect, since it will get
overwritten by spapr_rtc_realize().

> 
> > +    }
> > +
> > +    return err;
> > +}
> > +
> >  static const VMStateDescription vmstate_spapr = {
> >      .name = "spapr",
> > -    .version_id = 2,
> > +    .version_id = 3,
> >      .minimum_version_id = 1,
> > +    .post_load = spapr_post_load,
> >      .fields = (VMStateField[]) {
> >          VMSTATE_UNUSED(4), /* used to be @next_irq */
> 
> Shouldn't we make the rtc_offset field conditional on version < 3 as
> well here? In fact, you could do the same for the legacy next_irq. That
> way we don't need to move data over the wire that doesn't get used anymore.

Uh, yeah, I guess we can do that.  It's just a bit awkward, because
the vmstatedescription includes built in handling for "earliest
version" but not "last version", so I'll need to write a custom test
function.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgpissz34lJIS.pgp
Description: PGP signature