[Qemu-devel] [PATCH]Fix a error in mc146818rtc.c

[Qemu-devel] [PATCH]Fix a error in mc146818rtc.c

[Lb peace]

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

If you use hwclock in guest os ,you will find the result of hwclock isn't
changed after changing host os's clock.
I find this issue is generated in this patch:

http://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03353.html
Before this patch,the result will be changed if you change host's clock.
It makes use of the following codes in qemu-timer.c:
        if (now < last) {
            notifier_list_notify(&clock->reset_notifiers, &now);
        }
It is useless if you register a QEMU_CLOCK_REALTIME's clock_reset_notifier,
---
 hw/timer/mc146818rtc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index df54546..821c27e 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -879,7 +879,7 @@ static void rtc_realizefn(DeviceState *dev, Error
**errp)
     check_update_timer(s);

     s->clock_reset_notifier.notify = rtc_notify_clock_reset;
-    qemu_clock_register_reset_notifier(QEMU_CLOCK_REALTIME,
+    qemu_clock_register_reset_notifier(rtc_clock,
                                        &s->clock_reset_notifier);

     s->suspend_notifier.notify = rtc_notify_suspend;
--

[-- Attachment #2: Type: text/html, Size: 1707 bytes --]

Re: [Qemu-devel] [PATCH]Fix a error in mc146818rtc.c

[Alex Bligh]

On 30 Jun 2014, at 20:53, Lb peace <peaceustc@gmail.com> wrote:

> If you use hwclock in guest os ,you will find the result of hwclock isn't changed after changing host os's clock.
> I find this issue is generated in this patch:

I find it hard to believe that the patch you mention is the problem,
as it's an automated output of perl simply changing one calling
convention to another.

Is this because you are using an unusual clock= option?

Alex



> http://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03353.html
> Before this patch,the result will be changed if you change host's clock.
> It makes use of the following codes in qemu-timer.c:
>         if (now < last) {
>             notifier_list_notify(&clock->reset_notifiers, &now);
>         }
> It is useless if you register a QEMU_CLOCK_REALTIME's clock_reset_notifier,
> ---
>  hw/timer/mc146818rtc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index df54546..821c27e 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -879,7 +879,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>      check_update_timer(s);
>  
>      s->clock_reset_notifier.notify = rtc_notify_clock_reset;
> -    qemu_clock_register_reset_notifier(QEMU_CLOCK_REALTIME,
> +    qemu_clock_register_reset_notifier(rtc_clock,
>                                         &s->clock_reset_notifier);
>  
>      s->suspend_notifier.notify = rtc_notify_suspend;
> -- 
> 

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH]Fix a error in mc146818rtc.c

[Lb peace]

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

1)“ it's an automated output of perl simply changing one calling convention
to another”.What do you mean... I
can not find the point :)
2) The problem is :
      a.use QEMU to run linux-0.2.img with command qemu-system-i386
linux-0.2.img or sth. like that
      b.hwclock ,and we get a date1(2014-07-01 12:00,e.g.)
      c.change the clock of host before date1(2014-04-01 12:00),called date2
      d.hwclock,and we get another date3(2014-07-01,12:01)
      e. before that patch date3 is synchronized with host,so it changed to
a time   date3>date2 && date3<date1
          after that patch  date3 is not synchronized with host.It has its
own clock,so date3 >date1
3)why we set a reset_notifiers of QEMU_CLOCK_REALTIME? Any comment to that?
A QEMU_CLOCK_REALTIME clock will
 not call its  reset_notifiers when the time is adjusted to a time-point
before.The reset_notifiers is useless for this type of QEMUClock.




2014-07-01 4:08 GMT+08:00 Alex Bligh <alex@alex.org.uk>:

