diff mbox

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

Message ID 1462388992-25242-14-git-send-email-Suravee.Suthikulpanit@amd.com
State New
Headers show

Commit Message

Suthikulpanit, Suravee May 4, 2016, 7:09 p.m. UTC 
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>

---
 arch/x86/kvm/svm.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

-- 
1.9.1

Comments

Suthikulpanit, Suravee June 1, 2016, 6:18 p.m. UTC | #1 
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
diff mbox

Patch

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 721e514..f903d33 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -35,6 +35,7 @@ 
 #include <linux/trace_events.h>
 #include <linux/slab.h>
 
+#include <asm/apic.h>
 #include <asm/perf_event.h>
 #include <asm/tlbflush.h>
 #include <asm/desc.h>
@@ -183,6 +184,7 @@  struct vcpu_svm {
 	u32 ldr_reg;
 	struct page *avic_backing_page;
 	u64 *avic_physical_id_cache;
+	bool avic_is_blocking;
 };
 
 #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK	(0xFF)
@@ -1315,6 +1317,61 @@  free_avic:
 	return err;
 }
 
+/**
+ * This function is called during VCPU halt/unhalt.
+ */
+static int 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;
+
+	/* ID = 0xff (broadcast), ID > 0xff (reserved) */
+	if (h_physical_id >= AVIC_MAX_PHYSICAL_ID_COUNT)
+		return -EINVAL;
+
+	entry = READ_ONCE(*(svm->avic_physical_id_cache));
+	WARN_ON(is_run == !!(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK));
+
+	entry &= ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
+	if (is_run)
+		entry |= AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
+	WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
+
+	return 0;
+}
+
+static int avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu, bool is_load)
+{
+	u64 entry;
+	int h_physical_id = __default_cpu_present_to_apicid(cpu);
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	if (!kvm_vcpu_apicv_active(vcpu))
+		return 0;
+
+	/* ID = 0xff (broadcast), ID > 0xff (reserved) */
+	if (h_physical_id >= AVIC_MAX_PHYSICAL_ID_COUNT)
+		return -EINVAL;
+
+	entry = READ_ONCE(*(svm->avic_physical_id_cache));
+	WARN_ON(is_load && (entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK));
+
+	entry &= ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
+	if (is_load) {
+		entry &= ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
+		entry |= (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK);
+		if (!svm->avic_is_blocking)
+			entry |= AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
+	}
+	WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
+
+	return 0;
+}
+
 static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -1378,6 +1435,11 @@  static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id)
 			goto free_page4;
 	}
 
+	/* We initialize this flag to one to make sure that the is_running
+	 * bit would be set the first time the vcpu is loaded.
+	 */
+	svm->avic_is_blocking = false;
+
 	svm->nested.hsave = page_address(hsave_page);
 
 	svm->msrpm = page_address(msrpm_pages);
@@ -1454,6 +1516,8 @@  static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 	/* This assumes that the kernel never uses MSR_TSC_AUX */
 	if (static_cpu_has(X86_FEATURE_RDTSCP))
 		wrmsrl(MSR_TSC_AUX, svm->tsc_aux);
+
+	avic_vcpu_load(vcpu, cpu, true);
 }
 
 static void svm_vcpu_put(struct kvm_vcpu *vcpu)
@@ -1461,6 +1525,8 @@  static void svm_vcpu_put(struct kvm_vcpu *vcpu)
 	struct vcpu_svm *svm = to_svm(vcpu);
 	int i;
 
+	avic_vcpu_load(vcpu, 0, false);
+
 	++vcpu->stat.host_state_reload;
 	kvm_load_ldt(svm->host.ldt);
 #ifdef CONFIG_X86_64
@@ -1476,6 +1542,18 @@  static void svm_vcpu_put(struct kvm_vcpu *vcpu)
 		wrmsrl(host_save_user_msrs[i], svm->host_user_msrs[i]);
 }
 
+static void svm_vcpu_blocking(struct kvm_vcpu *vcpu)
+{
+	to_svm(vcpu)->avic_is_blocking = true;
+	avic_set_running(vcpu, false);
+}
+
+static void svm_vcpu_unblocking(struct kvm_vcpu *vcpu)
+{
+	to_svm(vcpu)->avic_is_blocking = false;
+	avic_set_running(vcpu, true);
+}
+
 static unsigned long svm_get_rflags(struct kvm_vcpu *vcpu)
 {
 	return to_svm(vcpu)->vmcb->save.rflags;
@@ -4883,6 +4961,8 @@  static struct kvm_x86_ops svm_x86_ops = {
 	.prepare_guest_switch = svm_prepare_guest_switch,
 	.vcpu_load = svm_vcpu_load,
 	.vcpu_put = svm_vcpu_put,
+	.vcpu_blocking = svm_vcpu_blocking,
+	.vcpu_unblocking = svm_vcpu_unblocking,
 
 	.update_bp_intercept = update_bp_intercept,
 	.get_msr = svm_get_msr,