qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Scott Wood
Subject: Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
Date: Wed, 19 Jun 2013 17:25:34 -0500

On 06/16/2013 02:11:58 PM, Andreas Färber wrote:
Am 15.06.2013 00:57, schrieb Scott Wood:
> ...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.

Sure.  It doesn't need to be perfect to be a big help.

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

It wasn't "write-only mode" -- I've accepted and acted on plenty of other feedback in this and other patchsets (in fact, some of that feedback told me specifically to use things like qdev_init_nofail, which apparently is deprecated). And I'm not opposed to cleaning up/modernizing existing e500 code (or delegating it to a coworker, which includes Alex, who works for us part-time) if it's clear what is expected. I was just looking for help with a part of QEMU that I find pretty opaque, and was put off by the tone of the initial complaint.

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

Thanks.

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

My concern isn't how good the error reporting is, but how early we detect the error. If qdev_init_nofail() goes away, then when will platform code have a chance to create a non-KVM openpic device instead? From "devices must not create children during @realize" it sounds like it might be too late at that point.

Originally, I had the creation of the kernel MPIC object done by a factory function. If the kernel object was able to be created, then the function created a qdev object and passed the kernel ID as a property. Otherwise, it returned NULL so that platform code knew to create a "normal" openpic device (or error out if the user explicitly asked for an in-kernel pic). Alex asked me to change this to the qdev_init_nofail() approach. It sounds like we may need to change back.

-Scott



reply via email to

[Prev in Thread] Current Thread [Next in Thread]