Re: [Xen-devel] [PATCH for-4.13] x86/vlapic: allow setting APIC_SPIV_FOCUS_DISABLED in x2APIC mode

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel