Re: [libvirt] [PATCH v2 2/3] qemu: Add support for gic-version machine option

2015-10-01 Thread Pavel Fedin 
 Hello!

> > And also we could add some another boolean, which would allow to disable 
> > in-kernel GIC
> emulation
> >(kernel_irqchip=off). This works with any machine type, BTW, not only with 
> >ARM. Something
> like  >kvm='off'/>.
> >
>
> I don't know what that is.  Is that only GIC related?

 No, not only GIC. It applies to any possible irqchip. When set to off, this 
option disables using
KVM acceleration for the irqchip, and qemu's software emulation is used instead.

> Where could I find more details?  

 Something like "aarch64-system-qemu -machine virt,?" i guess. Or similar 
command line for different
arch and different machine. It is implemented as machine option, but applies to 
any machine, not
only to virt, and not only for ARM.
 The option is also mentioned on http://wiki.qemu.org/KVM, but information is 
outdated (it defaults
to on since long ago).

> If that makes sense for anything, we can sure do
> that, the only reason why I would be against this, which I can come up
> now, is if there is nobody using it.

 Yes, this option was bitrot in the ARM kernel, but i have recently fixed it
(https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=c2f58514cfb374d5368c9da
945f1765cd48eb0da). I understand that the primary use of this on e.g. x86 would 
be for testing
purposes only, but for ARM it can be more useful, because you cannot run 
everything on everything.
For example, if you want GICv2 system, but your host is pure GICv3 without 
backwards compatibility,
you could want to use this option, so that you use software-emulated GICv2. Of 
course this would not
give you the top performance, but this is still usable, i tested it. Perhaps 
other systems, like
PowerPC, can also take advantage of it (from KVM code i see there are several 
different irqchips for
PPC, and i'm in doubt that any of these irqchips can emulate everything else).
 There is one more complication with this option - CP15 timer currently will 
not work, because
virtual timer accesses cannot be intercepted by the hypervisor. I am 
considering fixing this in the
future, there are at least two possible solutions. But, still, you can run, for 
example, vexpress
guest with a little bit patched device tree or reconfigured kernel (arch-timer 
disabled), because
this hardware has another, memory-mapped timer, which can be used with software 
emulation. And it
works.
 Well, if you think that this is not ready for the wide public yet, we can 
leave out this part.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 2/3] qemu: Add support for gic-version machine option

2015-10-01 Thread Pavel Fedin 
 Hello!

 > Indentation's off here.

 Damn, sorry, overlooked...

> Also before this patch we would allow def->gic_version == 2 for any
> machine type.  I don't have a problem with this since GIC doesn't make
> sense anywhere else then on ARM machines,

 I'm OK with this. I used 0 for 'no version supplied' just because libvirt 
originally does this.

> but shouldn't we check for
> the fact that the request is for ARM (in case, for example, if ppc64
> gains some 'virt' machine type)?  Because we have no guarantee that
> it's ARM just based on the machine type.

 Yes, i guess we should. 

> I'd change this to:
> 
> if (gic != 2) {
> if (!caps)
> error;
> append_cmd();
> }

 You know, if we are talking about making changes in parser code, we could do 
more. Actually, as i
said in my cover letter, qemu supports more than just 2 or 3. We can also 
specify 'host' for 'best
possible'. Could we accommodate this somehow too? I believe in order to do 
this, we should change
parameter type from numeric to string.

 And also we could add some another boolean, which would allow to disable 
in-kernel GIC emulation
(kernel_irqchip=off). This works with any machine type, BTW, not only with ARM. 
Something like .

 I believe these changes could go as a separate patch, after we discuss details.

> If you're ok with that, just let me know and I'll push it with the
> following diff squashed in, right after the release:

 Yes, ACK.

> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("gic-version option is available "
>   "only for virt machine"));

 Then "...only for ARM virt machine".

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 2/3] qemu: Add support for gic-version machine option

2015-10-01 Thread Martin Kletzander 

On Thu, Oct 01, 2015 at 09:53:00AM +0300, Pavel Fedin wrote:

Hello!

> Indentation's off here.

Damn, sorry, overlooked...


Also before this patch we would allow def->gic_version == 2 for any
machine type.  I don't have a problem with this since GIC doesn't make
sense anywhere else then on ARM machines,


I'm OK with this. I used 0 for 'no version supplied' just because libvirt 
originally does this.


but shouldn't we check for
the fact that the request is for ARM (in case, for example, if ppc64
gains some 'virt' machine type)?  Because we have no guarantee that
it's ARM just based on the machine type.


Yes, i guess we should.


I'd change this to:

if (gic != 2) {
if (!caps)
error;
append_cmd();
}


You know, if we are talking about making changes in parser code, we could do 
more. Actually, as i
said in my cover letter, qemu supports more than just 2 or 3. We can also 
specify 'host' for 'best
possible'. Could we accommodate this somehow too? I believe in order to do 
this, we should change
parameter type from numeric to string.



Yes, we can certainly do that.


And also we could add some another boolean, which would allow to disable 
in-kernel GIC emulation
(kernel_irqchip=off). This works with any machine type, BTW, not only with ARM. 
Something like .



I don't know what that is.  Is that only GIC related?  Where could I
find more details?  If that makes sense for anything, we can sure do
that, the only reason why I would be against this, which I can come up
now, is if there is nobody using it.


I believe these changes could go as a separate patch, after we discuss details.



Yeah, sure.


If you're ok with that, just let me know and I'll push it with the
following diff squashed in, right after the release:


Yes, ACK.


+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("gic-version option is available "
  "only for virt machine"));


Then "...only for ARM virt machine".



