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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 24 April 2017 10:09
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; konrad.wilk@xxxxxxxxxx;
> boris.ostrovsky@xxxxxxxxxx; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>
> Subject: Re: [PATCH v2 1/9] xen/vpci: introduce basic handlers to trap
> accesses to the PCI config space
> 
> On Fri, Apr 21, 2017 at 05:07:43PM +0100, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Roger Pau Monne [mailto:roger.pau@xxxxxxxxxx]
> > > Sent: 20 April 2017 16:18
> > > To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > > Cc: konrad.wilk@xxxxxxxxxx; boris.ostrovsky@xxxxxxxxxx; Roger Pau
> Monne
> > > <roger.pau@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Wei Liu
> > > <wei.liu2@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Andrew
> Cooper
> > > <Andrew.Cooper3@xxxxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > > Subject: [PATCH v2 1/9] xen/vpci: introduce basic handlers to trap
> accesses
> > > to the PCI config space
> > >
> > > This functionality is going to reside in vpci.c (and the corresponding 
> > > vpci.h
> > > header), and should be arch-agnostic. The handlers introduced in this
> patch
> > > setup the basic functionality required in order to trap accesses to the 
> > > PCI
> > > config space, and allow decoding the address and finding the
> corresponding
> > > handler that should handle the access (although no handlers are
> > > implemented).
> > >
> > > Note that the traps to the PCI IO ports registers (0xcf8/0xcfc) are setup
> > > inside of a x86 HVM file, since that's not shared with other arches.
> > >
> > > A new XEN_X86_EMU_VPCI x86 domain flag is added in order to signal
> Xen
> > > whether
> > > a domain should use the newly introduced vPCI handlers, this is only
> enabled
> > > for PVH Dom0 at the moment.
> > >
> > > A very simple user-space test is also provided, so that the basic
> functionality
> > > of the vPCI traps can be asserted. This has been proven quite helpful
> during
> > > development, since the logic to handle partial accesses or accesses that
> > > expand
> > > across multiple registers is not trivial.
> > >
> > > The handlers for the registers are added to a red-black tree, that indexes
> > > them
> > > based on their offset. Since Xen needs to handle partial accesses to the
> > > registers and access that expand across multiple registers the logic in
> > > xen_vpci_{read/write} is kind of convoluted, I've tried to properly
> comment
> > > it
> > > in order to make it easier to understand.
> > >
> >
> > Since config space is not exactly huge, I'm wondering why you used an r-b
> tree rather than a direct map from register to handler?
> 
> Hello,
> 
> For local PCI the configuration space it's 256byte only, which means using 1/2
> a page (256 * 8) so that Xen can store a pointer for each possible register.
> The extended configuration space (ECAM) extends the space to 4K, which
> means we
> would use 8 pages per device (4096*8), I think that's too much.

Ok, but I still think that adding an r-b tree implementation is just more 
complexity in the way that io handlers are registered in Xen.

TBH, the whole thing needs a clean-up. We don't have proper range-based handler 
registration for port IO or MMIO at all (instead we potentially call the 
'accept' function for every handler for every I/O). We then have (IIRC) an 
ordered list for MSI-X BAR registrations and now you're proposing an r-b system 
for PCI config space. On top of that, there is then the rangeset based ioreq 
server selection that occurs if the I/O falls through all of this and needs 
sending outside Xen. There really has to be at least some scope for 
unificiation here; it's getting way too convoluted.

  Paul

> 
> Roger.

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