Re: [Qemu-devel] [PATCH v2] hpet: recover timer offset correctly

[Pavel Dovgalyuk]

> -----Original Message-----
> From: Juan Quintela [mailto:address@hidden
> Sent: Wednesday, January 10, 2018 12:51 PM
> To: Pavel Dovgalyuk
> Cc: 'Pavel Dovgalyuk'; address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden
> Subject: Re: [PATCH v2] hpet: recover timer offset correctly
> 
> "Pavel Dovgalyuk" <address@hidden> wrote:
> >> From: Juan Quintela [mailto:address@hidden
> >> "Pavel Dovgalyuk" <address@hidden> wrote:
> >> >> From: Juan Quintela [mailto:address@hidden
> >> If you *don't* use a needed function then please just increase the
> >> version.  You are just breaking compatibility anyways.  The whole point
> >> of subsections is that they are optional.  If they are mandatory (this
> >> case), then they bring no advantage at all.
> >
> > Thanks, I thought that the sections are skipped automatically when
> > there is no code for loading
> > them.
> >
> >> What dave is asked for your previous version is that you disable the
> >> section for old machine types.  Look at how to use DEFINE_PROP_* for
> >> this use case.
> >
> > How do you like this one?
> 
> Much better, thanks.
> 
> > +static bool hpet_offset_needed(void *opaque)
> > +{
> > +    HPETState *s = opaque;
> > +
> > +    return s->hpet_offset_saved;
> > +}
> > +
> 
> If this is only one optimization, this test is ok.  If it makes things
> go worse, you can add something there like && hpet_enabled() or
> whatever.  Remember that I don't understand HPET.

Right. Please check the new version of the patch.

> > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > index 263de97..8897302 100644
> > --- a/include/hw/compat.h
> > +++ b/include/hw/compat.h
> > @@ -13,6 +13,10 @@
> >          .driver   = "virtio-tablet-device",\
> >          .property = "wheel-axis",\
> >          .value    = "false",\
> > +    },{\
> > +        .driver   = "hpet",\
> > +        .property = "hpet-offset-saved",\
> > +        .value    = "off",\
> >      },
> >
> >  #define HW_COMPAT_2_9 \
> 
> This should be on 2_11 not 2_10 O:-)
> 
> But for the vmstate bits:
> 
> Reviewed-by: Juan Quintela <address@hidden>

Thanks.

Pavel Dovgalyuk