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.
Unless you mean that we should fallback to having
v->domain->arch.hvm.assisted_xapic depend on those other features...?
>
>> @@ -1124,9 +1125,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v,
>> uint32_t leaf,
>> * and wrmsr in the guest will run without VMEXITs (see
>> * vmx_vlapic_msr_changed()).
>> */
>> - if ( cpu_has_vmx_virtualize_x2apic_mode &&
>> - cpu_has_vmx_apic_reg_virt &&
>> - cpu_has_vmx_virtual_intr_delivery )
>> + if ( cpu_has_vmx_apic_reg_virt && cpu_has_vmx_virtual_intr_delivery
>> &&
>> + v->domain->arch.hvm.assisted_x2apic )
>> res->a |= XEN_HVM_CPUID_X2APIC_VIRT;
>
> While within the 80 cols limit, I think it would help readability if you
> kept it at one sub-condition per line.
Sure.
Thank you,
Jane.
