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

[Suravee Suthikulpanit]

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@xxxxxxxxxx>
> > Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> > ---
> >   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