Re: PCI Passthrough ARM Design : Draft1

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2015-06-17 at 15:18 +0100, Stefano Stabellini wrote:
> On Wed, 17 Jun 2015, Ian Campbell wrote:
> > On Wed, 2015-06-17 at 14:40 +0100, Stefano Stabellini wrote:
> > > On Wed, 17 Jun 2015, Ian Campbell wrote:
> > > > On Wed, 2015-06-17 at 13:53 +0100, Stefano Stabellini wrote:
> > > > > On Wed, 17 Jun 2015, Ian Campbell wrote:
> > > > > > On Tue, 2015-06-16 at 18:16 +0100, Stefano Stabellini wrote:
> > > > > > > I wrote this before reading the rest of the thread with your alternative
> > > > > > > suggestion.  Both approaches can work, maybe it is true that having the
> > > > > > > guest requesting mappings is better. And we should certainly do the same
> > > > > > > thing for PVH and ARM guests.
> > > > > > > 
> > > > > > > If we have the guest requesting the mapping, obviously the hypercall
> > > > > > > implementation should check whether mapping the memory region has been
> > > > > > > previously allowed by the toolstack, that should still call
> > > > > > > xc_domain_iomem_permission.
> > > > > > 
> > > > > > Whoever makes the initial decision/setup we still need to decide what to
> > > > > > do about reads and writes to the device's BAR registers. Who deals with
> > > > > > these and how, and if writes are allowed and if so how is this then
> > > > > > reflected in the guest p2m.
> > > > > > 
> > > > > > I would very much prefer ARM and x86 PVH to use the same approach to
> > > > > > this.
> > > > > > 
> > > > > > For x86 HVM I believe QEMU takes care of this, by emulating the
> > > > > > reads/writes to CFG space and making hypercalls (domctls?) to update the
> > > > > > p2m. There is no QEMU to do this in the ARM or x86 PVH cases.
> > > > > > 
> > > > > > For x86 PV I believe pciback deals with the register reads/writes but it
> > > > > > doesn't need to do anything wrt the p2m because that is a guest internal
> > > > > > construct. This obviously doesn't apply to ARM or x86 PVH either since
> > > > > > the p2m is managed by Xen in both those cases.
> > > > > > 
> > > > > > So it seems that neither of the existing solutions are a good match for
> > > > > > either ARM or x86 PVH.
> > > > > > 
> > > > > > _If_ it were permissible to implement all BAR registers as r/o then this
> > > > > > might simplify things, however I suspect this is not something we can
> > > > > > get away with in the general case (i.e. a driver for a given PCI device
> > > > > > _knows_ that BAR<N> is writeable...).
> > > > > > 
> > > > > > So I think we do need to allow guest writes to the BAR registers. I
> > > > > > think it would be a bit strange (or even open potentially subtle
> > > > > > (security?) issues) to require the guest to write the BAR register and
> > > > > > to update the p2m to match as well.
> > > > > 
> > > > > Why would it be a security risk?
> > > > 
> > > > No idea, it seemed like a vague possibility. I did say "(security?)" not
> > > > "SECURITY" ;-)
> > > > 
> > > > >  I don't think it is important that the
> > > > > interface between pcifront and pciback refrects the hardware interface.
> > > > > But it is important that the interface is documented and complies to the
> > > > > expected behaviour.
> > > > > 
> > > > > I think that if we go with XENMEM_add_to_physmap_range, it would be OK
> > > > > for pcifront to both write to the vBAR (actually writing to the ring to
> > > > > pciback), then call XENMEM_add_to_physmap_range.
> > > > > 
> > > > > On the other hand, if we go with XEN_DOMCTL_memory_mapping, I would make
> > > > > the virtual BAR read-only, that is less preferable but still acceptable,
> > > > > as long as we document it.
> > > > 
> > > > Given that this appears to be how pv pci works today I think this turns
> > > > out to be preferable and saves us having to argue the toss about who
> > > > should do the p2m updates on a BAR update.
> > > > 
> > > > Which would point towards the toolstack doing the layout and telling
> > > > pciback somehow what the bars for each device are and that's it.
> > > > 
> > > > (making most of the rest of your reply moot, if you think its really
> > > > important to let guests move BARs about I can go through it again)
> > > 
> > > I think is unimportant, in fact having the guest setup the mappings was
> > > my original suggestion.
> > 
> > I can't reconcile the two halves of this statement, are you saying you
> > are still in favour of the guest dealing with the mappings or not?
> 
> Sorry!
> 
> I do not think is important to let guests move BARs. In fact having the
> toolstack doing the layout was my original suggestion and I still think
> can work correctly.

Ah, so s/guest/toolstack/ on the statement above makes it consistent,
phew!

> I don't have a strong feeling on whether it should be the toolstack or
> the guest to do the mapping. It can work either way. I would certainly
> avoid having Dom0 do the mapping.

I think it should be the toolstack. Allow the guest to do it means
implementing writeable BARs which is a much bigger yakk to shave and it
doesn't seem to be necessary.

Ian