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

[Andreas Färber]

Am 15.06.2013 00:57, schrieb 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.

FWIW I have over 53000 unread.

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

+1 - but surely not all changes would get communicated on such a list.

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

No, they can't, because FROM_SYSBUS() needs to be replaced with
per-device macros like I asked you to introduce for your new device. And
what code to put in which function is not automatic either.

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

Right. But if you're complaining about QOM and QOM realize, address your
complaints to Anthony instead. :)
We're a community, and sending patches in write-only mode conflicts with
my understanding of being e500 co-maintainer. I have no personal
advantage of this e500 KVM PIC, it just makes more work for me. So it's
your job to keep the code from bitrotting, especially when not everyone
can actually compile-test it.

Anyway, I have just sent Alex a fixup patch to squash as code says more
than a thousand words - now you have no more excuses for the future. :P

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

Today realize is called as part of your qdev_init_nofail() or via
object_property_set_bool().
In the future it will be called last thing before the machine starts
executing - therefore moving basic initializations into instance_init.
Documented in include/hw/qdev-core.h.

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

No, you don't have to and you shouldn't. SysBusDeviceClass::init is
legacy cruft. As you can see from my patch, kvm_openpic_realize allows
for even better error reporting.

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

I mentioned them for completeness. Which of them apply to your devices
is for you to determine.

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

Yes, Alex should've kept an eye on it before applying it, but your
colleague Bharat also has experiences with QOM if you have questions.

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

No. It's implemented using OBJECT_CHECK(), see include/qom/object.h.
KVM_OPENPIC() is supposed to allow any input type, not just SysBusDevice.

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

Answered in the link I gave (quote):
* DO use QOM cast macros (based on struct layout)
* DON'T rely on qdev's DO_UPCAST() macro (field names)

You're not supposed to touch parent fields outside VMStateDescription -
that's exactly what the QOM cast macros and per-type variables hide.

Andreas

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

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