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

Suravee Suthikulpanit
2016-08-18T12:24:12Z

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