>
>
> On 30 Jun 2014, at 20:53, Lb peace <peaceustc@gmail.com> wrote:
>
> > If you use hwclock in guest os ,you will find the result of hwclock
> isn't changed after changing host os's clock.
> > I find this issue is generated in this patch:
>
> I find it hard to believe that the patch you mention is the problem,
> as it's an automated output of perl simply changing one calling
> convention to another.
>
> Is this because you are using an unusual clock= option?
>
> Alex
>
>
>
> > http://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03353.html
> > Before this patch,the result will be changed if you change host's clock.
> > It makes use of the following codes in qemu-timer.c:
> >         if (now < last) {
> >             notifier_list_notify(&clock->reset_notifiers, &now);
> >         }
> > It is useless if you register a QEMU_CLOCK_REALTIME's
> clock_reset_notifier,
> > ---
> >  hw/timer/mc146818rtc.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> > index df54546..821c27e 100644
> > --- a/hw/timer/mc146818rtc.c
> > +++ b/hw/timer/mc146818rtc.c
> > @@ -879,7 +879,7 @@ static void rtc_realizefn(DeviceState *dev, Error
> **errp)
> >      check_update_timer(s);
> >
> >      s->clock_reset_notifier.notify = rtc_notify_clock_reset;
> > -    qemu_clock_register_reset_notifier(QEMU_CLOCK_REALTIME,
> > +    qemu_clock_register_reset_notifier(rtc_clock,
> >                                         &s->clock_reset_notifier);
> >
> >      s->suspend_notifier.notify = rtc_notify_suspend;
> > --
> >
>
> --
> Alex Bligh
>
>
>
>
>

[-- Attachment #2: Type: text/html, Size: 5259 bytes --]

Re: [Qemu-devel] [PATCH]Fix a error in mc146818rtc.c

[Alex Bligh]

On 1 Jul 2014, at 02:12, Lb peace <peaceustc@gmail.com> wrote:

> 1)“ it's an automated output of perl simply changing one calling convention to another”.What do you mean... I 
> can not find the point :)

I'd thought this was the huge patch which was generated by
perl to change the API, but it's not.

> 2) The problem is :
>       a.use QEMU to run linux-0.2.img with command qemu-system-i386 linux-0.2.img or sth. like that
>       b.hwclock ,and we get a date1(2014-07-01 12:00,e.g.)
>       c.change the clock of host before date1(2014-04-01 12:00),called date2
>       d.hwclock,and we get another date3(2014-07-01,12:01)
>       e. before that patch date3 is synchronized with host,so it changed to a time   date3>date2 && date3<date1
>           after that patch  date3 is not synchronized with host.It has its own clock,so date3 >date1

OK.

Your patch changes QEMU_CLOCK_REALTIME (a clock type) to rtc_clock (an instance of a clock type)

rtc_clock is set up in vl.c as follows:

    rtc_clock = QEMU_CLOCK_HOST;
    ...
    value = qemu_opt_get(opts, "clock");
    if (value) {
        if (!strcmp(value, "host")) {
            rtc_clock = QEMU_CLOCK_HOST;
        } else if (!strcmp(value, "rt")) {
            rtc_clock = QEMU_CLOCK_REALTIME;
        } else if (!strcmp(value, "vm")) {
            rtc_clock = QEMU_CLOCK_VIRTUAL;
        } else {
            fprintf(stderr, "qemu: invalid option value '%s'\n", value);
            exit(1);
        }
    }

So by default it uses the QEMU_CLOCK_HOST option, rather than QEMU_CLOCK_REALTIME.

> 3)why we set a reset_notifiers of QEMU_CLOCK_REALTIME? Any comment to that? A QEMU_CLOCK_REALTIME clock will 
>  not call its  reset_notifiers when the time is adjusted to a time-point before.The reset_notifiers is useless for this type of QEMUClock.

I think this is a staightforward bug. I suspect the issue is that at that point
in the patch series I hadn't get converted the reset_notifier stuff to use the
new API. It looks like I introduced it. Therefore:

Reviewed-by: Alex Bligh <alex@alex.org.uk>

Stefan & Paolo copied

Alex

