| Message ID | 20191120173920.8705-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-4.13] x86/vlapic: allow setting APIC_SPIV_FOCUS_DISABLED in x2APIC mode | expand |
On 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.
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
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.
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;
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(+)