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

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

On 15/02/2022 15:21, 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 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:

Unsure too.

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

Yes, that's better.

Thank you,

Jane.