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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 04/11/2022 16:18, Roger Pau Monne wrote:
> The current reporting of the hardware assisted APIC options is done by
> checking "virtualize APIC accesses" which is not very helpful, as that
> feature doesn't avoid a vmexit, instead it does provide some help in
> order to detect APIC MMIO accesses in vmexit processing.
>
> Repurpose the current reporting of xAPIC assistance to instead report
> such feature as present when there's support for "TPR shadow" and
> "APIC register virtualization" because in that case some xAPIC MMIO
> register accesses are handled directly by the hardware, without
> requiring a vmexit.
>
> For symetry also change assisted x2APIC reporting to require
> "virtualize x2APIC mode" and "APIC register virtualization", dropping
> the option to also be reported when "virtual interrupt delivery" is
> available.  Presence of the "virtual interrupt delivery" feature will
> be reported using a different option.
>
> Fixes: 2ce11ce249 ('x86/HVM: allow per-domain usage of hardware virtualized 
> APIC')
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> I find the logic in vmx_vlapic_msr_changed() hard to follow, but I
> don't want to rewrite the function logic at this point.
> ---
> Changes since v1:
>  - Fix Viridian MSR tip conditions.
> ---
>  xen/arch/x86/hvm/viridian/viridian.c |  2 +-
>  xen/arch/x86/hvm/vmx/vmcs.c          |  8 ++++----
>  xen/arch/x86/hvm/vmx/vmx.c           | 25 ++++++++++++++++++-------
>  xen/arch/x86/traps.c                 |  4 +---
>  4 files changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/viridian/viridian.c 
> b/xen/arch/x86/hvm/viridian/viridian.c
> index 25dca93e8b..44eb3d0519 100644
> --- 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;

This check is broken before and after.  It needs to be keyed on
virtualised interrupt delivery, not register acceleration.

But doing this correctly needs a per-domain vintr setting, which we
don't have yet.

It is marginally less broken with this change, than without, but that's
not saying much.

>          if ( viridian_feature_mask(d) & HVMPV_hcall_ipi )
>              res->a |= CPUID4A_SYNTHETIC_CLUSTER_IPI;
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index a1aca1ec04..7bb96e1a8e 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1136,7 +1136,7 @@ static int construct_vmcs(struct vcpu *v)
>  
>      if ( !has_assisted_xapic(d) )
>          v->arch.hvm.vmx.secondary_exec_control &=
> -            ~SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
> +            ~SECONDARY_EXEC_APIC_REGISTER_VIRT;
>  
>      if ( cpu_has_vmx_secondary_exec_control )
>          __vmwrite(SECONDARY_VM_EXEC_CONTROL,
> @@ -2156,10 +2156,10 @@ int __init vmx_vmcs_init(void)
>      if ( !ret )
>      {
>          /* Check whether hardware supports accelerated xapic and x2apic. */
> -        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
> +        assisted_xapic_available = cpu_has_vmx_tpr_shadow &&
> +                                   cpu_has_vmx_apic_reg_virt;
>          assisted_x2apic_available = cpu_has_vmx_virtualize_x2apic_mode &&
> -                                    (cpu_has_vmx_apic_reg_virt ||
> -                                     cpu_has_vmx_virtual_intr_delivery);
> +                                    cpu_has_vmx_apic_reg_virt;

apic reg virt already depends on tpr shadow, so that part of the
condition is redundant.

virtualise x2apic mode and apic reg virt aren't dependent, but they do
only ever appear together in hardware.

Keeping the conditionals might be ok to combat a bad outer hypervisor,
but ...

>          register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1);
>      }
>  
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index e624b415c9..bf0fe3355c 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3405,25 +3405,29 @@ static void vmx_install_vlapic_mapping(struct vcpu *v)
>  
>  void vmx_vlapic_msr_changed(struct vcpu *v)
>  {
> +    bool virtualize_x2apic_mode = has_assisted_x2apic(v->domain) ||
> +                                  (cpu_has_vmx_virtualize_x2apic_mode &&
> +                                   cpu_has_vmx_virtual_intr_delivery);

... this is still wrong, and ...

>      struct vlapic *vlapic = vcpu_vlapic(v);
>      unsigned int msr;
>  
> -    if ( !has_assisted_xapic(v->domain) &&
> -         !has_assisted_x2apic(v->domain) )
> +    if ( !cpu_has_vmx_virtualize_apic_accesses &&
> +         !virtualize_x2apic_mode )
>          return;

... this surely cannot be right.

While attempting to figure ^ out, I've found yet another regression vs
4.16.  Because virt intr delivery is set in the execution controls
system-wide and not controlled per domain, we'll take a vmentry failure
on SKX/CLX/ICX when trying to build an HVM domain without xAPIC
acceleration.


This, combined with the ABI errors (/misfeatures) that we really don't
want to escape into the world but I haven't finished fixing yet, means
that the only appropriate course of action is to revert.

I'd really hoped to avoid a full revert, but we've run out of time.

~Andrew