lkml.org 
[lkml]   [2016]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PART1 RFC v2 10/10] svm: Manage vcpu load/unload when enable AVIC
From
Date
Hi,

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.

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

Thanks,
Suravee

\
 
 \ /
  Last update: 2016-03-14 13:21    [W:0.078 / U:23.732 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site