Re: [PATCH 4/4] x86/iommu: add PVH support to the inclusive options

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>,
	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 4/4] x86/iommu: add PVH support to the inclusive options
Date: Tue, 31 Jul 2018 17:15:22 +0200	[thread overview]
Message-ID: <20180731151522.gnhoydkm42vloldi@mac> (raw)
In-Reply-To: <5B60779E02000078001D9898@prv1-mh.provo.novell.com>

On Tue, Jul 31, 2018 at 08:52:14AM -0600, Jan Beulich wrote:
> >>> On 27.07.18 at 17:31, <roger.pau@citrix.com> wrote:
> > Several people have reported hardware issues (malfunctioning USB
> > controllers) due to iommu page faults. Those faults are caused by
> > missing RMRR (VTd) or IRVS (AMD-Vi) entries in the ACPI tables. Those
> > can be worked around on VTd hardware by manually adding RMRR entries
> > on the command line, this is however limited to Intel hardware and
> > quite cumbersome to do.
> > 
> > In order to solve those issues add PVH support to the inclusive option
> > that identity maps all regions marked as reserved in the memory map.
> > Note that regions used by devices emulated by Xen (LAPIC, IO-APIC or
> > PCIe MCFG regions) are specifically avoided. Note that this option
> > currently relies on no MSIX MMIO areas residing in a reserved region,
> > or else Xen won't be able to trap those accesses.
> 
> But that would be a firmware bug anyway: These are sub-ranges
> of PCI device BARs, and those must not overlap reserved ranges
> in E820.

Considering this is already a workaround for broken firmware I won't
be surprised that on certain hardware BARs overlap with reserved
ranges.

> > --- a/xen/drivers/passthrough/x86/iommu.c
> > +++ b/xen/drivers/passthrough/x86/iommu.c
> > @@ -20,6 +20,8 @@
> >  #include <xen/softirq.h>
> >  #include <xsm/xsm.h>
> >  
> > +#include <asm/apicdef.h>
> > +#include <asm/io_apic.h>
> 
> Why? You're looking for the guest view of things, which I don't
> think you can derive from definitions in these two headers.

I can use the domain state for the lapic and the ioapic...

> > +    /* ... or the PCIe MCFG regions. */
> > +    for ( i = 0; i < pci_mmcfg_config_num; i++ )
> > +    {
> > +        unsigned long addr = PFN_DOWN(pci_mmcfg_config[i].address);
> > +
> > +        if ( pfn >= addr + (pci_mmcfg_config[i].start_bus_number << 8) &&
> > +             pfn < addr + (pci_mmcfg_config[i].end_bus_number << 8) )
> > +            return false;
> > +    }
> 
> Same here - this would better use domain state.

... but pci mcfg regions are currently added after the iommu
initialization in the HVM dom0 builder. I can shuffle that call so
that iommu initialization is performed after the mcfg regions are
added.

Thanks, Roger.

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