Re: [Qemu-devel] [PATCH v7 3/3] sPAPR: EEH support for VFIO PCI device

[Alexander Graf]

On 28.05.14 00:27, Benjamin Herrenschmidt wrote:
> On Wed, 2014-05-28 at 00:22 +0200, Alexander Graf wrote:
> > > +void spapr_eeh_rtas_init(sPAPREnvironment *spapr)
> > > +{
> > > +    spapr_rtas_register("ibm,set-eeh-option",
> > > +                        rtas_ibm_set_eeh_option);
> > > +    spapr_rtas_register("ibm,get-config-addr-info2",
> > > +                        rtas_ibm_get_config_addr_info2);
> > > +    spapr_rtas_register("ibm,read-slot-reset-state2",
> > > +                        rtas_ibm_read_slot_reset_state2);
> > > +    spapr_rtas_register("ibm,set-slot-reset",
> > > +                        rtas_ibm_set_slot_reset);
> > > +    spapr_rtas_register("ibm,configure-pe",
> > > +                        rtas_ibm_configure_pe);
> > > +    spapr_rtas_register("ibm,slot-error-detail",
> > > +                        rtas_ibm_slot_error_detail);
> > This goes straight from RTAS to VFIO code. NAK.
> It would be much appreciated if instead of "NAK" you actually provided
> at least a sentence explaining how you think this should go...

You mean the same way I did the last few times?

> In any case, the above isn't the problem, we register rtas functions
> called rtas_ibm_*, that's fine.

They're called rtas_ibm, not rtas_vfio. They simply live in the wrong 
file for starters. Once you start moving them out of *vfio*.c and make 
sure you don't sneak in vfio headers into spapr_rtas.c all the 
abstraction should come naturally.

> The problem you have is with what's inside these functions, correct ?
> You want some kind of PCI Dev ops... am I understanding properly ?
> Instead of having them call the VFIO ioctl's directly.

We have a nice bus hierarchy. The RTAS call tells you the PHB's ID. The 
machine knows about all of its PHBs, so you can find that one.

From the PHB we can then find a PCIDevice by identifier if we received 
one in the RTAS call.

If we received a PE, we need to act on a virtual PE object (not 
available yet I think) which then would call at least into each VFIO 
container associated with that guest PE. We don't want to call into 
devices here, since we could have 2 VFIO devices in one PE and don't 
want to do a PE level operation twice.

If we want to simplify things (and I'm fine with that), we can also 
restrict a "guest PE" to only span at most a single VFIO container 
context and unlimited emulated devices. That way we don't need to worry 
about potential side effects.

I can't tell you what the "guest PE" would best be modeled at. Maybe a 
simple list/hash inside the PHB is the answer. But we need to ensure 
that the guest knows about its scope of action. And we need to make sure 
that we maintain abstraction levels to keep at least a minimum level of 
sanity for the reader.


Alex