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

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

On 15/02/2022 10:19, 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 15.02.2022 11:14, Jane Malalane wrote:
>> On 15/02/2022 07:09, 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 14.02.2022 18:09, Jane Malalane wrote:
>>>> On 14/02/2022 13:18, 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 14.02.2022 14:11, Jane Malalane wrote:
>>>>>> 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.
>>>>>>>
>>>>>> 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.
>>>>>
>>>>> To rephrase my earlier point: Which kind of decisions are the consumer(s)
>>>>> of us reporting hardware assistance going to take? In how far is there a
>>>>> risk that "some assistance" is overall going to lead to a loss of
>>>>> performance? I guess I'd need to see comment and actual code all in one
>>>>> place ...
>>>>>
>>>> So, I was thinking of adding something along the lines of:
>>>>
>>>> +=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
>>>> +Enables or disables hardware assisted virtualization for xAPIC. This
>>>> +allows accessing APIC registers without a VM-exit. Notice enabling
>>>> +this does not guarantee full virtualization for xAPIC, as this can
>>>> +only be achieved if hardware supports “APIC-register virtualization”
>>>> +and “virtual-interrupt delivery”. The default is settable via
>>>> +L<xl.conf(5)>.
>>>
>>> But isn't this contradictory? Doesn't lack of APIC-register virtualization
>>> mean VM exits upon (most) accesses?
>>
>> Yes, it does mean. I guess the alternative wouuld be then to require
>> APIC-register virtualization for enabling xAPIC. But also, although this
>> doesn't provide much acceleration, even getting a VM exit is some
>> assistance if compared to instead getting an EPT fault and having to
>> decode the access.
> 
> I agree here, albeit I'd like to mention that EPT faults are also VM
> exits. All my earlier comment was about is that this piece of doc
> wants to express reality, whichever way it is that things end up
> being implemented.

Oh yes. Right, I see how this info could be misleading.

How about this?...

+=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
+
+B<(x86 only)> Enables or disables hardware assisted virtualization for
+xAPIC. With this option enabled, a memory-mapped APIC access will be
+decoded by hardware and either issue a VM exit with an exit reason
+instead of an EPT fault or altogether avoid a VM exit. Notice
+full virtualization for xAPIC can only be achieved if hardware
+supports “APIC-register virtualization” and “virtual-interrupt
+delivery”. The default is settable via L<xl.conf(5)>.

+=item B<assisted_x2apic=BOOLEAN>
+
+B<(x86 only)> Enables or disables hardware assisted virtualization for
+x2APIC. With this option enabled, an MSR-Based APIC access will either
+issue a VM exit or altogether avoid one. Notice full virtualization
+for x2APIC can only be achieved if hardware supports “APIC-register
+virtualization” and “virtual-interrupt delivery”. The default is
+settable via L<xl.conf(5)>.


...because with only VIRTUALIZE_APIC_ACCESSES enabled, hardware decodes 
accesses to the xAPIC page and the VM exit gives an exit reason.
And if VIRTUALIZE_X2APIC_MODE is set, although no assistance is provided 
w.r.t. to decoding x2APIC accesses as the MSR that the VM tried to 
access is already part of the vmexit information, VM exits for accesses 
to the TPR MSR are avoided, regardless of whether shadow TPR is set or 
not for e.g.

I hope this makes sense but I welcome any other suggestions/corrections.

Thank you,

Jane.