xen/arch/x86/hvm/vlapic.c | 9 +++++++++ 1 file changed, 9 insertions(+)
Current code unconditionally prevents setting APIC_SPIV_FOCUS_DISABLED
regardless of the processor model, which is not correct according to
the specification.
Fix it by allowing setting APIC_SPIV_FOCUS_DISABLED based on the
domain cpuid policy.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Juergen Gross <jgross@suse.com>
---
xen/arch/x86/hvm/vlapic.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 9466258d6f..b318b4ed5c 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -977,6 +977,7 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t msr_content)
{
struct vlapic *vlapic = vcpu_vlapic(v);
uint32_t offset = (msr - MSR_X2APIC_FIRST) << 4;
+ const struct cpuid_policy *cpuid = v->domain->arch.cpuid;
/* The timer handling at least is unsafe outside of current context. */
ASSERT(v == current);
@@ -993,6 +994,14 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t msr_content)
case APIC_SPIV:
if ( msr_content & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
+ /*
+ * APIC_SPIV_FOCUS_DISABLED is not supported on
+ * Intel Pentium 4 and Xeon processors.
+ */
+ ((cpuid->x86_vendor != X86_VENDOR_INTEL ||
+ get_cpu_family(cpuid->basic.raw_fms, NULL,
+ NULL) != 15) ?
+ APIC_SPIV_FOCUS_DISABLED : 0) |
(VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI
? APIC_SPIV_DIRECTED_EOI : 0)) )
return X86EMUL_EXCEPTION;
--
2.24.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn Wed, Nov 20, 2019 at 06:39:20PM +0100, Roger Pau Monne wrote: > case APIC_SPIV: > if ( msr_content & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED | > + /* > + * APIC_SPIV_FOCUS_DISABLED is not supported on s/not supported/reserved/ would be better I think. Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 20.11.2019 18:39, Roger Pau Monne wrote:
> Current code unconditionally prevents setting APIC_SPIV_FOCUS_DISABLED
> regardless of the processor model, which is not correct according to
> the specification.
>
> Fix it by allowing setting APIC_SPIV_FOCUS_DISABLED based on the
> domain cpuid policy.
Would you mind adding half a sentence to clarify whether this is a
problem observed in practice, or simply found by code inspection?
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -977,6 +977,7 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t msr_content)
> {
> struct vlapic *vlapic = vcpu_vlapic(v);
> uint32_t offset = (msr - MSR_X2APIC_FIRST) << 4;
> + const struct cpuid_policy *cpuid = v->domain->arch.cpuid;
We commonly name such variables "cp".
> @@ -993,6 +994,14 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t msr_content)
>
> case APIC_SPIV:
> if ( msr_content & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
> + /*
> + * APIC_SPIV_FOCUS_DISABLED is not supported on
> + * Intel Pentium 4 and Xeon processors.
> + */
> + ((cpuid->x86_vendor != X86_VENDOR_INTEL ||
> + get_cpu_family(cpuid->basic.raw_fms, NULL,
> + NULL) != 15) ?
> + APIC_SPIV_FOCUS_DISABLED : 0) |
Do we actually need this (virtual) family check? If I'm not mistaken
- physical family 0xf CPUs don't support x2APIC,
- in xAPIC mode, writing reserved bits wouldn't fault - such writes
would simply be ignored.
Therefore I see no risk in always permitting the bit to get set
(like the corresponding xAPIC logic does, sadly by using a literal
number instead of APIC_SPIV_*). On the contrary, code seeing an
x2APIC might assume the flag to be settable regardless of family
(because implicitly it wouldn't expect to be on family 0xf).
If yes - "Xeon" is way too broad a statement here. Yes, Intel's doc
as well as old code elsewhere in Xen say so too, but since then the
name was used for a large range of family 6 server CPUs as well.
> (VLAPIC_VERSION & APIC_LVR_DIRECTED_EOI
> ? APIC_SPIV_DIRECTED_EOI : 0)) )
> return X86EMUL_EXCEPTION;
>
If you already take care of this family difference, wouldn't you
better address the other one (affecting the low 4 bits) as well
(at least in the xAPIC case)? (FAOD if the virtual family
dependency was dropped above, then I'd rather not see one
introduced for this case either.)
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On Thu, Nov 21, 2019 at 10:48:16AM +0100, Jan Beulich wrote:
> On 20.11.2019 18:39, Roger Pau Monne wrote:
> > Current code unconditionally prevents setting APIC_SPIV_FOCUS_DISABLED
> > regardless of the processor model, which is not correct according to
> > the specification.
> >
> > Fix it by allowing setting APIC_SPIV_FOCUS_DISABLED based on the
> > domain cpuid policy.
>
> Would you mind adding half a sentence to clarify whether this is a
> problem observed in practice, or simply found by code inspection?
This was found by trying to boot the pvshim with x2APIC enabled, since
Xen will try to disable focus at init_bsp_APIC for the BSP. Note that
AFAICT this is not done for the APs, and setup_local_APIC will also
avoid setting the focus disabled bit.
I think the setting of the focus disable bit in init_bsp_APIC can be
removed, but that's a different issue.
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -977,6 +977,7 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t msr_content)
> > {
> > struct vlapic *vlapic = vcpu_vlapic(v);
> > uint32_t offset = (msr - MSR_X2APIC_FIRST) << 4;
> > + const struct cpuid_policy *cpuid = v->domain->arch.cpuid;
>
> We commonly name such variables "cp".
Ack, in any case this is going away.
>
> > @@ -993,6 +994,14 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t msr_content)
> >
> > case APIC_SPIV:
> > if ( msr_content & ~(APIC_VECTOR_MASK | APIC_SPIV_APIC_ENABLED |
> > + /*
> > + * APIC_SPIV_FOCUS_DISABLED is not supported on
> > + * Intel Pentium 4 and Xeon processors.
> > + */
> > + ((cpuid->x86_vendor != X86_VENDOR_INTEL ||
> > + get_cpu_family(cpuid->basic.raw_fms, NULL,
> > + NULL) != 15) ?
> > + APIC_SPIV_FOCUS_DISABLED : 0) |
>
> Do we actually need this (virtual) family check? If I'm not mistaken
> - physical family 0xf CPUs don't support x2APIC,
I assumed we exposed x2APIC support to all guests, regardless of
whether the underlying hardware supports it or not.
> - in xAPIC mode, writing reserved bits wouldn't fault - such writes
> would simply be ignored.
> Therefore I see no risk in always permitting the bit to get set
> (like the corresponding xAPIC logic does, sadly by using a literal
> number instead of APIC_SPIV_*). On the contrary, code seeing an
> x2APIC might assume the flag to be settable regardless of family
> (because implicitly it wouldn't expect to be on family 0xf).
Right. TBH I had my doubts about this and I was also considering to
always allow setting the bit, at the end of day this is all
emulated. Since you agree I will drop the family dependence always
allow setting focus disabled.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.