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
