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

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

On 04/03/2022 08:17, 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 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 ...
Is the following any better...?

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

HW assisted xAPIC virtualization will be reported if HW, at the minimum, 
  supports virtualize_apic_accesses as this feature alone means that an 
access to the APIC page will cause an APIC-access VM exit. An 
APIC-access VM exit provides a VMM with information about the access 
causing the VM exit, unlike a regular EPT fault, thus simplifying some 
internal handling.

HW assisted x2APIC virtualization will be reported if HW supports 
virtualize_x2apic_mode and, at least, either apic_reg_virt or 
virtual_intr_delivery. This is due to apic_reg_virt and 
virtual_intr_delivery preventing a VM exit from occuring or at least 
replacing a regular EPT fault VM-exit with an APIC-access VM-exit on 
read and write APIC accesses, respectively.
This also means that sysctl follows the conditionals in 
vmx_vlapic_msr_changed().

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

I previously didn't add here any info about the assistance that each CPU 
bit provides to avoid repitition, as I talk about that in patch 2, but I 
interpreted from your comment that it might be helpful to add that here 
too.
>>>
>>>> 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.
I agree with you, by "assistance" in my response I meant "no VM-exits" 
but yes there is assistance, beyond absence of a VM exit, with 
virtualize_apic_acesses alone.>> - 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
Sounds good.


Thanks again,

Jane.