Oh, good catch, I'll fix that, thanks.


Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia




signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 2/3] qemu: Add support for gic-version machine option

2015-09-30 Thread Pavel Fedin 
Support for GICv3 has been recently introduced in qemu using gic-version
option for the 'virt' machine. The option can actually take values of
'2', '3' and 'host', however, since in libvirt this is a numeric
parameter, we limit it only to 2 and 3. Value of 2 is not added to the
command line in order to keep backward compatibility with older qemu
versions.

Signed-off-by: Pavel Fedin 
---
 src/qemu/qemu_command.c | 43 ++-
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bb1835c..a47e188 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7702,19 +7702,6 @@ qemuBuildCpuArgStr(virQEMUDriverPtr driver,
 have_cpu = true;
 }
 
-if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) {
-if (def->gic_version && def->gic_version != 2) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("gic version '%u' is not supported"),
-   def->gic_version);
-goto cleanup;
-}
-
-/* There's no command line argument currently to turn on/off GIC. It's
- * done automatically by qemu-system-aarch64. But if this changes, lets
- * put the code here. */
-}
-
 if (virBufferCheckError() < 0)
 goto cleanup;
 
@@ -7931,6 +7918,36 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
 return -1;
 }
 
+   if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) {
+   if (def->gic_version) {
+   if (!STREQ(def->os.machine, "virt") &&
+   !STRPREFIX(def->os.machine, "virt-")) {
+   virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("gic-version option is available "
+ "only for virt machine"));
+virBufferFreeAndReset();
+return -1;
+   }
+
+   /* 2 is the default, so we don't put it as option for
+* backwards compatibility
+*/
+if (def->gic_version != 2) {
+if (virQEMUCapsGet(qemuCaps,
+   QEMU_CAPS_MACH_VIRT_GIC_VERSION)) {
+virBufferAsprintf(, ",gic-version=%d",
+  def->gic_version);
+} else {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("gic-version option is not available "
+ "with this QEMU binary"));
+virBufferFreeAndReset();
+return -1;
+}
+}
+   }
+   }
+
 virCommandAddArgBuffer(cmd, );
 }
 
-- 
2.1.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 2/3] qemu: Add support for gic-version machine option

2015-09-30 Thread Martin Kletzander 

On Wed, Sep 30, 2015 at 02:04:10PM +0300, Pavel Fedin wrote:

Support for GICv3 has been recently introduced in qemu using gic-version
option for the 'virt' machine. The option can actually take values of
'2', '3' and 'host', however, since in libvirt this is a numeric
parameter, we limit it only to 2 and 3. Value of 2 is not added to the
command line in order to keep backward compatibility with older qemu
versions.

Signed-off-by: Pavel Fedin 
---
src/qemu/qemu_command.c | 43 ++-
1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bb1835c..a47e188 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7702,19 +7702,6 @@ qemuBuildCpuArgStr(virQEMUDriverPtr driver,
have_cpu = true;
}

-if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) {
-if (def->gic_version && def->gic_version != 2) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("gic version '%u' is not supported"),
-   def->gic_version);
-goto cleanup;
-}
-
-/* There's no command line argument currently to turn on/off GIC. It's
- * done automatically by qemu-system-aarch64. But if this changes, lets
- * put the code here. */
-}
-
if (virBufferCheckError() < 0)
goto cleanup;

@@ -7931,6 +7918,36 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
return -1;
}

+   if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) {
+   if (def->gic_version) {
+   if (!STREQ(def->os.machine, "virt") &&
+   !STRPREFIX(def->os.machine, "virt-")) {
+   virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("gic-version option is available "
+ "only for virt machine"));
+virBufferFreeAndReset();
+return -1;
+   }


Indentation's off here.

Also before this patch we would allow def->gic_version == 2 for any
machine type.  I don't have a problem with this since GIC doesn't make
sense anywhere else then on ARM machines, but shouldn't we check for
the fact that the request is for ARM (in case, for example, if ppc64
gains some 'virt' machine type)?  Because we have no guarantee that
it's ARM just based on the machine type.


+
+   /* 2 is the default, so we don't put it as option for
+* backwards compatibility
+*/
+if (def->gic_version != 2) {
+if (virQEMUCapsGet(qemuCaps,
+   QEMU_CAPS_MACH_VIRT_GIC_VERSION)) {
+virBufferAsprintf(, ",gic-version=%d",
+  def->gic_version);
+} else {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("gic-version option is not available "
+ "with this QEMU binary"));
+virBufferFreeAndReset();
+return -1;


I'd change this to:

if (gic != 2) {
   if (!caps)
   error;
   append_cmd();
}

If you're ok with that, just let me know and I'll push it with the
following diff squashed in, right after the release:

diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c
index a47e1883d976..72149bdd1eef 100644
--- i/src/qemu/qemu_command.c
+++ w/src/qemu/qemu_command.c
@@ -7918,35 +7918,36 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
return -1;
}

-   if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) {
-   if (def->gic_version) {
-   if (!STREQ(def->os.machine, "virt") &&
-   !STRPREFIX(def->os.machine, "virt-")) {
-   virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) {
+if (def->gic_version) {
+if ((def->os.arch != VIR_ARCH_ARMV7L &&
+ def->os.arch != VIR_ARCH_AARCH64) ||
+(STRNEQ(def->os.machine, "virt") &&
+ !STRPREFIX(def->os.machine, "virt-"))) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
   _("gic-version option is available "
 "only for virt machine"));
virBufferFreeAndReset();
return -1;
-   }
+}

-   /* 2 is the default, so we don't put it as option for
-* backwards compatibility
-*/
+/* 2 is the default, so we don't put it as option for
+