[PATCH v2 8/9] KVM: arm64: Detect arithmetic overflow is inside hyp/nVHE.

[Elena Petrova]

From: George Popescu 

Whenever an arithmetic overflow: addition, substraction, multiplication,
division or negating happens inside the hyp/nVHE code,
an __ubsan_handle_*_overflow is called.

All the overflow handlers are sharing the same structure called
overflow_data.

Signed-off-by: George Popescu 
Change-Id: Iec1ef331e471efbb35a39ffaee0641107a3a0e3a
Signed-off-by: Elena Petrova 
---
 arch/arm64/include/asm/kvm_ubsan.h | 10 ++--
 arch/arm64/kvm/hyp/nvhe/ubsan.c| 40 ++
 arch/arm64/kvm/kvm_ubsan_buffer.c  | 20 ++-
 3 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_ubsan.h 
b/arch/arm64/include/asm/kvm_ubsan.h
index 93c1b695097a..da4a3b4e28e0 100644
--- a/arch/arm64/include/asm/kvm_ubsan.h
+++ b/arch/arm64/include/asm/kvm_ubsan.h
@@ -22,7 +22,8 @@ struct kvm_ubsan_info {
UBSAN_UNREACHABLE_DATA,
UBSAN_SHIFT_OUT_OF_BOUNDS,
UBSAN_INVALID_DATA,
-   UBSAN_TYPE_MISMATCH
+   UBSAN_TYPE_MISMATCH,
+   UBSAN_OVERFLOW_DATA
} type;
union {
struct out_of_bounds_data out_of_bounds_data;
@@ -30,6 +31,7 @@ struct kvm_ubsan_info {
struct shift_out_of_bounds_data shift_out_of_bounds_data;
struct invalid_value_data invalid_value_data;
struct type_mismatch_data type_mismatch_data;
+   struct overflow_data overflow_data;
};
union {
struct ubsan_values u_val;
@@ -41,4 +43,8 @@ void __ubsan_handle_builtin_unreachable(void *_data);
 void __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs);
 void __ubsan_handle_load_invalid_value(void *_data, void *val);
 void __ubsan_handle_type_mismatch(struct type_mismatch_data  *_data, void 
*ptr);
-
+void __ubsan_handle_add_overflow(void *data, void *lhs, void *rhs);
+void __ubsan_handle_sub_overflow(void *data, void *lhs, void *rhs);
+void __ubsan_handle_mul_overflow(void *data, void *lhs, void *rhs);
+void __ubsan_handle_negate_overflow(void *_data, void *old_val);
+void __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs);
diff --git a/arch/arm64/kvm/hyp/nvhe/ubsan.c b/arch/arm64/kvm/hyp/nvhe/ubsan.c
index a9f72d4bcab7..f16842ff7316 100644
--- a/arch/arm64/kvm/hyp/nvhe/ubsan.c
+++ b/arch/arm64/kvm/hyp/nvhe/ubsan.c
@@ -47,15 +47,45 @@ static void write_type_mismatch_data(struct 
type_mismatch_data_common *data, voi
}
 }
 
-void __ubsan_handle_add_overflow(void *_data, void *lhs, void *rhs) {}
+static void write_overflow_data(struct overflow_data *data, void *lval, void 
*rval, char op)
+{
+   struct kvm_ubsan_info *slot = kvm_ubsan_buffer_next_slot();
+
+   if (slot) {
+   slot->type = UBSAN_OVERFLOW_DATA;
+   slot->overflow_data = *data;
+   slot->u_val.op = op;
+   slot->u_val.lval = lval;
+   if (op != '!')
+   slot->u_val.rval = rval;
+   }
+}
+
+void __ubsan_handle_add_overflow(void *_data, void *lhs, void *rhs)
+{
+   write_overflow_data(_data, lhs, rhs, '+');
+}
 
-void __ubsan_handle_sub_overflow(void *_data, void *lhs, void *rhs) {}
+void __ubsan_handle_sub_overflow(void *_data, void *lhs, void *rhs)
+{
+   write_overflow_data(_data, lhs, rhs, '-');
+}
 
-void __ubsan_handle_mul_overflow(void *_data, void *lhs, void *rhs) {}
+void __ubsan_handle_mul_overflow(void *_data, void *lhs, void *rhs)
+{
+   write_overflow_data(_data, lhs, rhs, '*');
+}
 
-void __ubsan_handle_negate_overflow(void *_data, void *old_val) {}
+void __ubsan_handle_negate_overflow(void *_data, void *old_val)
+{
+   write_overflow_data(_data, old_val, NULL, '!');
+}
+
+void __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs)
+{
+   write_overflow_data(_data, lhs, rhs, '/');
+}
 
-void __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs) {}
 
 void __ubsan_handle_type_mismatch(struct type_mismatch_data *data, void *ptr)
 {
diff --git a/arch/arm64/kvm/kvm_ubsan_buffer.c 
b/arch/arm64/kvm/kvm_ubsan_buffer.c
index b7823dedf8b1..2c7060cbb48b 100644
--- a/arch/arm64/kvm/kvm_ubsan_buffer.c
+++ b/arch/arm64/kvm/kvm_ubsan_buffer.c
@@ -39,7 +39,25 @@ void __kvm_check_ubsan_data(struct kvm_ubsan_info *slot)
__ubsan_handle_type_mismatch(>type_mismatch_data,
slot->u_val.lval);
break;
-   }
+   case UBSAN_OVERFLOW_DATA:
+   if (slot->u_val.op == '/') {
+   __ubsan_handle_divrem_overflow(>overflow_data,
+   slot->u_val.lval, slot->u_val.rval);
+   } else if (slot->u_val.op == '!') {
+   __ubsan_handle_negate_overflow(>overflow_data,
+   slot->u_val.lval);
+   } else if (slot->u_val.op == '+') {
+   

[PATCH v2 3/9] KVM: arm64: Enable UBSAN_BOUNDS for the both the kernel and hyp/nVHE

[Elena Petrova]

From: George Popescu 

If an out of bounds happens inside the hyp/nVHE code, the ubsan_out_of_bounds
handler stores the logging data inside the kvm_ubsan_buffer. The one responsible
for printing is the kernel ubsan_out_of_bounds handler. The process of
decapsulating the data from the buffer is straightforward.

Signed-off-by: George Popescu 
Signed-off-by: Elena Petrova 
---
 arch/arm64/include/asm/kvm_ubsan.h | 19 ++-
 arch/arm64/kvm/hyp/nvhe/ubsan.c| 14 --
 arch/arm64/kvm/kvm_ubsan_buffer.c  | 10 ++
 3 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_ubsan.h 
b/arch/arm64/include/asm/kvm_ubsan.h
index fb32c7fd65d4..4f471acb88b0 100644
--- a/arch/arm64/include/asm/kvm_ubsan.h
+++ b/arch/arm64/include/asm/kvm_ubsan.h
@@ -9,6 +9,23 @@
 #define UBSAN_MAX_TYPE 6
 #define KVM_UBSAN_BUFFER_SIZE 1000
 
+
+struct ubsan_values {
+   void *lval;
+   void *rval;
+   char op;
+};
+
 struct kvm_ubsan_info {
-   int type;
+   enum {
+   UBSAN_OUT_OF_BOUNDS,
+   } type;
+   union {
+   struct out_of_bounds_data out_of_bounds_data;
+   };
+   union {
+   struct ubsan_values u_val;
+   };
 };
+
+void __ubsan_handle_out_of_bounds(void *_data, void *index);
diff --git a/arch/arm64/kvm/hyp/nvhe/ubsan.c b/arch/arm64/kvm/hyp/nvhe/ubsan.c
index 8a194fb1f6cf..55a8f6db8555 100644
--- a/arch/arm64/kvm/hyp/nvhe/ubsan.c
+++ b/arch/arm64/kvm/hyp/nvhe/ubsan.c
@@ -13,7 +13,6 @@
 #include 
 #include 
 #include 
-#include 
 
 DEFINE_KVM_DEBUG_BUFFER(struct kvm_ubsan_info, kvm_ubsan_buffer,
 kvm_ubsan_buff_wr_ind, KVM_UBSAN_BUFFER_SIZE);
@@ -44,7 +43,18 @@ void __ubsan_handle_type_mismatch(struct type_mismatch_data 
*data, void *ptr) {}
 
 void __ubsan_handle_type_mismatch_v1(void *_data, void *ptr) {}
 
-void __ubsan_handle_out_of_bounds(void *_data, void *index) {}
+void __ubsan_handle_out_of_bounds(void *_data, void *index)
+{
+   struct kvm_ubsan_info *slot;
+   struct out_of_bounds_data *data = _data;
+
+   slot = kvm_ubsan_buffer_next_slot();
+   if (slot) {
+   slot->type = UBSAN_OUT_OF_BOUNDS;
+   slot->out_of_bounds_data = *data;
+   slot->u_val.lval = index;
+   }
+}
 
 void __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs) {}
 
diff --git a/arch/arm64/kvm/kvm_ubsan_buffer.c 
b/arch/arm64/kvm/kvm_ubsan_buffer.c
index 4a1959ba9f68..a1523f86be3c 100644
--- a/arch/arm64/kvm/kvm_ubsan_buffer.c
+++ b/arch/arm64/kvm/kvm_ubsan_buffer.c
@@ -17,6 +17,15 @@
 DECLARE_KVM_DEBUG_BUFFER(struct kvm_ubsan_info, kvm_ubsan_buffer,
kvm_ubsan_buff_wr_ind, KVM_UBSAN_BUFFER_SIZE);
 
+void __kvm_check_ubsan_data(struct kvm_ubsan_info *slot)
+{
+   switch (slot->type) {
+   case UBSAN_OUT_OF_BOUNDS:
+   __ubsan_handle_out_of_bounds(>out_of_bounds_data,
+   slot->u_val.lval);
+   break;
+   }
+}
 
 void iterate_kvm_ubsan_buffer(unsigned long left, unsigned long right)
 {
@@ -26,6 +35,7 @@ void iterate_kvm_ubsan_buffer(unsigned long left, unsigned 
long right)
slot = (struct kvm_ubsan_info *) 
this_cpu_ptr_nvhe_sym(kvm_ubsan_buffer);
for (i = left; i < right; ++i) {
/* check ubsan data */
+   __kvm_check_ubsan_data(slot + i);
slot[i].type = 0;
}
 }
-- 
2.30.0.284.gd98b1dd5eaa7-goog

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 5/9] KVM: arm64: Enable shift out of bounds undefined behaviour check for hyp/nVHE

[Elena Petrova]

From: George Popescu 

__ubsan_handle_shift_out_of_bounds data is passed to the buffer inside
hyp/nVHE. This data is passed to the original handler from kernel.

The 64bit values of the shift expression operands are stored as the lhs
and rhs pointers, so there is no need to dereference them.

Signed-off-by: George Popescu 
Signed-off-by: Elena Petrova 
---
 arch/arm64/include/asm/kvm_ubsan.h |  5 -
 arch/arm64/kvm/hyp/nvhe/ubsan.c| 14 +-
 arch/arm64/kvm/kvm_ubsan_buffer.c  |  4 
 3 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_ubsan.h 
b/arch/arm64/include/asm/kvm_ubsan.h
index 70c6f2541d07..0eef0e11a93b 100644
--- a/arch/arm64/include/asm/kvm_ubsan.h
+++ b/arch/arm64/include/asm/kvm_ubsan.h
@@ -19,11 +19,13 @@ struct ubsan_values {
 struct kvm_ubsan_info {
enum {
UBSAN_OUT_OF_BOUNDS,
-   UBSAN_UNREACHABLE_DATA
+   UBSAN_UNREACHABLE_DATA,
+   UBSAN_SHIFT_OUT_OF_BOUNDS
} type;
union {
struct out_of_bounds_data out_of_bounds_data;
struct unreachable_data unreachable_data;
+   struct shift_out_of_bounds_data shift_out_of_bounds_data;
};
union {
struct ubsan_values u_val;
@@ -32,3 +34,4 @@ struct kvm_ubsan_info {
 
 void __ubsan_handle_out_of_bounds(void *_data, void *index);
 void __ubsan_handle_builtin_unreachable(void *_data);
+void __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs);
diff --git a/arch/arm64/kvm/hyp/nvhe/ubsan.c b/arch/arm64/kvm/hyp/nvhe/ubsan.c
index 5e55897b2d72..1069ed5036d5 100644
--- a/arch/arm64/kvm/hyp/nvhe/ubsan.c
+++ b/arch/arm64/kvm/hyp/nvhe/ubsan.c
@@ -56,7 +56,19 @@ void __ubsan_handle_out_of_bounds(void *_data, void *index)
}
 }
 
-void __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs) {}
+void __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs)
+{
+   struct kvm_ubsan_info *slot;
+   struct shift_out_of_bounds_data *data = _data;
+
+   slot = kvm_ubsan_buffer_next_slot();
+   if (slot) {
+   slot->type = UBSAN_SHIFT_OUT_OF_BOUNDS;
+   slot->shift_out_of_bounds_data = *data;
+   slot->u_val.lval = lhs;
+   slot->u_val.rval = rhs;
+   }
+}
 
 void __ubsan_handle_builtin_unreachable(void *_data)
 {
diff --git a/arch/arm64/kvm/kvm_ubsan_buffer.c 
b/arch/arm64/kvm/kvm_ubsan_buffer.c
index e51949c275aa..b80045883047 100644
--- a/arch/arm64/kvm/kvm_ubsan_buffer.c
+++ b/arch/arm64/kvm/kvm_ubsan_buffer.c
@@ -27,6 +27,10 @@ void __kvm_check_ubsan_data(struct kvm_ubsan_info *slot)
case UBSAN_UNREACHABLE_DATA:
__ubsan_handle_builtin_unreachable(>unreachable_data);
break;
+   case UBSAN_SHIFT_OUT_OF_BOUNDS:
+   
__ubsan_handle_shift_out_of_bounds(>shift_out_of_bounds_data,
+   slot->u_val.lval, slot->u_val.rval);
+   break;
}
 }
 
-- 
2.30.0.284.gd98b1dd5eaa7-goog

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 2/9] KVM: arm64: Add a buffer that can pass UBSan data from hyp/nVHE to kernel

[Elena Petrova]

From: George Popescu 

Share a buffer between the kernel and the hyp/nVHE code by using the
macros from kvm_debug_buffer.h.
The hyp/nVHE code requires a write index which counts how many elements
have been writtens inside the buffer and the kernel requires a read
index which counts how many elements have been read from the buffer.
The write index and the buffer are shared with the kernel in read-only.

The kvm_debug_buffer_ind returns the reading and writing points of the
circular buffer and updates the reading index.

Data collected from UBSan handlers inside hyp/nVHE is stored in the
kvm_ubsan_buffer.
This buffer stores only UBSan data because it should not be preoccupied
by other mechanisms data structures and functionalities.

Also, for the moment the buffer is mapped inside .bss, where both the kernel
and the hyp/nVHE code have Read/Write rights, but in the future this will change
and the kernel will not be able to acess hyp/nVHE's .bss. At that point the 
buffer
will only need to be mapped in order for this patch to work.

Change-Id: I696409db1de629b082abfe4c7f6bf066f12b539f
Signed-off-by: Elena Petrova 
---
 arch/arm64/include/asm/assembler.h| 11 +++
 arch/arm64/include/asm/kvm_debug_buffer.h | 36 
 arch/arm64/include/asm/kvm_host.h |  8 -
 arch/arm64/include/asm/kvm_ubsan.h| 14 
 arch/arm64/kvm/Makefile   |  2 ++
 arch/arm64/kvm/arm.c  |  9 +
 arch/arm64/kvm/hyp/nvhe/host.S|  4 +++
 arch/arm64/kvm/hyp/nvhe/ubsan.c   | 23 +
 arch/arm64/kvm/kvm_ubsan_buffer.c | 40 +++
 9 files changed, 146 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/include/asm/kvm_debug_buffer.h
 create mode 100644 arch/arm64/include/asm/kvm_ubsan.h
 create mode 100644 arch/arm64/kvm/kvm_ubsan_buffer.c

