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

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

On 03/03/2022 11:37, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
> unless you have verified the sender and know the content is safe.
> 
> 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.
- 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.

- 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;


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

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.