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