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: 24 April 2017 11:09
> 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 10:34:15AM +0100, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Roger Pau Monne
> > > Sent: 24 April 2017 10:09
> > > 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 Fri, Apr 21, 2017 at 05:07:43PM +0100, Paul Durrant wrote:
> > > > > -----Original Message-----
> > > > > From: Roger Pau Monne [mailto:roger.pau@citrix.com]
> > > > > Sent: 20 April 2017 16:18
> > > > > To: xen-devel@lists.xenproject.org
> > > > > Cc: konrad.wilk@oracle.com; boris.ostrovsky@oracle.com; Roger Pau
> > > Monne
> > > > > <roger.pau@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei
> Liu
> > > > > <wei.liu2@citrix.com>; Jan Beulich <jbeulich@suse.com>; Andrew
> > > Cooper
> > > > > <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> > > > > 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.

What I mean is that we should have some form of range-based IO handler registration framework and then that can be used for port IO, MMIO and PCI config space. For external config space emulation then yes of course the external emulated needs to claim the whole space for that SBDF, but that's just a degenerate case of claiming a specific range within the SBDF.
Thus, if Xen can steer port IO, MMIO or PCI config accesses by range then we can potentially use that framework to register internal emulation handlers or a special emulation handler that sends the requests out to an ioreq server.

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

Indeed. Hopefully I've explained what I was thinking above.

  Paul

> 
> Roger.

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