diff --git a/arch/arm64/include/asm/assembler.h 
b/arch/arm64/include/asm/assembler.h
index bf125c591116..ebc18a8a0e1f 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -258,6 +258,17 @@ alternative_endif
ldr \dst, [\dst, \tmp]
.endm
 
+   /*
+ * @sym: The name of the per-cpu variable
+ * @reg: value to store
+ * @tmp1: scratch register
+ * @tmp2: scratch register
+ */
+.macro str_this_cpu sym, reg, tmp1, tmp2
+adr_this_cpu \tmp1, \sym, \tmp2
+str \reg, [\tmp1]
+.endm
+
 /*
  * vma_vm_mm - get mm pointer from vma pointer (vma->vm_mm)
  */
diff --git a/arch/arm64/include/asm/kvm_debug_buffer.h 
b/arch/arm64/include/asm/kvm_debug_buffer.h
new file mode 100644
index ..e5375c2cff1a
--- /dev/null
+++ b/arch/arm64/include/asm/kvm_debug_buffer.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright 2020 Google LLC
+ * Author: George Popescu 
+ */
+
+#include 
+
+
+#define KVM_DEBUG_BUFFER_SIZE 1000
+
+#ifdef __KVM_NVHE_HYPERVISOR__
+#define DEFINE_KVM_DEBUG_BUFFER(type_name, buffer_name, write_ind, size)\
+   DEFINE_PER_CPU(type_name, buffer_name)[size];   \
+   DEFINE_PER_CPU(unsigned long, write_ind) = 0;
+
+#define DECLARE_KVM_DEBUG_BUFFER(type_name, buffer_name, write_ind, size)\
+   DECLARE_PER_CPU(type_name, buffer_name)[size];  \
+   DECLARE_PER_CPU(unsigned long, write_ind);
+#else
+#define DECLARE_KVM_DEBUG_BUFFER(type_name, buffer_name, write_ind, size)\
+   DECLARE_KVM_NVHE_PER_CPU(type_name, buffer_name)[size]; \
+DECLARE_KVM_NVHE_PER_CPU(unsigned long, write_ind);
+#endif //__KVM_NVHE_HYPERVISOR__
+
+#ifdef __ASSEMBLY__
+#include 
+
+.macro clear_buffer tmp1, tmp2, tmp3
+mov \tmp1, 0
+#ifdef CONFIG_UBSAN
+str_this_cpu kvm_ubsan_buff_wr_ind, \tmp1, \tmp2, \tmp3
+#endif //CONFIG_UBSAN
+.endm
+
+#endif
\ No newline at end of file
diff --git a/arch/arm64/include/asm/kvm_host.h 
b/arch/arm64/include/asm/kvm_host.h
index 11beda85ee7e..385aa82c3fec 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -569,6 +569,12 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);
 void kvm_arm_halt_guest(struct kvm *kvm);
 void kvm_arm_resume_guest(struct kvm *kvm);
 
+
+#ifdef CONFIG_UBSAN
+extern void __kvm_check_ubsan_buffer(void);
+#else
+static inline void __kvm_check_ubsan_buffer(void) {}
+#endif /* CONFIG_UBSAN */
 #define kvm_call_hyp_nvhe(f, ...)  
\
({  \
struct arm_smccc_res res;   \
@@ -576,7 +582,7 @@ void kvm_arm_resume_guest(struct kvm *kvm);
arm_smccc_1_1_hvc(KVM_HOST_SMCCC_FUNC(f),   \
  ##__VA_ARGS__, ); \
WARN_ON(res.a0 != SMCCC_RET_SUCCESS);   \
-   

[PATCH v2 4/9] KVM: arm64: Enable UBsan check for unreachable code inside hyp/nVHE code

[Elena Petrova]

From: George Popescu 

The data from __ubsan_handle_builtin_unreachable is passed to the buffer
and printed inside the kernel by its simetric handler.

Signed-off-by: George Popescu 
Change-Id: I71d789b7f4ec3d4c787012a061b7f5d7952cee19
Signed-off-by: Elena Petrova 
---
 arch/arm64/include/asm/kvm_ubsan.h |  3 +++
 arch/arm64/kvm/hyp/nvhe/ubsan.c| 12 +++-
 arch/arm64/kvm/kvm_ubsan_buffer.c  |  3 +++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/kvm_ubsan.h 
b/arch/arm64/include/asm/kvm_ubsan.h
index 4f471acb88b0..70c6f2541d07 100644
--- a/arch/arm64/include/asm/kvm_ubsan.h
+++ b/arch/arm64/include/asm/kvm_ubsan.h
@@ -19,9 +19,11 @@ struct ubsan_values {
 struct kvm_ubsan_info {
enum {
UBSAN_OUT_OF_BOUNDS,
+   UBSAN_UNREACHABLE_DATA
} type;
union {
struct out_of_bounds_data out_of_bounds_data;
+   struct unreachable_data unreachable_data;
};
union {
struct ubsan_values u_val;
@@ -29,3 +31,4 @@ struct kvm_ubsan_info {
 };
 
 void __ubsan_handle_out_of_bounds(void *_data, void *index);
+void __ubsan_handle_builtin_unreachable(void *_data);
diff --git a/arch/arm64/kvm/hyp/nvhe/ubsan.c b/arch/arm64/kvm/hyp/nvhe/ubsan.c
index 55a8f6db8555..5e55897b2d72 100644
--- a/arch/arm64/kvm/hyp/nvhe/ubsan.c
+++ b/arch/arm64/kvm/hyp/nvhe/ubsan.c
@@ -58,6 +58,16 @@ void __ubsan_handle_out_of_bounds(void *_data, void *index)
 
 void __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs) {}
 
-void __ubsan_handle_builtin_unreachable(void *_data) {}
+void __ubsan_handle_builtin_unreachable(void *_data)
+{
+   struct kvm_ubsan_info *slot;
+   struct unreachable_data *data = _data;
+
+   slot = kvm_ubsan_buffer_next_slot();
+   if (slot) {
+   slot->type = UBSAN_UNREACHABLE_DATA;
+   slot->unreachable_data = *data;
+   }
+}
 
 void __ubsan_handle_load_invalid_value(void *_data, void *val) {}
diff --git a/arch/arm64/kvm/kvm_ubsan_buffer.c 
b/arch/arm64/kvm/kvm_ubsan_buffer.c
index a1523f86be3c..e51949c275aa 100644
--- a/arch/arm64/kvm/kvm_ubsan_buffer.c
+++ b/arch/arm64/kvm/kvm_ubsan_buffer.c
@@ -24,6 +24,9 @@ void __kvm_check_ubsan_data(struct kvm_ubsan_info *slot)
__ubsan_handle_out_of_bounds(>out_of_bounds_data,
slot->u_val.lval);
break;
+   case UBSAN_UNREACHABLE_DATA:
+   __ubsan_handle_builtin_unreachable(>unreachable_data);
+   break;
}
 }
 
-- 
2.30.0.284.gd98b1dd5eaa7-goog

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 9/9] KVM: arm64: Add UBSan tests for PKVM.

[Elena Petrova]

From: George-Aurelian Popescu 

Test the UBsan functionality inside hyp/nVHE.
Because modules are not supported inside of hyp/nVHE code, the default
testing module for UBSan can not be used.
New functions have to be defined inside of hyp/nVHE.
They are called in kvm_get_mdcr_el2, to test UBSAN whenever a VM starts.

Change-Id: Icf998da0af023c74d45be90788ac9f694e61c97c
Signed-off-by: Elena Petrova 
---
 arch/arm64/include/asm/assembler.h  |  17 ++-
 arch/arm64/include/asm/kvm_debug_buffer.h   |  10 +-
 arch/arm64/include/asm/kvm_ubsan.h  |   2 +-
 arch/arm64/kvm/hyp/include/hyp/test_ubsan.h | 112 
 arch/arm64/kvm/hyp/nvhe/hyp-main.c  |   3 +
 arch/arm64/kvm/kvm_ubsan_buffer.c   |   1 -
 6 files changed, 128 insertions(+), 17 deletions(-)
 create mode 100644 arch/arm64/kvm/hyp/include/hyp/test_ubsan.h

diff --git a/arch/arm64/include/asm/assembler.h 
b/arch/arm64/include/asm/assembler.h
index ebc18a8a0e1f..8422b0d925e8 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -259,16 +259,15 @@ alternative_endif
.endm
 
/*
- * @sym: The name of the per-cpu variable
- * @reg: value to store
- * @tmp1: scratch register
- * @tmp2: scratch register
- */
-.macro str_this_cpu sym, reg, tmp1, tmp2
-adr_this_cpu \tmp1, \sym, \tmp2
+* @sym: The name of the per-cpu variable
+* @reg: value to store
+* @tmp1: scratch register
+* @tmp2: scratch register
+*/
+   .macro str_this_cpu sym, reg, tmp1, tmp2
+   adr_this_cpu \tmp1, \sym, \tmp2
 str \reg, [\tmp1]
-.endm
-
+   .endm
 /*
  * vma_vm_mm - get mm pointer from vma pointer (vma->vm_mm)
  */
diff --git a/arch/arm64/include/asm/kvm_debug_buffer.h 
b/arch/arm64/include/asm/kvm_debug_buffer.h
index e5375c2cff1a..361b473bb004 100644
--- a/arch/arm64/include/asm/kvm_debug_buffer.h
+++ b/arch/arm64/include/asm/kvm_debug_buffer.h
@@ -3,10 +3,8 @@
  * Copyright 2020 Google LLC
  * Author: George Popescu 
  */
-
 #include 
 
-
 #define KVM_DEBUG_BUFFER_SIZE 1000
 
 #ifdef __KVM_NVHE_HYPERVISOR__
@@ -20,17 +18,17 @@
 #else
 #define DECLARE_KVM_DEBUG_BUFFER(type_name, buffer_name, write_ind, size)\
DECLARE_KVM_NVHE_PER_CPU(type_name, buffer_name)[size]; \
-DECLARE_KVM_NVHE_PER_CPU(unsigned long, write_ind);
+   DECLARE_KVM_NVHE_PER_CPU(unsigned long, write_ind);
 #endif //__KVM_NVHE_HYPERVISOR__
 
 #ifdef __ASSEMBLY__
 #include 
 
 .macro clear_buffer tmp1, tmp2, tmp3
-mov \tmp1, 0
+   mov \tmp1, 0
 #ifdef CONFIG_UBSAN
-str_this_cpu kvm_ubsan_buff_wr_ind, \tmp1, \tmp2, \tmp3
+   str_this_cpu kvm_ubsan_buff_wr_ind, \tmp1, \tmp2, \tmp3
 #endif //CONFIG_UBSAN
 .endm
 
-#endif
\ No newline at end of file
+#endif
diff --git a/arch/arm64/include/asm/kvm_ubsan.h 
b/arch/arm64/include/asm/kvm_ubsan.h
index da4a3b4e28e0..0b8bed08d48e 100644
--- a/arch/arm64/include/asm/kvm_ubsan.h
+++ b/arch/arm64/include/asm/kvm_ubsan.h
@@ -9,7 +9,6 @@
 #define UBSAN_MAX_TYPE 6
 #define KVM_UBSAN_BUFFER_SIZE 1000
 
-
 struct ubsan_values {
void *lval;
void *rval;
@@ -18,6 +17,7 @@ struct ubsan_values {
 
 struct kvm_ubsan_info {
enum {
+   UBSAN_NONE,
UBSAN_OUT_OF_BOUNDS,
UBSAN_UNREACHABLE_DATA,
UBSAN_SHIFT_OUT_OF_BOUNDS,
diff --git a/arch/arm64/kvm/hyp/include/hyp/test_ubsan.h 
b/arch/arm64/kvm/hyp/include/hyp/test_ubsan.h
new file mode 100644
index ..07759c0d1e0e
--- /dev/null
+++ b/arch/arm64/kvm/hyp/include/hyp/test_ubsan.h
@@ -0,0 +1,112 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include 
+
+typedef void(*test_ubsan_fp)(void);
+
+static void test_ubsan_add_overflow(void)
+{
+   volatile int val = INT_MAX;
+
+   val += 2;
+}
+
+static void test_ubsan_sub_overflow(void)
+{
+   volatile int val = INT_MIN;
+   volatile int val2 = 2;
+
+   val -= val2;
+}
+
+static void test_ubsan_mul_overflow(void)
+{
+   volatile int val = INT_MAX / 2;
+
+   val *= 3;
+}
+
+static void test_ubsan_negate_overflow(void)
+{
+   volatile int val = INT_MIN;
+
+   val = -val;
+}
+
+static void test_ubsan_divrem_overflow(void)
+{
+   volatile int val = 16;
+   volatile int val2 = 0;
+
+   val /= val2;
+}
+
+static void test_ubsan_shift_out_of_bounds(void)
+{
+   volatile int val = -1;
+   int val2 = 10;
+
+   val2 <<= val;
+}
+
+static void test_ubsan_out_of_bounds(void)
+{
+   volatile int i = 4, j = 5;
+   volatile int arr[4];
+   
+   arr[j] = i;
+}
+
+static void test_ubsan_load_invalid_value(void)
+{
+   volatile char *dst, *src;
+   bool val, val2, *ptr;
+   char c = 4;
+
+   dst = (char *)
+   src = 
+   *dst = *src;
+
+   ptr = 
+   val2 = val;
+}
+
+static void test_ubsan_misaligned_access(void)
+{
+   volatile char arr[5] 

[PATCH v2 1/9] KVM: arm64: Enable UBSan instrumentation in nVHE hyp code

[Elena Petrova]

From: George-Aurelian Popescu 

Implement UBSan handlers inside nVHe hyp code, as empty functions for the
moment, so the undefined behaviours, that are triggered there, will be
linked to them, not to the ones defined in kernel-proper lib/ubsan.c.

In this way, enabling UBSAN_MISC won't cause a link error.

Change-Id: I4a468b33251fa099ddfc05a7cefa520cb8817994
Signed-off-by: Elena Petrova 
---
 arch/arm64/kvm/hyp/nvhe/Makefile |  3 ++-
 arch/arm64/kvm/hyp/nvhe/ubsan.c  | 30 ++
 2 files changed, 32 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/kvm/hyp/nvhe/ubsan.c

diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
index 1f1e351c5fe2..2a683e7c6c5b 100644
--- a/arch/arm64/kvm/hyp/nvhe/Makefile
+++ b/arch/arm64/kvm/hyp/nvhe/Makefile
@@ -10,6 +10,8 @@ obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o 
hyp-init.o host.o \
 hyp-main.o hyp-smp.o psci-relay.o
 obj-y += ../vgic-v3-sr.o ../aarch32.o ../vgic-v2-cpuif-proxy.o ../entry.o \
 ../fpsimd.o ../hyp-entry.o ../exception.o
+obj-$(CONFIG_UBSAN) += ubsan.o
+CFLAGS_ubsan.nvhe.o += -I $(srctree)/lib/
 
 ##
 ## Build rules for compiling nVHE hyp code
@@ -61,7 +63,6 @@ KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_FTRACE) 
$(CC_FLAGS_SCS), $(KBUILD_CFLAG
 # cause crashes. Just disable it.
 GCOV_PROFILE   := n
 KASAN_SANITIZE := n
-UBSAN_SANITIZE := n
 KCOV_INSTRUMENT:= n
 
 # Skip objtool checking for this directory because nVHE code is compiled with
diff --git a/arch/arm64/kvm/hyp/nvhe/ubsan.c b/arch/arm64/kvm/hyp/nvhe/ubsan.c
new file mode 100644
index ..a5db6b61ceb2
--- /dev/null
+++ b/arch/arm64/kvm/hyp/nvhe/ubsan.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2020 Google LLC
+ * Author: George Popescu 
+ */
+#include 
+#include 
+#include 
+
+void __ubsan_handle_add_overflow(void *_data, void *lhs, void *rhs) {}
+
+void __ubsan_handle_sub_overflow(void *_data, void *lhs, void *rhs) {}
+
+void __ubsan_handle_mul_overflow(void *_data, void *lhs, void *rhs) {}
+
+void __ubsan_handle_negate_overflow(void *_data, void *old_val) {}
+
+void __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs) {}
+
+void __ubsan_handle_type_mismatch(struct type_mismatch_data *data, void *ptr) 
{}
+
+void __ubsan_handle_type_mismatch_v1(void *_data, void *ptr) {}
+
+void __ubsan_handle_out_of_bounds(void *_data, void *index) {}
+
+void __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs) {}
+
+void __ubsan_handle_builtin_unreachable(void *_data) {}
+
+void __ubsan_handle_load_invalid_value(void *_data, void *val) {}
-- 
2.30.0.284.gd98b1dd5eaa7-goog

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 6/9] KVM: arm64: __ubsan_handle_load_invalid_value EL2 implementation.

[Elena Petrova]

From: George Popescu 

The handler for the load invalid value undefined behaviour is
implemented at EL2. The EL2 handler's parameters are stored inside the buffer.
They are used by the symetric handler from EL1.

Signed-off-by: George Popescu 
Signed-off-by: Elena Petrova 
---
 arch/arm64/include/asm/kvm_ubsan.h |  5 -
 arch/arm64/kvm/hyp/nvhe/ubsan.c| 14 +-
 arch/arm64/kvm/kvm_ubsan_buffer.c  |  6 +-
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_ubsan.h 
b/arch/arm64/include/asm/kvm_ubsan.h
index 0eef0e11a93b..95ac6728ffd1 100644
--- a/arch/arm64/include/asm/kvm_ubsan.h
+++ b/arch/arm64/include/asm/kvm_ubsan.h
@@ -20,12 +20,14 @@ struct kvm_ubsan_info {
enum {
UBSAN_OUT_OF_BOUNDS,
UBSAN_UNREACHABLE_DATA,
-   UBSAN_SHIFT_OUT_OF_BOUNDS
+   UBSAN_SHIFT_OUT_OF_BOUNDS,
+   UBSAN_INVALID_DATA
} type;
union {
struct out_of_bounds_data out_of_bounds_data;
struct unreachable_data unreachable_data;
struct shift_out_of_bounds_data shift_out_of_bounds_data;
+   struct invalid_value_data invalid_value_data;
};
union {
struct ubsan_values u_val;
@@ -35,3 +37,4 @@ struct kvm_ubsan_info {
 void __ubsan_handle_out_of_bounds(void *_data, void *index);
 void __ubsan_handle_builtin_unreachable(void *_data);
 void __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs);
+void __ubsan_handle_load_invalid_value(void *_data, void *val);
diff --git a/arch/arm64/kvm/hyp/nvhe/ubsan.c b/arch/arm64/kvm/hyp/nvhe/ubsan.c
index 1069ed5036d5..3143f7722be2 100644
--- a/arch/arm64/kvm/hyp/nvhe/ubsan.c
+++ b/arch/arm64/kvm/hyp/nvhe/ubsan.c
@@ -82,4 +82,16 @@ void __ubsan_handle_builtin_unreachable(void *_data)
}
 }
 
-void __ubsan_handle_load_invalid_value(void *_data, void *val) {}
+void __ubsan_handle_load_invalid_value(void *_data, void *val)
+{
+   struct kvm_ubsan_info *slot;
+   struct invalid_value_data *data = _data;
+
+   slot = kvm_ubsan_buffer_next_slot();
+   if (slot) {
+   slot->type = UBSAN_INVALID_DATA;
+   slot->invalid_value_data = *data;
+   slot->u_val.lval = val;
+   }
+
+}
diff --git a/arch/arm64/kvm/kvm_ubsan_buffer.c 
b/arch/arm64/kvm/kvm_ubsan_buffer.c
index b80045883047..5439f7a91636 100644
--- a/arch/arm64/kvm/kvm_ubsan_buffer.c
+++ b/arch/arm64/kvm/kvm_ubsan_buffer.c
@@ -31,7 +31,11 @@ void __kvm_check_ubsan_data(struct kvm_ubsan_info *slot)

__ubsan_handle_shift_out_of_bounds(>shift_out_of_bounds_data,
slot->u_val.lval, slot->u_val.rval);
break;
-   }
+   case UBSAN_INVALID_DATA:
+   __ubsan_handle_load_invalid_value(>invalid_value_data,
+   slot->u_val.lval);
+   break;
+   }
 }
 
 void iterate_kvm_ubsan_buffer(unsigned long left, unsigned long right)
-- 
2.30.0.284.gd98b1dd5eaa7-goog

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 0/9] UBSan Enablement for hyp/nVHE code

