Re: [PATCH v4 3/5] vpci/msix: carve p2m hole for MSIX MMIO regions

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Julien Grall <julien.grall@arm.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v4 3/5] vpci/msix: carve p2m hole for MSIX MMIO regions
Date: Tue, 20 Nov 2018 15:35:33 +0100	[thread overview]
Message-ID: <20181120143533.7kcwmaaqspoxdiku@mac> (raw)
In-Reply-To: <5BF2CF1202000078001FD86C@prv1-mh.provo.novell.com>

On Mon, Nov 19, 2018 at 07:56:18AM -0700, Jan Beulich wrote:
> >>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
> > Make sure the MSIX MMIO regions don't have p2m entries setup, so that
> > accesses to them trap into the hypervisor and can be handled by vpci.
> > 
> > Commit 042678762 ("x86/iommu: add map-reserved dom0-iommu option to
> > map reserved memory ranges") added mappings for all the reserved
> > regions into the PVH Dom0 p2m, and some of those reserved regions
> > might contain MSIX MMIO regions, hence the need to make sure there are
> > no mappings established.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> despite ...
> 
> > --- a/xen/drivers/vpci/header.c
> > +++ b/xen/drivers/vpci/header.c
> > @@ -89,6 +89,17 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
> >      bool map = cmd & PCI_COMMAND_MEMORY;
> >      unsigned int i;
> >  
> > +    /*
> > +     * Make sure there are no mappings in the MSIX MMIO areas, so that accesses
> > +     * can be trapped (and emulated) by Xen when the memory decoding bit is
> > +     * enabled.
> > +     *
> > +     * FIXME: punching holes after the p2m has been set up might be racy for
> > +     * DomU usage, needs to be revisited.
> > +     */
> > +    if ( map && !rom_only && vpci_make_msix_hole(pdev) )
> > +        return;
> 
> ... this still worrying me, as I think it'll need to be re-implemented
> altogether at that later point.

I expect this whole BAR mapping and unmapping to be limited to Dom0,
DomU should have a much more static physmap so that it can be setup by
the toolstack at creation time and the guest should not be allowed to
modify it.

That being set I also don't specially like this code, it's too
complex and requires iteration over all the devices for every
map/unmap operation. I have plans to improve this, which should be
discussed in a different email, but that's post-4.12.

> Afaict this patch is independent of the earlier two (leaving aside
> some fuzz that would be easy to resolve), which means it could
> go in ahead of them.

I plan to resend the series quite soon with the other comments fixed,
so let me know if I should drop this patch, I will also keep an eye on
the repo.

Thanks, Roger.

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