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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

On 04/11/2022 14:22, 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.
>
> 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.
> ---
>   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 c4fa0a8b32..bafd8e90de 100644
> --- a/xen/arch/x86/hvm/viridian/viridian.c
> +++ b/xen/arch/x86/hvm/viridian/viridian.c
> @@ -201,7 +201,7 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t 
> leaf,
>            * Suggest x2APIC mode by default, unless xAPIC registers are hardware
>            * virtualized and x2APIC ones aren't.
>            */
> -        if ( !cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtualize_x2apic_mode )
> +        if ( !has_assisted_xapic(d) || has_assisted_x2apic(d) )
So, not sure why this is separated from patch 1 but stated this way it 
seems counterintuitive. We only want to use the viridian MSRs if they 
are going to be more efficient.. which I think is only in the case where 
we have neither an x2apic not an assisted xapic (hence we would trap for 
MMIO).
  Paul

>               res->a |= CPUID4A_MSR_BASED_APIC;
>           if ( viridian_feature_mask(d) & HVMPV_hcall_ipi )
>               res->a |= CPUID4A_SYNTHETIC_CLUSTER_IPI;