[Elena Petrova]

Respin of George's patch series enabling UBSAN for hyp/nVHE code.

Modification in v2:
  * CONFIG_KVM_ARM_DEBUG_BUFFER removed; __kvm_check_ubsan_buffer is
called directly instead of via __kvm_arm_check_debug_buffer.
  * Bugfixing commits removed as these are already upstream.
  * Some code brought up to date, i.e. moved from entry.S to host.S.
  * Merged "Add support for creating and checking a buffer" and
"Add a buffer that can pass UBSan data from hyp/nVHE" into
one commit as these changes don't work without each other.

George Popescu (9):
  KVM: arm64: Enable UBSan instrumentation in nVHE hyp code
  KVM: arm64: Add a buffer that can pass UBSan data from hyp/nVHE to
kernel
  KVM: arm64: Enable UBSAN_BOUNDS for the both the kernel and hyp/nVHE
  KVM: arm64: Enable UBsan check for unreachable code inside hyp/nVHE
code
  KVM: arm64: Enable shift out of bounds undefined behaviour check for
hyp/nVHE
  KVM: arm64: __ubsan_handle_load_invalid_value EL2 implementation.
  KVM: arm64: Detect type mismatch undefined behaviour from hyp/nVHE
code
  KVM: arm64: Detect arithmetic overflow is inside hyp/nVHE.
  KVM: arm64: Add UBSan tests for PKVM.

 arch/arm64/include/asm/assembler.h  |  10 ++
 arch/arm64/include/asm/kvm_debug_buffer.h   |  34 
 arch/arm64/include/asm/kvm_host.h   |   8 +-
 arch/arm64/include/asm/kvm_ubsan.h  |  50 ++
 arch/arm64/kvm/Makefile |   2 +
 arch/arm64/kvm/arm.c|   9 ++
 arch/arm64/kvm/hyp/include/hyp/test_ubsan.h | 112 +
 arch/arm64/kvm/hyp/nvhe/Makefile|   3 +-
 arch/arm64/kvm/hyp/nvhe/host.S  |   4 +
 arch/arm64/kvm/hyp/nvhe/hyp-main.c  |   3 +
 arch/arm64/kvm/hyp/nvhe/ubsan.c | 164 
 arch/arm64/kvm/kvm_ubsan_buffer.c   |  81 ++
 12 files changed, 478 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm64/include/asm/kvm_debug_buffer.h
 create mode 100644 arch/arm64/include/asm/kvm_ubsan.h
 create mode 100644 arch/arm64/kvm/hyp/include/hyp/test_ubsan.h
 create mode 100644 arch/arm64/kvm/hyp/nvhe/ubsan.c
 create mode 100644 arch/arm64/kvm/kvm_ubsan_buffer.c

-- 
2.30.0.284.gd98b1dd5eaa7-goog

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 7/9] KVM: arm64: Detect type mismatch undefined behaviour from hyp/nVHE code

[Elena Petrova]

From: George Popescu 

Type mismatch undefiend behaviour handler provides two handlers with two
data structures type_mismatch_data and type_mismatch_data_v1. Both can be
stored inside a common data structure: type_mismatch_data_common, which
differs of type_mismatch_data only by keeping a pointer to a
struct source_location.
In this way, the buffer keeps the data encapsulated inside of a struct
type_mismatch_data, because pointers from nVHE can not be passed to the
kernel.

Inside the kernel call the __ubsan_handle_type_mismatch_data with the
data from the buffer.

Signed-off-by: George Popescu 
Signed-off-by: Elena Petrova 
---
 arch/arm64/include/asm/kvm_ubsan.h |  6 -
 arch/arm64/kvm/hyp/nvhe/ubsan.c| 41 --
 arch/arm64/kvm/kvm_ubsan_buffer.c  |  5 +++-
 3 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_ubsan.h 
b/arch/arm64/include/asm/kvm_ubsan.h
index 95ac6728ffd1..93c1b695097a 100644
--- a/arch/arm64/include/asm/kvm_ubsan.h
+++ b/arch/arm64/include/asm/kvm_ubsan.h
@@ -21,13 +21,15 @@ struct kvm_ubsan_info {
UBSAN_OUT_OF_BOUNDS,
UBSAN_UNREACHABLE_DATA,
UBSAN_SHIFT_OUT_OF_BOUNDS,
-   UBSAN_INVALID_DATA
+   UBSAN_INVALID_DATA,
+   UBSAN_TYPE_MISMATCH
} type;
union {
struct out_of_bounds_data out_of_bounds_data;
struct unreachable_data unreachable_data;
struct shift_out_of_bounds_data shift_out_of_bounds_data;
struct invalid_value_data invalid_value_data;
+   struct type_mismatch_data type_mismatch_data;
};
union {
struct ubsan_values u_val;
@@ -38,3 +40,5 @@ void __ubsan_handle_out_of_bounds(void *_data, void *index);
 void __ubsan_handle_builtin_unreachable(void *_data);
 void __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs);
 void __ubsan_handle_load_invalid_value(void *_data, void *val);
+void __ubsan_handle_type_mismatch(struct type_mismatch_data  *_data, void 
*ptr);
+
diff --git a/arch/arm64/kvm/hyp/nvhe/ubsan.c b/arch/arm64/kvm/hyp/nvhe/ubsan.c
index 3143f7722be2..a9f72d4bcab7 100644
--- a/arch/arm64/kvm/hyp/nvhe/ubsan.c
+++ b/arch/arm64/kvm/hyp/nvhe/ubsan.c
@@ -29,6 +29,24 @@ static inline struct kvm_ubsan_info 
*kvm_ubsan_buffer_next_slot(void)
return res;
 }
 
+static void write_type_mismatch_data(struct type_mismatch_data_common *data, 
void *lval)
+{
+   struct kvm_ubsan_info *slot;
+   struct type_mismatch_data *aux_cont;
+
+   slot = kvm_ubsan_buffer_next_slot();
+   if (slot) {
+   slot->type = UBSAN_TYPE_MISMATCH;
+   aux_cont = >type_mismatch_data;
+   aux_cont->location.file_name = data->location->file_name;
+   aux_cont->location.reported = data->location->reported;
+   aux_cont->type = data->type;
+   aux_cont->alignment = data->alignment;
+   aux_cont->type_check_kind = data->type_check_kind;
+   slot->u_val.lval = lval;
+   }
+}
+
 void __ubsan_handle_add_overflow(void *_data, void *lhs, void *rhs) {}
 
 void __ubsan_handle_sub_overflow(void *_data, void *lhs, void *rhs) {}
@@ -39,9 +57,28 @@ void __ubsan_handle_negate_overflow(void *_data, void 
*old_val) {}
 
 void __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs) {}
 
-void __ubsan_handle_type_mismatch(struct type_mismatch_data *data, void *ptr) 
{}
+void __ubsan_handle_type_mismatch(struct type_mismatch_data *data, void *ptr)
+{
+   struct type_mismatch_data_common common_data = {
+   .location = >location,
+   .type = data->type,
+   .alignment = data->alignment,
+   .type_check_kind = data->type_check_kind
+   };
+   write_type_mismatch_data(_data, ptr);
+}
 
-void __ubsan_handle_type_mismatch_v1(void *_data, void *ptr) {}
+void __ubsan_handle_type_mismatch_v1(void *_data, void *ptr)
+{
+   struct type_mismatch_data_v1 *data = _data;
+   struct type_mismatch_data_common common_data = {
+   .location = >location,
+   .type = data->type,
+   .alignment = 1UL << data->log_alignment,
+   .type_check_kind = data->type_check_kind
+   };
+   write_type_mismatch_data(_data, ptr);
+}
 
 void __ubsan_handle_out_of_bounds(void *_data, void *index)
 {
diff --git a/arch/arm64/kvm/kvm_ubsan_buffer.c 
b/arch/arm64/kvm/kvm_ubsan_buffer.c
index 5439f7a91636..b7823dedf8b1 100644
--- a/arch/arm64/kvm/kvm_ubsan_buffer.c
+++ b/arch/arm64/kvm/kvm_ubsan_buffer.c
@@ -35,6 +35,10 @@ void __kvm_check_ubsan_data(struct kvm_ubsan_info *slot)
__ubsan_handle_load_invalid_value(>invalid_value_data,
slot->u_val.lval);
break;
+   case UBSAN_TYPE_MISMATCH:
+   __ubsan_handle_type_mismatch(>type_mismatch_data,
+   

Re: [PATCH v13 07/15] iommu/smmuv3: Allow stage 1 invalidation with unmanaged ASIDs

[Auger Eric]

Hi Jean,

On 1/14/21 6:33 PM, Jean-Philippe Brucker wrote:
> Hi Eric,
> 
> On Thu, Jan 14, 2021 at 05:58:27PM +0100, Auger Eric wrote:
  The uacce-devel branches from
> https://github.com/Linaro/linux-kernel-uadk do provide this at the moment
> (they track the latest sva/zip-devel branch
> https://jpbrucker.net/git/linux/ which is roughly based on mainline.)
>> As I plan to respin shortly, please could you confirm the best branch to
>> rebase on still is that one (uacce-devel from the linux-kernel-uadk git
>> repo). Is it up to date? Commits seem to be quite old there.
> 
> Right I meant the uacce-devel-X branches. The uacce-devel-5.11 branch
> currently has the latest patches

OK thanks!

Eric
> 
> Thanks,
> Jean
> 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v13 07/15] iommu/smmuv3: Allow stage 1 invalidation with unmanaged ASIDs

[Jean-Philippe Brucker]

Hi Eric,

On Thu, Jan 14, 2021 at 05:58:27PM +0100, Auger Eric wrote:
> >>  The uacce-devel branches from
> >>> https://github.com/Linaro/linux-kernel-uadk do provide this at the moment
> >>> (they track the latest sva/zip-devel branch
> >>> https://jpbrucker.net/git/linux/ which is roughly based on mainline.)
> As I plan to respin shortly, please could you confirm the best branch to
> rebase on still is that one (uacce-devel from the linux-kernel-uadk git
> repo). Is it up to date? Commits seem to be quite old there.

Right I meant the uacce-devel-X branches. The uacce-devel-5.11 branch
currently has the latest patches

Thanks,
Jean
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/5] vfio/iommu_type1: Fixes vfio_dma_populate_bitmap to avoid dirty lose

[Alex Williamson]

On Thu, 14 Jan 2021 21:05:23 +0800
Keqian Zhu  wrote:

> Hi Alex,
> 
> On 2021/1/13 5:20, Alex Williamson wrote:
> > On Thu, 7 Jan 2021 17:28:57 +0800
> > Keqian Zhu  wrote:
> >   
> >> Defer checking whether vfio_dma is of fully-dirty in update_user_bitmap
> >> is easy to lose dirty log. For example, after promoting pinned_scope of
> >> vfio_iommu, vfio_dma is not considered as fully-dirty, then we may lose
> >> dirty log that occurs before vfio_iommu is promoted.
> >>
> >> The key point is that pinned-dirty is not a real dirty tracking way, it
> >> can't continuously track dirty pages, but just restrict dirty scope. It
> >> is essentially the same as fully-dirty. Fully-dirty is of full-scope and
> >> pinned-dirty is of pinned-scope.
> >>
> >> So we must mark pinned-dirty or fully-dirty after we start dirty tracking
> >> or clear dirty bitmap, to ensure that dirty log is marked right away.  
> > 
> > I was initially convinced by these first three patches, but upon
> > further review, I think the premise is wrong.  AIUI, the concern across
> > these patches is that our dirty bitmap is only populated with pages
> > dirtied by pinning and we only take into account the pinned page dirty
> > scope at the time the bitmap is retrieved by the user.  You suppose
> > this presents a gap where if a vendor driver has not yet identified
> > with a page pinning scope that the entire bitmap should be considered
> > dirty regardless of whether that driver later pins pages prior to the
> > user retrieving the dirty bitmap.  
> Yes, this is my concern. And there are some other scenarios.
> 
> 1. If a non-pinned iommu-backed domain is detached between starting
> dirty log and retrieving dirty bitmap, then the dirty log is missed
> (As you said in the last paragraph).
> 
> 2. If all vendor drivers pinned (means iommu pinned_scope is true),
> and an vendor driver do pin/unpin pair between starting dirty log and
> retrieving dirty bitmap, then the dirty log of these unpinned pages
> are missed.

Can you identity where this happen?  I believe this assertion is
incorrect.  When dirty logging is enabled vfio_dma_populate_bitmap()
marks all existing pinned pages as dirty.  In the scenario you
describe, the iommu pinned page dirty scope is irrelevant.  We only
track pinned or external DMA access pages for exactly this reason.
Unpinning a page never clears the dirty bitmap, only the user
retrieving the bitmap or disabling dirty logging clears the bitmap.  A
page that has been unpinned is transiently dirty, it is not repopulated
in the bitmap after the user has retrieved the bitmap because the
device no longer has access to it to consider it perpetually dirty.

> > I don't think this is how we intended the cooperation between the
> > iommu driver and vendor driver to work.  By pinning pages a vendor
> > driver is not declaring that only their future dirty page scope is
> > limited to pinned pages, instead they're declaring themselves as a
> > participant in dirty page tracking and take responsibility for
> > pinning any necessary pages.  For example we might extend
> > VFIO_IOMMU_DIRTY_PAGES_FLAG_START to trigger a blocking
> > notification to groups to not only begin dirty tracking, but also
> > to synchronously register their current device DMA footprint.  This
> > patch would require a vendor driver to possibly perform a
> > gratuitous page pinning in order to set the scope prior to dirty
> > logging being enabled, or else the initial bitmap will be fully
> > dirty.  
> I get what you mean ;-). You said that there is time gap between we
> attach a device and this device declares itself is dirty traceable.
> 
> However, this makes it difficult to management dirty log tracking (I
> list two bugs). If the vfio devices can declare themselves dirty
> traceable when attach to container, then the logic of dirty tracking
> is much more clear ;-) .

