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

[Alexander Graf]

Am 21.03.2013 um 21:50 schrieb Scott Wood <address@hidden>:

> On 03/21/2013 03:41:19 AM, Alexander Graf wrote:
>> On 14.02.2013, at 07:32, Scott Wood wrote:
>> > +DeviceState *kvm_openpic_create(BusState *bus, int model)
>> > +{
>> > +    KVMState *s = kvm_state;
>> > +    DeviceState *dev;
>> > +    struct kvm_create_device cd = {0};
>> > +    int ret;
>> > +
>> > +    if (!kvm_check_extension(s, KVM_CAP_DEVICE_CTRL)) {
>> > +        return NULL;
>> > +    }
>> > +
>> > +    switch (model) {
>> > +    case OPENPIC_MODEL_FSL_MPIC_20:
>> > +        cd.type = KVM_DEV_TYPE_FSL_MPIC_20;
>> > +        break;
>> > +
>> > +    case OPENPIC_MODEL_FSL_MPIC_42:
>> > +        cd.type = KVM_DEV_TYPE_FSL_MPIC_42;
>> > +        break;
>> > +
>> > +    default:
>> > +        qemu_log_mask(LOG_UNIMP, "%s: unknown openpic model %d\n",
>> > +                      __func__, model);
>> > +        return NULL;
>> > +    }
>> > +
>> > +    ret = kvm_vm_ioctl(s, KVM_CREATE_DEVICE, &cd);
>> > +    if (ret < 0) {
>> > +        fprintf(stderr, "%s: can't create device %d: %s\n", __func__, 
>> > cd.type,
>> > +                strerror(errno));
>> > +        return NULL;
>> > +    }
>> 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.

> 
>> >     /* 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.

> 
>> If the host kvm version can't handle an in-kernel MPIC, it should simply 
>> default to false. If it supports one, it defaults to true.
> 
> OK, I misread the existing code and thought that the in-kernel irqchip would 
> never be used unless explicitly requested.
> 
>> Whenever the user defines something explicitly with -machine, that wins.
> 
> Then we'd need kvm_irqchip_wanted to be a tristate -- on, off, or 
> unspecified.  At that point it might be better to drop it entirely and just 
> open-code the option check.

Yup :)

Alex

> 
> -Scott