[Qemu-devel] [patch 0/3] add QMP command to reset rtc interrupt backlog (v3)

[Qemu-devel] [patch 0/3] add QMP command to reset rtc interrupt backlog (v3)

[mtosatti]

It is necessary to reset RTC interrupt backlog if guest time is
synchronized via a different mechanism, such as QGA's guest-set-time
command.

Failing to do so causes both corrections to be applied (summed),
resulting in an incorrect guest time.

--- 

changelog:

v3: 
- add object_property_add_alias (Paolo)
- fix #ifdefs                   (Paolo)

[Qemu-devel] [patch 1/3] mc146818rtc: add rtc_reset_reinjection QMP command

[mtosatti]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: rtc-reset --]
[-- Type: text/plain, Size: 2876 bytes --]

It is necessary to reset RTC interrupt reinjection backlog if
guest time is synchronized via a different mechanism, such as 
QGA's guest-set-time command.

Failing to do so causes both corrections to be applied (summed),
resulting in an incorrect guest time.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu/hw/timer/mc146818rtc.c
===================================================================
--- qemu.orig/hw/timer/mc146818rtc.c
+++ qemu/hw/timer/mc146818rtc.c
@@ -26,6 +26,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/timer/mc146818rtc.h"
 #include "qapi/visitor.h"
+#include "qmp-commands.h"
 
 #ifdef TARGET_I386
 #include "hw/i386/apic.h"
@@ -84,6 +85,9 @@ typedef struct RTCState {
     Notifier clock_reset_notifier;
     LostTickPolicy lost_tick_policy;
     Notifier suspend_notifier;
+#ifdef TARGET_I386
+    QLIST_ENTRY(RTCState) link;
+#endif
 } RTCState;
 
 static void rtc_set_time(RTCState *s);
@@ -522,6 +526,20 @@ static void rtc_get_time(RTCState *s, st
         rtc_from_bcd(s, s->cmos_data[RTC_CENTURY]) * 100 - 1900;
 }
 
+#ifdef TARGET_I386
+static QLIST_HEAD(, RTCState) rtc_devices =
+    QLIST_HEAD_INITIALIZER(rtc_devices);
+
+void qmp_rtc_reset_reinjection(Error **errp)
+{
+    RTCState *s;
+
+    QLIST_FOREACH(s, &rtc_devices, link) {
+        s->irq_coalesced = 0;
+    }
+}
+#endif
+
 static void rtc_set_time(RTCState *s)
 {
     struct tm tm;
@@ -911,6 +929,10 @@ ISADevice *rtc_init(ISABus *bus, int bas
     } else {
         isa_init_irq(isadev, &s->irq, RTC_ISA_IRQ);
     }
+#ifdef TARGET_I386
+    QLIST_INSERT_HEAD(&rtc_devices, s, link);
+#endif
+
     return isadev;
 }
 
Index: qemu/qapi-schema.json
===================================================================
--- qemu.orig/qapi-schema.json
+++ qemu/qapi-schema.json
@@ -4722,3 +4722,15 @@
               'btn'     : 'InputBtnEvent',
               'rel'     : 'InputMoveEvent',
               'abs'     : 'InputMoveEvent' } }
+
+##
+# @: rtc-reset-reinjection
+#
+# This command will reset RTC's interrupt reinjection backlog.
+# Can be used if another mechanism to synchronize guest time
+# is in effect, for example QEMU guest agents guest-set-time
+# command.
+#
+# Since: 2.1
+##
+{ 'command': 'rtc-reset-reinjection' }
Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -3572,3 +3572,26 @@ Example:
                    } } ] }
 
 EQMP
+
+#if defined (TARGET_I386)
+    {
+        .name       = "rtc_reset_reinjection",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_rtc_reset_reinjection,
+    },
+#endif
+
+SQMP
+rtc-reset-reinjection
+---------------------
+
+Reset RTC's interrupt reinjection backlog.
+
+Arguments: None.
+
+Example:
+
+-> { "execute": "rtc-reset-reinjection" }
+<- { "return": {} }
+
+EQMP

Re: [Qemu-devel] [patch 1/3] mc146818rtc: add rtc_reset_reinjection QMP command

[Eric Blake]

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

On 06/02/2014 11:51 AM, mtosatti@redhat.com wrote:
> It is necessary to reset RTC interrupt reinjection backlog if
> guest time is synchronized via a different mechanism, such as 
> QGA's guest-set-time command.
> 
> Failing to do so causes both corrections to be applied (summed),
> resulting in an incorrect guest time.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Index: qemu/hw/timer/mc146818rtc.c

Still no --- separator between your commit message and the patch body.
Are you using 'git send-email'?


