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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.02.2022 16:10, Jane Malalane wrote:
> 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?...

Getting close. The thing I can't judge is whether this level of technical
detail is suitable for this doc. Just one further remark:

> +=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

As said before, EPT faults also are VM exits and also provide an exit
reason. Therefore maybe "... and either issue a VM exit with a more
specific exit reason than an EPT fault would provide, or altogether
avoid a VM exit" or "... and either issue a more specific VM exit than
just an EPT fault, or altogether avoid a VM exit"?

Jan

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