Re: [libvirt PATCH v2 3/8] conf: Introduce VIR_DOMAIN_TIMER_NAME_ARMVTIMER

2020-02-14 Thread Andrea Bolognani 
On Fri, 2020-02-14 at 11:19 +0100, Andrew Jones wrote:
> On Thu, Feb 13, 2020 at 05:30:21PM +0100, Andrea Bolognani wrote:
> > On Thu, 2020-02-13 at 14:26 +0100, Ján Tomko wrote:
> > > Okay, so this name is a libvirt invention.
> > 
> > I believe "ARM virtual timer" is the official name for the device;
> > the name "armvtimer" is an abbreviation that me and Drew (CC'd) have
> > agreed upon, and I understand that's fairly commonly used too.
> > 
> > Drew can confirm whether this is actually the case.
> 
> To be precise the ARM architecture reference manual refers to the CPU's
> builtin timer as the "Generic Timer". All CPUs which the QEMU arm 'virt'
> machine type supports implement the Generic Timer, and all implementations
> of the Generic Timer must include the virtual counter. The virtual counter
> is used to indicate virtual time. In both QEMU and KVM when we want to
> refer to the virtual counter based timer we call it the "vtimer".
> 
> So, while libvirt is indeed inventing the name "armvtimer", I think it's
> appropriate.

There's plenty of precedents for carrying a name used in KVM/QEMU
over to libvirt, so we're good on that front.

> > > And the timer itself is present on all ARM/virt guests and cannot be
> > > disabled, correct?
> > 
> > I believe so but, once again, it'd be better if Drew confirmed it :)
> 
> Correct. While the Generic Timer is an optional feature of the CPU, all
> CPUs that the ARM/virt machine type support have it and it cannot be
> removed.
> 
> So, all ARM/virt guests have vtimers, but not all ARM guests must have
> vtimers. If libvirt can start non-'virt' ARM guests, which have different
> CPUs, then there's a chance that the guest will not have a vtimer. I'm
> not sure if we want to try and incorporate that much specificity
> into the libvirt name, though. I think if a machine/CPU doesn't have a
> vtimer than this XML element should just not be valid.

We're explicitly preventing non-virt guests to configure the timer,
so no problem here.


Since all outstanding questions have been answered, I'm going to push
the series now. Thanks Drew for providing these last nuggets of
information, and of course thanks Jano for the review :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v2 3/8] conf: Introduce VIR_DOMAIN_TIMER_NAME_ARMVTIMER

