Re: [PATCH v2 1/9] xen/vpci: introduce basic handlers to trap accesses to the PCI config space

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 25 April 2017 09:27
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; konrad.wilk@oracle.com;
> boris.ostrovsky@oracle.com; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>
> Subject: Re: [PATCH v2 1/9] xen/vpci: introduce basic handlers to trap
> accesses to the PCI config space
> 
> On Mon, Apr 24, 2017 at 12:50:58PM +0100, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Roger Pau Monne
> > > Sent: 24 April 2017 12:03
> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> > > Cc: xen-devel@lists.xenproject.org; konrad.wilk@oracle.com;
> > > boris.ostrovsky@oracle.com; Ian Jackson <Ian.Jackson@citrix.com>; Wei
> Liu
> > > <wei.liu2@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew
> Cooper
> > > <Andrew.Cooper3@citrix.com>
> > > Subject: Re: [PATCH v2 1/9] xen/vpci: introduce basic handlers to trap
> > > accesses to the PCI config space
> > > IMHO I'm not sure Xen needs PCI register based trapping granularity. I
> would
> > > argue that whatever (IOREQ or Xen internal function) that wants to trap
> > > access
> > > to a specific PCI config device register needs to take care of all the
> > > registers for that device.
> > >
> >
> > Having distinct handers for distinct groups of makes sense though... e.g.
> being able to register a BAR handler for each BAR and then maybe an MSI-X
> capability handler for wherever that appears in the capability chain, etc. If
> you don't allow such registration at the top level then it ends up getting done
> at the next level.
> 
> Yes, that's what's done here. Handlers for specific registers are added at the
> next level (vPCI). See patches 5, 6, 8 or 9 for examples.
> 
> > That said, it may make more sense to have a top level of emulation that
> just handles all register reads and writes to config space and then a second
> level that has callbacks for BAR enumeration, bus master enable, MSI-X
> mask/unmask, etc.
> >
> > > I will look into hooking this code (vPCI) into the existing hvm_*_ioreq
> > > functionality, so that vPCI claims the full PCI config space for each device
> it
> > > manages.
> >
> > Cool.
> 
> I've been looking into this, and I have to say this whole emulation handling is
> a mess.

Too right. It's pretty horrible.

>The fact that Xen differentiates between internal and external
> (IOREQ)
> handlers so early in the code (hvmemul_do_io) makes it far from trivial to
> unify internal and external handlers, the more that external handlers have
> grown a complex set of infrastructure that internal handlers don't have at
> all.
> 

Indeed. Arguably that's because the external emulation is asynchronous and therefore requires more infrastructure but I think a lot of the abstraction is the wrong way round.

> Ideally I think the IOREQ filtering code should be generalized to apply to both
> internal and external handlers, and the difference between external and
> internal handlers should just be the set of functions that they use.

Exactly.

> External
> ones would always use generic IOREQ functions for pushing requests to the
> external emulators, while internal ones would just implement their own
> functions.
> 

Yep. We are definitely on the same wavelength :-)

> That said, I think this is a non-trivial amount of work, that will further
> delay this series. I don't see an easy way to integrate this code with the
> current IOREQ code at all. I'm willing to do this, but I would rather have this
> series merged first, so that other people can start working on PVH Dom0.
> 

Fair enough. If you've looked and come to that conclusion then I trust your judgement.

> ATM, the only think I can see that could be easily shared between the IOREQ
> code and vPCI is the PCI address decoding code.
> 

Yes, maybe some utility functions/macros can be generalized. It's not much, but it's a start. Once 4.9 is out of the door I think there should be an I/O emulation cleanup/rationalization item for 4.10 which of course I'm happy to help with.

Cheers,

  Paul

> Thanks, Roger.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel