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

[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 <gic
kvm='off'/>.

 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