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.coop...@citrix.com>
> Signed-off-by: Jane Malalane <jane.malal...@citrix.com>
>
> 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