There's only one bug above afaict, which should be easily fixed.  I
think if you actually dig into the problem of a device declaring
themselves as dirty tracking capable, when the IOMMU backend works on
group, not devices, and it's groups that are attached to containers,
you might see that the logic is not so clear.  I also don't see this as
a significant issue, you're focusing on a niche scenario where a device
is hot-added to a VM, which is immediately migrated before the device
identifies itself by pinning pages.  In that scenario the iommu dirty
scope would be overly broad and we'd indicate all pages are dirty.
This errors on the side of reporting too much is dirty, which still
provides a correct result to the user.  The more common scenario would
be migration of a "long" running, stable VM, where drivers are active
and devices have already pinned pages if they intend to.

> > Therefore, I don't see that this series is necessary or correct.
> > Kirti, does this match your thinking?
> > 
> > Thinking about these semantics, it seems there might still be an
> > issue if a group with 

RE: [PATCH v13 07/15] iommu/smmuv3: Allow stage 1 invalidation with unmanaged ASIDs

[Shameerali Kolothum Thodi]

Hi Eric,

> -Original Message-
> From: Auger Eric [mailto:eric.au...@redhat.com]
> Sent: 14 January 2021 16:58
> To: Shameerali Kolothum Thodi ;
> Jean-Philippe Brucker 
> Cc: Xieyingtai ; alex.william...@redhat.com;
> wangxingang ; k...@vger.kernel.org;
> m...@kernel.org; linux-ker...@vger.kernel.org; vivek.gau...@arm.com;
> io...@lists.linux-foundation.org; qubingbing ;
> Zengtao (B) ; zhangfei@linaro.org;
> eric.auger@gmail.com; w...@kernel.org; kvmarm@lists.cs.columbia.edu;
> robin.mur...@arm.com
> Subject: Re: [PATCH v13 07/15] iommu/smmuv3: Allow stage 1 invalidation with
> unmanaged ASIDs
> 
> Hi Shameer, Jean-Philippe,
> 
> On 12/4/20 11:23 AM, Auger Eric wrote:
> > Hi Shameer, Jean-Philippe,
> >
> > On 12/4/20 11:20 AM, Shameerali Kolothum Thodi wrote:
> >> Hi Jean,
> >>
> >>> -Original Message-
> >>> From: Jean-Philippe Brucker [mailto:jean-phili...@linaro.org]
> >>> Sent: 04 December 2020 09:54
> >>> To: Shameerali Kolothum Thodi
> 
> >>> Cc: Auger Eric ; wangxingang
> >>> ; Xieyingtai ;
> >>> k...@vger.kernel.org; m...@kernel.org; j...@8bytes.org;
> w...@kernel.org;
> >>> io...@lists.linux-foundation.org; linux-ker...@vger.kernel.org;
> >>> vivek.gau...@arm.com; alex.william...@redhat.com;
> >>> zhangfei@linaro.org; robin.mur...@arm.com;
> >>> kvmarm@lists.cs.columbia.edu; eric.auger@gmail.com; Zengtao (B)
> >>> ; qubingbing 
> >>> Subject: Re: [PATCH v13 07/15] iommu/smmuv3: Allow stage 1 invalidation
> with
> >>> unmanaged ASIDs
> >>>
> >>> Hi Shameer,
> >>>
> >>> On Thu, Dec 03, 2020 at 06:42:57PM +, Shameerali Kolothum Thodi
> wrote:
>  Hi Jean/zhangfei,
>  Is it possible to have a branch with minimum required SVA/UACCE related
> >>> patches
>  that are already public and can be a "stable" candidate for future respin
> of
> >>> Eric's series?
>  Please share your thoughts.
> >>>
> >>> By "stable" you mean a fixed branch with the latest SVA/UACCE patches
> >>> based on mainline?
> >>
> >> Yes.
> >>
> >>  The uacce-devel branches from
> >>> https://github.com/Linaro/linux-kernel-uadk do provide this at the moment
> >>> (they track the latest sva/zip-devel branch
> >>> https://jpbrucker.net/git/linux/ which is roughly based on mainline.)
> As I plan to respin shortly, please could you confirm the best branch to
> rebase on still is that one (uacce-devel from the linux-kernel-uadk git
> repo). Is it up to date? Commits seem to be quite old there.

I think it is the uacce-devel-5.11 branch, but will wait for Jean or Zhangfei
to confirm.

Thanks,
Shameer

> Thanks
> 
> Eric
> >>
> >> Thanks.
> >>
> >> Hi Eric,
> >>
> >> Could you please take a look at the above branches and see whether it make
> sense
> >> to rebase on top of either of those?
> >>
> >> From vSVA point of view, it will be less rebase hassle if we can do that.
> >
> > Sure. I will rebase on top of this ;-)
> >
> > Thanks
> >
> > Eric
> >>
> >> Thanks,
> >> Shameer
> >>
> >>> Thanks,
> >>> Jean
> >>
> >
> > ___
> > iommu mailing list
> > io...@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/iommu
> >

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v13 07/15] iommu/smmuv3: Allow stage 1 invalidation with unmanaged ASIDs

[Auger Eric]

Hi Shameer, Jean-Philippe,

On 12/4/20 11:23 AM, Auger Eric wrote:
> Hi Shameer, Jean-Philippe,
> 
> On 12/4/20 11:20 AM, Shameerali Kolothum Thodi wrote:
>> Hi Jean,
>>
>>> -Original Message-
>>> From: Jean-Philippe Brucker [mailto:jean-phili...@linaro.org]
>>> Sent: 04 December 2020 09:54
>>> To: Shameerali Kolothum Thodi 
>>> Cc: Auger Eric ; wangxingang
>>> ; Xieyingtai ;
>>> k...@vger.kernel.org; m...@kernel.org; j...@8bytes.org; w...@kernel.org;
>>> io...@lists.linux-foundation.org; linux-ker...@vger.kernel.org;
>>> vivek.gau...@arm.com; alex.william...@redhat.com;
>>> zhangfei@linaro.org; robin.mur...@arm.com;
>>> kvmarm@lists.cs.columbia.edu; eric.auger@gmail.com; Zengtao (B)
>>> ; qubingbing 
>>> Subject: Re: [PATCH v13 07/15] iommu/smmuv3: Allow stage 1 invalidation with
>>> unmanaged ASIDs
>>>
>>> Hi Shameer,
>>>
>>> On Thu, Dec 03, 2020 at 06:42:57PM +, Shameerali Kolothum Thodi wrote:
 Hi Jean/zhangfei,
 Is it possible to have a branch with minimum required SVA/UACCE related
>>> patches
 that are already public and can be a "stable" candidate for future respin 
 of
>>> Eric's series?
 Please share your thoughts.
>>>
>>> By "stable" you mean a fixed branch with the latest SVA/UACCE patches
>>> based on mainline? 
>>
>> Yes. 
>>
>>  The uacce-devel branches from
>>> https://github.com/Linaro/linux-kernel-uadk do provide this at the moment
>>> (they track the latest sva/zip-devel branch
>>> https://jpbrucker.net/git/linux/ which is roughly based on mainline.)
As I plan to respin shortly, please could you confirm the best branch to
rebase on still is that one (uacce-devel from the linux-kernel-uadk git
repo). Is it up to date? Commits seem to be quite old there.

Thanks

Eric
>>
>> Thanks. 
>>
>> Hi Eric,
>>
>> Could you please take a look at the above branches and see whether it make 
>> sense
>> to rebase on top of either of those?
>>
>> From vSVA point of view, it will be less rebase hassle if we can do that.
> 
> Sure. I will rebase on top of this ;-)
> 
> Thanks
> 
> Eric
>>
>> Thanks,
>> Shameer
>>
>>> Thanks,
>>> Jean
>>
> 
> ___
> iommu mailing list
> io...@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
> 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvmarm:next 29/32] arch/arm64/kvm/sys_regs.c:1544:13: warning: initializer overrides prior initialization of this subobject

[kernel test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
head:   7ba8b4380afbdbb29d53c50bee6563cd7457fc34
commit: 11663111cd49b4c6dd27479774e420f139e4c447 [29/32] KVM: arm64: Hide PMU 
registers from userspace when not available
config: arm64-randconfig-r031-20210114 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 
6077d55381a6aa3e947ef7abdc36a7515c598c8a)
reproduce (this is a W=1 build):
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# 
https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git/commit/?id=11663111cd49b4c6dd27479774e420f139e4c447
git remote add kvmarm 
https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git
git fetch --no-tags kvmarm next
git checkout 11663111cd49b4c6dd27479774e420f139e4c447
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot 

All warnings (new ones prefixed by >>):

>> arch/arm64/kvm/sys_regs.c:1544:13: warning: initializer overrides prior 
>> initialization of this subobject [-Winitializer-overrides]
 .reset = reset_pmcr, .reg = PMCR_EL0 },
  ^~
   arch/arm64/kvm/sys_regs.c:1543:4: note: previous initialization is here
   { PMU_SYS_REG(SYS_PMCR_EL0), .access = access_pmcr,
 ^
   arch/arm64/kvm/sys_regs.c:949:24: note: expanded from macro 'PMU_SYS_REG'
   SYS_DESC(r), .reset = reset_unknown, .visibility = pmu_visibility
 ^
   arch/arm64/kvm/sys_regs.c:1556:38: warning: initializer overrides prior 
initialization of this subobject [-Winitializer-overrides]
 .access = access_pmceid, .reset = NULL },
   ^~~~
   include/linux/stddef.h:8:14: note: expanded from macro 'NULL'
   #define NULL ((void *)0)
^~~
   arch/arm64/kvm/sys_regs.c:1555:4: note: previous initialization is here
   { PMU_SYS_REG(SYS_PMCEID0_EL0),
 ^~~~
   arch/arm64/kvm/sys_regs.c:949:24: note: expanded from macro 'PMU_SYS_REG'
   SYS_DESC(r), .reset = reset_unknown, .visibility = pmu_visibility
 ^
   arch/arm64/kvm/sys_regs.c:1558:38: warning: initializer overrides prior 
initialization of this subobject [-Winitializer-overrides]
 .access = access_pmceid, .reset = NULL },
   ^~~~
   include/linux/stddef.h:8:14: note: expanded from macro 'NULL'
   #define NULL ((void *)0)
^~~
   arch/arm64/kvm/sys_regs.c:1557:4: note: previous initialization is here
   { PMU_SYS_REG(SYS_PMCEID1_EL0),
 ^~~~
   arch/arm64/kvm/sys_regs.c:949:24: note: expanded from macro 'PMU_SYS_REG'
   SYS_DESC(r), .reset = reset_unknown, .visibility = pmu_visibility
 ^
   arch/arm64/kvm/sys_regs.c:1562:43: warning: initializer overrides prior 
initialization of this subobject [-Winitializer-overrides]
 .access = access_pmu_evtyper, .reset = NULL },
^~~~
   include/linux/stddef.h:8:14: note: expanded from macro 'NULL'
   #define NULL ((void *)0)
^~~
   arch/arm64/kvm/sys_regs.c:1561:4: note: previous initialization is here
   { PMU_SYS_REG(SYS_PMXEVTYPER_EL0),
 ^~~
   arch/arm64/kvm/sys_regs.c:949:24: note: expanded from macro 'PMU_SYS_REG'
   SYS_DESC(r), .reset = reset_unknown, .visibility = pmu_visibility
 ^
   arch/arm64/kvm/sys_regs.c:1564:42: warning: initializer overrides prior 
initialization of this subobject [-Winitializer-overrides]
 .access = access_pmu_evcntr, .reset = NULL },
   ^~~~
   include/linux/stddef.h:8:14: note: expanded from macro 'NULL'
   #define NULL ((void *)0)
^~~
   arch/arm64/kvm/sys_regs.c:1563:4: note: previous initialization is here
   { PMU_SYS_REG(SYS_PMXEVCNTR_EL0),
 ^~
   arch/arm64/kvm/sys_regs.c:949:24: note: expanded from macro 'PMU_SYS_REG'
   SYS_DESC(r), .reset = reset_unknown, .visibility = pmu_visibility
 ^
   arch/arm64/kvm/sys_regs.c:1570:13: warning: initializer overrides prior 
initialization of this subobject [-Winitializer-overrides]
 

Re: [PATCH 1/5] vfio/iommu_type1: Fixes vfio_dma_populate_bitmap to avoid dirty lose

[Keqian Zhu]

Hi Alex,

On 2021/1/13 5:20, Alex Williamson wrote:
> On Thu, 7 Jan 2021 17:28:57 +0800
> Keqian Zhu  wrote:
> 
>> Defer checking whether vfio_dma is of fully-dirty in update_user_bitmap
>> is easy to lose dirty log. For example, after promoting pinned_scope of
>> vfio_iommu, vfio_dma is not considered as fully-dirty, then we may lose
>> dirty log that occurs before vfio_iommu is promoted.
>>
>> The key point is that pinned-dirty is not a real dirty tracking way, it
>> can't continuously track dirty pages, but just restrict dirty scope. It
>> is essentially the same as fully-dirty. Fully-dirty is of full-scope and
>> pinned-dirty is of pinned-scope.
>>
>> So we must mark pinned-dirty or fully-dirty after we start dirty tracking
>> or clear dirty bitmap, to ensure that dirty log is marked right away.
> 
> I was initially convinced by these first three patches, but upon
> further review, I think the premise is wrong.  AIUI, the concern across
> these patches is that our dirty bitmap is only populated with pages
> dirtied by pinning and we only take into account the pinned page dirty
> scope at the time the bitmap is retrieved by the user.  You suppose
> this presents a gap where if a vendor driver has not yet identified
> with a page pinning scope that the entire bitmap should be considered
> dirty regardless of whether that driver later pins pages prior to the
> user retrieving the dirty bitmap.
Yes, this is my concern. And there are some other scenarios.

1. If a non-pinned iommu-backed domain is detached between starting dirty log 
and retrieving
dirty bitmap, then the dirty log is missed (As you said in the last paragraph).

2. If all vendor drivers pinned (means iommu pinned_scope is true), and an 
vendor driver do pin/unpin
pair between starting dirty log and retrieving dirty bitmap, then the dirty log 
of these unpinned pages
are missed.

> 
> I don't think this is how we intended the cooperation between the iommu
> driver and vendor driver to work.  By pinning pages a vendor driver is
> not declaring that only their future dirty page scope is limited to
> pinned pages, instead they're declaring themselves as a participant in
> dirty page tracking and take responsibility for pinning any necessary
> pages.  For example we might extend VFIO_IOMMU_DIRTY_PAGES_FLAG_START
> to trigger a blocking notification to groups to not only begin dirty
> tracking, but also to synchronously register their current device DMA
> footprint.  This patch would require a vendor driver to possibly perform
> a gratuitous page pinning in order to set the scope prior to dirty
> logging being enabled, or else the initial bitmap will be fully dirty.
I get what you mean ;-). You said that there is time gap between we attach a 
device
and this device declares itself is dirty traceable.

