Re: [RFC PATCH 11/13] KVM: SVM: Add logic to switch between APIC and x2APIC virtualization mode

[Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>]

Maxim,

On 2/25/22 3:03 AM, Maxim Levitsky wrote:
> On Sun, 2022-02-20 at 20:19 -0600, Suravee Suthikulpanit wrote:
>> ....
>>
>> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>> index 3543b7a4514a..3306b74f1d8b 100644
>> --- a/arch/x86/kvm/svm/avic.c
>> +++ b/arch/x86/kvm/svm/avic.c
>> @@ -79,6 +79,50 @@ static inline enum avic_modes avic_get_vcpu_apic_mode(struct vcpu_svm *svm)
>>   		return AVIC_MODE_NONE;
>>   }
>>   
>> +static inline void avic_set_x2apic_msr_interception(struct vcpu_svm *svm, bool disable)
>> +{
>> +	int i;
>> +
>> +	for (i = 0x800; i <= 0x8ff; i++)
>> +		set_msr_interception(&svm->vcpu, svm->msrpm, i,
>> +				     !disable, !disable);
>> +}
>> +
>> +void avic_activate_vmcb(struct vcpu_svm *svm)
>> +{
>> +	struct vmcb *vmcb = svm->vmcb01.ptr;
>> +
>> +	vmcb->control.int_ctl |= AVIC_ENABLE_MASK;
>> +
>> +	if (svm->x2apic_enabled) {
> Use apic_x2apic_mode here as well

Okay

> 
>> +		vmcb->control.int_ctl |= X2APIC_MODE_MASK;
>> +		vmcb->control.avic_physical_id &= ~X2AVIC_MAX_PHYSICAL_ID;
>> +		vmcb->control.avic_physical_id |= X2AVIC_MAX_PHYSICAL_ID;
> Why not just use
> 
> phys_addr_t ppa = __sme_set(page_to_phys(kvm_svm->avic_physical_id_table_page));;
> vmcb->control.avic_physical_id = ppa | X2AVIC_MAX_PHYSICAL_ID;
> 

Sorry, I don't quiet understand this part. We just want to update certain bits in the VMCB register.

>> ...
>> +void avic_deactivate_vmcb(struct vcpu_svm *svm)
>> +{
>> +	struct vmcb *vmcb = svm->vmcb01.ptr;
>> +
>> +	vmcb->control.int_ctl &= ~(AVIC_ENABLE_MASK | X2APIC_MODE_MASK);
>> +
>> +	if (svm->x2apic_enabled)
>> +		vmcb->control.avic_physical_id &= ~X2AVIC_MAX_PHYSICAL_ID;
>> +	else
>> +		vmcb->control.avic_physical_id &= ~AVIC_MAX_PHYSICAL_ID;
>> +
>> +	/* Enabling MSR intercept for x2APIC registers */
>> +	avic_set_x2apic_msr_interception(svm, true);
>> +}
>> +
>>   /* Note:
>>    * This function is called from IOMMU driver to notify
>>    * SVM to schedule in a particular vCPU of a particular VM.
>> @@ -195,13 +239,12 @@ void avic_init_vmcb(struct vcpu_svm *svm)
>>   	vmcb->control.avic_backing_page = bpa & AVIC_HPA_MASK;
>>   	vmcb->control.avic_logical_id = lpa & AVIC_HPA_MASK;
>>   	vmcb->control.avic_physical_id = ppa & AVIC_HPA_MASK;
>> -	vmcb->control.avic_physical_id |= AVIC_MAX_PHYSICAL_ID;
>>   	vmcb->control.avic_vapic_bar = APIC_DEFAULT_PHYS_BASE & VMCB_AVIC_APIC_BAR_MASK;
>>   
>>   	if (kvm_apicv_activated(svm->vcpu.kvm))
>> -		vmcb->control.int_ctl |= AVIC_ENABLE_MASK;
>> +		avic_activate_vmcb(svm);
> Why not set AVIC_ENABLE_MASK in avic_activate_vmcb ?

It's already doing "vmcb->control.int_ctl |= X2APIC_MODE_MASK;" in avic_activate_vmcb().

>>   	else
>> -		vmcb->control.int_ctl &= ~AVIC_ENABLE_MASK;
>> +		avic_deactivate_vmcb(svm);
>>   }
>>   
>>   static u64 *avic_get_physical_id_entry(struct kvm_vcpu *vcpu,
>> @@ -657,6 +700,13 @@ void avic_update_vapic_bar(struct vcpu_svm *svm, u64 data)
>>   		 svm->x2apic_enabled ? "x2APIC" : "xAPIC");
>>   	vmcb_mark_dirty(svm->vmcb, VMCB_AVIC);
>>   	kvm_vcpu_update_apicv(&svm->vcpu);
>> +
>> +	/*
>> +	 * The VM could be running w/ AVIC activated switching from APIC
>> +	 * to x2APIC mode. We need to all refresh to make sure that all
>> +	 * x2AVIC configuration are being done.
>> +	 */
>> +	svm_refresh_apicv_exec_ctrl(&svm->vcpu);
> 
> 
> That also should be done in avic_set_virtual_apic_mode  instead, but otherwise should be fine.

Agree, and will be updated to use svm_set_virtual_apic_mode() in v2.

> Also it seems that .avic_set_virtual_apic_mode will cover you on the case when x2apic is disabled
> in the guest cpuid - kvm_set_apic_base checks if the guest cpuid has x2apic support and refuses
> to enable it if it is not set.
> 
> But still a WARN_ON_ONCE won't hurt to see that you are not enabling x2avic when not supported.

Not sure if we need this. The logic for activating x2AVIC in VMCB already
check if the guest x2APIC mode is set, which can only happen if x2APIC CPUID
is set.

>>   }
>>   
>>   void svm_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
>> @@ -722,9 +772,9 @@ void svm_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
>>   		 * accordingly before re-activating.
>>   		 */
>>   		avic_post_state_restore(vcpu);
>> -		vmcb->control.int_ctl |= AVIC_ENABLE_MASK;
>> +		avic_activate_vmcb(svm);
>>   	} else {
>> -		vmcb->control.int_ctl &= ~AVIC_ENABLE_MASK;
>> +		avic_deactivate_vmcb(svm);
>>   	}
>>   	vmcb_mark_dirty(vmcb, VMCB_AVIC);
>>   
>> @@ -1019,7 +1069,6 @@ void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>   		return;
>>   
>>   	entry = READ_ONCE(*(svm->avic_physical_id_cache));
>> -	WARN_ON(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK);
> Why?

For AVIC, this WARN_ON is designed to catch the scenario when the vCPU is calling
avic_vcpu_load() while it is already running. However, with x2AVIC support,
the vCPU can switch from xAPIC to x2APIC mode while in running state
(i.e. the AVIC is_running is set). This warning is currently observed due to
the call from svm_refresh_apicv_exec_ctrl().

Regards,
Suravee