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.
> @@ -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.
Jan