> Index: qemu/qapi-schema.json
> ===================================================================
> --- qemu.orig/qapi-schema.json
> +++ qemu/qapi-schema.json
> @@ -4722,3 +4722,15 @@
>                'btn'     : 'InputBtnEvent',
>                'rel'     : 'InputMoveEvent',
>                'abs'     : 'InputMoveEvent' } }
> +
> +##
> +# @: rtc-reset-reinjection

s/: // to resemble most other commands

> +#
> +# This command will reset RTC's interrupt reinjection backlog.

s/RTC's/the RTC/

> +# Can be used if another mechanism to synchronize guest time
> +# is in effect, for example QEMU guest agents guest-set-time

s/agents/agent's/

> +# command.
> +#
> +# Since: 2.1
> +##
> +{ 'command': 'rtc-reset-reinjection' }
> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -3572,3 +3572,26 @@ Example:
>                     } } ] }
>  
>  EQMP
> +
> +#if defined (TARGET_I386)
> +    {
> +        .name       = "rtc_reset_reinjection",

s/rtc_reset_reinjection/rtc-reset-reinjection/

> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_rtc_reset_reinjection,
> +    },
> +#endif
> +
> +SQMP
> +rtc-reset-reinjection
> +---------------------

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [patch 1/3] mc146818rtc: add rtc_reset_reinjection QMP command

[Marcelo Tosatti]

On Mon, Jun 02, 2014 at 01:31:29PM -0600, Eric Blake wrote:
> On 06/02/2014 11:51 AM, mtosatti@redhat.com wrote:
> > It is necessary to reset RTC interrupt reinjection backlog if
> > guest time is synchronized via a different mechanism, such as 
> > QGA's guest-set-time command.
> > 
> > Failing to do so causes both corrections to be applied (summed),
> > resulting in an incorrect guest time.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > 
> > Index: qemu/hw/timer/mc146818rtc.c
> 
> Still no --- separator between your commit message and the patch body.
> Are you using 'git send-email'?

OK, diffstats on every patch.

> > Index: qemu/qapi-schema.json
> > ===================================================================
> > --- qemu.orig/qapi-schema.json
> > +++ qemu/qapi-schema.json
> > @@ -4722,3 +4722,15 @@
> >                'btn'     : 'InputBtnEvent',
> >                'rel'     : 'InputMoveEvent',
> >                'abs'     : 'InputMoveEvent' } }
> > +
> > +##
> > +# @: rtc-reset-reinjection
> 
> s/: // to resemble most other commands

Several commands have ":". What is the correct syntax and why?

> > +# This command will reset RTC's interrupt reinjection backlog.
> 
> s/RTC's/the RTC/
> 
> > +# Can be used if another mechanism to synchronize guest time
> > +# is in effect, for example QEMU guest agents guest-set-time
> 
> s/agents/agent's/

Fixed.