> 
> 
> 
> 
> 2014-07-01 4:08 GMT+08:00 Alex Bligh <alex@alex.org.uk>:
> 
> 
> On 30 Jun 2014, at 20:53, Lb peace <peaceustc@gmail.com> wrote:
> 
> > If you use hwclock in guest os ,you will find the result of hwclock isn't changed after changing host os's clock.
> > I find this issue is generated in this patch:
> 
> I find it hard to believe that the patch you mention is the problem,
> as it's an automated output of perl simply changing one calling
> convention to another.
> 
> Is this because you are using an unusual clock= option?
> 
> Alex
> 
> 
> 
> > http://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03353.html
> > Before this patch,the result will be changed if you change host's clock.
> > It makes use of the following codes in qemu-timer.c:
> >         if (now < last) {
> >             notifier_list_notify(&clock->reset_notifiers, &now);
> >         }
> > It is useless if you register a QEMU_CLOCK_REALTIME's clock_reset_notifier,
> > ---
> >  hw/timer/mc146818rtc.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> > index df54546..821c27e 100644
> > --- a/hw/timer/mc146818rtc.c
> > +++ b/hw/timer/mc146818rtc.c
> > @@ -879,7 +879,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
> >      check_update_timer(s);
> >
> >      s->clock_reset_notifier.notify = rtc_notify_clock_reset;
> > -    qemu_clock_register_reset_notifier(QEMU_CLOCK_REALTIME,
> > +    qemu_clock_register_reset_notifier(rtc_clock,
> >                                         &s->clock_reset_notifier);
> >
> >      s->suspend_notifier.notify = rtc_notify_suspend;
> > --
> >
> 
> --
> Alex Bligh
> 
> 
> 
> 
> 

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH]Fix a error in mc146818rtc.c

[Stefan Hajnoczi]

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

On Tue, Jul 01, 2014 at 03:53:08AM +0800, Lb peace wrote:
> If you use hwclock in guest os ,you will find the result of hwclock isn't
> changed after changing host os's clock.
> I find this issue is generated in this patch:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03353.html
> Before this patch,the result will be changed if you change host's clock.
> It makes use of the following codes in qemu-timer.c:
>         if (now < last) {
>             notifier_list_notify(&clock->reset_notifiers, &now);
>         }
> It is useless if you register a QEMU_CLOCK_REALTIME's clock_reset_notifier,
> ---

Missing Signed-off-by: Your Name <your@email.com>

All patches in QEMU must have a Signed-off-by.  For more details on the
meaning of S-o-b, see:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/SubmittingPatches;h=689e2371095cc5dfea9927120009341f369159aa;hb=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#l297

>  hw/timer/mc146818rtc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index df54546..821c27e 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -879,7 +879,7 @@ static void rtc_realizefn(DeviceState *dev, Error
> **errp)

Please use git-send-email(1) to send patches.  Or make sure there is no
line-wrapping or other mangling of the patch.

git-am(1) cannot apply your patch because of the line-wrapping.

You can find the guidelines for submitting patches to QEMU here:
http://qemu-project.org/Contribute/SubmitAPatch

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

Re: [Qemu-devel] [PATCH]Fix a error in mc146818rtc.c

[Paolo Bonzini]

Il 30/06/2014 21:53, Lb peace ha scritto:
> If you use hwclock in guest os ,you will find the result of hwclock
> isn't changed after changing host os's clock.
> I find this issue is generated in this patch:
>
> http://lists.gnu.org/archive/html/qemu-devel/2013-08/msg03353.html
> Before this patch,the result will be changed if you change host's clock.
> It makes use of the following codes in qemu-timer.c:
>         if (now < last) {
>             notifier_list_notify(&clock->reset_notifiers, &now);
>         }
> It is useless if you register a QEMU_CLOCK_REALTIME's clock_reset_notifier,
> ---
>  hw/timer/mc146818rtc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index df54546..821c27e 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -879,7 +879,7 @@ static void rtc_realizefn(DeviceState *dev, Error
> **errp)
>      check_update_timer(s);
>
>      s->clock_reset_notifier.notify = rtc_notify_clock_reset;
> -    qemu_clock_register_reset_notifier(QEMU_CLOCK_REALTIME,
> +    qemu_clock_register_reset_notifier(rtc_clock,
>                                         &s->clock_reset_notifier);
>
>      s->suspend_notifier.notify = rtc_notify_suspend;
> --
>

The patch looks good, but it lacks your sign off.

Please read http://elinux.org/Developer_Certificate_Of_Origin and, if 
you agree, reply to this email with this line:

Signed-off-by: Your Real Name <peaceustc@gmail.com>

(with "Your Real Name" replaced by your real name, of course).

Paolo