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

[Alexander Graf]

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?

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


Alex