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

[Andreas Färber]

Am 13.06.2013 19:32, schrieb 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

I do expect people to read at least the subject of patch series on
qemu-devel, short of individual review comments. CPU, PReP PCI,
Versatile PCI, ISA and more recently virtio series had been posted.
Some such review comments have been collected into the above Wiki page
because this is a recurring topic unfortunately.

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

Which exactly is the reason for my grief: Your ignorance is making other
people even more work, and at least Alex should've spotted it - I can't
review all patches just because they might or might not be touching on QOM.
Just as we are supposed to not copy old Coding Style in new patches, we
should be applying new patterns and conventions in new patches, too.

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

static void kvm_openpic_init(Object *obj) should initialize simple
variables, MemoryRegions that don't depend on parameters and any QOM
properties.

static void kvm_openpic_realize(DeviceState *dev, Error **errp) should
initialize the rest.

kvm_openpic_unrealize(Device *dev, Error **errp) and
kvm_openpic_finalize(Object *obj) would be their counterparts for cleanup.

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

I'm not talking about style churn, I'm talking about using the wrong
infrastructure and making it even more difficult to drop FROM_SYSBUS()
macro.

Again, whether or not some particular other file uses some style doesn't
mean that it's okay to apply that to new files. Instead of complaining
it would've been a task of five minutes to supply Alex with a fixup
patch to squash/follow-up or to post a v3. QOM realize is merged since
January 2013 and was presented by Anthony in January 2012.

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

Afterwards I saw that you copied the "model" property from the non-KVM.
What I was pointing about here is that unlike qdev QOM allows to have
arbitrary levels of inheritence, i.e. you can have a kvm-openpic base
class and specialized subclasses like mipc42-kvm-openpic rather than
kvm-openpic,model=42 given that the user can't specify arbitrary
integers - whether two or three doesn't really matter in that respect.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg