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

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