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

["Radim Krčmář" <rkrcmar@redhat.com>]

2016-03-14 18:48+0700, Suravee Suthikulpanit:
> On 03/10/2016 04:46 AM, Radim Krčmář wrote:
>>2016-03-04 14:46-0600, Suravee Suthikulpanit:
>>>From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>>
>>>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@amd.com>
>>>---
>>>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.

The iommu part might end up having different requirements for this
function, so this husk can only add work when compared to waiting.
And avic_update_iommu is logically separable, so it would be nicer as a
short separate patch anyway.  (It's not a problem if you leave it.)

>>>+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.

(A second mail will be related to this.)

>>  - 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.

vcpu_load isn't as hot vcpu_run, but it's still called often and the
most useful optimization is to avoid unnecessary operations ...
(I think the split code is going to be easier to understand as well.)

>>  - 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.

(Physical APIC ID doesn't affect the valid bit at all.)

>                        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.

Yes, APM (rev 3.23, vol 2, table 15-24):
  Valid bit. When set, indicates that this entry contains a valid vAPIC
  backing page pointer. If cleared, this table entry contains no
  information.

> 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.

I agree with your analysis and setting the backing page and valid bit on
LAPIC creation seems better to me.

Thanks.