[2/2] x86/iommu: avoid mapping the APIC configuration space for hwdom
diff mbox series

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

Commit Message

Roger Pau Monne July 23, 2019, 3:48 p.m. UTC
Current code only prevents mapping the io-apic page into the guest
physical memory map. Expand the range to be 0xFECx_xxxx as described
in the Intel 3 Series Chipset Datasheet section 3.3.1 "APIC
Configuration Space (FEC0_0000h–FECF_FFFFh)".

AMD also lists this address range in the AMD SR5690 Databook, section
2.4.2 "Non-SB IOAPIC Support".

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 | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Jan Beulich July 23, 2019, 4:09 p.m. UTC | #1
On 23.07.2019 17:48, Roger Pau Monne wrote:
> Current code only prevents mapping the io-apic page into the guest
> physical memory map. Expand the range to be 0xFECx_xxxx as described
> in the Intel 3 Series Chipset Datasheet section 3.3.1 "APIC
> Configuration Space (FEC0_0000h–FECF_FFFFh)".
> 
> AMD also lists this address range in the AMD SR5690 Databook, section
> 2.4.2 "Non-SB IOAPIC Support".

But that's chipset specific. I don't think we can blindly assume
this range. Just in case one small remark on the change itself as
well:

> @@ -229,10 +229,9 @@ static bool __hwdom_init hwdom_iommu_map(const struct domain *d,
>       /* 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) )
> -            return false;
> +    /* ... or the APIC Configuration Space. */
> +    if ( pfn >= 0xfec00 && pfn <= 0xfecff )
> +        return false;

Despite the chipset documentation calling it just APIC, in our
code I think it would be better if a connection to IO-APIC was
made, to avoid ambiguity.

Jan
Andrew Cooper July 23, 2019, 4:45 p.m. UTC | #2
On 23/07/2019 17:09, Jan Beulich wrote:
> On 23.07.2019 17:48, Roger Pau Monne wrote:
>> Current code only prevents mapping the io-apic page into the guest
>> physical memory map. Expand the range to be 0xFECx_xxxx as described
>> in the Intel 3 Series Chipset Datasheet section 3.3.1 "APIC
>> Configuration Space (FEC0_0000h–FECF_FFFFh)".
>>
>> AMD also lists this address range in the AMD SR5690 Databook, section
>> 2.4.2 "Non-SB IOAPIC Support".
> But that's chipset specific. I don't think we can blindly assume
> this range.

The IO-APIC has always lived in that region since its introduction, and
the location isn't even configurable on newer chipsets (If I've read the
SAD routing rules in Skylake correctly.  All that can be configured is
multiple IO-APICs being mapped adjacent to each other.)

While this isn't the inbound MSI range (and definitely fixed in the
architecture), it isn't plausibly going to change.

~Andrew
Jan Beulich July 24, 2019, 9:43 a.m. UTC | #3
On 23.07.2019 18:45, Andrew Cooper wrote:
> On 23/07/2019 17:09, Jan Beulich wrote:
>> On 23.07.2019 17:48, Roger Pau Monne wrote:
>>> Current code only prevents mapping the io-apic page into the guest
>>> physical memory map. Expand the range to be 0xFECx_xxxx as described
>>> in the Intel 3 Series Chipset Datasheet section 3.3.1 "APIC
>>> Configuration Space (FEC0_0000h–FECF_FFFFh)".
>>>
>>> AMD also lists this address range in the AMD SR5690 Databook, section
>>> 2.4.2 "Non-SB IOAPIC Support".
>> But that's chipset specific. I don't think we can blindly assume
>> this range.
> 
> The IO-APIC has always lived in that region since its introduction, and
> the location isn't even configurable on newer chipsets (If I've read the
> SAD routing rules in Skylake correctly.  All that can be configured is
> multiple IO-APICs being mapped adjacent to each other.)

I'm pretty sure I've seen IO-APICs outside that range.

I'm not entirely opposed to a change like this, but I think it wants to
come with better description (taking into account the chipset- rather
than architecture-defined-ness) and either retaining of the loop the
patch currently replaces, or an explanation why the loop is strictly
unnecessary.

Jan
Jan Beulich July 25, 2019, 12:47 p.m. UTC | #4
On 24.07.2019 11:43, Jan Beulich wrote:
> On 23.07.2019 18:45, Andrew Cooper wrote:
>> On 23/07/2019 17:09, Jan Beulich wrote:
>>> On 23.07.2019 17:48, Roger Pau Monne wrote:
>>>> Current code only prevents mapping the io-apic page into the guest
>>>> physical memory map. Expand the range to be 0xFECx_xxxx as described
>>>> in the Intel 3 Series Chipset Datasheet section 3.3.1 "APIC
>>>> Configuration Space (FEC0_0000h–FECF_FFFFh)".
>>>>
>>>> AMD also lists this address range in the AMD SR5690 Databook, section
>>>> 2.4.2 "Non-SB IOAPIC Support".
>>> But that's chipset specific. I don't think we can blindly assume
>>> this range.
>>
>> The IO-APIC has always lived in that region since its introduction, and
>> the location isn't even configurable on newer chipsets (If I've read the
>> SAD routing rules in Skylake correctly.  All that can be configured is
>> multiple IO-APICs being mapped adjacent to each other.)
> 
> I'm pretty sure I've seen IO-APICs outside that range.

 From my AMD Fam15 system:

<7>ACPI: Local APIC address 0xfee00000
<6>IOAPIC[0]: apic_id 0, version 33, address 0xfec00000, GSI 0-23
<6>IOAPIC[1]: apic_id 1, version 33, address 0xc8000000, GSI 24-55

Jan
Roger Pau Monne July 25, 2019, 1:34 p.m. UTC | #5
On Thu, Jul 25, 2019 at 12:47:01PM +0000, Jan Beulich wrote:
> On 24.07.2019 11:43, Jan Beulich wrote:
> > On 23.07.2019 18:45, Andrew Cooper wrote:
> >> On 23/07/2019 17:09, Jan Beulich wrote:
> >>> On 23.07.2019 17:48, Roger Pau Monne wrote:
> >>>> Current code only prevents mapping the io-apic page into the guest
> >>>> physical memory map. Expand the range to be 0xFECx_xxxx as described
> >>>> in the Intel 3 Series Chipset Datasheet section 3.3.1 "APIC
> >>>> Configuration Space (FEC0_0000h–FECF_FFFFh)".
> >>>>
> >>>> AMD also lists this address range in the AMD SR5690 Databook, section
> >>>> 2.4.2 "Non-SB IOAPIC Support".
> >>> But that's chipset specific. I don't think we can blindly assume
> >>> this range.
> >>
> >> The IO-APIC has always lived in that region since its introduction, and
> >> the location isn't even configurable on newer chipsets (If I've read the
> >> SAD routing rules in Skylake correctly.  All that can be configured is
> >> multiple IO-APICs being mapped adjacent to each other.)
> > 
> > I'm pretty sure I've seen IO-APICs outside that range.
> 
>  From my AMD Fam15 system:
> 
> <7>ACPI: Local APIC address 0xfee00000
> <6>IOAPIC[0]: apic_id 0, version 33, address 0xfec00000, GSI 0-23
> <6>IOAPIC[1]: apic_id 1, version 33, address 0xc8000000, GSI 24-55

Hm, I guess the only option is to then blacklist the proposed range
plus any of the pages of the io-apics on the system. I can send a new
version without dropping the current io-apic blacklisting, but then
I'm not sure there's much value in adding the FEC0_0000h–FECF_FFFFh
range.

Thanks, Roger.
Jan Beulich July 25, 2019, 1:38 p.m. UTC | #6
On 25.07.2019 15:34, Roger Pau Monné  wrote:
> On Thu, Jul 25, 2019 at 12:47:01PM +0000, Jan Beulich wrote:
>> On 24.07.2019 11:43, Jan Beulich wrote:
>>> On 23.07.2019 18:45, Andrew Cooper wrote:
>>>> On 23/07/2019 17:09, Jan Beulich wrote:
>>>>> On 23.07.2019 17:48, Roger Pau Monne wrote:
>>>>>> Current code only prevents mapping the io-apic page into the guest
>>>>>> physical memory map. Expand the range to be 0xFECx_xxxx as described
>>>>>> in the Intel 3 Series Chipset Datasheet section 3.3.1 "APIC
>>>>>> Configuration Space (FEC0_0000h–FECF_FFFFh)".
>>>>>>
>>>>>> AMD also lists this address range in the AMD SR5690 Databook, section
>>>>>> 2.4.2 "Non-SB IOAPIC Support".
>>>>> But that's chipset specific. I don't think we can blindly assume
>>>>> this range.
>>>>
>>>> The IO-APIC has always lived in that region since its introduction, and
>>>> the location isn't even configurable on newer chipsets (If I've read the
>>>> SAD routing rules in Skylake correctly.  All that can be configured is
>>>> multiple IO-APICs being mapped adjacent to each other.)
>>>
>>> I'm pretty sure I've seen IO-APICs outside that range.
>>
>>   From my AMD Fam15 system:
>>
>> <7>ACPI: Local APIC address 0xfee00000
>> <6>IOAPIC[0]: apic_id 0, version 33, address 0xfec00000, GSI 0-23
>> <6>IOAPIC[1]: apic_id 1, version 33, address 0xc8000000, GSI 24-55
> 
> Hm, I guess the only option is to then blacklist the proposed range
> plus any of the pages of the io-apics on the system. I can send a new
> version without dropping the current io-apic blacklisting, but then
> I'm not sure there's much value in adding the FEC0_0000h–FECF_FFFFh
> range.

Neither am I, hence my initial reaction. I'm surprised you don't see
much value in there anymore - after all it's quite a bit larger an
area that gets guarded against getting populated, as we're unlikely
to see many systems with this space fully (or even just mostly) used
by many, many IO-APICs. As said, I'd be fine acking the patch with
the loop left in place, and with the description refined.

Jan

Patch
diff mbox series

diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 88a87cf7a4..4dabfb2391 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -195,7 +195,7 @@  static bool __hwdom_init hwdom_iommu_map(const struct domain *d,
                                          unsigned long max_pfn)
 {
     mfn_t mfn = _mfn(pfn);
-    unsigned int i, type;
+    unsigned int type;
 
     /*
      * Set up 1:1 mapping for dom0. Default to include only conventional RAM
@@ -229,10 +229,9 @@  static bool __hwdom_init hwdom_iommu_map(const struct domain *d,
     /* 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) )
-            return false;
+    /* ... or the APIC Configuration Space. */
+    if ( pfn >= 0xfec00 && pfn <= 0xfecff )
+        return false;
     /*
      * ... or the PCIe MCFG regions.
      * TODO: runtime added MMCFG regions are not checked to make sure they