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

[David Gibson <david@gibson.dropbear.id.au>]

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

On Tue, Dec 23, 2014 at 07:41:22AM +0100, Alexander Graf wrote:
> 
> 
> 
> > Am 23.12.2014 um 04:56 schrieb David Gibson <david@gibson.dropbear.id.au>:
> > 
> >> 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 <david@gibson.dropbear.id.au>
> >>> ---
> >>> 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().
> 
> It's not about setting it on the command line,

I realise that, but as I understand it, putting the qom property there
inevitably means that it *could* be set from the command line, and
that doesn't sound like a good thing to me.

> it's a question on
> how we separate our interfaces. The way you handle it right now we
> need to make the RTAS code link with the RTC code. With QOM
> properties this would happen with a by-name convention both sides
> use.

Well, yes, and my point is that that there *should not* be a general
interface to set the offset, because setting the offset never makes
sense, except for this backwards compatibility shim.

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]