2020-02-14 Thread Andrew Jones 
On Thu, Feb 13, 2020 at 05:30:21PM +0100, Andrea Bolognani wrote:
> On Thu, 2020-02-13 at 14:26 +0100, Ján Tomko wrote:
> > On Fri, Feb 07, 2020 at 03:27:03PM +0100, Andrea Bolognani wrote:
> > > +++ b/src/conf/domain_conf.c
> > > @@ -1063,6 +1063,7 @@ VIR_ENUM_IMPL(virDomainTimerName,
> > >   "tsc",
> > >   "kvmclock",
> > >   "hypervclock",
> > > +  "armvtimer",
> > > );
> > 
> > Okay, so this name is a libvirt invention.
> 
> I believe "ARM virtual timer" is the official name for the device;
> the name "armvtimer" is an abbreviation that me and Drew (CC'd) have
> agreed upon, and I understand that's fairly commonly used too.
> 
> Drew can confirm whether this is actually the case.

To be precise the ARM architecture reference manual refers to the CPU's
builtin timer as the "Generic Timer". All CPUs which the QEMU arm 'virt'
machine type supports implement the Generic Timer, and all implementations
of the Generic Timer must include the virtual counter. The virtual counter
is used to indicate virtual time. In both QEMU and KVM when we want to
refer to the virtual counter based timer we call it the "vtimer".

So, while libvirt is indeed inventing the name "armvtimer", I think it's
appropriate.

> 
> > And the timer itself is present on all ARM/virt guests and cannot be
> > disabled, correct?
> 
> I believe so but, once again, it'd be better if Drew confirmed it :)

Correct. While the Generic Timer is an optional feature of the CPU, all
CPUs that the ARM/virt machine type support have it and it cannot be
removed.

So, all ARM/virt guests have vtimers, but not all ARM guests must have
vtimers. If libvirt can start non-'virt' ARM guests, which have different
CPUs, then there's a chance that the guest will not have a vtimer. I'm
not sure if we want to try and incorporate that much specificity
into the libvirt name, though. I think if a machine/CPU doesn't have a
vtimer than this XML element should just not be valid.

Thanks,
drew

Re: [libvirt PATCH v2 3/8] conf: Introduce VIR_DOMAIN_TIMER_NAME_ARMVTIMER

2020-02-13 Thread Andrea Bolognani 
On Thu, 2020-02-13 at 14:26 +0100, Ján Tomko wrote:
> On Fri, Feb 07, 2020 at 03:27:03PM +0100, Andrea Bolognani wrote:
> > +++ b/src/conf/domain_conf.c
> > @@ -1063,6 +1063,7 @@ VIR_ENUM_IMPL(virDomainTimerName,
> >   "tsc",
> >   "kvmclock",
> >   "hypervclock",
> > +  "armvtimer",
> > );
> 
> Okay, so this name is a libvirt invention.

I believe "ARM virtual timer" is the official name for the device;
the name "armvtimer" is an abbreviation that me and Drew (CC'd) have
agreed upon, and I understand that's fairly commonly used too.

Drew can confirm whether this is actually the case.

> And the timer itself is present on all ARM/virt guests and cannot be
> disabled, correct?

I believe so but, once again, it'd be better if Drew confirmed it :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH v2 3/8] conf: Introduce VIR_DOMAIN_TIMER_NAME_ARMVTIMER

2020-02-13 Thread Ján Tomko 

On Fri, Feb 07, 2020 at 03:27:03PM +0100, Andrea Bolognani wrote:

This new timer model will be used to control the behavior of the
virtual timer for KVM ARM/virt guests.

Signed-off-by: Andrea Bolognani 
---
docs/schemas/domaincommon.rng | 1 +
src/conf/domain_conf.c| 1 +
src/conf/domain_conf.h| 1 +
src/libxl/libxl_conf.c| 1 +
src/libxl/xen_common.c| 1 +
src/qemu/qemu_command.c   | 2 ++
src/qemu/qemu_domain.c| 3 +++
7 files changed, 10 insertions(+)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 9577d26c2a..29b6b95357 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1239,6 +1239,7 @@

  hpet
  pit
+  armvtimer

  
  
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 51ae520897..78d964ed9e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1063,6 +1063,7 @@ VIR_ENUM_IMPL(virDomainTimerName,
  "tsc",
  "kvmclock",
  "hypervclock",
+  "armvtimer",
);


Okay, so this name is a libvirt invention.

And the timer itself is present on all ARM/virt guests and cannot be
disabled, correct?

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [libvirt PATCH v2 3/8] conf: Introduce VIR_DOMAIN_TIMER_NAME_ARMVTIMER

2020-02-10 Thread Masayoshi Mizuma 
On Fri, Feb 07, 2020 at 03:27:03PM +0100, Andrea Bolognani wrote:
> This new timer model will be used to control the behavior of the
> virtual timer for KVM ARM/virt guests.
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  docs/schemas/domaincommon.rng | 1 +
>  src/conf/domain_conf.c| 1 +
>  src/conf/domain_conf.h| 1 +
>  src/libxl/libxl_conf.c| 1 +
>  src/libxl/xen_common.c| 1 +
>  src/qemu/qemu_command.c   | 2 ++
>  src/qemu/qemu_domain.c| 3 +++
>  7 files changed, 10 insertions(+)
> 
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 9577d26c2a..29b6b95357 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1239,6 +1239,7 @@
>  
>hpet
>pit
> +  armvtimer
>  
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 51ae520897..78d964ed9e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1063,6 +1063,7 @@ VIR_ENUM_IMPL(virDomainTimerName,
>"tsc",
>"kvmclock",
>"hypervclock",
> +  "armvtimer",
>  );
>  
>  VIR_ENUM_IMPL(virDomainTimerTrack,
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 2db3c19473..867a9c7661 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1993,6 +1993,7 @@ typedef enum {
>  VIR_DOMAIN_TIMER_NAME_TSC,
>  VIR_DOMAIN_TIMER_NAME_KVMCLOCK,
>  VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK,
> +VIR_DOMAIN_TIMER_NAME_ARMVTIMER,
>  
>  VIR_DOMAIN_TIMER_NAME_LAST
>  } virDomainTimerNameType;
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index ee6b23895c..56deca7b7c 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -359,6 +359,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>  case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
>  case VIR_DOMAIN_TIMER_NAME_RTC:
>  case VIR_DOMAIN_TIMER_NAME_PIT:
> +case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("unsupported timer type (name) '%s'"),
> 
> virDomainTimerNameTypeToString(clock.timers[i]->name));
> diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c
> index 415549a42c..9a385eba0d 100644
> --- a/src/libxl/xen_common.c
> +++ b/src/libxl/xen_common.c
> @@ -2182,6 +2182,7 @@ xenFormatCPUFeatures(virConfPtr conf, virDomainDefPtr 
> def)
>  case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
>  case VIR_DOMAIN_TIMER_NAME_RTC:
>  case VIR_DOMAIN_TIMER_NAME_PIT:
> +case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
>  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("unsupported timer type (name) '%s'"),
> 
> virDomainTimerNameTypeToString(def->clock.timers[i]->name));
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 52a74c7acf..71ae1f72e5 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6199,6 +6199,7 @@ qemuBuildClockCommandLine(virCommandPtr cmd,
>  case VIR_DOMAIN_TIMER_NAME_TSC:
>  case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
>  case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
> +case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
>  /* Timers above are handled when building -cpu.  */
>  case VIR_DOMAIN_TIMER_NAME_LAST:
>  break;
> @@ -6609,6 +6610,7 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
>  if (timer->frequency > 0)
>  virBufferAsprintf(&buf, ",tsc-frequency=%lu", 
> timer->frequency);
>  break;
> +case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
>  case VIR_DOMAIN_TIMER_NAME_PLATFORM:
>  case VIR_DOMAIN_TIMER_NAME_PIT:
>  case VIR_DOMAIN_TIMER_NAME_RTC:
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 1b4825a539..68348464a8 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5428,6 +5428,9 @@ qemuDomainDefValidateClockTimers(const virDomainDef 
> *def,
>  return -1;
>  }
>  break;
> +
> +case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
> +break;
>  }
>  }
>  
> -- 
> 2.24.1
> 

