Message ID | 1469439131-11308-11-git-send-email-suravee.suthikulpanit@amd.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
2016-07-25 04:32-0500, Suravee Suthikulpanit: > From: Suravee Suthikulpanit <Suravee.Suthikulpanit@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.suthikulpanit@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); > } -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 69e62862..16b4d1d 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -776,6 +776,7 @@ struct kvm_arch { bool disabled_lapic_found; /* Struct members for AVIC */ + u32 avic_vm_id; u32 ldr_mode; struct page *avic_logical_id_table_page; struct page *avic_physical_id_table_page; diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 16ef31b..90dc0fb 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -96,6 +96,18 @@ MODULE_DEVICE_TABLE(x86cpu, svm_cpu_id); #define AVIC_UNACCEL_ACCESS_OFFSET_MASK 0xFF0 #define AVIC_UNACCEL_ACCESS_VECTOR_MASK 0xFFFFFFFF +/* AVIC GATAG is encoded using VM and VCPU IDs */ +#define AVIC_VCPU_ID_BITS 8 +#define AVIC_VCPU_ID_MASK ((1 << AVIC_VCPU_ID_BITS) - 1) + +#define AVIC_VM_ID_BITS 24 +#define AVIC_VM_ID_MASK ((1 << AVIC_VM_ID_BITS) - 1) + +#define AVIC_GATAG(x, y) (((x & AVIC_VM_ID_MASK) << AVIC_VCPU_ID_BITS) | \ + (y & AVIC_VCPU_ID_MASK)) +#define AVIC_GATAG_TO_VMID(x) ((x >> AVIC_VCPU_ID_BITS) & AVIC_VM_ID_MASK) +#define AVIC_GATAG_TO_VCPUID(x) (x & AVIC_VM_ID_BITS) + static bool erratum_383_found __read_mostly; static const u32 host_save_user_msrs[] = { @@ -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; +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), + 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. */ + id = find_next_zero_bit(avic_vm_id_bm, 1, 1); + if (id <= AVIC_VM_ID_MASK) + __set_bit(id, avic_vm_id_bm); + else + id = -EINVAL; + + 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(); + if (ret) + return ret; + return kvm_init(&svm_x86_ops, sizeof(struct vcpu_svm), __alignof__(struct vcpu_svm), THIS_MODULE); } @@ -5083,6 +5160,7 @@ static int __init svm_init(void) static void __exit svm_exit(void) { kvm_exit(); + avic_vm_id_deinit(); } module_init(svm_init)