[libvirt PATCH] conf: Use unsigned long long for timer frequency

Jiri Denemark posted 1 patch 3 years, 4 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/d78c1d870fcb6b72147048f56114b6569e45ca43.1605114052.git.jdenemar@redhat.com
src/conf/domain_conf.c  | 6 +++---
src/conf/domain_conf.h  | 2 +-
src/qemu/qemu_command.c | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
[libvirt PATCH] conf: Use unsigned long long for timer frequency
Posted by Jiri Denemark 3 years, 4 months ago
Although the code in qemuProcessStartValidateTSC works as if the
timer frequency was already unsigned long long (by using an appropriate
temporary variable), the virDomainTimerDef structure actually defines
frequency as unsigned long, which is not guaranteed to be 64b.

Fixes support for frequencies higher than 2^32 - 1 on 32b systems.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/conf/domain_conf.c  | 6 +++---
 src/conf/domain_conf.h  | 2 +-
 src/qemu/qemu_command.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a3ca332279..9199771dc0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14053,7 +14053,7 @@ virDomainTimerDefParseXML(xmlNodePtr node,
         }
     }
 
-    ret = virXPathULong("string(./@frequency)", ctxt, &def->frequency);
+    ret = virXPathULongLong("string(./@frequency)", ctxt, &def->frequency);
     if (ret == -1) {
         def->frequency = 0;
     } else if (ret < 0) {
@@ -22851,7 +22851,7 @@ virDomainTimerDefCheckABIStability(virDomainTimerDefPtr src,
     if (src->name == VIR_DOMAIN_TIMER_NAME_TSC) {
         if (src->frequency != dst->frequency) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("Target TSC frequency %lu does not match source %lu"),
+                           _("Target TSC frequency %llu does not match source %llu"),
                            dst->frequency, src->frequency);
             return false;
         }
@@ -28166,7 +28166,7 @@ virDomainTimerDefFormat(virBufferPtr buf,
 
     if (def->name == VIR_DOMAIN_TIMER_NAME_TSC) {
         if (def->frequency > 0)
-            virBufferAsprintf(buf, " frequency='%lu'", def->frequency);
+            virBufferAsprintf(buf, " frequency='%llu'", def->frequency);
 
         if (def->mode != -1) {
             const char *mode
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 77656c8ae3..16c050a3ea 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2207,7 +2207,7 @@ struct _virDomainTimerDef {
     int track;  /* boot|guest|wall */
 
     /* frequency & mode are only valid for name='tsc' */
-    unsigned long frequency; /* in Hz, unspecified = 0 */
+    unsigned long long frequency; /* in Hz, unspecified = 0 */
     int mode;       /* auto|native|emulate|paravirt */
 };
 
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b0c2a5efb5..0fecb9f6e7 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6357,7 +6357,7 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
             break;
         case VIR_DOMAIN_TIMER_NAME_TSC:
             if (timer->frequency > 0)
-                virBufferAsprintf(&buf, ",tsc-frequency=%lu", timer->frequency);
+                virBufferAsprintf(&buf, ",tsc-frequency=%llu", timer->frequency);
             break;
         case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
             switch (timer->tickpolicy) {
-- 
2.29.2

Re: [libvirt PATCH] conf: Use unsigned long long for timer frequency
Posted by Martin Kletzander 3 years, 4 months ago
On Wed, Nov 11, 2020 at 06:00:52PM +0100, Jiri Denemark wrote:
>Although the code in qemuProcessStartValidateTSC works as if the
>timer frequency was already unsigned long long (by using an appropriate
>temporary variable), the virDomainTimerDef structure actually defines
>frequency as unsigned long, which is not guaranteed to be 64b.
>
>Fixes support for frequencies higher than 2^32 - 1 on 32b systems.
>
>Signed-off-by: Jiri Denemark <jdenemar@redhat.com>

Yeah, sure, why not.  I mean:

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

>---
> src/conf/domain_conf.c  | 6 +++---
> src/conf/domain_conf.h  | 2 +-
> src/qemu/qemu_command.c | 2 +-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>

Tests when?
Re: [libvirt PATCH] conf: Use unsigned long long for timer frequency
Posted by Jiri Denemark 3 years, 4 months ago
On Wed, Nov 11, 2020 at 22:47:34 +0100, Martin Kletzander wrote:
> On Wed, Nov 11, 2020 at 06:00:52PM +0100, Jiri Denemark wrote:
> >Although the code in qemuProcessStartValidateTSC works as if the
> >timer frequency was already unsigned long long (by using an appropriate
> >temporary variable), the virDomainTimerDef structure actually defines
> >frequency as unsigned long, which is not guaranteed to be 64b.
> >
> >Fixes support for frequencies higher than 2^32 - 1 on 32b systems.
> >
> >Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> 
> Yeah, sure, why not.  I mean:
> 
> Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
> 
> >---
> > src/conf/domain_conf.c  | 6 +++---
> > src/conf/domain_conf.h  | 2 +-
> > src/qemu/qemu_command.c | 2 +-
> > 3 files changed, 5 insertions(+), 5 deletions(-)
> >
> 
> Tests when?

Tests were added in "schema: Add support for high TSC frequency" which
caused our CI to fail on i686. This is just a fallout of the trivial
schema change accompanied with proper tests :-)

Jirka