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

[Scott Wood]

On 06/14/2013 09:59:18 AM, Andreas Färber wrote:
> 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,

I have over 12000 currently unread e-mails from that list.  They are  
not separated into "patch" and "other".  Even among the patches, they  
get posted at least twice due to the (unique to QEMU and KVM as far as  
I can tell) practice of reposting everything before a pull request.

There's just no way I can keep up with all of this, *plus* all the  
non-QEMU stuff I need to keep up with.  Sorry.  I generally rely on  
Alex to guide me on things like qdev/QOM.  Quite frankly, I didn't even  
realize I was using QOM.  I thought this was still qdev.  I even create  
it using qdev_create()...

> short of individual review comments. CPU, PReP PCI,
> Versatile PCI, ISA and more recently virtio series had been posted.

...which of those would make me think "hmm, there's something in here  
that I need to read before submitting patches for in-kernel mpic"?

I'm not trying to be difficult -- I'm just trying to point out that  
there's room for improvement in how the QEMU community communicates to  
developers what is expected.  Maybe an announcement list that just  
contains important updates and summaries?  Also, even starting on  
http://wiki.qemu.org/ I don't see any obvious path to get to  
QOMConventions.  It's not even linked to from the main QOM page.

I do understand the frustration of having to correct people on the same  
things over and over -- I experience it myself in other contexts.  But  
there are more constructive ways to deal with it than exclamation  
points.

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

I'm usually the first person to complain about bad copy and paste, but  
this is a situation where the KVM version of openpic is supposed to  
interface with the rest of the system in exactly the same way as the  
regular openpic.  It really does not make sense to write the glue code  
from scratch.  And it's not as if the rest of QEMU had just been fixed,  
and this patch reintroduces the old stuff.  I count 166 instances of  
"SysBusDevice busdev" and only 9 instances of "SysBusDevice  
parent_obj".  Perhaps these could all be fixed up in an automated way?

And "making other people even more work" goes both ways...

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

When would kvm_openpic_realize/unrealize/finalize get called?

Note that we must create the kernel side of the device in  
kvm_openpic_init(), because we need to return failure if it's not  
supported, so that the platform can fall back onto creating a normal  
QEMU openpic instead.

Also note that an in-kernel MPIC cannot be destroyed, without  
destroying the entire VM.  So I'm not sure what unrealize/finalize  
would do.

All of this is basically done the way Alex told/showed me to do it.

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

Sigh.  From what you said above, it seemed like you were asking me to  
do this:

#define KVM_OPENPIC(obj) FROM_SYSBUS(KVMOpenPICState, (obj))

Do you mean you want me to use DO_UPCAST directly?  And is it forbidden  
for DO_UPCAST to be opencoded?

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

It would have taken me much more than 5 minutes because I had no clue  
what "QOM realize" was, not to mention other unresolved questions about  
what you would want in a v3.

Alex, are you expecting a v3 or are you happy with the version you  
already put in ppc-next?

-Scott