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

[Scott Wood <scottwood@freescale.com>]

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