Re: [PATCH v4 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.03.2022 16:00, Jane Malalane wrote:
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3344,16 +3344,11 @@ static void vmx_install_vlapic_mapping(struct vcpu *v)
>  
>  void vmx_vlapic_msr_changed(struct vcpu *v)
>  {
> -    int virtualize_x2apic_mode;
>      struct vlapic *vlapic = vcpu_vlapic(v);
>      unsigned int msr;
>  
> -    virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt ||
> -                                cpu_has_vmx_virtual_intr_delivery) &&
> -                               cpu_has_vmx_virtualize_x2apic_mode );
> -
> -    if ( !cpu_has_vmx_virtualize_apic_accesses &&
> -         !virtualize_x2apic_mode )
> +    if ( !has_assisted_xapic(v->domain) &&
> +         !has_assisted_x2apic(v->domain) )
>          return;

This is not an equivalent replacement: The earlier condition was not the
AND of all three sub-features. This is the reason for ...

> @@ -3363,28 +3358,24 @@ void vmx_vlapic_msr_changed(struct vcpu *v)
>      if ( !vlapic_hw_disabled(vlapic) &&
>           (vlapic_base_address(vlapic) == APIC_DEFAULT_PHYS_BASE) )
>      {
> -        if ( virtualize_x2apic_mode && vlapic_x2apic_mode(vlapic) )
> +        if ( has_assisted_x2apic(v->domain) && vlapic_x2apic_mode(vlapic) )
>          {
>              v->arch.hvm.vmx.secondary_exec_control |=
>                  SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE;
> -            if ( cpu_has_vmx_apic_reg_virt )
> -            {
> -                for ( msr = MSR_X2APIC_FIRST;
> -                      msr <= MSR_X2APIC_FIRST + 0xff; msr++ )
> -                    vmx_clear_msr_intercept(v, msr, VMX_MSR_R);
>  
> -                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);
> -            }
> -            if ( cpu_has_vmx_virtual_intr_delivery )
> -            {
> -                vmx_clear_msr_intercept(v, MSR_X2APIC_TPR, VMX_MSR_W);
> -                vmx_clear_msr_intercept(v, MSR_X2APIC_EOI, VMX_MSR_W);
> -                vmx_clear_msr_intercept(v, MSR_X2APIC_SELF, VMX_MSR_W);
> -            }
> +            for ( msr = MSR_X2APIC_FIRST;
> +                  msr <= MSR_X2APIC_FIRST + 0xff; msr++ )
> +                vmx_clear_msr_intercept(v, msr, VMX_MSR_R);
> +
> +            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);
> +
> +            vmx_clear_msr_intercept(v, MSR_X2APIC_TPR, VMX_MSR_W);
> +            vmx_clear_msr_intercept(v, MSR_X2APIC_EOI, VMX_MSR_W);
> +            vmx_clear_msr_intercept(v, MSR_X2APIC_SELF, VMX_MSR_W);
>          }

... you wanting to make these adjustments, but at the same time it means
with certain feature combinations we would now intercept all x2APIC MSR
accesses when some don't need intercepting, which may slow things down
for guests.

Just to be clear - the main part of the discussion imo continues to be
needed on patch 1, to sort what dependencies on features we want where.
One that's clear, what's wanted here should be mostly straightforward.

Jan