Reviewed-by: Masayoshi Mizuma

[libvirt PATCH v2 3/8] conf: Introduce VIR_DOMAIN_TIMER_NAME_ARMVTIMER

2020-02-07 Thread Andrea Bolognani 
This new timer model will be used to control the behavior of the
virtual timer for KVM ARM/virt guests.

Signed-off-by: Andrea Bolognani 
---
 docs/schemas/domaincommon.rng | 1 +
 src/conf/domain_conf.c| 1 +
 src/conf/domain_conf.h| 1 +
 src/libxl/libxl_conf.c| 1 +
 src/libxl/xen_common.c| 1 +
 src/qemu/qemu_command.c   | 2 ++
 src/qemu/qemu_domain.c| 3 +++
 7 files changed, 10 insertions(+)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 9577d26c2a..29b6b95357 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1239,6 +1239,7 @@
 
   hpet
   pit
+  armvtimer
 
   
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 51ae520897..78d964ed9e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1063,6 +1063,7 @@ VIR_ENUM_IMPL(virDomainTimerName,
   "tsc",
   "kvmclock",
   "hypervclock",
+  "armvtimer",
 );
 
 VIR_ENUM_IMPL(virDomainTimerTrack,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 2db3c19473..867a9c7661 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1993,6 +1993,7 @@ typedef enum {
 VIR_DOMAIN_TIMER_NAME_TSC,
 VIR_DOMAIN_TIMER_NAME_KVMCLOCK,
 VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK,
+VIR_DOMAIN_TIMER_NAME_ARMVTIMER,
 
 VIR_DOMAIN_TIMER_NAME_LAST
 } virDomainTimerNameType;
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index ee6b23895c..56deca7b7c 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -359,6 +359,7 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
 case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
 case VIR_DOMAIN_TIMER_NAME_RTC:
 case VIR_DOMAIN_TIMER_NAME_PIT:
+case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unsupported timer type (name) '%s'"),

virDomainTimerNameTypeToString(clock.timers[i]->name));
diff --git a/src/libxl/xen_common.c b/src/libxl/xen_common.c
index 415549a42c..9a385eba0d 100644
--- a/src/libxl/xen_common.c
+++ b/src/libxl/xen_common.c
@@ -2182,6 +2182,7 @@ xenFormatCPUFeatures(virConfPtr conf, virDomainDefPtr def)
 case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
 case VIR_DOMAIN_TIMER_NAME_RTC:
 case VIR_DOMAIN_TIMER_NAME_PIT:
+case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unsupported timer type (name) '%s'"),

virDomainTimerNameTypeToString(def->clock.timers[i]->name));
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 52a74c7acf..71ae1f72e5 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6199,6 +6199,7 @@ qemuBuildClockCommandLine(virCommandPtr cmd,
 case VIR_DOMAIN_TIMER_NAME_TSC:
 case VIR_DOMAIN_TIMER_NAME_KVMCLOCK:
 case VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK:
+case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
 /* Timers above are handled when building -cpu.  */
 case VIR_DOMAIN_TIMER_NAME_LAST:
 break;
@@ -6609,6 +6610,7 @@ qemuBuildCpuCommandLine(virCommandPtr cmd,
 if (timer->frequency > 0)
 virBufferAsprintf(&buf, ",tsc-frequency=%lu", 
timer->frequency);
 break;
+case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
 case VIR_DOMAIN_TIMER_NAME_PLATFORM:
 case VIR_DOMAIN_TIMER_NAME_PIT:
 case VIR_DOMAIN_TIMER_NAME_RTC:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1b4825a539..68348464a8 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5428,6 +5428,9 @@ qemuDomainDefValidateClockTimers(const virDomainDef *def,
 return -1;
 }
 break;
+
+case VIR_DOMAIN_TIMER_NAME_ARMVTIMER:
+break;
 }
 }
 
-- 
2.24.1