2016-07-25 04:32-0500, Suravee Suthikulpanit:
> From: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com>
>
> Introduces per-VM AVIC ID and helper functions to manage the IDs.
> Currently, the ID will be used to implement 32-bit AVIC IOMMU GA tag.
>
> The ID is 24-bit one-based indexing value, and is managed via helper
> functions to get the next ID, or to free an ID once a VM is destroyed.
> There should be no ID conflict for any active VMs.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpa...@amd.com>
> ---
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> @@ -242,6 +254,10 @@ static int avic;
> module_param(avic, int, S_IRUGO);
> #endif
>
> +/* AVIC VM ID bit masks and lock */
> +static unsigned long *avic_vm_id_bm;
Please expand "bm" to "bitmap" to match KVM conventions.
> +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.
> + sizeof(long), GFP_KERNEL);
> + if (!avic_vm_id_bm)
> + return -ENOMEM;
> + return 0;
> +}
> +
> +static inline void avic_vm_id_deinit(void)
> +{
> + if (!avic_vm_id_bm)
> + return;
> +
> + kfree(avic_vm_id_bm);
> + avic_vm_id_bm = NULL;
> +}
> +
> +static inline int avic_get_next_vm_id(void)
> +{
> + int id;
> +
> + spin_lock(&avic_vm_id_lock);
> +
> + /* AVIC VM ID is one-based. */
Why?
> + id = find_next_zero_bit(avic_vm_id_bm, 1, 1);
The second argument is size, so this should always return 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.
> +
> + spin_unlock(&avic_vm_id_lock);
> + return id;
> +}
> +
> +static inline int avic_free_vm_id(int id)
> +{
> + if (id <= 0 || id > AVIC_VM_ID_MASK)
> + return -EINVAL;
> +
> + spin_lock(&avic_vm_id_lock);
> + __clear_bit(id, avic_vm_id_bm);
> + spin_unlock(&avic_vm_id_lock);
> + return 0;
> +}
> +
> static void avic_vm_destroy(struct kvm *kvm)
> {
> struct kvm_arch *vm_data = &kvm->arch;
>
> + avic_free_vm_id(vm_data->avic_vm_id);
> +
> if (vm_data->avic_logical_id_table_page)
> __free_page(vm_data->avic_logical_id_table_page);
> if (vm_data->avic_physical_id_table_page)
> @@ -1300,6 +1367,10 @@ static int avic_vm_init(struct kvm *kvm)
> if (!avic)
> return 0;
>
> + vm_data->avic_vm_id = avic_get_next_vm_id();
> + if (vm_data->avic_vm_id < 0)
> + return vm_data->avic_vm_id;
> +
> /* Allocating physical APIC ID table (4KB) */
> p_page = alloc_page(GFP_KERNEL);
> if (!p_page)
> @@ -5076,6 +5147,12 @@ static struct kvm_x86_ops svm_x86_ops = {
>
> 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.
> + if (ret)
> + return ret;
> +
> return kvm_init(&svm_x86_ops, sizeof(struct vcpu_svm),
> __alignof__(struct vcpu_svm), THIS_MODULE);
> }
