Re: [RFCv2 PATCH 07/12] KVM: SVM: Introduce helper function kvm_get_apic_id

[Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>]

Maxim,

On 3/24/22 9:14 PM, Maxim Levitsky wrote:
> On Tue, 2022-03-08 at 10:39 -0600, Suravee Suthikulpanit wrote:
>> This function returns the currently programmed guest physical
>> APIC ID of a vCPU in both xAPIC and x2APIC modes.
>>
>> Suggested-by: Maxim Levitsky <mlevitsk@redhat.com>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>   arch/x86/kvm/lapic.c    | 23 +++++++++++++++++++++++
>>   arch/x86/kvm/lapic.h    |  5 +----
>>   arch/x86/kvm/svm/avic.c | 21 +++++++++++++++++----
>>   3 files changed, 41 insertions(+), 8 deletions(-)
>>
>> ...
>>
>> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>> index 4d7a8743196e..7e5a39a8e698 100644
>> --- a/arch/x86/kvm/svm/avic.c
>> +++ b/arch/x86/kvm/svm/avic.c
>> @@ -441,14 +441,21 @@ static void avic_invalidate_logical_id_entry(struct kvm_vcpu *vcpu)
>>   
>>   static int avic_handle_ldr_update(struct kvm_vcpu *vcpu)
>>   {
>> -	int ret = 0;
>> +	int ret;
>>   	struct vcpu_svm *svm = to_svm(vcpu);
>>   	u32 ldr = kvm_lapic_get_reg(vcpu->arch.apic, APIC_LDR);
>> -	u32 id = kvm_xapic_id(vcpu->arch.apic);
>> +	u32 id;
>> +
>> +	ret = kvm_get_apic_id(vcpu, &id);
>> +	if (ret)
>> +		return ret;
> What about apic_id == 0?

The "id" is the returned apic ID. The "ret" is to let caller know if
the APIC look up is successful or not. "ret == 0" means success and
the value in "id" is valid.

>>   
>>   	if (ldr == svm->ldr_reg)
>>   		return 0;
>>   
>> +	if (id == X2APIC_BROADCAST)
>> +		return -EINVAL;
>> +
> 
> Why this is needed? avic_handle_ldr_update is called either
> when guest writes to APIC_LDR (should not reach here),
> or if LDR got changed while AVIC was inhibited (also
> thankfully KVM doesn't allow it to be changed in x2APIC mode,
> and it does reset it when enabling x2apic).

At this point, we don't need to handle LDR update in x2APIC case.
I will add apic_x2apic_mode() check here.

> 
> 
> 
>>   	avic_invalidate_logical_id_entry(vcpu);
>>   
>>   	if (ldr)
>> @@ -464,7 +471,12 @@ static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
>>   {
>>   	u64 *old, *new;
>>   	struct vcpu_svm *svm = to_svm(vcpu);
>> -	u32 id = kvm_xapic_id(vcpu->arch.apic);
>> +	u32 id;
>> +	int ret;
>> +
>> +	ret = kvm_get_apic_id(vcpu, &id);
>> +	if (ret)
>> +		return 1;
> 
> Well this function is totally broken anyway and I woudn't even bother touching it,
> maximum, just stick 'return 0' in the very start of this function if the apic is
> in x2apic mode now.
> 
> Oh well...
> 

Sorry, I'm not sure if I understand what you mean by "broken".

This function setup/update the AVIC physical APIC ID table, whenever the APIC ID is
initialize or updated. It is needed in both xAPIC and x2APIC cases.

Regards,
Suravee