Re: [PART1 V5 13/13] svm: Manage vcpu load/unload when enable AVIC

[Suravee Suthikulpanit]

Hi Paolo/Radim,

I was a bit behind on catching up with the AVIC stuff the past couple 
weeks. Thank you for the help cleaning up on this patch and pulling this 
in to your tree. I can see now that it has been pulled into the 4.7-rc1. 
I really appreciate your help.

Please see a minor comment below.

On 5/17/16 07:09, Paolo Bonzini wrote:
> On 10/05/2016 17:43, Paolo Bonzini wrote:
> > On 04/05/2016 21:09, Suravee Suthikulpanit wrote:
> > > From: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> > >
> > > When a vcpu is loaded/unloaded to a physical core, we need to update
> > > host physical APIC ID information in the Physical APIC-ID table
> > > accordingly.
> > >
> > > Also, when vCPU is blocking/un-blocking (due to halt instruction),
> > > we need to make sure that the is-running bit in set accordingly in the
> > > physical APIC-ID table.
> > >
> > > Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> > > Reviewed-by: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> > > ---
> >
> > I think this is the only patch that needs a little more work, because
> > there are a bunch of unused return values that really should be
> > WARN_ON.  In addition the load and put cases are different enough that
> > they should be separate functions.
> >
> > Can you please test this?
> >
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index f3dbf1d33a61..3168d6c8d24f 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -184,7 +184,7 @@ struct vcpu_svm {
> >   	u32 ldr_reg;
> >   	struct page *avic_backing_page;
> >   	u64 *avic_physical_id_cache;
> > -	bool avic_is_blocking;
> > +	bool avic_is_running;
> >   };
> >
> >   #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK	(0xFF)
> > @@ -1321,18 +1321,20 @@ free_avic:
> >   /**
> >    * This function is called during VCPU halt/unhalt.
> >    */
> > -static int avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
> > +static void avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
> >   {
> >   	u64 entry;
> >   	int h_physical_id = __default_cpu_present_to_apicid(vcpu->cpu);
> >   	struct vcpu_svm *svm = to_svm(vcpu);
> >
> >   	if (!kvm_vcpu_apicv_active(vcpu))
> > -		return 0;
> > +		return;
> > +
> > +	svm->avic_is_running = is_run;

Shouldn't we do this below --->
> >   	/* ID = 0xff (broadcast), ID > 0xff (reserved) */
> > -	if (h_physical_id >= AVIC_MAX_PHYSICAL_ID_COUNT)
> > -		return -EINVAL;
> > +	if (WARN_ON(h_physical_id >= AVIC_MAX_PHYSICAL_ID_COUNT))
> > +		return;

<--- HERE
> >   	entry = READ_ONCE(*(svm->avic_physical_id_cache));
> >   	WARN_ON(is_run == !!(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK));
> > @@ -1341,36 +1343,45 @@ static int avic_set_running(struct kvm_vcpu *vcpu, bool is_run)
> >   	if (is_run)
> >   		entry |= AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
> >   	WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
> > -
> > -	return 0;
> >   }
> >
> > [....]
> >
> > The two functions now have the same signature as their callers,
> > svm_vcpu_load and svm_vcpu_put.
>
> Radim, does this look sane?  I plan to include it in my pull request
> (I'm running AMD autotest now and it passed the first few tests).
>
> Thanks,
>
> Paolo


I have also tested the changes in the 4.7-rc1 on the CZ hardware w/ AVIC 
and everything looks good.

As for the nested virtualization. Currently, this is not working with 
AVIC enabled on the host, but it works fine w/ AVIC disabled. I'll look 
into enabling nested virtualization w/ AVIC enable next.

Thank you for all your help,
Suravee