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

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

On 11/02/2022 11:46, 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 11.02.2022 12:29, Roger Pau Monné wrote:
>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote:
>>> On 10/02/2022 10:03, Roger Pau Monné wrote:
>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>> index 7ab15e07a0..4060aef1bd 100644
>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>> @@ -343,6 +343,15 @@ 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;
>>>>> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
>>>>> +                                     cpu_has_vmx_virtual_intr_delivery) 
>>>>> &&
>>>>> +                                    cpu_has_vmx_virtualize_x2apic_mode;
>>>>
>>>> I've been think about this, and it seems kind of asymmetric that for
>>>> xAPIC mode we report hw assisted support only with
>>>> virtualize_apic_accesses available, while for x2APIC we require
>>>> virtualize_x2apic_mode plus either apic_reg_virt or
>>>> virtual_intr_delivery.
>>>>
>>>> I think we likely need to be more consistent here, and report hw
>>>> assisted x2APIC support as long as virtualize_x2apic_mode is
>>>> available.
>>>>
>>>> This will likely have some effect on patch 2 also, as you will have to
>>>> adjust vmx_vlapic_msr_changed.
>>>>
>>>> Thanks, Roger.
>>>
>>> Any other thoughts on this? As on one hand it is asymmetric but also
>>> there isn't much assistance with only virtualize_x2apic_mode set as, in
>>> this case, a VM exit will be avoided only when trying to access the TPR
>>> register.
>>
>> I've been thinking about this, and reporting hardware assisted
>> x{2}APIC virtualization with just
>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or
>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While
>> those provide some assistance to the VMM in order to handle APIC
>> accesses, it will still require a trap into the hypervisor to handle
>> most of the accesses.
>>
>> So maybe we should only report hardware assisted support when the
>> mentioned features are present together with
>> SECONDARY_EXEC_APIC_REGISTER_VIRT?
> 
> Not sure - "some assistance" seems still a little better than none at all.
> Which route to go depends on what exactly we intend the bit to be used for.
> 
> Jan
> 
True. I intended this bit to be specifically for enabling 
assisted_x{2}apic. So, would it be inconsistent to report hardware 
assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE 
but still claim that x{2}apic is virtualized if no MSR accesses are 
intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you 
say, the guest gets at least "some assistance" instead of none but we 
still claim x{2}apic virtualization when it is actually complete? Maybe 
I could also add a comment alluding to this in the xl documentation.

Thanks,

Jane.