However, this makes it difficult to management dirty log tracking (I list two 
bugs).
If the vfio devices can declare themselves dirty traceable when attach to 
container,
then the logic of dirty tracking is much more clear ;-) .

> 
> Therefore, I don't see that this series is necessary or correct.  Kirti,
> does this match your thinking?
> 
> Thinking about these semantics, it seems there might still be an issue
> if a group with non-pinned-page dirty scope is detached with dirty
> logging enabled.  It seems this should in fact fully populate the dirty
> bitmaps at the time it's removed since we don't know the extent of its
> previous DMA, nor will the group be present to trigger the full bitmap
> when the user retrieves the dirty bitmap.  Creating fully populated
> bitmaps at the time tracking is enabled negates our ability to take
> advantage of later enlightenment though.  Thanks,
> 
Since that you want to allow the time gap between we attach device and the 
device
declare itself dirty traceable, I am going to fix these two bugs in patch v2. 
Do you
agree?

Thanks,
Keqian

> Alex
> 
>> Fixes: d6a4c185660c ("vfio iommu: Implementation of ioctl for dirty pages 
>> tracking")
>> Signed-off-by: Keqian Zhu 
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 33 ++---
>>  1 file changed, 22 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c 
>> b/drivers/vfio/vfio_iommu_type1.c
>> index bceda5e8baaa..b0a26e8e0adf 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -224,7 +224,7 @@ static void vfio_dma_bitmap_free(struct vfio_dma *dma)
>>  dma->bitmap = NULL;
>>  }
>>  
>> -static void vfio_dma_populate_bitmap(struct vfio_dma *dma, size_t pgsize)
>> +static void vfio_dma_populate_bitmap_pinned(struct vfio_dma *dma, size_t 
>> pgsize)
>>  {
>>  struct rb_node *p;
>>  unsigned long pgshift = __ffs(pgsize);
>> @@ -236,6 +236,25 @@ static void vfio_dma_populate_bitmap(struct vfio_dma 
>> *dma, size_t pgsize)
>>  }
>>  }
>>  
>> +static void vfio_dma_populate_bitmap_full(struct vfio_dma *dma, size_t 
>> pgsize)
>> +{
>> +unsigned long pgshift 

[PATCH v3 1/3] KVM: arm64: Adjust partial code of hyp stage-1 map and guest stage-2 map

[Yanan Wang]

Procedures of hyp stage-1 map and guest stage-2 map are quite different,
but they are tied closely by function kvm_set_valid_leaf_pte().
So adjust the relative code for ease of code maintenance in the future.

Signed-off-by: Will Deacon 
Signed-off-by: Yanan Wang 
---
 arch/arm64/kvm/hyp/pgtable.c | 55 ++--
 1 file changed, 28 insertions(+), 27 deletions(-)

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index bdf8e55ed308..a11ac874bc2a 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -170,10 +170,9 @@ static void kvm_set_table_pte(kvm_pte_t *ptep, kvm_pte_t 
*childp)
smp_store_release(ptep, pte);
 }
 
-static bool kvm_set_valid_leaf_pte(kvm_pte_t *ptep, u64 pa, kvm_pte_t attr,
-  u32 level)
+static kvm_pte_t kvm_init_valid_leaf_pte(u64 pa, kvm_pte_t attr, u32 level)
 {
-   kvm_pte_t old = *ptep, pte = kvm_phys_to_pte(pa);
+   kvm_pte_t pte = kvm_phys_to_pte(pa);
u64 type = (level == KVM_PGTABLE_MAX_LEVELS - 1) ? KVM_PTE_TYPE_PAGE :
   KVM_PTE_TYPE_BLOCK;
 
@@ -181,12 +180,7 @@ static bool kvm_set_valid_leaf_pte(kvm_pte_t *ptep, u64 
pa, kvm_pte_t attr,
pte |= FIELD_PREP(KVM_PTE_TYPE, type);
pte |= KVM_PTE_VALID;
 
-   /* Tolerate KVM recreating the exact same mapping. */
-   if (kvm_pte_valid(old))
-   return old == pte;
-
-   smp_store_release(ptep, pte);
-   return true;
+   return pte;
 }
 
 static int kvm_pgtable_visitor_cb(struct kvm_pgtable_walk_data *data, u64 addr,
@@ -341,12 +335,17 @@ static int hyp_map_set_prot_attr(enum kvm_pgtable_prot 
prot,
 static bool hyp_map_walker_try_leaf(u64 addr, u64 end, u32 level,
kvm_pte_t *ptep, struct hyp_map_data *data)
 {
+   kvm_pte_t new, old = *ptep;
u64 granule = kvm_granule_size(level), phys = data->phys;
 
if (!kvm_block_mapping_supported(addr, end, phys, level))
return false;
 
-   WARN_ON(!kvm_set_valid_leaf_pte(ptep, phys, data->attr, level));
+   /* Tolerate KVM recreating the exact same mapping */
+   new = kvm_init_valid_leaf_pte(phys, data->attr, level);
+   if (old != new && !WARN_ON(kvm_pte_valid(old)))
+   smp_store_release(ptep, new);
+
data->phys += granule;
return true;
 }
@@ -465,27 +464,30 @@ static bool stage2_map_walker_try_leaf(u64 addr, u64 end, 
u32 level,
   kvm_pte_t *ptep,
   struct stage2_map_data *data)
 {
+   kvm_pte_t new, old = *ptep;
u64 granule = kvm_granule_size(level), phys = data->phys;
+   struct page *page = virt_to_page(ptep);
 
if (!kvm_block_mapping_supported(addr, end, phys, level))
return false;
 
-   /*
-* If the PTE was already valid, drop the refcount on the table
-* early, as it will be bumped-up again in stage2_map_walk_leaf().
-* This ensures that the refcount stays constant across a valid to
-* valid PTE update.
-*/
-   if (kvm_pte_valid(*ptep))
-   put_page(virt_to_page(ptep));
+   new = kvm_init_valid_leaf_pte(phys, data->attr, level);
+   if (kvm_pte_valid(old)) {
+   /* Tolerate KVM recreating the exact same mapping */
+   if (old == new)
+   goto out;
 
-   if (kvm_set_valid_leaf_pte(ptep, phys, data->attr, level))
-   goto out;
+   /*
+* There's an existing different valid leaf entry, so perform
+* break-before-make.
+*/
+   kvm_set_invalid_pte(ptep);
+   kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, data->mmu, addr, level);
+   put_page(page);
+   }
 
-   /* There's an existing valid leaf entry, so perform break-before-make */
-   kvm_set_invalid_pte(ptep);
-   kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, data->mmu, addr, level);
-   kvm_set_valid_leaf_pte(ptep, phys, data->attr, level);
+   smp_store_release(ptep, new);
+   get_page(page);
 out:
data->phys += granule;
return true;
@@ -527,7 +529,7 @@ static int stage2_map_walk_leaf(u64 addr, u64 end, u32 
level, kvm_pte_t *ptep,
}
 
if (stage2_map_walker_try_leaf(addr, end, level, ptep, data))
-   goto out_get_page;
+   return 0;
 
if (WARN_ON(level == KVM_PGTABLE_MAX_LEVELS - 1))
return -EINVAL;
@@ -551,9 +553,8 @@ static int stage2_map_walk_leaf(u64 addr, u64 end, u32 
level, kvm_pte_t *ptep,
}
 
kvm_set_table_pte(ptep, childp);
-
-out_get_page:
get_page(page);
+
return 0;
 }
 
-- 
2.19.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu

[PATCH v3 2/3] KVM: arm64: Filter out the case of only changing permissions from stage-2 map path

[Yanan Wang]

(1) During running time of a a VM with numbers of vCPUs, if some vCPUs
access the same GPA almost at the same time and the stage-2 mapping of
the GPA has not been built yet, as a result they will all cause
translation faults. The first vCPU builds the mapping, and the followed
ones end up updating the valid leaf PTE. Note that these vCPUs might
want different access permissions (RO, RW, RX, RWX, etc.).

(2) It's inevitable that we sometimes will update an existing valid leaf
PTE in the map path, and we perform break-before-make in this case.
Then more unnecessary translation faults could be caused if the
*break stage* of BBM is just catched by other vCPUS.

With (1) and (2), something unsatisfactory could happen: vCPU A causes
a translation fault and builds the mapping with RW permissions, vCPU B
then update the valid leaf PTE with break-before-make and permissions
are updated back to RO. Besides, *break stage* of BBM may trigger more
translation faults. Finally, some useless small loops could occur.

We can make some optimization to solve above problems: When we need to
update a valid leaf PTE in the map path, let's filter out the case where
this update only change access permissions, and don't update the valid
leaf PTE here in this case. Instead, let the vCPU enter back the guest
and it will exit next time to go through the relax_perms path without
break-before-make if it still wants more permissions.

Signed-off-by: Yanan Wang 
---
 arch/arm64/include/asm/kvm_pgtable.h |  5 +
 arch/arm64/kvm/hyp/pgtable.c | 32 ++--
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_pgtable.h 
b/arch/arm64/include/asm/kvm_pgtable.h
index 52ab38db04c7..8886d43cfb11 100644
--- a/arch/arm64/include/asm/kvm_pgtable.h
+++ b/arch/arm64/include/asm/kvm_pgtable.h
@@ -157,6 +157,11 @@ void kvm_pgtable_stage2_destroy(struct kvm_pgtable *pgt);
  * If device attributes are not explicitly requested in @prot, then the
  * mapping will be normal, cacheable.
  *
+ * Note that the update of a valid leaf PTE in this function will be aborted,
+ * if it's trying to recreate the exact same mapping or only change the access
+ * permissions. Instead, the vCPU will exit one more time from guest if still
+ * needed and then go through the path of relaxing permissions.
+ *
  * Note that this function will both coalesce existing table entries and split
  * existing block mappings, relying on page-faults to fault back areas outside
  * of the new mapping lazily.
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index a11ac874bc2a..4d177ce1d536 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -45,6 +45,10 @@
 
 #define KVM_PTE_LEAF_ATTR_HI_S2_XN BIT(54)
 
+#define KVM_PTE_LEAF_ATTR_S2_PERMS (KVM_PTE_LEAF_ATTR_LO_S2_S2AP_R | \
+KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W | \
+KVM_PTE_LEAF_ATTR_HI_S2_XN)
+
 struct kvm_pgtable_walk_data {
struct kvm_pgtable  *pgt;
struct kvm_pgtable_walker   *walker;
@@ -460,22 +464,27 @@ static int stage2_map_set_prot_attr(enum kvm_pgtable_prot 
prot,
return 0;
 }
 
-static bool stage2_map_walker_try_leaf(u64 addr, u64 end, u32 level,
-  kvm_pte_t *ptep,
-  struct stage2_map_data *data)
+static int stage2_map_walker_try_leaf(u64 addr, u64 end, u32 level,
+ kvm_pte_t *ptep,
+ struct stage2_map_data *data)
 {
kvm_pte_t new, old = *ptep;
u64 granule = kvm_granule_size(level), phys = data->phys;
struct page *page = virt_to_page(ptep);
 
if (!kvm_block_mapping_supported(addr, end, phys, level))
-   return false;
+   return -E2BIG;
 
new = kvm_init_valid_leaf_pte(phys, data->attr, level);
if (kvm_pte_valid(old)) {
-   /* Tolerate KVM recreating the exact same mapping */
-   if (old == new)
-   goto out;
+   /*
+* Skip updating the PTE if we are trying to recreate the exact
+* same mapping or only change the access permissions. Instead,
+* the vCPU will exit one more time from guest if still needed
+* and then go through the path of relaxing permissions.
+*/
+   if (!((old ^ new) & (~KVM_PTE_LEAF_ATTR_S2_PERMS)))
+   return -EAGAIN;
 
/*
 * There's an existing different valid leaf entry, so perform
@@ -488,9 +497,8 @@ static bool stage2_map_walker_try_leaf(u64 addr, u64 end, 
u32 level,
 
smp_store_release(ptep, new);
get_page(page);
-out:
data->phys += granule;
-   return true;
+   return 0;
 }
 
 static int stage2_map_walk_table_pre(u64 

[PATCH v3 0/3] Some optimization for stage-2 translation

[Yanan Wang]

Hi,
This patch series(v3) make some optimization for stage-2 translation.

About patch-1:
Procedures of hyp stage-1 map and guest stage-2 map are quite different,
but they are now tied closely by function kvm_set_valid_leaf_pte().
So adjust the relative code for ease of code maintenance in the future.

About patch-2:
There have been the separate map handler and perms handler used independently
for mapping and relaxing permissions in the new page-table infrastructure for
stage-2, yet there is still a specific case where we end up changing the access
permissions in the map path, and something unsatisfactory could happen because
of current handling for this case.

To solve above problem, we can filter out this case from the map path and abort
the PTE update. Instead, let the vCPU enter back the guest and it will exit next
time to go through the relax_perms path if still needed.

About patch-3:
We now set the pfn dirty and mark the page dirty before calling fault
handlers in user_mem_abort(), so we might end up having spurious dirty
pages if update of permissions or mapping has failed. Let's move these
two operations after the fault handlers, and they will be done only if
the fault has been handled successfully.

When an -EAGAIN errno is returned from the map handler, we hope to the
vcpu to enter guest directly instead of exiting back to userspace, so
adjust the return value at the end of function.

---

Changelogs

v2->v3:
- Rebased on top of v5.11-rc3
- Refine the commit messages
- Make some adjustment about return value in patch-2 and patch-3
- v2: 
https://lore.kernel.org/lkml/20201216122844.25092-1-wangyana...@huawei.com/

v1->v2:
- Make part of the diff a seperate patch (patch-1)
- Add Will's Signed-off-by for patch-1
- Return an errno when meeting changing permissions case in map path
- Add a new patch (patch-3)
- v1: 
https://lore.kernel.org/lkml/20201211080115.21460-1-wangyana...@huawei.com/

---

Yanan Wang (3):
  KVM: arm64: Adjust partial code of hyp stage-1 map and guest stage-2
map
  KVM: arm64: Filter out the case of only changing permissions from
stage-2 map path
  KVM: arm64: Mark the page dirty only if the fault is handled
successfully

 arch/arm64/include/asm/kvm_pgtable.h |  5 ++
 arch/arm64/kvm/hyp/pgtable.c | 83 
 arch/arm64/kvm/mmu.c | 13 +++--
 3 files changed, 60 insertions(+), 41 deletions(-)

-- 
2.19.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 3/3] KVM: arm64: Mark the page dirty only if the fault is handled successfully

[Yanan Wang]

We now set the pfn dirty and mark the page dirty before calling fault
handlers in user_mem_abort(), so we might end up having spurious dirty
pages if update of permissions or mapping has failed. Let's move these
two operations after the fault handlers, and they will be done only if
the fault has been handled successfully.

When an -EAGAIN errno is returned from the map handler, we hope to the
vcpu to enter guest directly instead of exiting back to userspace, so
adjust the return value at the end of function.

Signed-off-by: Yanan Wang 
---
 arch/arm64/kvm/mmu.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 7d2257cc5438..77cb2d28f2a4 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -879,11 +879,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, 
phys_addr_t fault_ipa,
if (vma_pagesize == PAGE_SIZE && !force_pte)
vma_pagesize = transparent_hugepage_adjust(memslot, hva,
   , _ipa);
-   if (writable) {
+   if (writable)
prot |= KVM_PGTABLE_PROT_W;
-   kvm_set_pfn_dirty(pfn);
-   mark_page_dirty(kvm, gfn);
-   }
 
if (fault_status != FSC_PERM && !device)
clean_dcache_guest_page(pfn, vma_pagesize);
@@ -911,11 +908,17 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, 
phys_addr_t fault_ipa,
 memcache);
}
 
+   /* Mark the page dirty only if the fault is handled successfully */
+   if (writable && !ret) {
+   kvm_set_pfn_dirty(pfn);
+   mark_page_dirty(kvm, gfn);
+   }
+
 out_unlock:
spin_unlock(>mmu_lock);
kvm_set_pfn_accessed(pfn);
kvm_release_pfn_clean(pfn);
-   return ret;
+   return ret != -EAGAIN ? ret : 0;
 }
 
 /* Resolve the access fault by making the page young again. */
-- 
2.19.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 4/5] vfio/iommu_type1: Carefully use unmap_unpin_all during dirty tracking

[Keqian Zhu]

On 2021/1/13 3:53, Alex Williamson wrote:
> On Tue, 12 Jan 2021 20:04:38 +0800
> Keqian Zhu  wrote:
> 
>> On 2021/1/12 5:49, Alex Williamson wrote:
>>> On Thu, 7 Jan 2021 17:29:00 +0800
>>> Keqian Zhu  wrote:
>>>   
 If we detach group during dirty page tracking, we shouldn't remove
 vfio_dma, because dirty log will lose.

 But we don't prevent unmap_unpin_all in vfio_iommu_release, because
 under normal procedure, dirty tracking has been stopped.  
>>>
>>> This looks like it's creating a larger problem than it's fixing, it's
>>> not our job to maintain the dirty bitmap regardless of what the user
>>> does.  If the user detaches the last group in a container causing the
>>> mappings within that container to be deconstructed before the user has
>>> collected dirty pages, that sounds like a user error.  A container with
>>> no groups is de-privileged and therefore loses all state.  Thanks,
>>>
>>> Alex  
>>
>> Hi Alex,
>>
>> This looks good to me ;-). That's a reasonable constraint for user behavior.
>>
>> What about replacing this patch with an addition to the uapi document of
>> VFIO_GROUP_UNSET_CONTAINER? User should pay attention to this when call this
>> ioctl during dirty tracking.
> 
> Here's the current uapi comment:
> 
> /**
>  * VFIO_GROUP_UNSET_CONTAINER - _IO(VFIO_TYPE, VFIO_BASE + 5)
>  *
>  * Remove the group from the attached container.  This is the
>  * opposite of the SET_CONTAINER call and returns the group to
>  * an initial state.  All device file descriptors must be released
>  * prior to calling this interface.  When removing the last group
>  * from a container, the IOMMU will be disabled and all state lost,
>  * effectively also returning the VFIO file descriptor to an initial
>  * state.
>  * Return: 0 on success, -errno on failure.
>  * Availability: When attached to container
>  */
> 
> So we already indicate that "all state" of the container is lost when
> removing the last group, I don't see that it's necessarily to
> explicitly include dirty bitmap state beyond that statement.  Without
> mappings there can be no dirty bitmap to track.
OK :-) .

> 
>  > And any comments on other patches? thanks.
> 
> I had a difficult time mapping the commit log to the actual code
> change, I'll likely have some wording suggestions.  Is patch 5/5 still
> necessary if this patch is dropped?  Thanks,
> 
I think the 5th patch is still necessary. vfio_sanity_check_pfn_list() is used 
to check
whether pfn_list of vfio_dma is empty. but we apply this check just for 
external domain.
If the iommu backed domain also pin some pages, then this check fails. So I 
think we should
use this check only when all domains are about to be removed.

Besides, this patch should extract the "WARN_ON(iommu->notifier.head);" just 
for external domain.

Thanks,
Keqian

> Alex
> 
 Fixes: d6a4c185660c ("vfio iommu: Implementation of ioctl for dirty pages 
 tracking")
 Signed-off-by: Keqian Zhu 
 ---
  drivers/vfio/vfio_iommu_type1.c | 14 --
  1 file changed, 12 insertions(+), 2 deletions(-)

 diff --git a/drivers/vfio/vfio_iommu_type1.c 
 b/drivers/vfio/vfio_iommu_type1.c
 index 26b7eb2a5cfc..9776a059904d 100644
 --- a/drivers/vfio/vfio_iommu_type1.c
 +++ b/drivers/vfio/vfio_iommu_type1.c
 @@ -2373,7 +2373,12 @@ static void vfio_iommu_type1_detach_group(void 
 *iommu_data,
if (list_empty(>external_domain->group_list)) {
vfio_sanity_check_pfn_list(iommu);
  
 -  if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu))
 +  /*
 +   * During dirty page tracking, we can't remove
 +   * vfio_dma because dirty log will lose.
 +   */
 +  if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu) &&
 +  !iommu->dirty_page_tracking)
vfio_iommu_unmap_unpin_all(iommu);
  
kfree(iommu->external_domain);
 @@ -2406,10 +2411,15 @@ static void vfio_iommu_type1_detach_group(void 
 *iommu_data,
 * iommu and external domain doesn't exist, then all the
 * mappings go away too. If it's the last domain with iommu and
 * external domain exist, update accounting
 +   *
 +   * Note: During dirty page tracking, we can't remove vfio_dma
 +   * because dirty log will lose. Just update accounting is a good
 +   * choice.
 */
if (list_empty(>group_list)) {
if (list_is_singular(>domain_list)) {
 -  if (!iommu->external_domain)
 +  if (!iommu->external_domain &&
 +  !iommu->dirty_page_tracking)

[PATCH 4/6] KVM: arm64: Refactor filtering of ID registers

[Marc Zyngier]

Our current ID register filtering is starting to be a mess of if()
statements, and isn't going to get any saner.

Let's turn it into a switch(), which has a chance of being more
readable, and introduce a FEATURE() macro that allows easy generation
of feature masks.

No functionnal change intended.

Signed-off-by: Marc Zyngier 
---
 arch/arm64/kvm/sys_regs.c | 51 +--
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 2bea0494b81d..dda16d60197b 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -9,6 +9,7 @@
  *  Christoffer Dall 
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -1016,6 +1017,8 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
return true;
 }
 
+#define FEATURE(x) (GENMASK_ULL(x##_SHIFT + 3, x##_SHIFT))
+
 /* Read a sanitised cpufeature ID register by sys_reg_desc */
 static u64 read_id_reg(const struct kvm_vcpu *vcpu,
struct sys_reg_desc const *r, bool raz)
@@ -1024,36 +1027,38 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
 
-   if (id == SYS_ID_AA64PFR0_EL1) {
+   switch (id) {
+   case SYS_ID_AA64PFR0_EL1:
if (!vcpu_has_sve(vcpu))
-   val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
-   val &= ~(0xfUL << ID_AA64PFR0_AMU_SHIFT);
-   val &= ~(0xfUL << ID_AA64PFR0_CSV2_SHIFT);
-   val |= ((u64)vcpu->kvm->arch.pfr0_csv2 << 
ID_AA64PFR0_CSV2_SHIFT);
-   val &= ~(0xfUL << ID_AA64PFR0_CSV3_SHIFT);
-   val |= ((u64)vcpu->kvm->arch.pfr0_csv3 << 
ID_AA64PFR0_CSV3_SHIFT);
-   } else if (id == SYS_ID_AA64PFR1_EL1) {
-   val &= ~(0xfUL << ID_AA64PFR1_MTE_SHIFT);
-   } else if (id == SYS_ID_AA64ISAR1_EL1 && !vcpu_has_ptrauth(vcpu)) {
-   val &= ~((0xfUL << ID_AA64ISAR1_APA_SHIFT) |
-(0xfUL << ID_AA64ISAR1_API_SHIFT) |
-(0xfUL << ID_AA64ISAR1_GPA_SHIFT) |
-(0xfUL << ID_AA64ISAR1_GPI_SHIFT));
-   } else if (id == SYS_ID_AA64DFR0_EL1) {
-   u64 cap = 0;
-
+   val &= ~FEATURE(ID_AA64PFR0_SVE);
+   val &= ~FEATURE(ID_AA64PFR0_AMU);
+   val &= ~FEATURE(ID_AA64PFR0_CSV2);
+   val |= FIELD_PREP(FEATURE(ID_AA64PFR0_CSV2), 
(u64)vcpu->kvm->arch.pfr0_csv2);
+   val &= ~FEATURE(ID_AA64PFR0_CSV3);
+   val |= FIELD_PREP(FEATURE(ID_AA64PFR0_CSV3), 
(u64)vcpu->kvm->arch.pfr0_csv3);
+   break;
+   case SYS_ID_AA64PFR1_EL1:
+   val &= ~FEATURE(ID_AA64PFR1_MTE);
+   break;
+   case SYS_ID_AA64ISAR1_EL1:
+   if (!vcpu_has_ptrauth(vcpu))
+   val &= ~(FEATURE(ID_AA64ISAR1_APA) |
+FEATURE(ID_AA64ISAR1_API) |
+FEATURE(ID_AA64ISAR1_GPA) |
+FEATURE(ID_AA64ISAR1_GPI));
+   break;
+   case SYS_ID_AA64DFR0_EL1:
/* Limit guests to PMUv3 for ARMv8.1 */
-   if (kvm_vcpu_has_pmu(vcpu))
-   cap = ID_AA64DFR0_PMUVER_8_1;
-
val = cpuid_feature_cap_perfmon_field(val,
-   ID_AA64DFR0_PMUVER_SHIFT,
-   cap);
-   } else if (id == SYS_ID_DFR0_EL1) {
+ ID_AA64DFR0_PMUVER_SHIFT,
+ kvm_vcpu_has_pmu(vcpu) ? 
ID_AA64DFR0_PMUVER_8_1 : 0);
+   break;
+   case SYS_ID_DFR0_EL1:
/* Limit guests to PMUv3 for ARMv8.1 */
val = cpuid_feature_cap_perfmon_field(val,
  ID_DFR0_PERFMON_SHIFT,
  kvm_vcpu_has_pmu(vcpu) ? 
ID_DFR0_PERFMON_8_1 : 0);
+   break;
}
 
return val;
-- 
2.29.2

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 5/6] KVM: arm64: Limit the debug architecture to ARMv8.0

[Marc Zyngier]

Let's not pretend we support anything but ARMv8.0 as far as the
debug architecture is concerned.

Signed-off-by: Marc Zyngier 
---
 arch/arm64/kvm/sys_regs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index dda16d60197b..8f79ec1fffa7 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1048,6 +1048,9 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 FEATURE(ID_AA64ISAR1_GPI));
break;
case SYS_ID_AA64DFR0_EL1:
+   /* Limit debug to ARMv8.0 */
+   val &= ~FEATURE(ID_AA64DFR0_DEBUGVER);
+   val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6);
/* Limit guests to PMUv3 for ARMv8.1 */
val = cpuid_feature_cap_perfmon_field(val,
  ID_AA64DFR0_PMUVER_SHIFT,
-- 
2.29.2

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 6/6] KVM: arm64: Upgrade PMU support to ARMv8.4

[Marc Zyngier]

Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be
pretty easy. All that is required is support for PMMIR_EL1, which
is read-only, and for which returning 0 is a valid option.

Let's just do that and adjust what we return to the guest.

Signed-off-by: Marc Zyngier 
---
 arch/arm64/kvm/sys_regs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 8f79ec1fffa7..2f4ecbd2abfb 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1051,10 +1051,10 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
/* Limit debug to ARMv8.0 */
val &= ~FEATURE(ID_AA64DFR0_DEBUGVER);
val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6);
-   /* Limit guests to PMUv3 for ARMv8.1 */
+   /* Limit guests to PMUv3 for ARMv8.4 */
val = cpuid_feature_cap_perfmon_field(val,
  ID_AA64DFR0_PMUVER_SHIFT,
- kvm_vcpu_has_pmu(vcpu) ? 
ID_AA64DFR0_PMUVER_8_1 : 0);
+ kvm_vcpu_has_pmu(vcpu) ? 
ID_AA64DFR0_PMUVER_8_4 : 0);
break;
case SYS_ID_DFR0_EL1:
/* Limit guests to PMUv3 for ARMv8.1 */
@@ -1496,6 +1496,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
{ SYS_DESC(SYS_PMINTENSET_EL1), access_pminten, reset_unknown, 
PMINTENSET_EL1 },
{ SYS_DESC(SYS_PMINTENCLR_EL1), access_pminten, reset_unknown, 
PMINTENSET_EL1 },
+   { SYS_DESC(SYS_PMMIR_EL1), trap_raz_wi },
 
{ SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 },
{ SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 },
-- 
2.29.2

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 2/6] KVM: arm64: Fix AArch32 PMUv3 capping

[Marc Zyngier]

We shouldn't expose *any* PMU capability when no PMU has been
configured for this VM.

Signed-off-by: Marc Zyngier 
---
 arch/arm64/kvm/sys_regs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 0c0832472c4a..ce08d28ab15c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1048,8 +1048,8 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
} else if (id == SYS_ID_DFR0_EL1) {
/* Limit guests to PMUv3 for ARMv8.1 */
val = cpuid_feature_cap_perfmon_field(val,
-   ID_DFR0_PERFMON_SHIFT,
-   ID_DFR0_PERFMON_8_1);
+ ID_DFR0_PERFMON_SHIFT,
+ kvm_vcpu_has_pmu(vcpu) ? 
ID_DFR0_PERFMON_8_1 : 0);
}
 
return val;
-- 
2.29.2

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 3/6] KVM: arm64: Add handling of AArch32 PCMEID{2, 3} PMUv3 registers

[Marc Zyngier]

Despite advertising support for AArch32 PMUv3p1, we fail to handle
the PMCEID{2,3} registers, which conveniently alias with with the top
bits of PMCEID{0,1}_EL1.

Implement these registers with the usual AA32(HI/LO) aliasing
mechanism.

Signed-off-by: Marc Zyngier 
---
 arch/arm64/kvm/sys_regs.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index ce08d28ab15c..2bea0494b81d 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -685,14 +685,18 @@ static bool access_pmselr(struct kvm_vcpu *vcpu, struct 
sys_reg_params *p,
 static bool access_pmceid(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
  const struct sys_reg_desc *r)
 {
-   u64 pmceid;
+   u64 pmceid, mask, shift;
 
BUG_ON(p->is_write);
 
if (pmu_access_el0_disabled(vcpu))
return false;
 
+   get_access_mask(r, , );
+
pmceid = kvm_pmu_get_pmceid(vcpu, (p->Op2 & 1));
+   pmceid &= mask;
+   pmceid >>= shift;
 
p->regval = pmceid;
 
@@ -1895,8 +1899,8 @@ static const struct sys_reg_desc cp15_regs[] = {
{ Op1( 0), CRn( 9), CRm(12), Op2( 3), access_pmovs },
{ Op1( 0), CRn( 9), CRm(12), Op2( 4), access_pmswinc },
{ Op1( 0), CRn( 9), CRm(12), Op2( 5), access_pmselr },
-   { Op1( 0), CRn( 9), CRm(12), Op2( 6), access_pmceid },
-   { Op1( 0), CRn( 9), CRm(12), Op2( 7), access_pmceid },
+   { AA32(LO), Op1( 0), CRn( 9), CRm(12), Op2( 6), access_pmceid },
+   { AA32(LO), Op1( 0), CRn( 9), CRm(12), Op2( 7), access_pmceid },
{ Op1( 0), CRn( 9), CRm(13), Op2( 0), access_pmu_evcntr },
{ Op1( 0), CRn( 9), CRm(13), Op2( 1), access_pmu_evtyper },
{ Op1( 0), CRn( 9), CRm(13), Op2( 2), access_pmu_evcntr },
@@ -1904,6 +1908,8 @@ static const struct sys_reg_desc cp15_regs[] = {
{ Op1( 0), CRn( 9), CRm(14), Op2( 1), access_pminten },
{ Op1( 0), CRn( 9), CRm(14), Op2( 2), access_pminten },
{ Op1( 0), CRn( 9), CRm(14), Op2( 3), access_pmovs },
+   { AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 4), access_pmceid },
+   { AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 5), access_pmceid },
 
/* PRRR/MAIR0 */
{ AA32(LO), Op1( 0), CRn(10), CRm( 2), Op2( 0), access_vm_reg, NULL, 
MAIR_EL1 },
-- 
2.29.2

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 0/6] KVM: arm64: More PMU/debug ID register fixes

[Marc Zyngier]

This is a respin of a series I posted in the 5.7 time frame, and
completely forgot about it until I noticed again that a few things
where not what I remembered... Given how long it has been, I'm
pretending this is v1 again.

Anyway, this covers a few gaps in our ID register handling for PMU and
Debug, plus the upgrade of the PMU support to 8.4 when possible.

I don't plan to take this into 5.11, but this is a likely candidate
for 5.12.

Marc Zyngier (6):
  KVM: arm64: Fix missing RES1 in emulation of DBGBIDR
  KVM: arm64: Fix AArch32 PMUv3 capping
  KVM: arm64: Add handling of AArch32 PCMEID{2,3} PMUv3 registers
  KVM: arm64: Refactor filtering of ID registers
  KVM: arm64: Limit the debug architecture to ARMv8.0
  KVM: arm64: Upgrade PMU support to ARMv8.4

 arch/arm64/kvm/sys_regs.c | 75 +++
 1 file changed, 45 insertions(+), 30 deletions(-)

-- 
2.29.2

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 1/6] KVM: arm64: Fix missing RES1 in emulation of DBGBIDR

