diff mbox

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

Message ID 1469439131-11308-11-git-send-email-suravee.suthikulpanit@amd.com (mailing list archive)
State New, archived
Headers show

Commit Message

Suthikulpanit, Suravee July 25, 2016, 9:32 a.m. UTC
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>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/svm.c              | 78 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)

Comments

Radim Krčmář Aug. 12, 2016, 2:16 p.m. UTC | #1
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
Suthikulpanit, Suravee Aug. 18, 2016, 12:24 p.m. UTC | #2
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 mbox

Patch

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)