Re: [PATCH for-4.17 v2] hvm/apic: repurpose the reporting of the APIC assist options

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

On Mon, Nov 07, 2022 at 05:58:04PM +0100, Jan Beulich wrote:
> On 04.11.2022 17:18, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/viridian/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian/viridian.c
> > @@ -197,7 +197,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, 
> > uint32_t leaf,
> >          res->a = CPUID4A_RELAX_TIMER_INT;
> >          if ( viridian_feature_mask(d) & HVMPV_hcall_remote_tlb_flush )
> >              res->a |= CPUID4A_HCALL_REMOTE_TLB_FLUSH;
> > -        if ( !cpu_has_vmx_apic_reg_virt )
> > +        if ( !has_assisted_xapic(d) )
> >              res->a |= CPUID4A_MSR_BASED_APIC;
> 
> Isn't this too restrictive when considering x2APIC? IOW is there anything
> wrong with leaving this as is?

Using cpu_has_vmx_apic_reg_virt won't be correct, as a domain can have
it disabled now after this change.

When using x2APIC accesses will already be done using MSRs, so the
hint is not useful in that mode.

> > @@ -3432,6 +3436,10 @@ void vmx_vlapic_msr_changed(struct vcpu *v)
> >                  vmx_set_msr_intercept(v, MSR_X2APIC_PPR, VMX_MSR_R);
> >                  vmx_set_msr_intercept(v, MSR_X2APIC_TMICT, VMX_MSR_R);
> >                  vmx_set_msr_intercept(v, MSR_X2APIC_TMCCT, VMX_MSR_R);
> > +
> > +                v->arch.hvm.vmx.secondary_exec_control |=
> > +                    SECONDARY_EXEC_APIC_REGISTER_VIRT;
> > +
> >              }
> 
> Nit: stray trailing blank line inside the block.

Oh, thanks.  I will wait for Andrews feedback then, I think the extra
blank can likely be removed at commit if we agree this is OK.

> Everything else looks plausible to me, but from prior discussion I
> wonder whether the result isn't still going to be too coarse grained
> for Andrew's taste.

Ack, thanks, I think this is the best that we can do given the status
of the release, but would likely need to be quick or else it's gonna
be too late.

Roger.