[Marc Zyngier]

The AArch32 CP14 DBGDIDR has bit 15 set to RES1, which our current
emulation doesn't set. Just add the missing bit.

Reported-by: Peter Maydell 
Signed-off-by: Marc Zyngier 
---
 arch/arm64/kvm/sys_regs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 3313dedfa505..0c0832472c4a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1711,7 +1711,7 @@ static bool trap_dbgidr(struct kvm_vcpu *vcpu,
p->regval = dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) |
 (((dfr >> ID_AA64DFR0_BRPS_SHIFT) & 0xf) << 24) |
 (((dfr >> ID_AA64DFR0_CTX_CMPS_SHIFT) & 0xf) << 20)
-| (6 << 16) | (el3 << 14) | (el3 << 12));
+| (6 << 16) | (1 << 15) | (el3 << 14) | (el3 << 
12));
return true;
}
 }
-- 
2.29.2

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 9/9] KVM: selftests: aarch64/vgic-v3 init sequence tests

[Eric Auger]

The tests exercise the VGIC_V3 device creation including the
associated KVM_DEV_ARM_VGIC_GRP_ADDR group attributes:

- KVM_VGIC_V3_ADDR_TYPE_DIST/REDIST (vcpu_first and vgic_first)
- KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION (redist_regions).

Another test dedicates to KVM_DEV_ARM_VGIC_GRP_REDIST_REGS group
and especially the GICR_TYPER read. The goal was to test the case
recently fixed by commit 23bde34771f1
("KVM: arm64: vgic-v3: Drop the reporting of GICR_TYPER.Last for userspace").

The API under test can be found at
Documentation/virt/kvm/devices/arm-vgic-v3.rst

Signed-off-by: Eric Auger 
---
 tools/testing/selftests/kvm/Makefile  |   1 +
 .../testing/selftests/kvm/aarch64/vgic_init.c | 453 ++
 .../testing/selftests/kvm/include/kvm_util.h  |   5 +
 tools/testing/selftests/kvm/lib/kvm_util.c|  51 ++
 4 files changed, 510 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/vgic_init.c

diff --git a/tools/testing/selftests/kvm/Makefile 
b/tools/testing/selftests/kvm/Makefile
index fe41c6a0fa67..c3c2cb6fe8a1 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -68,6 +68,7 @@ TEST_GEN_PROGS_x86_64 += steal_time
 
 TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
 TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list-sve
+TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
 TEST_GEN_PROGS_aarch64 += demand_paging_test
 TEST_GEN_PROGS_aarch64 += dirty_log_test
 TEST_GEN_PROGS_aarch64 += dirty_log_perf_test
diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c 
b/tools/testing/selftests/kvm/aarch64/vgic_init.c
new file mode 100644
index ..e8caa64c0395
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
@@ -0,0 +1,453 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * vgic init sequence tests
+ *
+ * Copyright (C) 2020, Red Hat, Inc.
+ */
+#define _GNU_SOURCE
+#include 
+#include 
+#include 
+#include 
+
+#include "test_util.h"
+#include "kvm_util.h"
+#include "processor.h"
+
+#define NR_VCPUS   4
+
+#define REDIST_REGION_ATTR_ADDR(count, base, flags, index) (((uint64_t)(count) 
<< 52) | \
+   ((uint64_t)((base) >> 16) << 16) | ((uint64_t)(flags) << 12) | index)
+#define REG_OFFSET(vcpu, offset) (((uint64_t)vcpu << 32) | offset)
+
+#define GICR_TYPER 0x8
+
+static int access_redist_reg(int gicv3_fd, int vcpu, int offset,
+uint32_t *val, bool write)
+{
+   uint64_t attr = REG_OFFSET(vcpu, offset);
+
+   return kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_REDIST_REGS,
+attr, val, write);
+}
+
+static void guest_code(int cpu)
+{
+   GUEST_SYNC(0);
+   GUEST_SYNC(1);
+   GUEST_SYNC(2);
+   GUEST_DONE();
+}
+
+static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
+{
+   static int run;
+   struct ucall uc;
+   int ret;
+
+   vcpu_args_set(vm, vcpuid, 1, vcpuid);
+   ret = _vcpu_ioctl(vm, vcpuid, KVM_RUN, NULL);
+   get_ucall(vm, vcpuid, );
+   run++;
+
+   if (ret)
+   return -errno;
+   return 0;
+}
+
+int dist_rdist_tests(struct kvm_vm *vm)
+{
+   int ret, gicv3_fd, max_ipa_bits;
+   uint64_t addr;
+
+   max_ipa_bits = kvm_check_cap(KVM_CAP_ARM_VM_IPA_SIZE);
+
+   ret = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
+   if (ret) {
+   print_skip("GICv3 not supported");
+   exit(KSFT_SKIP);
+   }
+
+   ret = kvm_create_device(vm, 0, true);
+   TEST_ASSERT(ret == -ENODEV, "unsupported device");
+
+   /* Create the device */
+
+   gicv3_fd = kvm_create_device(vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
+   TEST_ASSERT(gicv3_fd > 0, "GICv3 device created");
+
+   /* Check attributes */
+
+   ret = kvm_device_check_attr(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+   KVM_VGIC_V3_ADDR_TYPE_DIST);
+   TEST_ASSERT(!ret, "KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_DIST 
supported");
+
+   ret = kvm_device_check_attr(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+   KVM_VGIC_V3_ADDR_TYPE_REDIST);
+   TEST_ASSERT(!ret, 
"KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_REDIST supported");
+
+   ret = kvm_device_check_attr(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, 0);
+   TEST_ASSERT(ret == -ENXIO, "attribute not supported");
+
+   /* misaligned DIST and REDIST addresses */
+
+   addr = 0x1000;
+   ret = kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+   KVM_VGIC_V3_ADDR_TYPE_DIST, , true);
+   TEST_ASSERT(ret == -EINVAL, "GICv3 dist base not 64kB aligned");
+
+   ret = kvm_device_access(gicv3_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+   KVM_VGIC_V3_ADDR_TYPE_REDIST, , true);
+   TEST_ASSERT(ret == -EINVAL, "GICv3 redist base not 64kB aligned");
+
+   /* out of range address */
+   if (max_ipa_bits) {
+   addr = 1ULL << max_ipa_bits;
+   

[PATCH v2 8/9] KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace

[Eric Auger]

Commit 23bde34771f1 ("KVM: arm64: vgic-v3: Drop the
reporting of GICR_TYPER.Last for userspace") temporarily fixed
a bug identified when attempting to access the GICR_TYPER
register before the redistributor region setting but dropped
the support of the LAST bit. Emulating the GICR_TYPER.Last bit
still makes sense for architecture compliance.

This patch restores its support (if the redistributor region
was set) while keeping the code safe.

Signed-off-by: Eric Auger 
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 7 ++-
 include/kvm/arm_vgic.h | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c 
b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 987e366c8000..7ff23c153128 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -277,6 +277,8 @@ static unsigned long vgic_uaccess_read_v3r_typer(struct 
kvm_vcpu *vcpu,
 gpa_t addr, unsigned int len)
 {
unsigned long mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
+   struct vgic_cpu *vgic_cpu = >arch.vgic_cpu;
+   struct vgic_redist_region *rdreg = vgic_cpu->rdreg;
int target_vcpu_id = vcpu->vcpu_id;
u64 value;
 
@@ -286,7 +288,9 @@ static unsigned long vgic_uaccess_read_v3r_typer(struct 
kvm_vcpu *vcpu,
if (vgic_has_its(vcpu->kvm))
value |= GICR_TYPER_PLPIS;
 
-   /* reporting of the Last bit is not supported for userspace */
+   if (rdreg && (vgic_cpu->rdreg_index == (rdreg->free_index - 1)))
+   value |= GICR_TYPER_LAST;
+
return extract_bytes(value, addr & 7, len);
 }
 
@@ -714,6 +718,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
return -EINVAL;
 
vgic_cpu->rdreg = rdreg;
+   vgic_cpu->rdreg_index = rdreg->free_index;
 
rd_base = rdreg->base + rdreg->free_index * KVM_VGIC_V3_REDIST_SIZE;
 
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 3d74f1060bd1..ec621180ef09 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -322,6 +322,7 @@ struct vgic_cpu {
 */
struct vgic_io_device   rd_iodev;
struct vgic_redist_region *rdreg;
+   u32 rdreg_index;
 
/* Contains the attributes and gpa of the LPI pending tables. */
u64 pendbaser;
-- 
2.21.3

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 7/9] KVM: arm64: Simplify argument passing to vgic_uaccess_[read|write]

[Eric Auger]

vgic_uaccess() takes a struct vgic_io_device argument, converts it
to a struct kvm_io_device and passes it to the read/write accessor
functions, which convert it back to a struct vgic_io_device.
Avoid the indirection by passing the struct vgic_io_device argument
directly to vgic_uaccess_{read,write}.

Signed-off-by: Eric Auger 

---

v1 -> v2:
- reworded the commit message as suggested by Alexandru
---
 arch/arm64/kvm/vgic/vgic-mmio.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
index b2d73fc0d1ef..48c6067fc5ec 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio.c
@@ -938,10 +938,9 @@ vgic_get_mmio_region(struct kvm_vcpu *vcpu, struct 
vgic_io_device *iodev,
return region;
 }
 
-static int vgic_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
+static int vgic_uaccess_read(struct kvm_vcpu *vcpu, struct vgic_io_device 
*iodev,
 gpa_t addr, u32 *val)
 {
-   struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
const struct vgic_register_region *region;
struct kvm_vcpu *r_vcpu;
 
@@ -960,10 +959,9 @@ static int vgic_uaccess_read(struct kvm_vcpu *vcpu, struct 
kvm_io_device *dev,
return 0;
 }
 
-static int vgic_uaccess_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
+static int vgic_uaccess_write(struct kvm_vcpu *vcpu, struct vgic_io_device 
*iodev,
  gpa_t addr, const u32 *val)
 {
-   struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
const struct vgic_register_region *region;
struct kvm_vcpu *r_vcpu;
 
@@ -986,9 +984,9 @@ int vgic_uaccess(struct kvm_vcpu *vcpu, struct 
vgic_io_device *dev,
 bool is_write, int offset, u32 *val)
 {
if (is_write)
-   return vgic_uaccess_write(vcpu, >dev, offset, val);
+   return vgic_uaccess_write(vcpu, dev, offset, val);
else
-   return vgic_uaccess_read(vcpu, >dev, offset, val);
+   return vgic_uaccess_read(vcpu, dev, offset, val);
 }
 
 static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
-- 
2.21.3

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 6/9] docs: kvm: devices/arm-vgic-v3: enhance KVM_DEV_ARM_VGIC_CTRL_INIT doc

[Eric Auger]

kvm_arch_vcpu_precreate() returns -EBUSY if the vgic is
already initialized. So let's document that KVM_DEV_ARM_VGIC_CTRL_INIT
must be called after all vcpu creations.

Signed-off-by: Eric Auger 

---

v1 -> v2:
- Must be called after all vcpu creations ->
  Must be called after all VCPUs have been created as per
  Alexandru's suggestion
---
 Documentation/virt/kvm/devices/arm-vgic-v3.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/virt/kvm/devices/arm-vgic-v3.rst 
b/Documentation/virt/kvm/devices/arm-vgic-v3.rst
index 5dd3bff51978..51e5e5762571 100644
--- a/Documentation/virt/kvm/devices/arm-vgic-v3.rst
+++ b/Documentation/virt/kvm/devices/arm-vgic-v3.rst
@@ -228,7 +228,7 @@ Groups:
 
 KVM_DEV_ARM_VGIC_CTRL_INIT
   request the initialization of the VGIC, no additional parameter in
-  kvm_device_attr.addr.
+  kvm_device_attr.addr. Must be called after all VCPUs have been created.
 KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES
   save all LPI pending bits into guest RAM pending tables.
 
-- 
2.21.3

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 5/9] KVM: arm: move has_run_once after the map_resources

[Eric Auger]

has_run_once is set to true at the beginning of
kvm_vcpu_first_run_init(). This generally is not an issue
except when exercising the code with KVM selftests. for instance,
if kvm_vgic_map_resources() fails due to erroneous user settings,
has_run_once is set and this prevents from continuing
executing the test. This patch moves the assignment after the
kvm_vgic_map_resources().

Signed-off-by: Eric Auger 

---

v1 -> v2:
- slight reword of the commit msg (for instance)
---
 arch/arm64/kvm/arm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 04c44853b103..580760e58980 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -573,8 +573,6 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
if (!kvm_arm_vcpu_is_finalized(vcpu))
return -EPERM;
 
-   vcpu->arch.has_run_once = true;
-
if (likely(irqchip_in_kernel(kvm))) {
/*
 * Map the VGIC hardware resources before running a vcpu the
@@ -591,6 +589,8 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
static_branch_inc(_irqchip_in_use);
}
 
+   vcpu->arch.has_run_once = true;
+
ret = kvm_timer_enable(vcpu);
if (ret)
return ret;
-- 
2.21.3

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 4/9] KVM: arm/arm64: vgic: Reset base address on kvm_vgic_dist_destroy()

[Eric Auger]

On vgic_dist_destroy(), the addresses are not reset. However for
kvm selftest purpose this would allow to continue the test execution
even after a failure when running KVM_RUN. So let's reset the
base addresses.

Signed-off-by: Eric Auger 

---

v1 -> v2:
- use dist-> in the else and add braces
---
 arch/arm64/kvm/vgic/vgic-init.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 052917deb149..cf6faa0aeddb 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -335,13 +335,16 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
kfree(dist->spis);
dist->spis = NULL;
dist->nr_spis = 0;
+   dist->vgic_dist_base = VGIC_ADDR_UNDEF;
 
-   if (kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
+   if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
list_for_each_entry_safe(rdreg, next, >rd_regions, list) {
list_del(>list);
kfree(rdreg);
}
INIT_LIST_HEAD(>rd_regions);
+   } else {
+   dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
}
 
if (vgic_has_its(kvm))
@@ -362,6 +365,7 @@ void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu)
vgic_flush_pending_lpis(vcpu);
 
INIT_LIST_HEAD(_cpu->ap_list_head);
+   vgic_cpu->rd_iodev.base_addr = VGIC_ADDR_UNDEF;
 }
 
 /* To be called with kvm->lock held */
-- 
2.21.3

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 3/9] KVM: arm64: vgic-v3: Fix error handling in vgic_v3_set_redist_base()

[Eric Auger]

vgic_v3_insert_redist_region() may succeed while
vgic_register_all_redist_iodevs fails. For example this happens
while adding a redistributor region overlapping a dist region. The
failure only is detected on vgic_register_all_redist_iodevs when
vgic_v3_check_base() gets called in vgic_register_redist_iodev().

In such a case, remove the newly added redistributor region and free
it.

Signed-off-by: Eric Auger 

---

v1 -> v2:
- fix the commit message and split declaration/assignment of rdreg
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c 
b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 013b737b658f..987e366c8000 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -860,8 +860,14 @@ int vgic_v3_set_redist_base(struct kvm *kvm, u32 index, 
u64 addr, u32 count)
 * afterwards will register the iodevs when needed.
 */
ret = vgic_register_all_redist_iodevs(kvm);
-   if (ret)
+   if (ret) {
+   struct vgic_redist_region *rdreg;
+
+   rdreg = vgic_v3_rdist_region_from_index(kvm, index);
+   list_del(>list);
+   kfree(rdreg);
return ret;
+   }
 
return 0;
 }
-- 
2.21.3

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 2/9] KVM: arm64: Fix KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION read

[Eric Auger]

The doc says:
"The characteristics of a specific redistributor region can
 be read by presetting the index field in the attr data.
 Only valid for KVM_DEV_TYPE_ARM_VGIC_V3"

Unfortunately the existing code fails to read the input attr data.

Fixes: 04c110932225 ("KVM: arm/arm64: Implement 
KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION")
Cc: sta...@vger.kernel.org#v4.17+
Signed-off-by: Eric Auger 
Reviewed-by: Alexandru Elisei 

---

v1 -> v2:
- in the commit message, remove the statement that the index always is 0
- add Alexandru's R-b
---
 arch/arm64/kvm/vgic/vgic-kvm-device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c 
b/arch/arm64/kvm/vgic/vgic-kvm-device.c
index 44419679f91a..2f66cf247282 100644
--- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
+++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
@@ -226,6 +226,9 @@ static int vgic_get_common_attr(struct kvm_device *dev,
u64 addr;
unsigned long type = (unsigned long)attr->attr;
 
+   if (copy_from_user(, uaddr, sizeof(addr)))
+   return -EFAULT;
+
r = kvm_vgic_addr(dev->kvm, type, , false);
if (r)
return (r == -ENODEV) ? -ENXIO : r;
-- 
2.21.3

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 1/9] KVM: arm64: vgic-v3: Fix some error codes when setting RDIST base

[Eric Auger]

KVM_DEV_ARM_VGIC_GRP_ADDR group doc says we should return
-EEXIST in case the base address of the redist is already set.
We currently return -EINVAL.

However we need to return -EINVAL in case a legacy REDIST address
is attempted to be set while REDIST_REGIONS were set. This case
is discriminated by looking at the count field.

Signed-off-by: Eric Auger 

---

v1 -> v2:
- simplify the check sequence
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c 
b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 15a6c98ee92f..013b737b658f 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -791,10 +791,6 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, 
uint32_t index,
size_t size = count * KVM_VGIC_V3_REDIST_SIZE;
int ret;
 
-   /* single rdist region already set ?*/
-   if (!count && !list_empty(rd_regions))
-   return -EINVAL;
-
/* cross the end of memory ? */
if (base + size < base)
return -EINVAL;
@@ -805,11 +801,14 @@ static int vgic_v3_insert_redist_region(struct kvm *kvm, 
uint32_t index,
} else {
rdreg = list_last_entry(rd_regions,
struct vgic_redist_region, list);
-   if (index != rdreg->index + 1)
-   return -EINVAL;
 
-   /* Cannot add an explicitly sized regions after legacy region */
-   if (!rdreg->count)
+   if ((!count) != (!rdreg->count))
+   return -EINVAL; /* Mix REDIST and REDIST_REGION */
+
+   if (!count)
+   return -EEXIST;
+
+   if (index != rdreg->index + 1)
return -EINVAL;
}
 
-- 
2.21.3

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 0/9] KVM/ARM: Some vgic fixes and init sequence KVM selftests

[Eric Auger]

While writting vgic v3 init sequence KVM selftests I noticed some
relatively minor issues. This was also the opportunity to try to
fix the issue laterly reported by Zenghui, related to the RDIST_TYPER
last bit emulation. The final patch is a first batch of VGIC init
sequence selftests. Of course they can be augmented with a lot more
register access tests, but let's try to move forward incrementally ...

Best Regards

Eric

This series can be found at:
https://github.com/eauger/linux/tree/vgic_kvmselftests_v2

History:
- Took into account all comments from Marc and Alexandru's except
  the has_run_once still after the map_resources (this would oblige
  me to revisit in depth the selftests)

Eric Auger (9):
  KVM: arm64: vgic-v3: Fix some error codes when setting RDIST base
  KVM: arm64: Fix KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION read
  KVM: arm64: vgic-v3: Fix error handling in vgic_v3_set_redist_base()
  KVM: arm/arm64: vgic: Reset base address on kvm_vgic_dist_destroy()
  KVM: arm: move has_run_once after the map_resources
  docs: kvm: devices/arm-vgic-v3: enhance KVM_DEV_ARM_VGIC_CTRL_INIT doc
  KVM: arm64: Simplify argument passing to vgic_uaccess_[read|write]
  KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace
  KVM: selftests: aarch64/vgic-v3 init sequence tests

 .../virt/kvm/devices/arm-vgic-v3.rst  |   2 +-
 arch/arm64/kvm/arm.c  |   4 +-
 arch/arm64/kvm/vgic/vgic-init.c   |   6 +-
 arch/arm64/kvm/vgic/vgic-kvm-device.c |   3 +
 arch/arm64/kvm/vgic/vgic-mmio-v3.c|  30 +-
 arch/arm64/kvm/vgic/vgic-mmio.c   |  10 +-
 include/kvm/arm_vgic.h|   1 +
 tools/testing/selftests/kvm/Makefile  |   1 +
 .../testing/selftests/kvm/aarch64/vgic_init.c | 453 ++
 .../testing/selftests/kvm/include/kvm_util.h  |   5 +
 tools/testing/selftests/kvm/lib/kvm_util.c|  51 ++
 11 files changed, 546 insertions(+), 20 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/vgic_init.c

-- 
2.21.3

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 8/9] KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace

[Auger Eric]

Hi Alexandru,

On 1/12/21 6:02 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 12/12/20 6:50 PM, Eric Auger wrote:
>> Commit 23bde34771f1 ("KVM: arm64: vgic-v3: Drop the
>> reporting of GICR_TYPER.Last for userspace") temporarily fixed
>> a bug identified when attempting to access the GICR_TYPER
>> register before the redistributor region setting but dropped
>> the support of the LAST bit. This patch restores its
>> support (if the redistributor region was set) while keeping the
>> code safe.
> 
> I suppose the reason for emulating GICR_TYPER.Last is for architecture 
> compliance,
> right? I think that should be in the commit message.
OK added this in the commit msg.
> 
>>
>> Signed-off-by: Eric Auger 
>> ---
>>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 7 ++-
>>  include/kvm/arm_vgic.h | 1 +
>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c 
>> b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> index 581f0f49..2f9ef6058f6e 100644
>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> @@ -277,6 +277,8 @@ static unsigned long vgic_uaccess_read_v3r_typer(struct 
>> kvm_vcpu *vcpu,
>>   gpa_t addr, unsigned int len)
>>  {
>>  unsigned long mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
>> +struct vgic_cpu *vgic_cpu = >arch.vgic_cpu;
>> +struct vgic_redist_region *rdreg = vgic_cpu->rdreg;
>>  int target_vcpu_id = vcpu->vcpu_id;
>>  u64 value;
>>  
>> @@ -286,7 +288,9 @@ static unsigned long vgic_uaccess_read_v3r_typer(struct 
>> kvm_vcpu *vcpu,
>>  if (vgic_has_its(vcpu->kvm))
>>  value |= GICR_TYPER_PLPIS;
>>  
>> -/* reporting of the Last bit is not supported for userspace */
>> +if (rdreg && (vgic_cpu->rdreg_index == (rdreg->free_index - 1)))
>> +value |= GICR_TYPER_LAST;
>> +
>>  return extract_bytes(value, addr & 7, len);
>>  }
>>  
>> @@ -714,6 +718,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu)
>>  return -EINVAL;
>>  
>>  vgic_cpu->rdreg = rdreg;
>> +vgic_cpu->rdreg_index = rdreg->free_index;
> 
> What happens if the next redistributor region we register has the base address
> adjacent to this one?
> 
> I'm really not familiar with the code, but is it not possible to create two
> Redistributor regions (via
> KVM_DEV_ARM_VGIC_GRP_ADDR(KVM_VGIC_V3_ADDR_TYPE_REDIST)) where the second
> Redistributor region start address is immediately after the last 
> Redistributor in
> the preceding region?
KVM_VGIC_V3_ADDR_TYPE_REDIST only allows to create a single rdist
region. Only KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION allows to register
several of them.

with KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, it is possible to register
adjacent rdist regions. vgic_v3_rdist_free_slot() previously returned
the 1st rdist region where enough space remains for inserting the new
reg. We put the rdist at the free index there.

But maybe I misunderstood your question?

Thanks

Eric
> 
> Thanks,
> Alex
>>  
>>  rd_base = rdreg->base + rdreg->free_index * KVM_VGIC_V3_REDIST_SIZE;
>>  
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index a8d8fdcd3723..596c069263a7 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -322,6 +322,7 @@ struct vgic_cpu {
>>   */
>>  struct vgic_io_device   rd_iodev;
>>  struct vgic_redist_region *rdreg;
>> +u32 rdreg_index;
>>  
>>  /* Contains the attributes and gpa of the LPI pending tables. */
>>  u64 pendbaser;
> 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/9] KVM: arm64: vgic-v3: Fix some error codes when setting RDIST base

[Auger Eric]

Hi Alexandru,

On 1/6/21 5:32 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 12/12/20 6:50 PM, Eric Auger wrote:
>> KVM_DEV_ARM_VGIC_GRP_ADDR group doc says we should return
>> -EEXIST in case the base address of the redist is already set.
>> We currently return -EINVAL.
>>
>> However we need to return -EINVAL in case a legacy REDIST address
>> is attempted to be set while REDIST_REGIONS were set. This case
>> is discriminated by looking at the count field.
>>
>> Signed-off-by: Eric Auger 
>> ---
>>  arch/arm64/kvm/vgic/vgic-mmio-v3.c | 9 +++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c 
>> b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> index 15a6c98ee92f..8e8a862def76 100644
>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> @@ -792,8 +792,13 @@ static int vgic_v3_insert_redist_region(struct kvm 
>> *kvm, uint32_t index,
>>  int ret;
>>  
>>  /* single rdist region already set ?*/
>> -if (!count && !list_empty(rd_regions))
>> -return -EINVAL;
>> +if (!count && !list_empty(rd_regions)) {
>> +rdreg = list_last_entry(rd_regions,
>> +   struct vgic_redist_region, list);
>> +if (rdreg->count)
>> +return -EINVAL; /* Mixing REDIST and REDIST_REGION API 
>> */
>> +return -EEXIST;
>> +}
> 
> A few instructions below:
> 
>     if (list_empty(rd_regions)) {
>         [..]
>     } else {
>         rdreg = list_last_entry(rd_regions,
>                     struct vgic_redist_region, list);
>         [..]
> 
>         /* Cannot add an explicitly sized regions after legacy region */
>         if (!rdreg->count)
>             return -EINVAL;
>     }
> 
> Isn't this testing for the same thing, but using the opposite condition? Or 
> am I
> misunderstanding the code (quite likely)?
the 1st test sequence handles the case where the legacy
KVM_VGIC_V3_ADDR_TYPE_REDIST is used (!count) while the second handles
the case where the REDIST_REGION is used. Nevertheless I think this can
be simplified into:

if (list_empty(rd_regions)) {
if (index != 0)
return -EINVAL;
} else {
rdreg = list_last_entry(rd_regions,
struct vgic_redist_region, list);

if ((!count) != (!rdreg->count))
return -EINVAL; /* Mix REDIST and REDIST_REGION */

if (!count)
return -EEXIST;

if (index != rdreg->index + 1)
return -EINVAL;
}






> 
> Looks to me like 
> KVM_DEV_ARM_VGIC_GRP_ADDR(KVM_VGIC_V3_ADDR_TYPE_REDIST{,_REGION})
> used to return -EEXIST (from vgic_check_ioaddr()) before commit ccc27bf5be7b7
> ("KVM: arm/arm64: Helper to register a new redistributor region") which added 
> the
> vgic_v3_insert_redist_region() function, so bringing back the -EEXIST return 
> code
> looks the right thing to me.

OK thank you for the detailed study.

Eric
> 
> Thanks,
> Alex
>>  
>>  /* cross the end of memory ? */
>>  if (base + size < base)
> 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/9] KVM: arm: move has_run_once after the map_resources

[Auger Eric]

Hi Alexandru,

On 1/12/21 3:55 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> On 12/12/20 6:50 PM, Eric Auger wrote:
>> has_run_once is set to true at the beginning of
>> kvm_vcpu_first_run_init(). This generally is not an issue
>> except when exercising the code with KVM selftests. Indeed,
>> if kvm_vgic_map_resources() fails due to erroneous user settings,
>> has_run_once is set and this prevents from continuing
>> executing the test. This patch moves the assignment after the
>> kvm_vgic_map_resources().
>>
>> Signed-off-by: Eric Auger 
>> ---
>>  arch/arm64/kvm/arm.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index c0ffb019ca8b..331fae6bff31 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -540,8 +540,6 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>>  if (!kvm_arm_vcpu_is_finalized(vcpu))
>>  return -EPERM;
>>  
>> -vcpu->arch.has_run_once = true;
>> -
>>  if (likely(irqchip_in_kernel(kvm))) {
>>  /*
>>   * Map the VGIC hardware resources before running a vcpu the
>> @@ -560,6 +558,8 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>>  static_branch_inc(_irqchip_in_use);
>>  }
>>  
>> +vcpu->arch.has_run_once = true;
> 
> I have a few concerns regarding this:
> 
> 1. Moving has_run_once = true here seems very arbitrary to me - 
> kvm_timer_enable()
> and kvm_arm_pmu_v3_enable(), below it, can both fail because of erroneous user
> values. If there's a reason why the assignment cannot be moved at the end of 
> the
> function, I think it should be clearly stated in a comment for the people who
> might be tempted to write similar tests for the timer or pmu.

Setting has_run_once = true at the entry of the function looks to me
even more arbitrary. I agree with you that eventually has_run_once may
be moved at the very end but maybe this can be done later once timer,
pmu tests haven ben written
> 
> 2. There are many ways that kvm_vgic_map_resources() can fail, other than
> incorrect user settings. I started digging into how
> kvm_vgic_map_resources()->vgic_v2_map_resources() can fail for a VGIC V2 and 
> this
> is what I managed to find before I gave up:
> 
> * vgic_init() can fail in:
>     - kvm_vgic_dist_init()
>     - vgic_v3_init()
>     - kvm_vgic_setup_default_irq_routing()
> * vgic_register_dist_iodev() can fail in:
>     - vgic_v3_init_dist_iodev()
>     - kvm_io_bus_register_dev()(*)
> * kvm_phys_addr_ioremap() can fail in:
>     - kvm_mmu_topup_memory_cache()
>     - kvm_pgtable_stage2_map()

I changed the commit msg so that "incorrect user settings" sounds as an
example.
> 
> So if any of the functions below fail, are we 100% sure it is safe to allow 
> the
> user to execute kvm_vgic_map_resources() again?

I think additional tests will confirm this. However at the moment,
moving the assignment, which does not look wrong to me, allows to
greatly simplify the tests so I would tend to say that it is worth.
> 
> (*) It looks to me like kvm_io_bus_register_dev() doesn't take into account a
> caller that tries to register the same device address range and it will create
> another identical range. Is this intentional? Is it a bug that should be 
> fixed? Or
> am I misunderstanding the function?

doesn't kvm_io_bus_cmp() do the check?

Thanks

Eric
> 
> Thanks,
> Alex
>> +
>>  ret = kvm_timer_enable(vcpu);
>>  if (ret)
>>  return ret;
> 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm