Re: [PART2 PATCH v5 10/12] svm: Introduces AVIC per-VM ID

[Suravee Suthikulpanit]

Hi Radim,

On 8/12/16 21:16, Radim KrÄmÃÅ wrote:
> > +static DEFINE_SPINLOCK(avic_vm_id_lock);
> > > +
> > >  static void svm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
> > >  static void svm_flush_tlb(struct kvm_vcpu *vcpu);
> > >  static void svm_complete_interrupts(struct vcpu_svm *svm);
> > > @@ -1280,10 +1296,61 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
> > >  	return 0;
> > >  }
> > >
> > > +static inline int avic_vm_id_init(void)
> > > +{
> > > +	if (avic_vm_id_bm)
> > > +		return 0;
> > > +
> > > +	avic_vm_id_bm = kcalloc(BITS_TO_LONGS(AVIC_VM_ID_MASK),
> Allocation is off by one.  avic_get_next_vm_id() uses
>   if (id <= AVIC_VM_ID_MASK)
>     __set_bit(id, avic_vm_id_bm);
>
> and id=AVIC_VM_ID_MASK is stored in the AVIC_VM_ID_MASK+1 th bit.

Ah... right. Sorry :(

> > > +static inline int avic_get_next_vm_id(void)
> > > +{
> > > +	int id;
> > > +
> > > +	spin_lock(&avic_vm_id_lock);
> > > +
> > > +	/* AVIC VM ID is one-based. */
> Why?

I use VM-ID 0 to represent unassigned ID since we use it to encode 
ga_tag, and ga_tag=0 out of reset by hardware.

> > > +	id = find_next_zero_bit(avic_vm_id_bm, 1, 1);
> The second argument is size, so this should always return 1. :)

My bad. I'll change to (AVIC_VM_ID_MASK + 1).

> > > +	if (id <= AVIC_VM_ID_MASK)
> > > +		__set_bit(id, avic_vm_id_bm);
> > > +	else
> > > +		id = -EINVAL;
> It is not really a problem that can be handled with changing the values,
> so a temporary error would be nicer ... ENOMEM could be confusing and
> EAGAIN lead to a loop, but I still like them better.

Ok. I think EAGAIN is better in this case.

> > >  static int __init svm_init(void)
> > >  {
> > > +	int ret;
> > > +
> > > +	ret = avic_vm_id_init();
> This is certainly useless when the CPU doesn't have AVIC, so we could
> make it conditional.
>
> I would prefer to make the bitmap allocated at module load, though:
>
>   static DECLARE_BITMAP(avic_vm_id_bm, AVIC_VM_ID_MASK + 1);
>
> The size is 2 KiB with 24 bit AVIC_VM_ID_MASK, which is IMO much better
> than having extra lines of code dealing with allocation and failures.

I also prefer this suggestion.

Thanks again,
Suravee