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

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

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.

Thanks,

Jane.