On Tue, Dec 23, 2014 at 07:41:22AM +0100, Alexander Graf wrote:
>
>
>
> > Am 23.12.2014 um 04:56 schrieb David Gibson <da...@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 <da...@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
pgpRSwZvqJxWC.pgp
Description: PGP signature
