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


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


Attachment: signature.asc
Description: PGP signature

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

Reply via email to