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

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Mon, Apr 24, 2017 at 10:34:15AM +0100, Paul Durrant wrote:
> > -----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.

But this complexity is completely hidden inside of the io handler itself that
traps the access to 0xcf8/cfc (or ECAM areas).

Do you mean that you would like this functionality to made available to IOREQ
clients also, so that they could register handlers for specific PCI registers
without owning the full configuration space of such device?

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

One way or another Xen needs to track handlers for the PCI config space, and
currently this is not implemented inside of Xen.

The MSI-X BAR tracking will go away once this code is also used for
PCI-passthrough to DomUs. The msixtbl code is just extremely messy, because
MSI-X interrupt handling for passthrough devices is partially handled in QEMU
and partially inside of Xen.

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

Yes, I agree that there's some room for sharing here, the address decoding done
in hvm_select_ioreq_server for PCI could be reused for vPCI also, it's just
that all this code expects a IOREQ server, and vPCI is not going to be an IOREQ
server.

Roger.

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