Re: [PART1 RFC v2 10/10] svm: Manage vcpu load/unload when enable AVIC

From: Suravee Suthikulpanit
Date: Mon Mar 14 2016 - 07:49:24 EST


On 03/10/2016 04:46 AM, Radim KrÄmÃÅ wrote:
2016-03-04 14:46-0600, Suravee Suthikulpanit:
From: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>

When a vcpu is loaded/unloaded to a physical core, we need to update
information in the Physical APIC-ID table accordingly.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
@@ -1508,6 +1510,146 @@ static int avic_vcpu_init(struct kvm *kvm, struct vcpu_svm *svm, int id)
+static inline int
+avic_update_iommu(struct kvm_vcpu *vcpu, int cpu, phys_addr_t pa, bool r)
+ if (!kvm_arch_has_assigned_device(vcpu->kvm))
+ return 0;
+ /* TODO: We will hook up with IOMMU API at later time */

(It'd be best to drop avic_update_iommu from this series and introduce
it when merging iommu support.)

I just kept it there to make code merging b/w part1 and 2 that I have been testing simpler. I didn't think it would cause much confusion. But if you think that might be the case, I can drop it for now.

+ return 0;
+static int avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu, bool is_load)

This function does a lot and there is only one thing that must be done
in svm_vcpu_load: change host physical APIC ID if the CPU changed.
The rest can be done elsewhere:
- is_running when blocking.

I added the logic here to track if the is_running is set when unloading since I noticed the case when the vCPU is busy doing some work for the guest, then get unloaded and later on get loaded w/o blocking/unblocking. So, in theory, it should be be set to running during unloaded period, and it should restore this flag if it is loaded again.

- kb_pg_ptr when the pointer changes = only on initialization?

The reason I put this here mainly because it is a convenient place to set the vAPIC bakcing page address since we already have to set up the host physical APIC id. I guess I can try setting this separately during vcpu create. But I don't think it would make much difference.

- valid when the kb_pg_ptr is valid = always for existing VCPUs?

According to the programming manual, the valid bit is set when we set the host physical APIC ID. However, in theory, the vAPIC backing page address is required for AVIC hardware to set bits in IRR register, while the host physical APIC ID is needed for sending doorbell to the target physical core. So, I would actually interpret the valid bit as it should be set when the vAPIC backing address is set.

In the current implementation, the valid bit is set during vcpu load, but is not unset it when unload. This actually reflect the interpretation of the description above.

If we decide to move the setting of vAPIC backing page address to the vcpu create phrase, we would set the valid bit at that point as well.

Please let me know if you think differently.

+static int avic_set_running(struct kvm_vcpu *vcpu, bool is_run)

avic_set_running should get address of the entry and write is_run to it.
(No need to touch avic_bk_page, h_phy_apic_id or do bound checks.)

I think you can cache the physical apic id table entry in *vcpu, so both
functions are going to be very simple.

I was actually thinking about doing this. Lemme try to come up with the logic to cache this.