Re: [PATCH v4 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.03.2022 16:00, Jane Malalane wrote:
> Add XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_xapic and
> XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_x2apic to report accelerated xapic
> and x2apic, on x86 hardware.
> No such features are currently implemented on AMD hardware.
> 
> For that purpose, also add an arch-specific "capabilities" parameter
> to struct xen_sysctl_physinfo.
> 
> Note that this interface is intended to be compatible with AMD so that
> AVIC support can be introduced in a future patch. Unlike Intel that
> has multiple controls for APIC Virtualization, AMD has one global
> 'AVIC Enable' control bit, so fine-graining of APIC virtualization
> control cannot be done on a common interface. Therefore, for xAPIC HW
> assisted virtualization support to be reported, HW must support
> virtualize_apic_accesses as well as apic_reg_virt.

Okay, here you now describe _what_ is being implemented, but I'm
afraid it still lacks justification (beyond making this re-usable for
AVIC, which imo can only be a secondary goal). You actually say ...

> For x2APIC HW
> assisted virtualization reporting, virtualize_x2apic_mode must be
> supported alongside apic_reg_virt and virtual_intr_delivery.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jane Malalane <jane.malalane@xxxxxxxxxx>
> 
> v4:
>  * Fallback to the original v2/v1 conditions for setting
>    assisted_xapic_available and assisted_x2apic_available so that in
>    the future APIC virtualization can be exposed on AMD hardware
>    since fine-graining of "AVIC" is not supported, i.e., AMD solely
>    uses "AVIC Enable". This also means that sysctl mimics what's
>    exposed in CPUID.

... more here: You claim similarity with CPUID. That's a possible route,
but we need to be clear that these CPUID flags are optimization hints
for the guest to use, while the new control is intended to be a functional
one. Hence it's not obvious that CPUID wants following, and not instead
the conditionals used in vmx_vlapic_msr_changed() (or yet something else).

What's worse though: What you say is true for x2APIC, but not for xAPIC.
Which effectively is in line with vmx_vlapic_msr_changed() and CPUID
handling also agreeing as far as x2APIC is concerned, but disagreeing on
the xAPIC side. I can only once again try to express that it may well be
that pre-existing code wants adjusting before actually making the changes
you're after.

> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -343,6 +343,16 @@ static int vmx_init_vmcs_config(bool bsp)
>              MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>      }
>  
> +    /* Check whether hardware supports accelerated xapic and x2apic. */
> +    if ( bsp )
> +    {
> +        assisted_xapic_available = (cpu_has_vmx_virtualize_apic_accesses &&
> +                                    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);
> +    }

If the conditions were to stay as they are, I'd like to suggest pulling
out the cpu_has_vmx_apic_reg_virt into the enclosing if()'s condition.
Additionally I think the comment wants to contain a pointer to what this
wants to remain in sync with. That other side may then want to gain a
comment pointing back here.

Jan