Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support

[Scott Wood]

On 06/13/2013 06:01:49 AM, Andreas Färber wrote:
> Am 12.06.2013 22:32, schrieb Scott Wood:
> > +typedef struct KVMOpenPICState {
> > +    SysBusDevice busdev;
>
> SysBusDevice parent_obj; please!
>
> http://wiki.qemu.org/QOMConventions

The word "QOMConventions" doesn't exist once in the QEMU source tree.   
How is one supposed to know that this documentation exists? :-P

Plus, this is just copied from the non-KVM MPIC file, and I see many  
other instances throughout the source tree.

> > +static int kvm_openpic_init(SysBusDevice *dev)
>
> Please make this instance_init + realize functions - "dev" should  
> rather
> be reserved for DeviceState.

Could you elaborate?  I'm really not familiar with this stuff, and have  
not found much documentation.  Again, this is patterned after the  
existing
non-KVM openpic file.

> > +{
> > +    KVMState *s = kvm_state;
> > +    KVMOpenPICState *opp = FROM_SYSBUS(typeof(*opp), dev);
>
> NACK, please introduce your own KVM_OPENPIC(obj) cast macro instead  
> for
> new devices - has been a topic for several weeks and months now.

There's way too much traffic on the list for me to know about something  
just because it's "been a topic".  Lately it's been too much for me to  
even scan the subject lines looking for things that look important,  
given that QEMU is only a fraction of what I spend my time on.

If you'd like to update hw/intc/openpic.c to comply with the style of  
the day, then this could be updated to match...

Also note that this is hardly the first time this patch has been posted  
(v1 was a few weeks ago, and there were RFC patches well before that).   
The first version may have even preceded this "topic".  This seems a  
bit late in the process for a bunch of style churn, when existing code  
hasn't been updated.

> > +    int kvm_openpic_model;
> > +    struct kvm_create_device cd = {0};
> > +    int ret, i;
> > +
> > +    if (!kvm_check_extension(s, KVM_CAP_DEVICE_CTRL)) {
> > +        return -EINVAL;
> > +    }
> > +
> > +    switch (opp->model) {
> > +    case OPENPIC_MODEL_FSL_MPIC_20:
> > +        kvm_openpic_model = KVM_DEV_TYPE_FSL_MPIC_20;
> > +        break;
> > +
> > +    case OPENPIC_MODEL_FSL_MPIC_42:
> > +        kvm_openpic_model = KVM_DEV_TYPE_FSL_MPIC_42;
> > +        break;
> > +
> > +    default:
> > +        return -EINVAL;
> > +    }
>
> If there's only two supported enum-style options, why not make it two
> devices with the value set as a class field?

I'm not 100% sure what you mean here, but it is not intended that there  
will always be only two supported options.  At the least we will  
probably support v4.3 at some point.

-Scott