Re: [PART1 V5 13/13] svm: Manage vcpu load/unload when enable AVIC

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

Hi Paolo/Radim,

I was a bit behind on catching up with the AVIC stuff the past couple 
weeks. Thank you for the help cleaning up on this patch and pulling this 
in to your tree. I can see now that it has been pulled into the 4.7-rc1. 
I really appreciate your help.

Please see a minor comment below.

On 5/17/16 07:09, Paolo Bonzini wrote:
>
>
> On 10/05/2016 17:43, Paolo Bonzini wrote:
>>
>>
>> On 04/05/2016 21:09, Suravee Suthikulpanit wrote:
>>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>>
>>> When a vcpu is loaded/unloaded to a physical core, we need to update
>>> host physical APIC ID information in the Physical APIC-ID table
>>> accordingly.
>>>
>>> Also, when vCPU is blocking/un-blocking (due to halt instruction),
>>> we need to make sure that the is-running bit in set accordingly in the
>>> physical APIC-ID table.
>>>
>>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
>>> ---
>>
>> I think this is the only patch that needs a little more work, because
>> there are a bunch of unused return values that really should be
>> WARN_ON.  In addition the load and put cases are different enough that
>> they should be separate functions.
>>
>> Can you please test this?
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index f3dbf1d33a61..3168d6c8d24f 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -184,7 +184,7 @@ struct vcpu_svm {
>>   	u32 ldr_reg;
>>   	struct page *avic_backing_page;
>>   	u64 *avic_physical_id_cache;
>> -	bool avic_is_blocking;
>> +	bool avic_is_running;
>>   };
>>
>>   #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK	(0xFF)
>> @@ -1321,18 +1321,20 @@ free_avic:
>>   /**
>>    * This function is called during VCPU halt/unhalt.
>>    */
>> -static int avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
>> +static void avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
>>   {
>>   	u64 entry;
>>   	int h_physical_id = __default_cpu_present_to_apicid(vcpu->cpu);
>>   	struct vcpu_svm *svm = to_svm(vcpu);
>>
>>   	if (!kvm_vcpu_apicv_active(vcpu))
>> -		return 0;
>> +		return;
>> +
>> +	svm->avic_is_running = is_run;

Shouldn't we do this below --->
>>
>>   	/* ID = 0xff (broadcast), ID > 0xff (reserved) */
>> -	if (h_physical_id >= AVIC_MAX_PHYSICAL_ID_COUNT)
>> -		return -EINVAL;
>> +	if (WARN_ON(h_physical_id >= AVIC_MAX_PHYSICAL_ID_COUNT))
>> +		return;

<--- HERE
>>
>>   	entry = READ_ONCE(*(svm->avic_physical_id_cache));
>>   	WARN_ON(is_run == !!(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK));
>> @@ -1341,36 +1343,45 @@ static int avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
>>   	if (is_run)
>>   		entry |= AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
>>   	WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
>> -
>> -	return 0;
>>   }
>>
>> [....]
>>
>> The two functions now have the same signature as their callers,
>> svm_vcpu_load and svm_vcpu_put.
>
> Radim, does this look sane?  I plan to include it in my pull request
> (I'm running AMD autotest now and it passed the first few tests).
>
> Thanks,
>
> Paolo
>


I have also tested the changes in the 4.7-rc1 on the CZ hardware w/ AVIC 
and everything looks good.

As for the nested virtualization. Currently, this is not working with 
AVIC enabled on the host, but it works fine w/ AVIC disabled. I'll look 
into enabling nested virtualization w/ AVIC enable next.

Thank you for all your help,
Suravee