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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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