[1/2] x86/iommu: avoid mapping the interrupt address range for hwdom
diff mbox series

Message ID 20190723154851.77627-2-roger.pau@citrix.com
State New
Headers show
Series
  • x86/iommu: fixes to hwdom_iommu_map
Related show

Commit Message

Roger Pau Monné July 23, 2019, 3:48 p.m. UTC
Current code only prevent mapping the lapic page into the guest
physical memory map. Expand the range to be 0xFEEx_xxxx as described
in the Intel VTd specification section 3.13 "Handling Requests to
Interrupt Address Range".

AMD also lists this address range in the AMD SR5690 Databook, section
2.4.4 "MSI Interrupt Handling and MSI to HT Interrupt Conversion".

Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/drivers/passthrough/x86/iommu.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

Comments

Jan Beulich July 23, 2019, 4:06 p.m. UTC | #1
On 23.07.2019 17:48, Roger Pau Monne wrote:
> Current code only prevent mapping the lapic page into the guest
> physical memory map. Expand the range to be 0xFEEx_xxxx as described
> in the Intel VTd specification section 3.13 "Handling Requests to
> Interrupt Address Range".

Right, and it being device side accesses that are of interest here,
the LAPIC address range (visible to the CPU only) shouldn't even
matter. Hence after some back and forth with myself I agree that
you remove the entire comment there.

> AMD also lists this address range in the AMD SR5690 Databook, section
> 2.4.4 "MSI Interrupt Handling and MSI to HT Interrupt Conversion".

Which raises the question about yet another patch to also exclude
the HT range.

> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Jan Beulich July 25, 2019, 10:22 a.m. UTC | #2
On 23.07.2019 17:48, Roger Pau Monne wrote:
> Current code only prevent mapping the lapic page into the guest
> physical memory map. Expand the range to be 0xFEEx_xxxx as described
> in the Intel VTd specification section 3.13 "Handling Requests to
> Interrupt Address Range".
> 
> AMD also lists this address range in the AMD SR5690 Databook, section
> 2.4.4 "MSI Interrupt Handling and MSI to HT Interrupt Conversion".
> 
> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I've committed this on the basis that it shouldn't hurt, but having
thought about this some more I'm not really sure I see the point:
The IOMMU special cases accesses into this range anyway, to redirect
lookup to the interrupt remapping table instead of the DMA remapping
one. Hence any mappings inserted into this range are simply useless,
but shouldn't otherwise hurt.

Jan
Roger Pau Monné July 25, 2019, 10:54 a.m. UTC | #3
On Thu, Jul 25, 2019 at 10:22:17AM +0000, Jan Beulich wrote:
> On 23.07.2019 17:48, Roger Pau Monne wrote:
> > Current code only prevent mapping the lapic page into the guest
> > physical memory map. Expand the range to be 0xFEEx_xxxx as described
> > in the Intel VTd specification section 3.13 "Handling Requests to
> > Interrupt Address Range".
> > 
> > AMD also lists this address range in the AMD SR5690 Databook, section
> > 2.4.4 "MSI Interrupt Handling and MSI to HT Interrupt Conversion".
> > 
> > Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I've committed this on the basis that it shouldn't hurt, but having
> thought about this some more I'm not really sure I see the point:
> The IOMMU special cases accesses into this range anyway, to redirect
> lookup to the interrupt remapping table instead of the DMA remapping
> one. Hence any mappings inserted into this range are simply useless,
> but shouldn't otherwise hurt.

Intel SDM contains:

"Software must ensure the second-level paging-structure entries are
programmed not to remap input addresses to the interrupt address
range. Hardware behavior is undefined for memory requests remapped to
the interrupt address range."

In section 3.13 (Handling Requests to Interrupt Address Range).

Since arch_iommu_hwdom_init/hwdom_iommu_map adds entries to both the
hap and the iommu page tables (or to hap only if shared) Xen should be
careful to not map this range because the iommu special cases this
range, but I'm not sure what hap does.

Thanks, Roger.
Paul Durrant July 25, 2019, 11:28 a.m. UTC | #4
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Roger Pau Monné
> Sent: 25 July 2019 11:54
> To: Jan Beulich <JBeulich@suse.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-devel@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH 1/2] x86/iommu: avoid mapping the interrupt address range for hwdom
> 
> On Thu, Jul 25, 2019 at 10:22:17AM +0000, Jan Beulich wrote:
> > On 23.07.2019 17:48, Roger Pau Monne wrote:
> > > Current code only prevent mapping the lapic page into the guest
> > > physical memory map. Expand the range to be 0xFEEx_xxxx as described
> > > in the Intel VTd specification section 3.13 "Handling Requests to
> > > Interrupt Address Range".
> > >
> > > AMD also lists this address range in the AMD SR5690 Databook, section
> > > 2.4.4 "MSI Interrupt Handling and MSI to HT Interrupt Conversion".
> > >
> > > Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >
> > I've committed this on the basis that it shouldn't hurt, but having
> > thought about this some more I'm not really sure I see the point:
> > The IOMMU special cases accesses into this range anyway, to redirect
> > lookup to the interrupt remapping table instead of the DMA remapping
> > one. Hence any mappings inserted into this range are simply useless,
> > but shouldn't otherwise hurt.
> 
> Intel SDM contains:
> 
> "Software must ensure the second-level paging-structure entries are
> programmed not to remap input addresses to the interrupt address
> range. Hardware behavior is undefined for memory requests remapped to
> the interrupt address range."
> 
> In section 3.13 (Handling Requests to Interrupt Address Range).
> 
> Since arch_iommu_hwdom_init/hwdom_iommu_map adds entries to both the
> hap and the iommu page tables (or to hap only if shared) Xen should be
> careful to not map this range because the iommu special cases this
> range, but I'm not sure what hap does.

Presumably such ranges should never end up in the P2M as they'd need to be trapped for emulation?

  Paul

> 
> Thanks, Roger.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
Jan Beulich July 25, 2019, 12:06 p.m. UTC | #5
On 25.07.2019 12:54, Roger Pau Monné  wrote:
> On Thu, Jul 25, 2019 at 10:22:17AM +0000, Jan Beulich wrote:
>> On 23.07.2019 17:48, Roger Pau Monne wrote:
>>> Current code only prevent mapping the lapic page into the guest
>>> physical memory map. Expand the range to be 0xFEEx_xxxx as described
>>> in the Intel VTd specification section 3.13 "Handling Requests to
>>> Interrupt Address Range".
>>>
>>> AMD also lists this address range in the AMD SR5690 Databook, section
>>> 2.4.4 "MSI Interrupt Handling and MSI to HT Interrupt Conversion".
>>>
>>> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> I've committed this on the basis that it shouldn't hurt, but having
>> thought about this some more I'm not really sure I see the point:
>> The IOMMU special cases accesses into this range anyway, to redirect
>> lookup to the interrupt remapping table instead of the DMA remapping
>> one. Hence any mappings inserted into this range are simply useless,
>> but shouldn't otherwise hurt.
> 
> Intel SDM contains:
> 
> "Software must ensure the second-level paging-structure entries are
> programmed not to remap input addresses to the interrupt address
> range. Hardware behavior is undefined for memory requests remapped to
> the interrupt address range."
> 
> In section 3.13 (Handling Requests to Interrupt Address Range).
> 
> Since arch_iommu_hwdom_init/hwdom_iommu_map adds entries to both the
> hap and the iommu page tables (or to hap only if shared) Xen should be
> careful to not map this range because the iommu special cases this
> range, but I'm not sure what hap does.

Hmm, in the shared-pt case this is indeed desirable in any event. I
have to admit that I didn't recall that even in the non-shared case
arch_iommu_hwdom_init() would fiddle with the HAP tables as well,
rather than just the IOMMU ones. I don't think this should remain
this way long term. Or was there a reason for this to be needed?

Jan

Patch
diff mbox series

diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 0fa6dcc3fd..88a87cf7a4 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -226,19 +226,9 @@  static bool __hwdom_init hwdom_iommu_map(const struct domain *d,
             return false;
     }
 
-    /*
-     * Check that it doesn't overlap with the LAPIC
-     * TODO: if the guest relocates the MMIO area of the LAPIC Xen should make
-     * sure there's nothing in the new address that would prevent trapping.
-     */
-    if ( has_vlapic(d) )
-    {
-        const struct vcpu *v;
-
-        for_each_vcpu(d, v)
-            if ( pfn == PFN_DOWN(vlapic_base_address(vcpu_vlapic(v))) )
-                return false;
-    }
+    /* Check that it doesn't overlap with the Interrupt Address Range. */
+    if ( pfn >= 0xfee00 && pfn <= 0xfeeff )
+        return false;
     /* ... or the IO-APIC */
     for ( i = 0; has_vioapic(d) && i < d->arch.hvm.nr_vioapics; i++ )
         if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) )