> > +# command.
> > +#
> > +# Since: 2.1
> > +##
> > +{ 'command': 'rtc-reset-reinjection' }
> > Index: qemu/qmp-commands.hx
> > ===================================================================
> > --- qemu.orig/qmp-commands.hx
> > +++ qemu/qmp-commands.hx
> > @@ -3572,3 +3572,26 @@ Example:
> >                     } } ] }
> >  
> >  EQMP
> > +
> > +#if defined (TARGET_I386)
> > +    {
> > +        .name       = "rtc_reset_reinjection",
> 
> s/rtc_reset_reinjection/rtc-reset-reinjection/

This is a function name.

Re: [Qemu-devel] [patch 1/3] mc146818rtc: add rtc_reset_reinjection QMP command

[Eric Blake]

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

On 06/02/2014 02:20 PM, Marcelo Tosatti wrote:
> On Mon, Jun 02, 2014 at 01:31:29PM -0600, Eric Blake wrote:
>> On 06/02/2014 11:51 AM, mtosatti@redhat.com wrote:
>>> It is necessary to reset RTC interrupt reinjection backlog if
>>> guest time is synchronized via a different mechanism, such as 
>>> QGA's guest-set-time command.
>>>

>>> +
>>> +##
>>> +# @: rtc-reset-reinjection
>>
>> s/: // to resemble most other commands
> 
> Several commands have ":". What is the correct syntax and why?

Alas, we don't have any automated program that strips these stylized
comments and turns them into formal documentation.  But the goal is that
some day we might, at which point, being consistent in our style is the
most likely to be successful.  The prevalent style appears to be:

##
# @command:
#
# Short summary
#
# @foo: describe mandatory option foo
#
# @bar: #optional describe optional option bar, and its default value
#       if omitted
#
# Returns: what to expect from the command
#
# Since: version it was introduced
##
{ 'command' ... }

Although I will admit that '@command' vs. '@command:' didn't have a
clear winner.  Maybe someone with OCD wants to do a pure cleanup patch
to get the file into a consistent state?  Until then, I'm pointing out
where things are definitely different (your '@: command' was an outlier)

>>>  EQMP
>>> +
>>> +#if defined (TARGET_I386)
>>> +    {
>>> +        .name       = "rtc_reset_reinjection",
>>
>> s/rtc_reset_reinjection/rtc-reset-reinjection/
> 
> This is a function name.

No, it is a QMP command name.  See "send-key" for an example.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [patch 2/3] add object_property_add_alias

[mtosatti]

[-- Attachment #1: qom-add-alias --]
[-- Type: text/plain, Size: 4518 bytes --]

Allowing addition of a link without keeping pointer-to-pointer.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu/include/qom/object.h
===================================================================
--- qemu.orig/include/qom/object.h
+++ qemu/include/qom/object.h
@@ -1073,6 +1073,19 @@ typedef enum {
 } ObjectPropertyLinkFlags;
 
 /**
+ * object_property_add_alias:
+ * @obj: the object to add a property to
+ * @name: the name of the property
+ * @alias: the alias object
+ * @errp: if an error occurs, a pointer to an area to store the area
+ *
+ * Add a link under obj, named name, pointing to alias.
+ *
+ */
+void object_property_add_alias(Object *obj, const char *name,
+                               Object *alias, Error **errp);
+
+/**
  * object_property_allow_set_link:
  *
  * The default implementation of the object_property_add_link() check()
Index: qemu/qom/object.c
===================================================================
--- qemu.orig/qom/object.c
+++ qemu/qom/object.c
@@ -1023,27 +1023,71 @@ out:
     g_free(type);
 }
 
+typedef struct {
+    Object *child;
+    Object **childp;
+    void (*check)(Object *, const char *, Object *, Error **);
+    ObjectPropertyLinkFlags flags;
+} LinkProperty;
+
+static void object_get_alias_property(Object *obj, Visitor *v, void *opaque,
+                                      const char *name, Error **errp)
+{
+    LinkProperty *prop = opaque;
+    Object *child = prop->child;
+    gchar *path;
+
+    path = object_get_canonical_path(child);
+    visit_type_str(v, &path, name, errp);
+    g_free(path);
+}
+
+static void object_release_alias_property(Object *obj, const char *name,
+                                         void *opaque)
+{
+    LinkProperty *prop = opaque;
+
+    g_free(prop);
+}
+
+void object_property_add_alias(Object *obj, const char *name,
+                               Object *alias, Error **errp)
+{
+    Error *local_err = NULL;
+    gchar *type;
+    LinkProperty *prop = g_malloc(sizeof(*prop));
+
+    type = g_strdup_printf("link<%s>", object_get_typename(OBJECT(alias)));
+
+    prop->child = alias;
+    prop->check = NULL;
+    prop->flags = 0;
+
+    object_property_add(obj, name, type, object_get_alias_property, NULL,
+                        object_release_alias_property, prop, &local_err);
+    if (local_err) {
+        g_free(prop);
+        error_propagate(errp, local_err);
+    }
+
+    g_free(type);
+}
+
 void object_property_allow_set_link(Object *obj, const char *name,
                                     Object *val, Error **errp)
 {
     /* Allow the link to be set, always */
 }
 
-typedef struct {
-    Object **child;
-    void (*check)(Object *, const char *, Object *, Error **);
-    ObjectPropertyLinkFlags flags;
-} LinkProperty;
-
 static void object_get_link_property(Object *obj, Visitor *v, void *opaque,
                                      const char *name, Error **errp)
 {
     LinkProperty *lprop = opaque;
-    Object **child = lprop->child;
+    Object *child = lprop->child;
     gchar *path;
 
-    if (*child) {
-        path = object_get_canonical_path(*child);
+    if (child) {
+        path = object_get_canonical_path(child);
         visit_type_str(v, &path, name, errp);
         g_free(path);
     } else {
@@ -1096,7 +1140,7 @@ static void object_set_link_property(Obj
 {
     Error *local_err = NULL;
     LinkProperty *prop = opaque;
-    Object **child = prop->child;
+    Object **child = prop->childp;
     Object *old_target = *child;
     Object *new_target = NULL;
     char *path = NULL;
@@ -1133,8 +1177,8 @@ static void object_release_link_property
 {
     LinkProperty *prop = opaque;
 
-    if ((prop->flags & OBJ_PROP_LINK_UNREF_ON_RELEASE) && *prop->child) {
-        object_unref(*prop->child);
+    if ((prop->flags & OBJ_PROP_LINK_UNREF_ON_RELEASE) && prop->child) {
+        object_unref(prop->child);
     }
     g_free(prop);
 }
@@ -1150,7 +1194,8 @@ void object_property_add_link(Object *ob
     LinkProperty *prop = g_malloc(sizeof(*prop));
     gchar *full_type;
 
-    prop->child = child;
+    prop->childp = child;
+    prop->child = *child;
     prop->check = check;
     prop->flags = flags;
 
@@ -1227,7 +1272,7 @@ Object *object_resolve_path_component(Ob
 
     if (object_property_is_link(prop)) {
         LinkProperty *lprop = prop->opaque;
-        return *lprop->child;
+        return lprop->child;
     } else if (object_property_is_child(prop)) {
         return prop->opaque;
     } else {

[Qemu-devel] [patch 3/3] mc146818rtc: add "rtc" link to "/machine"

[mtosatti]

[-- Attachment #1: rtc-add-link --]
[-- Type: text/plain, Size: 1341 bytes --]

Add a link to rtc under /machine providing a stable 
location for management apps to query "date" field.

{"execute":"qom-get","arguments":{"path":"/machine/rtc","property":"date"} }

Suggested by Paolo Bonzini.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Index: qemu/hw/timer/mc146818rtc.c
===================================================================
--- qemu.orig/hw/timer/mc146818rtc.c
+++ qemu/hw/timer/mc146818rtc.c
@@ -911,6 +911,9 @@ static void rtc_realizefn(DeviceState *d
 
     object_property_add(OBJECT(s), "date", "struct tm",
                         rtc_get_date, NULL, NULL, s, NULL);
+
+    object_property_add_alias(qdev_get_machine(), "rtc", OBJECT(s),
+                              &error_abort);
 }
 
 ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq)
@@ -954,11 +957,17 @@ static void rtc_class_initfn(ObjectClass
     dc->cannot_instantiate_with_device_add_yet = true;
 }
 
+static void rtc_finalize(Object *obj)
+{
+    object_property_del(qdev_get_machine(), "rtc", NULL);
+}
+
 static const TypeInfo mc146818rtc_info = {
     .name          = TYPE_MC146818_RTC,
     .parent        = TYPE_ISA_DEVICE,
     .instance_size = sizeof(RTCState),
     .class_init    = rtc_class_initfn,
+    .instance_finalize = rtc_finalize,
 };
 
 static void mc146818rtc_register_types(void)

Re: [Qemu-devel] [patch 0/3] add QMP command to reset rtc interrupt backlog (v3)

[Eric Blake]

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

On 06/02/2014 11:51 AM, mtosatti@redhat.com wrote:
> It is necessary to reset RTC interrupt backlog if guest time is
> synchronized via a different mechanism, such as QGA's guest-set-time
> command.
> 
> Failing to do so causes both corrections to be applied (summed),
> resulting in an incorrect guest time.

Please send v3 of a series as a new top-level thread instead of buried
as a reply to v2.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [patch 0/3] add QMP command to reset rtc interrupt backlog (v3)

[Marcelo Tosatti]

On Mon, Jun 02, 2014 at 01:05:50PM -0600, Eric Blake wrote:
> On 06/02/2014 11:51 AM, mtosatti@redhat.com wrote:
> > It is necessary to reset RTC interrupt backlog if guest time is
> > synchronized via a different mechanism, such as QGA's guest-set-time
> > command.
> > 
> > Failing to do so causes both corrections to be applied (summed),
> > resulting in an incorrect guest time.
> 
> Please send v3 of a series as a new top-level thread instead of buried
> as a reply to v2.

I prefer to delete whole threads i am not interested in.

What is the problem with threads for you ?

Re: [Qemu-devel] [patch 0/3] add QMP command to reset rtc interrupt backlog (v3)

[Eric Blake]

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

On 06/02/2014 01:20 PM, Marcelo Tosatti wrote:
> On Mon, Jun 02, 2014 at 01:05:50PM -0600, Eric Blake wrote:
>> On 06/02/2014 11:51 AM, mtosatti@redhat.com wrote:
>>> It is necessary to reset RTC interrupt backlog if guest time is
>>> synchronized via a different mechanism, such as QGA's guest-set-time
>>> command.
>>>
>>> Failing to do so causes both corrections to be applied (summed),
>>> resulting in an incorrect guest time.
>>
>> Please send v3 of a series as a new top-level thread instead of buried
>> as a reply to v2.
> 
> I prefer to delete whole threads i am not interested in.
> 
> What is the problem with threads for you ?

Threads are good.  But one thread per version of a series is better than
one thread for ALL versions of the series, as documented in our patch
submission guidelines:
http://wiki.qemu.org/Contribute/SubmitAPatch

Since most of contributors are already doing that, most reviewers have
gotten into the habit of assuming that any replies to a series are in
regards to things to fix, rather than looking for a new version of the
series.  I'd rather kill several threads for versions of a series, then
later learn that a new revision of the series needs my review; than kill
a single thread, at which point I no longer see any new versions in my
inbox but also lose any chance to be easily pulled back in to that topic.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]