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

[Scott Wood]

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.

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

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

-Scott