Re: [Qemu-ppc] [Qemu-devel] [RFC ppc-next PATCH 6/6] kvm/openpic: in-kernel mpic support

[Alexander Graf]

On 21.03.2013, at 22:59, Scott Wood wrote:

> On 03/21/2013 04:29:02 PM, Alexander Graf wrote:
>> Am 21.03.2013 um 21:50 schrieb Scott Wood <address@hidden>:
>> > On 03/21/2013 03:41:19 AM, Alexander Graf wrote:
>> >> Can't all the stuff above here just simply go into the qdev init function?
>> >
>> > Not if you want platform code to be able to fall back to a QEMU mpic if an 
>> > in-kernel mpic is unavailable.
>> Do we want that? We used to have a default like that in qemu-kvm back in the 
>> day. That was very confusing, as people started to report that their VMs 
>> turned out to be really slow.
>> I think we should not have fallback code. It makes things easier and more 
>> obvious. The default should just depend on the host's capabilities.
> 
> I don't follow.  What is the difference between "falling back" and "depending 
> on the host's capabilities"?  Either we can create an in-kernel MPIC or we 
> can't.  We could use KVM_CREATE_DEVICE_TEST to see if the device type is 
> supported separately from actually creating it, but I don't see what that 
> would accomplish other than adding more code.

We usually have settled on a tri-state way to change QEMU behavior for most 
machine options:

  -machine <opt> is not specified -> best possible behavior in the current 
system
  -machine <opt>=on -> turn the option on, fail if that doesn't work
  -machine <opt>=off -> turn the option off always

So for the in-kernel irqchip, we should follow the same model. If the -machine 
option is not passed in, we should try to allocate an in-kernel irqchip if 
possible. If the kernel advertises that it can do an in-kernel irqchip, but in 
fact it can't, I would consider that simply a bug we shouldn't worry about.

However, when the user says -machine kernel_irqchip=on, then we should create a 
kvm based irqchip. And if we can't do that, machine creation should just fail.

> 
>> >> >     /* MPIC */
>> >> >     mpic = g_new(qemu_irq, 256);
>> >> > -    dev = qdev_create(NULL, "openpic");
>> >> > -    qdev_prop_set_uint32(dev, "nb_cpus", smp_cpus);
>> >> > -    qdev_prop_set_uint32(dev, "model", params->mpic_version);
>> >> > +
>> >> > +    if (kvm_irqchip_wanted()) {
>> >> > +        dev = kvm_openpic_create(NULL, params->mpic_version);
>> >> This really should be just a
>> >>    dev = qdev_create(NULL, kvm_irqchip_wanted() ? "kvm-openpic" : 
>> >> "openpic");
>> >> The logic whether an in-kernel irqchip is available belongs into the 
>> >> default setting of kvm_irqchip_wanted.
>> >
>> > That is exactly what I was trying to avoid by introducing 
>> > kvm_irqchip_wanted.  We're no longer testing some vague generic irqchip 
>> > capability, but the presence of a specific type of device (and version 
>> > thereof).  How would the code that sets kvm_irqchip_wanted know what to 
>> > test for?
>> Then move the default code into the board file and check for the in-kernel 
>> mpic cap.
> 
> I'm not quite sure what you mean by "the default code" -- if you mean the 
> part that makes the decision whether to fall back or error out, that's 
> already in board code.

No, currently that lives mostly in kvm-all.c. I'm talking about the code that 
checks qemu_opt_get_bool("kernel_irqchip") and decides what to do based on that.


Alex