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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <574F26E2.8020803@amd.com>
Date:	Wed, 1 Jun 2016 13:18:10 -0500
From:	Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
To:	Paolo Bonzini <pbonzini@...hat.com>, <rkrcmar@...hat.com>,
	<joro@...tes.org>, <bp@...en8.de>, <gleb@...nel.org>,
	<alex.williamson@...hat.com>
CC:	<kvm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<wei@...hat.com>, <sherry.hurwitz@....com>
Subject: Re: [PART1 V5 13/13] svm: Manage vcpu load/unload when enable AVIC

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@....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@....com>
>>> Reviewed-by: Radim Krčmář <rkrcmar@...hat.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