| Message ID | 20190723154851.77627-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/iommu: fixes to hwdom_iommu_map | expand |
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
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
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
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
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.
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
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
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(-)