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
