Re: [Xen-devel] [PATCH v6 11/11] xen, common: do not implicitly permit access to mapped I/O memory

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Mon, 2014-04-28 at 14:54 +0100, Jan Beulich wrote:
> >>> On 28.04.14 at 15:33, <Ian.Campbell@xxxxxxxxxxxxx> wrote:
> > On Tue, 2014-04-22 at 10:12 +0100, Jan Beulich wrote:
> >> >>> On 21.04.14 at 15:45, <avanzini.arianna@xxxxxxxxx> wrote:
> >> >  tools/libxl/libxl_create.c | 17 +++++++++++++++++
> >> >  tools/libxl/libxl_pci.c    | 24 ++++++++++++++++++------
> >> 
> >> I doubt that doing it at this layer is correct: There are quite a few uses
> >> of xc_domain_memory_mapping() in qemu,
> > 
> > Here the toolstack grants permission to access the I/O mem etc
> > associated with a PCI device. Then later qemu uses it via the mapping.
> 
> Am I reading this right to mean that qemu doesn't really care about
> the current side effect of granting permissions?

I don't think so, it just cares that it happened at some point.

>  In that case the
> change would seem to be okay.
> 
> >> so I think as a first step you should retain libxc functionality
> >> unchanged by issuing both domctls there.
> > 
> > This doesn't fit very well with the use of non-PCI passthrough of
> > devices without qemu on ARM -- i.e. the direct iomem=[list] stuff which
> > Arianna is trying to make work with this series. Or does it?
> 
> Why would it not? How the resources got specified (iomem= or
> pci=) shouldn't matter here.

One path goes via qemu calling xc_domain_memory_map and the other via
libx[cl] having to do it, which is inconsistent and a bit confusing.
Having qemu continue to rely on just xc_domain_memory_map (which now
does both things) for pci= means we can't easily use
xc_domain_memory_map for the iomem= case (which wants only one bit of
functionality, having already called granted the perms explicitly)

> >> > @@ -886,14 +889,14 @@ static int do_pci_add(libxl__gc *gc, uint32_t 
> >> > domid, libxl_device_pci *pcidev, i
> >> >                  continue;
> >> >              size = end - start + 1;
> >> >              if (start) {
> >> > -                if (flags & PCI_BAR_IO) {
> >> > +                if ((flags & PCI_BAR_IO) && !hvm) {
> >> >                      rc = xc_domain_ioport_permission(ctx->xch, domid, 
> >> > start, size, 1);
> >> 
> >> If this whole thing gets unified, treating I/O ports and MMIO differently
> >> isn't an optimal choice.
> > 
> > Where are these granted for x86 HVM guests today -- I don't see qemu
> > calling this function. I must be missing something.
> 
> Right here I thought - what the patch does is _exclude_ HVM from
> being given the permission here (in favor of doing it elsewhere), and
> all I was trying to understand was whether we really need to go
> down the same route as in xend again where for everything there
> are two places where things get done.

Yes. First thing would be to understand where this does happen if not
here or in qemu.

Ian.


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