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

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

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