| Message ID | 1615186802-5908-1-git-send-email-igor.druzhinin@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.15] vtd: make sure QI/IR are disabled before initialisation | expand |
On 08.03.2021 08:00, Igor Druzhinin wrote: > BIOS might pass control to Xen leaving QI and/or IR in enabled and/or > partially configured state. In case of x2APIC code path where EIM is > enabled early in boot - those are correctly disabled by Xen before any > attempt to configure. But for xAPIC that step is missing which was > proven to cause QI initialization failures on some ICX based platforms > where QI is left pre-enabled and partially configured by BIOS. And those systems then tell us to avoid use of x2APIC? I would have expected that on modern systems we wouldn't see such quirky firmware behavior anymore. Anyway, half a sentence to this effect might help here, as without such firmware behavior the only way to run into this ought to be use of "no-x2apic" on the command line. Which in turn might require justification (and potentially a fix elsewhere in the code to make use of that option unnecessary). > Unify the behaviour between x2APIC and xAPIC code paths keeping that in > line with what Linux does. > > Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with some editing of the description. If no other need for a v2 arises, I suppose whatever you come up with could be folded in while committing. Also Cc-ing Ian for a release ack. Jan
On 08/03/2021 08:18, Jan Beulich wrote: > On 08.03.2021 08:00, Igor Druzhinin wrote: >> BIOS might pass control to Xen leaving QI and/or IR in enabled and/or >> partially configured state. In case of x2APIC code path where EIM is >> enabled early in boot - those are correctly disabled by Xen before any >> attempt to configure. But for xAPIC that step is missing which was >> proven to cause QI initialization failures on some ICX based platforms >> where QI is left pre-enabled and partially configured by BIOS. > > And those systems then tell us to avoid use of x2APIC? I would have > expected that on modern systems we wouldn't see such quirky firmware > behavior anymore. Anyway, half a sentence to this effect might help > here, as without such firmware behavior the only way to run into > this ought to be use of "no-x2apic" on the command line. Which in > turn might require justification (and potentially a fix elsewhere in > the code to make use of that option unnecessary). > >> Unify the behaviour between x2APIC and xAPIC code paths keeping that in >> line with what Linux does. >> >> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > with some editing of the description. If no other need for a v2 > arises, I suppose whatever you come up with could be folded in > while committing. How about: "... But for xAPIC that step is missing which was proven to cause QI initialization failures on some ICX based platforms where QI is left pre-enabled and partially configured by BIOS. That problem becomes hard to avoid since those platforms are shipped with x2APIC opt out being advertised by default at the same time by firmware. ..." Igor
On 08.03.2021 15:52, Igor Druzhinin wrote: > On 08/03/2021 08:18, Jan Beulich wrote: >> On 08.03.2021 08:00, Igor Druzhinin wrote: >>> BIOS might pass control to Xen leaving QI and/or IR in enabled and/or >>> partially configured state. In case of x2APIC code path where EIM is >>> enabled early in boot - those are correctly disabled by Xen before any >>> attempt to configure. But for xAPIC that step is missing which was >>> proven to cause QI initialization failures on some ICX based platforms >>> where QI is left pre-enabled and partially configured by BIOS. >> >> And those systems then tell us to avoid use of x2APIC? I would have >> expected that on modern systems we wouldn't see such quirky firmware >> behavior anymore. Anyway, half a sentence to this effect might help >> here, as without such firmware behavior the only way to run into >> this ought to be use of "no-x2apic" on the command line. Which in >> turn might require justification (and potentially a fix elsewhere in >> the code to make use of that option unnecessary). >> >>> Unify the behaviour between x2APIC and xAPIC code paths keeping that in >>> line with what Linux does. >>> >>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com> >> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> with some editing of the description. If no other need for a v2 >> arises, I suppose whatever you come up with could be folded in >> while committing. > > How about: > > "... But for xAPIC that step is missing which was proven to cause QI > initialization failures on some ICX based platforms where QI is left > pre-enabled and partially configured by BIOS. That problem becomes hard > to avoid since those platforms are shipped with x2APIC opt out being > advertised by default at the same time by firmware. > ..." SGTM. Thanks, Jan
> From: Igor Druzhinin <igor.druzhinin@citrix.com> > Sent: Monday, March 8, 2021 3:00 PM > > BIOS might pass control to Xen leaving QI and/or IR in enabled and/or > partially configured state. In case of x2APIC code path where EIM is > enabled early in boot - those are correctly disabled by Xen before any > attempt to configure. But for xAPIC that step is missing which was > proven to cause QI initialization failures on some ICX based platforms > where QI is left pre-enabled and partially configured by BIOS. > > Unify the behaviour between x2APIC and xAPIC code paths keeping that in > line with what Linux does. > > Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> > --- > xen/arch/x86/apic.c | 2 +- > xen/drivers/passthrough/vtd/iommu.c | 12 +++++++++++- > xen/include/asm-x86/apic.h | 1 + > 3 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c > index 7497ddb..8ab8214 100644 > --- a/xen/arch/x86/apic.c > +++ b/xen/arch/x86/apic.c > @@ -47,7 +47,7 @@ static bool __read_mostly tdt_enabled; > static bool __initdata tdt_enable = true; > boolean_param("tdt", tdt_enable); > > -static bool __read_mostly iommu_x2apic_enabled; > +bool __read_mostly iommu_x2apic_enabled; > > static struct { > int active; > diff --git a/xen/drivers/passthrough/vtd/iommu.c > b/xen/drivers/passthrough/vtd/iommu.c > index d136fe3..4aa7a31 100644 > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -2080,7 +2080,7 @@ static int __must_check init_vtd_hw(void) > u32 sts; > > /* > - * Basic VT-d HW init: set VT-d interrupt, clear VT-d faults. > + * Basic VT-d HW init: set VT-d interrupt, clear VT-d faults, etc. > */ > for_each_drhd_unit ( drhd ) > { > @@ -2090,6 +2090,16 @@ static int __must_check init_vtd_hw(void) > > clear_fault_bits(iommu); > > + /* > + * Disable interrupt remapping and queued invalidation if > + * already enabled by BIOS in case we've not initialized it yet. > + */ > + if ( !iommu_x2apic_enabled ) > + { > + disable_intremap(iommu); > + disable_qinval(iommu); > + } > + > spin_lock_irqsave(&iommu->register_lock, flags); > sts = dmar_readl(iommu->reg, DMAR_FECTL_REG); > sts &= ~DMA_FECTL_IM; > diff --git a/xen/include/asm-x86/apic.h b/xen/include/asm-x86/apic.h > index 8ddb896..2fe54bb 100644 > --- a/xen/include/asm-x86/apic.h > +++ b/xen/include/asm-x86/apic.h > @@ -24,6 +24,7 @@ enum apic_mode { > APIC_MODE_X2APIC /* x2APIC mode - common for large MP machines > */ > }; > > +extern bool iommu_x2apic_enabled; > extern u8 apic_verbosity; > extern bool directed_eoi_enabled; > > -- > 2.7.4
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c index 7497ddb..8ab8214 100644 --- a/xen/arch/x86/apic.c +++ b/xen/arch/x86/apic.c @@ -47,7 +47,7 @@ static bool __read_mostly tdt_enabled; static bool __initdata tdt_enable = true; boolean_param("tdt", tdt_enable); -static bool __read_mostly iommu_x2apic_enabled; +bool __read_mostly iommu_x2apic_enabled; static struct { int active; diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index d136fe3..4aa7a31 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -2080,7 +2080,7 @@ static int __must_check init_vtd_hw(void) u32 sts; /* - * Basic VT-d HW init: set VT-d interrupt, clear VT-d faults. + * Basic VT-d HW init: set VT-d interrupt, clear VT-d faults, etc. */ for_each_drhd_unit ( drhd ) { @@ -2090,6 +2090,16 @@ static int __must_check init_vtd_hw(void) clear_fault_bits(iommu); + /* + * Disable interrupt remapping and queued invalidation if + * already enabled by BIOS in case we've not initialized it yet. + */ + if ( !iommu_x2apic_enabled ) + { + disable_intremap(iommu); + disable_qinval(iommu); + } + spin_lock_irqsave(&iommu->register_lock, flags); sts = dmar_readl(iommu->reg, DMAR_FECTL_REG); sts &= ~DMA_FECTL_IM; diff --git a/xen/include/asm-x86/apic.h b/xen/include/asm-x86/apic.h index 8ddb896..2fe54bb 100644 --- a/xen/include/asm-x86/apic.h +++ b/xen/include/asm-x86/apic.h @@ -24,6 +24,7 @@ enum apic_mode { APIC_MODE_X2APIC /* x2APIC mode - common for large MP machines */ }; +extern bool iommu_x2apic_enabled; extern u8 apic_verbosity; extern bool directed_eoi_enabled;
BIOS might pass control to Xen leaving QI and/or IR in enabled and/or partially configured state. In case of x2APIC code path where EIM is enabled early in boot - those are correctly disabled by Xen before any attempt to configure. But for xAPIC that step is missing which was proven to cause QI initialization failures on some ICX based platforms where QI is left pre-enabled and partially configured by BIOS. Unify the behaviour between x2APIC and xAPIC code paths keeping that in line with what Linux does. Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com> --- xen/arch/x86/apic.c | 2 +- xen/drivers/passthrough/vtd/iommu.c | 12 +++++++++++- xen/include/asm-x86/apic.h | 1 + 3 files changed, 13 insertions(+), 2 deletions(-)