Re: [PATCH v3 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.02.2022 17:59, Jane Malalane wrote:
> On 24/02/2022 14:16, 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 18.02.2022 18:29, Jane Malalane wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -3333,15 +3333,15 @@ static void vmx_install_vlapic_mapping(struct vcpu 
>>> *v)
>>>   
>>>   void vmx_vlapic_msr_changed(struct vcpu *v)
>>>   {
>>> -    int virtualize_x2apic_mode;
>>> +    bool virtualize_x2apic_mode;
>>>       struct vlapic *vlapic = vcpu_vlapic(v);
>>>       unsigned int msr;
>>>   
>>>       virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt ||
>>>                                   cpu_has_vmx_virtual_intr_delivery) &&
>>> -                               cpu_has_vmx_virtualize_x2apic_mode );
>>> +                               v->domain->arch.hvm.assisted_x2apic );
>>
>> Following from my comment on patch 1, I'd expect this to become a simple
>> assignment of v->domain->arch.hvm.assisted_x2apic (at which point the
>> local variable could go away), just like ...
>>
>>> -    if ( !cpu_has_vmx_virtualize_apic_accesses &&
>>> +    if ( !v->domain->arch.hvm.assisted_xapic &&
>>>            !virtualize_x2apic_mode )
>>>           return;
>>
>> ... here.
> Previosuly we discussed setting v->domain->arch.hvm.assisted_xapic equal 
> to only cpu_has_vmx_virtualize_x2apic_mode, that's why I have those 
> additional checks as at least apic_reg_virt or virtual_intr_delivery are 
> needed for the subsequent parts of this function. I might be 
> misunderstanding your question.

My expectation would have been that assisted_x2apic_available is assigned
what is (in context above) assigned to virtualize_x2apic_mode (in patch 1).
Anything deviating from this needs, I think, explaining there.

> Unless you mean that we should fallback to having 
> v->domain->arch.hvm.assisted_xapic depend on those other features...?

No, xapic is fine afaic.

Jan