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

[Alexander Graf]

> Am 23.12.2014 um 04:56 schrieb David Gibson <address@hidden>:
> 
>> 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().

It's not about setting it on the command line, 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.

If you can show me that the code looks cleaner with native C exported 
functions, I could be persuaded otherwise though.

> 
>> 
>>> +    }
>>> +
>>> +    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.

Yeah, though I wouldn't call it awkward ;).

Alex