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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.03.2022 17:37, Jane Malalane wrote:
> On 03/03/2022 11:37, Jan Beulich wrote:
>> 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.
> 
> 
> I've been thinking about this. Considering what you say, I propose:
> 
> - having assisted_x2apic_available = cpu_has_vmx_virtualize_x2apic_mode 
> && (cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtual_intr_delivery). 
> This would mean that on Intel CPUs has_assisted_x2apic==1 would signify 
> that there is at least "some" assistance*, whereas on AMD it would 
> signify that there is full assistance (assistance here meaning no VM-exits).
> * apic_reg_virt prevents VM exits on execution of RDMSR and 
> virtual_intr_delivery prevents VM exits on execution of RDMSR, from what 
> I've gathered.

I agree with this part of the plan.

> - having assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses 
> && cpu_has_vmx_apic_reg_virt because apic_reg_virt is neccessary for 
> "any" assistance.

Not exactly, aiui: cpu_has_vmx_virtualize_apic_accesses alone is beneficial
because a separate VM exit is then used, simplifying some internal handling.
There might actually be room for improvement in our handling of this, as we
presently use the exit qualification only to accelerate EOI writes.

> - Currently, the code only sets SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE if 
> "some" assistance is guaranteed but sets 
> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES even if no assistance is 
> guaranteed. So the adjustment to the pre-existing code that I propose is
> adding cpu_has_vmx_apic_reg_virt to the initial check in 
> vmx_vlapic_msr_changed():
> 
>   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 &&
> +         !cpu_has_vmx_apic_reg_virt &&
>            !virtualize_x2apic_mode )
>           return;

I'd suggest the opposite for the xAPIC case: Leave the condition here
unchanged, but consider tightening the condition for the CPUID flag.
That'll bring xAPIC handling more in line with x2APIC one.

> which would then eventually just be what I currently have:
> +    if ( !has_assisted_xapic(v->domain) &&
> +         !has_assisted_x2apic(v->domain) )
>           return;

Yes, the eventual form is expected in any event.

Jan

> So, essentially, the only difference from v4 would be 
> assisted_x2apic_available = (cpu_has_vmx_virtualize_x2apic_mode &&
>                            (cpu_has_vmx_apic_reg_virt ||
>                             cpu_has_vmx_virtual_intr_delivery));      
> 
> sysctl would now coincide with CPUID for xAPIC but not x2APIC (for CPUID 
> the condition is the AND of all features unlike the 
> assisted_x2apic_available proposed). IOW, it would follow the 
> conditionals used in vmx_vlapic_msr_changed(), if we take the change to 
> vmx_vlapic_msr_changed() above.
> 
> Thank you,
> 
> Jane.