[PATCH v3 0/4] kvm: arm/arm64: vgic: APRn uaccess support.

[PATCH v3 0/4] kvm: arm/arm64: vgic: APRn uaccess support.

[wanghaibin]

v3: Coding style fix.
    Add the valid APRn access check which Marc proposed. 

v2: Split the patch again to make it easier for review
    some fixes were proposed by Marc

v1: the problem describe:
In the case (GICv2 on GICv3 migration), I did the test on my board as follow:
vm boot => migrate to destination => shutdown at destination => start at destination 
=> migrate back to source ... go round and begin again;

I tested many times, but unlucky, there maybe failed by accident when shutdown the vm 
at destination. (GICv3 on GICv3 migration, 1000+ times, That's OK).
while failed,  we can watch the interrupts in the vm, some vcpus of the vm can not 
receive the virt timer interrupt. And, these vcpus will 100% with top tool at host.

vgic_state debug file at destination shows(a active virt timer interrupt) that:
VCPU 0 TYP   ID TGT_ID PLAEHC     HWID   TARGET SRC PRI VCPU_ID
---------------------------------------------------------------
            ....................
       PPI   25      0 000001        0        1   0 160      -1
       PPI   26      0 000001        0        1   0 160      -1
       PPI   27      0 011111       27        1   0 160       0


I added log to print ICH* registers for VCPU0 at destination:
**HCR:0x1, VMCR:0xf0ec0001,SRE:0x0, ELRSR:0xe**
-----AP0R:0: 0x0------
-----AP0R:1: 0x0------
-----AP0R:2: 0x0------
-----AP0R:3: 0x0------
-----AP1R:0: 0x0------
-----AP1R:1: 0x0------
-----AP1R:2: 0x0------
-----AP1R:3: 0x0------
-----LR:0: 0xa0a0001b0000001b------
-----LR:1: 0x0------
-----LR:2: 0x0------
-----LR:3: 0x0------

and the ICH_AP1R0 value is 0x10000 at source.

At present, QEMU have supproted GICC_APRn put/set interface for emulated GICv2,
and kvm does not support the uaccess interface. This patchset try to support this.


wanghaibin (4):
  kvm: arm/arm64: vgic: Implement the vGICv2 GICC_APRn uaccess
    interface.
  kvm: arm/arm64: vgic-v2: Add GICH_APRn accessors for GICv2
  kvm: arm/arm64: vgic-v3: add ICH_AP[01]Rn accessors for GICv3
  kvm: arm/arm64: vgic: clean up vGICv3 ICC_APRn sysreg uaccess

 arch/arm64/kvm/vgic-sys-reg-v3.c | 25 +++++----------------
 virt/kvm/arm/vgic/vgic-mmio-v2.c | 22 ++++++++++++++++--
 virt/kvm/arm/vgic/vgic-mmio.c    | 28 +++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic-v2.c      | 18 +++++++++++++++
 virt/kvm/arm/vgic/vgic-v3.c      | 48 ++++++++++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic.h         |  7 ++++++
 6 files changed, 126 insertions(+), 22 deletions(-)

-- 
1.8.3.1

[PATCH v3 1/4] kvm: arm/arm64: vgic: Implement the vGICv2 GICC_APRn uaccess interface.

[wanghaibin]

The SPEC defined GICC_APR<n> (n = 0-3, 32-bit per register) to provide
information about interrupt active priorities for GICv2, and the user
access will traverse all of these registers no matter how many
priority levels we supported.

So we have to implement the uaccess interface cover all of these registers.

Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
---
 virt/kvm/arm/vgic/vgic-mmio-v2.c | 22 ++++++++++++++++++++--
 virt/kvm/arm/vgic/vgic-mmio.c    |  9 +++++++++
 virt/kvm/arm/vgic/vgic.h         |  3 +++
 3 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 37522e6..2c1297c 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -303,6 +303,23 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu,
 	vgic_set_vmcr(vcpu, &vmcr);
 }
 
+static unsigned long vgic_v2_uaccess_read_activeprio(struct kvm_vcpu *vcpu,
+						     gpa_t addr, unsigned int len)
+{
+	u32 idx = (addr & 0x0c) / sizeof(u32);
+
+	return vgic_get_apr(vcpu, idx);
+}
+
+static void vgic_v2_uaccess_write_activeprio(struct kvm_vcpu *vcpu,
+					     gpa_t addr, unsigned int len,
+					     unsigned long val)
+{
+       u32 idx = (addr & 0x0c) / sizeof(u32);
+
+       return vgic_set_apr(vcpu, idx, val);
+}
+
 static const struct vgic_register_region vgic_v2_dist_registers[] = {
 	REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL,
 		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
@@ -363,8 +380,9 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu,
 	REGISTER_DESC_WITH_LENGTH(GIC_CPU_ALIAS_BINPOINT,
 		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
 		VGIC_ACCESS_32bit),
-	REGISTER_DESC_WITH_LENGTH(GIC_CPU_ACTIVEPRIO,
-		vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
+	REGISTER_DESC_WITH_LENGTH_UACCESS(GIC_CPU_ACTIVEPRIO,
+		vgic_mmio_read_raz, vgic_mmio_write_wi,
+		vgic_v2_uaccess_read_activeprio, vgic_v2_uaccess_write_activeprio, 16,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH(GIC_CPU_IDENT,
 		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index c1e4bdd..fdf9804 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -492,6 +492,15 @@ static int match_region(const void *key, const void *elt)
 		       sizeof(regions[0]), match_region);
 }
 
+void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
+{
+}
+
+u32 vgic_get_apr(struct kvm_vcpu *vcpu, u32 idx)
+{
+	return 0;
+}
+
 void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
 {
 	if (kvm_vgic_global_state.type == VGIC_V2)
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index bba7fa2..c2be5b7 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -209,6 +209,9 @@ int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
 int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write,
 				    u32 intid, u64 *val);
 int kvm_register_vgic_device(unsigned long type);
+
+void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val);
+u32 vgic_get_apr(struct kvm_vcpu *vcpu, u32 idx);
 void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 int vgic_lazy_init(struct kvm *kvm);
-- 
1.8.3.1

[PATCH v3 2/4] kvm: arm/arm64: vgic-v2: Add GICH_APRn accessors for GICv2

[wanghaibin]

This patch is used for GICv2 on GICv2.

About GICV_APRn hardware register access,the SPEC says:
When System register access is disabled for EL2, these registers access
GICH_APRn, and all active priorities for virtual machines are held in
GICH_APRn regardless of interrupt group.

For GICv2 hardware, there are at most 5 priority bits are implemented in
GICH_LRn.Priority, so we only need to be concerned with GICH_APR0.
The other GICH_APRn (n = 1-3) access should be treated as raz/wi.

Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
---
 virt/kvm/arm/vgic/vgic-mmio.c |  5 +++++
 virt/kvm/arm/vgic/vgic-v2.c   | 18 ++++++++++++++++++
 virt/kvm/arm/vgic/vgic.h      |  2 ++
 3 files changed, 25 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index fdf9804..80261b7 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -494,10 +494,15 @@ static int match_region(const void *key, const void *elt)
 
 void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
 {
+	if (kvm_vgic_global_state.type == VGIC_V2)
+		vgic_v2_set_apr(vcpu, idx, val);
 }
 
 u32 vgic_get_apr(struct kvm_vcpu *vcpu, u32 idx)
 {
+	if (kvm_vgic_global_state.type == VGIC_V2)
+		return vgic_v2_get_apr(vcpu, idx);
+
 	return 0;
 }
 
diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index e4187e5..65daf35 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -172,6 +172,24 @@ void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr)
 	vcpu->arch.vgic_cpu.vgic_v2.vgic_lr[lr] = 0;
 }
 
+void vgic_v2_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
+{
+	struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2;
+
+	if (idx == 0)
+		cpu_if->vgic_apr = val;
+}
+
+u32 vgic_v2_get_apr(struct kvm_vcpu *vcpu, u32 idx)
+{
+	struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2;
+
+	if (idx == 0)
+		return cpu_if->vgic_apr;
+	else
+		return 0;
+}
+
 void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp)
 {
 	struct vgic_v2_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v2;
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index c2be5b7..441ded7 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -155,6 +155,8 @@ int vgic_v2_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
 			 int offset, u32 *val);
 int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write,
 			  int offset, u32 *val);
+void vgic_v2_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val);
+u32 vgic_v2_get_apr(struct kvm_vcpu *vcpu, u32 idx);
 void vgic_v2_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 void vgic_v2_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 void vgic_v2_enable(struct kvm_vcpu *vcpu);
-- 
1.8.3.1

[PATCH v3 3/4] kvm: arm/arm64: vgic-v3: add ICH_AP[01]Rn accessors for GICv3

[wanghaibin]

This patch is used for GICv2 on GICv3.

About GICV_APRn hardware register access,the SPEC says:
When System register access is enabled for EL2, these registers access
ICH_AP1Rn_EL2, and all active priorities for virtual machines are held
in ICH_AP1Rn_EL2 regardless of interrupt group.

For GICv3 hardware, we access the active priorities from ICH_AP1Rn_EL2
in this scene.

Aiming at the userspace access the undefined APR registers which the
hardwate doesn't implement, it will be treates ad raz/wi.

Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
---
 virt/kvm/arm/vgic/vgic-mmio.c | 16 +++++++++++++--
 virt/kvm/arm/vgic/vgic-v3.c   | 48 +++++++++++++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic.h      |  2 ++
 3 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 80261b7..738d800 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -494,14 +494,26 @@ static int match_region(const void *key, const void *elt)
 
 void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
 {
-	if (kvm_vgic_global_state.type == VGIC_V2)
+	u32 vgic_model = vcpu->kvm->arch.vgic.vgic_model;
+
+	if (kvm_vgic_global_state.type == VGIC_V2) {
 		vgic_v2_set_apr(vcpu, idx, val);
+	} else {
+		if (vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
+			vgic_v3_set_apr(vcpu, 1, idx, val);
+	}
 }
 
 u32 vgic_get_apr(struct kvm_vcpu *vcpu, u32 idx)
 {
-	if (kvm_vgic_global_state.type == VGIC_V2)
+	u32 vgic_model = vcpu->kvm->arch.vgic.vgic_model;
+
+	if (kvm_vgic_global_state.type == VGIC_V2) {
 		return vgic_v2_get_apr(vcpu, idx);
+	} else {
+		if (vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
+			return vgic_v3_get_apr(vcpu, 1, idx);
+	}
 
 	return 0;
 }
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 96ea597..2625dfd 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -160,6 +160,54 @@ void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr)
 	vcpu->arch.vgic_cpu.vgic_v3.vgic_lr[lr] = 0;
 }
 
+static bool vgic_v3_apr_access_valid(struct kvm_vcpu *vcpu, u32 idx)
+{
+	struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
+
+	if (idx > 3)
+		return false;
+
+	switch (vgic_v3_cpu->num_pri_bits) {
+	case 7:
+		return true;
+	case 6:
+		if (idx > 1)
+			return false;
+		break;
+	default:
+		if (idx > 0)
+			return false;
+	}
+
+	return true;
+}
+
+void vgic_v3_set_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx, u32 val)
+{
+	struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (!vgic_v3_apr_access_valid(vcpu, idx))
+		return;
+
+	if (group)
+		cpu_if->vgic_ap1r[idx] = val;
+	else
+		cpu_if->vgic_ap0r[idx] = val;
+}
+
+u32 vgic_v3_get_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx)
+{
+	struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (!vgic_v3_apr_access_valid(vcpu, idx))
+		return 0;
+
+	if (group)
+		return cpu_if->vgic_ap1r[idx];
+	else
+		return cpu_if->vgic_ap0r[idx];
+}
+
 void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcrp)
 {
 	struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 441ded7..19b0f8b 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -181,6 +181,8 @@ static inline void vgic_get_irq_kref(struct vgic_irq *irq)
 void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr);
 void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr);
 void vgic_v3_set_underflow(struct kvm_vcpu *vcpu);
+void vgic_v3_set_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx, u32 val);
+u32 vgic_v3_get_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx);
 void vgic_v3_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 void vgic_v3_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 void vgic_v3_enable(struct kvm_vcpu *vcpu);
-- 
1.8.3.1

[PATCH v3 4/4] kvm: arm/arm64: vgic: clean up vGICv3 ICC_APRn sysreg uaccess

[wanghaibin]

It will be better that hide the underlying implementation for emulated GIC.
This patch extend the vgic_set/get_apr func (with group parameter) more general,
so that, the vGICv3 ICC_APRn sysreg uaccess can call this general interface
for GICv3 on GICv3.

Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
---
 arch/arm64/kvm/vgic-sys-reg-v3.c | 25 +++++--------------------
 virt/kvm/arm/vgic/vgic-mmio-v2.c |  4 ++--
 virt/kvm/arm/vgic/vgic-mmio.c    | 10 ++++++----
 virt/kvm/arm/vgic/vgic.h         |  4 ++--
 4 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c
index 116786d..f0d8d36 100644
--- a/arch/arm64/kvm/vgic-sys-reg-v3.c
+++ b/arch/arm64/kvm/vgic-sys-reg-v3.c
@@ -188,23 +188,6 @@ static bool access_gic_grpen1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	return true;
 }
 
-static void vgic_v3_access_apr_reg(struct kvm_vcpu *vcpu,
-				   struct sys_reg_params *p, u8 apr, u8 idx)
-{
-	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
-	uint32_t *ap_reg;
-
-	if (apr)
-		ap_reg = &vgicv3->vgic_ap1r[idx];
-	else
-		ap_reg = &vgicv3->vgic_ap0r[idx];
-
-	if (p->is_write)
-		*ap_reg = p->regval;
-	else
-		p->regval = *ap_reg;
-}
-
 static bool access_gic_aprn(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 			    const struct sys_reg_desc *r, u8 apr)
 {
@@ -218,19 +201,21 @@ static bool access_gic_aprn(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	 */
 	switch (vgic_v3_cpu->num_pri_bits) {
 	case 7:
-		vgic_v3_access_apr_reg(vcpu, p, apr, idx);
 		break;
 	case 6:
 		if (idx > 1)
 			goto err;
-		vgic_v3_access_apr_reg(vcpu, p, apr, idx);
 		break;
 	default:
 		if (idx > 0)
 			goto err;
-		vgic_v3_access_apr_reg(vcpu, p, apr, idx);
 	}
 
+	if (p->is_write)
+		vgic_set_apr(vcpu, apr, idx, p->regval);
+	else
+		p->regval = vgic_get_apr(vcpu, apr, idx);
+
 	return true;
 err:
 	if (!p->is_write)
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 2c1297c..373e69f 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -308,7 +308,7 @@ static unsigned long vgic_v2_uaccess_read_activeprio(struct kvm_vcpu *vcpu,
 {
 	u32 idx = (addr & 0x0c) / sizeof(u32);
 
-	return vgic_get_apr(vcpu, idx);
+	return vgic_get_apr(vcpu, 0, idx);
 }
 
 static void vgic_v2_uaccess_write_activeprio(struct kvm_vcpu *vcpu,
@@ -317,7 +317,7 @@ static void vgic_v2_uaccess_write_activeprio(struct kvm_vcpu *vcpu,
 {
        u32 idx = (addr & 0x0c) / sizeof(u32);
 
-       return vgic_set_apr(vcpu, idx, val);
+       return vgic_set_apr(vcpu, 0, idx, val);
 }
 
 static const struct vgic_register_region vgic_v2_dist_registers[] = {
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 738d800..2cc64e4 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -492,7 +492,7 @@ static int match_region(const void *key, const void *elt)
 		       sizeof(regions[0]), match_region);
 }
 
-void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
+void vgic_set_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx, u32 val)
 {
 	u32 vgic_model = vcpu->kvm->arch.vgic.vgic_model;
 
@@ -501,10 +501,12 @@ void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
 	} else {
 		if (vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
 			vgic_v3_set_apr(vcpu, 1, idx, val);
+		else
+			vgic_v3_set_apr(vcpu, group, idx, val);
 	}
 }
 
-u32 vgic_get_apr(struct kvm_vcpu *vcpu, u32 idx)
+u32 vgic_get_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx)
 {
 	u32 vgic_model = vcpu->kvm->arch.vgic.vgic_model;
 
@@ -513,9 +515,9 @@ u32 vgic_get_apr(struct kvm_vcpu *vcpu, u32 idx)
 	} else {
 		if (vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
 			return vgic_v3_get_apr(vcpu, 1, idx);
+		else
+			return vgic_v3_get_apr(vcpu, group, idx);
 	}
-
-	return 0;
 }
 
 void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 19b0f8b..eb1cd73 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -214,8 +214,8 @@ int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write,
 				    u32 intid, u64 *val);
 int kvm_register_vgic_device(unsigned long type);
 
-void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val);
-u32 vgic_get_apr(struct kvm_vcpu *vcpu, u32 idx);
+void vgic_set_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx, u32 val);
+u32 vgic_get_apr(struct kvm_vcpu *vcpu, u8 group, u32 idx);
 void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 int vgic_lazy_init(struct kvm *kvm);
-- 
1.8.3.1

Re: [PATCH v3 0/4] kvm: arm/arm64: vgic: APRn uaccess support.

[Christoffer Dall]

Hi Wanghaibin,

On Wed, Aug 23, 2017 at 09:05:22AM +0800, wanghaibin wrote:
> v3: Coding style fix.
>     Add the valid APRn access check which Marc proposed. 
> 
> v2: Split the patch again to make it easier for review
>     some fixes were proposed by Marc

Usually we put the changelog at the end of the description, before the
diff state.  I suggest you have a look at other patch series on the
kvmarm list or on the ARM linux mailing list and see how most people do
it.

> 
> v1: the problem describe:
> In the case (GICv2 on GICv3 migration), I did the test on my board as follow:
> vm boot => migrate to destination => shutdown at destination => start at destination 
> => migrate back to source ... go round and begin again;
> 
> I tested many times, but unlucky, there maybe failed by accident when shutdown the vm 
> at destination. (GICv3 on GICv3 migration, 1000+ times, That's OK).
> while failed,  we can watch the interrupts in the vm, some vcpus of the vm can not 
> receive the virt timer interrupt. And, these vcpus will 100% with top tool at host.
> 
> vgic_state debug file at destination shows(a active virt timer interrupt) that:
> VCPU 0 TYP   ID TGT_ID PLAEHC     HWID   TARGET SRC PRI VCPU_ID
> ---------------------------------------------------------------
>             ....................
>        PPI   25      0 000001        0        1   0 160      -1
>        PPI   26      0 000001        0        1   0 160      -1
>        PPI   27      0 011111       27        1   0 160       0
> 
> 
> I added log to print ICH* registers for VCPU0 at destination:
> **HCR:0x1, VMCR:0xf0ec0001,SRE:0x0, ELRSR:0xe**
> -----AP0R:0: 0x0------
> -----AP0R:1: 0x0------
> -----AP0R:2: 0x0------
> -----AP0R:3: 0x0------
> -----AP1R:0: 0x0------
> -----AP1R:1: 0x0------
> -----AP1R:2: 0x0------
> -----AP1R:3: 0x0------
> -----LR:0: 0xa0a0001b0000001b------
> -----LR:1: 0x0------
> -----LR:2: 0x0------
> -----LR:3: 0x0------
> 
> and the ICH_AP1R0 value is 0x10000 at source.
> 
> At present, QEMU have supproted GICC_APRn put/set interface for emulated GICv2,
> and kvm does not support the uaccess interface. This patchset try to support this.
> 

So I feel like this series is horribly complicated for what it does, and
does things in the reverse order.  If you really want to take your
appraoch, it would be much nicer if you first changed existing functions
and added infrastructure, and then in the end wired it up to the user
space ABI.  In other words, reverse your patches.

However, I think I have a simpler approach to solving this.  Please have
a look at this:

commit 1d49c5ef047a2218379aa170d9a3bdd39cd70e3a (HEAD -> gicv2-apr-uaccess)
Author: Christoffer Dall <cdall@linaro.org>
Date:   Thu Aug 31 22:24:25 2017 +0200

    KVM: arm/arm64: Support uaccess of GICC_APRn
    
    When migrating guests around we need to know the active priorities to
    ensure functional virtual interrupt prioritization by the GIC.
    
    This commit clarifies the API and how active priorities of interrupts in
    different groups are represented, and implements the accessor functions
    for the uaccess register range.
    
    We live with a slight layering violation in accessing GICv3 data
    structures from vgic-mmio-v2.c, because anything else just adds too much
    complexity for us to deal with (it's not like there's a benefit
    elsewhere in the code of an intermediate representation as is the case
    with the VMCR).  We accept this, because while doing v3 processing from
    a file named something-v2.c can look strange at first, this really is
    specific to dealing with the user space interface for something that
    looks like a GICv2.
    
    Signed-off-by: Christoffer Dall <cdall@linaro.org>

diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
index b2f60ca..b3ce126 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
@@ -83,6 +83,11 @@ Groups:
 
     Bits for undefined preemption levels are RAZ/WI.
 
+    Note that this differs from a CPU's view of the APRs on hardware in which
+    a GIC without the security extensions expose group 0 and group 1 active
+    priorities in separate register groups, whereas we show a combined view
+    similar to GICv2's GICH_APR.
+
     For historical reasons and to provide ABI compatibility with userspace we
     export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask
     field in the lower 5 bits of a word, meaning that userspace must always
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 37522e6..5436989 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -303,6 +303,45 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu,
 	vgic_set_vmcr(vcpu, &vmcr);
 }
 
+static unsigned long vgic_mmio_read_apr(struct kvm_vcpu *vcpu,
+					gpa_t addr, unsigned int len)
+{
+	int n; /* which APRn is this */
+
+	n = (addr >> 2) & 0x3;
+
+	if (kvm_vgic_global_state.type == VGIC_V2) {
+		/* GICv2 hardware systems support max. 32 groups */
+		if (n != 0)
+			return 0;
+		return vcpu->arch.vgic_cpu.vgic_v2.vgic_apr;
+	} else {
+		struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+		/* GICv3 only uses ICH_AP1Rn for memory mapped (GICv2) guests */
+		return vgicv3->vgic_ap0r[n] | vgicv3->vgic_ap1r[n];
+	}
+}
+
+static void vgic_mmio_write_apr(struct kvm_vcpu *vcpu,
+				gpa_t addr, unsigned int len,
+				unsigned long val)
+{
+	int n; /* which APRn is this */
+
+	n = (addr >> 2) & 0x3;
+
+	if (kvm_vgic_global_state.type == VGIC_V2) {
+		/* GICv2 hardware systems support max. 32 groups */
+		if (n != 0)
+			return;
+		vcpu->arch.vgic_cpu.vgic_v2.vgic_apr = val;
+	} else {
+		struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+		/* GICv3 only uses ICH_AP1Rn for memory mapped (GICv2) guests */
+		vgicv3->vgic_ap1r[n] = val;
+	}
+}
+
 static const struct vgic_register_region vgic_v2_dist_registers[] = {
 	REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL,
 		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
@@ -364,7 +403,7 @@ static const struct vgic_register_region vgic_v2_cpu_registers[] = {
 		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH(GIC_CPU_ACTIVEPRIO,
-		vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
+		vgic_mmio_read_apr, vgic_mmio_write_apr, 16,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH(GIC_CPU_IDENT,
 		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,


Thanks,
-Christoffer

Re: [PATCH v3 0/4] kvm: arm/arm64: vgic: APRn uaccess support.

[wanghaibin]

On 2017/9/1 4:33, Christoffer Dall wrote:

> Hi Wanghaibin,
> 
> On Wed, Aug 23, 2017 at 09:05:22AM +0800, wanghaibin wrote:
>> v3: Coding style fix.
>>     Add the valid APRn access check which Marc proposed. 
>>
>> v2: Split the patch again to make it easier for review
>>     some fixes were proposed by Marc
> 
> Usually we put the changelog at the end of the description, before the
> diff state.  I suggest you have a look at other patch series on the
> kvmarm list or on the ARM linux mailing list and see how most people do
> it.


OK, Pay attention next time.

Thanks.

> 
>>
>> v1: the problem describe:
>> In the case (GICv2 on GICv3 migration), I did the test on my board as follow:
>> vm boot => migrate to destination => shutdown at destination => start at destination 
>> => migrate back to source ... go round and begin again;
>>
>> I tested many times, but unlucky, there maybe failed by accident when shutdown the vm 
>> at destination. (GICv3 on GICv3 migration, 1000+ times, That's OK).
>> while failed,  we can watch the interrupts in the vm, some vcpus of the vm can not 
>> receive the virt timer interrupt. And, these vcpus will 100% with top tool at host.
>>
>> vgic_state debug file at destination shows(a active virt timer interrupt) that:
>> VCPU 0 TYP   ID TGT_ID PLAEHC     HWID   TARGET SRC PRI VCPU_ID
>> ---------------------------------------------------------------
>>             ....................
>>        PPI   25      0 000001        0        1   0 160      -1
>>        PPI   26      0 000001        0        1   0 160      -1
>>        PPI   27      0 011111       27        1   0 160       0
>>
>>
>> I added log to print ICH* registers for VCPU0 at destination:
>> **HCR:0x1, VMCR:0xf0ec0001,SRE:0x0, ELRSR:0xe**
>> -----AP0R:0: 0x0------
>> -----AP0R:1: 0x0------
>> -----AP0R:2: 0x0------
>> -----AP0R:3: 0x0------
>> -----AP1R:0: 0x0------
>> -----AP1R:1: 0x0------
>> -----AP1R:2: 0x0------
>> -----AP1R:3: 0x0------
>> -----LR:0: 0xa0a0001b0000001b------
>> -----LR:1: 0x0------
>> -----LR:2: 0x0------
>> -----LR:3: 0x0------
>>
>> and the ICH_AP1R0 value is 0x10000 at source.
>>
>> At present, QEMU have supproted GICC_APRn put/set interface for emulated GICv2,
>> and kvm does not support the uaccess interface. This patchset try to support this.
>>
> 
> So I feel like this series is horribly complicated for what it does, and
> does things in the reverse order.  If you really want to take your
> appraoch, it would be much nicer if you first changed existing functions
> and added infrastructure, and then in the end wired it up to the user
> space ABI.  In other words, reverse your patches.


No problem. The patch order can be adjusted if you feel necessary (this
depends on the results of the simpler patch discussion).

> 
> However, I think I have a simpler approach to solving this.  Please have
> a look at this:
> 
> commit 1d49c5ef047a2218379aa170d9a3bdd39cd70e3a (HEAD -> gicv2-apr-uaccess)
> Author: Christoffer Dall <cdall@linaro.org>
> Date:   Thu Aug 31 22:24:25 2017 +0200
> 
>     KVM: arm/arm64: Support uaccess of GICC_APRn
>     
>     When migrating guests around we need to know the active priorities to
>     ensure functional virtual interrupt prioritization by the GIC.
>     
>     This commit clarifies the API and how active priorities of interrupts in
>     different groups are represented, and implements the accessor functions
>     for the uaccess register range.
>     
>     We live with a slight layering violation in accessing GICv3 data
>     structures from vgic-mmio-v2.c, because anything else just adds too much
>     complexity for us to deal with (it's not like there's a benefit
>     elsewhere in the code of an intermediate representation as is the case
>     with the VMCR).  We accept this, because while doing v3 processing from
>     a file named something-v2.c can look strange at first, this really is
>     specific to dealing with the user space interface for something that
>     looks like a GICv2.
>


I have different opinions here.

Form Marc's proposed, I guess we take the vgic-v2.c/ vgic-v3.c as the hardware abstraction
layer for GICH_* / ICH_* registers. These files provide a series of methods to interactive with
registers (not only vgic_vmcr, such as vgic_hcr, vgic_elrsr, vgic_lr), these registers only can
be changed by the provided methods (eq: vgic_v2/v3_fold/populate_lr_state,  vgic_v2/v3_set_underflow)
and finally, these methods are invoking by the uniform interface (vgic_set_underflow,
vgic_populate/fold_lr, these uniform interfaces invoked the corresponding method through HW probed info)

In a word, I think we should design the high cohesion and low coupling code, and we've been doing this too.
We should strictly restrict other modules or files to know or access the vgic_v2/v3_cpu_if or
its registers (Unfortunately, vgic_sre and vgic_aprn are breaking the rules currently, the patch 4 fix
vgicv3 sys access, and this patchset design followed this rule).

My understanding is as mentioned above, maybe not correct.

Thandks.

     
>     Signed-off-by: Christoffer Dall <cdall@linaro.org>
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> index b2f60ca..b3ce126 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> @@ -83,6 +83,11 @@ Groups:
>  
>      Bits for undefined preemption levels are RAZ/WI.
>  
> +    Note that this differs from a CPU's view of the APRs on hardware in which
> +    a GIC without the security extensions expose group 0 and group 1 active
> +    priorities in separate register groups, whereas we show a combined view
> +    similar to GICv2's GICH_APR.
> +
>      For historical reasons and to provide ABI compatibility with userspace we
>      export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask
>      field in the lower 5 bits of a word, meaning that userspace must always
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 37522e6..5436989 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -303,6 +303,45 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu,
>  	vgic_set_vmcr(vcpu, &vmcr);
>  }
>  
> +static unsigned long vgic_mmio_read_apr(struct kvm_vcpu *vcpu,
> +					gpa_t addr, unsigned int len)
> +{
> +	int n; /* which APRn is this */
> +
> +	n = (addr >> 2) & 0x3;
> +
> +	if (kvm_vgic_global_state.type == VGIC_V2) {
> +		/* GICv2 hardware systems support max. 32 groups */
> +		if (n != 0)
> +			return 0;
> +		return vcpu->arch.vgic_cpu.vgic_v2.vgic_apr;
> +	} else {
> +		struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +		/* GICv3 only uses ICH_AP1Rn for memory mapped (GICv2) guests */
> +		return vgicv3->vgic_ap0r[n] | vgicv3->vgic_ap1r[n];


Put aside the discussion of the abstract layer,  same to Marc's proposed:
Avoid to allow userspace to save/restore undefined APR register,  that doesn't
make sense.

Thanks.

> +	}
> +}
> +
> +static void vgic_mmio_write_apr(struct kvm_vcpu *vcpu,
> +				gpa_t addr, unsigned int len,
> +				unsigned long val)
> +{
> +	int n; /* which APRn is this */
> +
> +	n = (addr >> 2) & 0x3;
> +
> +	if (kvm_vgic_global_state.type == VGIC_V2) {
> +		/* GICv2 hardware systems support max. 32 groups */
> +		if (n != 0)
> +			return;
> +		vcpu->arch.vgic_cpu.vgic_v2.vgic_apr = val;
> +	} else {
> +		struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +		/* GICv3 only uses ICH_AP1Rn for memory mapped (GICv2) guests */
> +		vgicv3->vgic_ap1r[n] = val;
> +	}
> +}
> +
>  static const struct vgic_register_region vgic_v2_dist_registers[] = {
>  	REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL,
>  		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
> @@ -364,7 +403,7 @@ static const struct vgic_register_region vgic_v2_cpu_registers[] = {
>  		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_LENGTH(GIC_CPU_ACTIVEPRIO,
> -		vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
> +		vgic_mmio_read_apr, vgic_mmio_write_apr, 16,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_LENGTH(GIC_CPU_IDENT,
>  		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
> 
> 
> Thanks,
> -Christoffer
> 
> .
> 

Re: [PATCH v3 0/4] kvm: arm/arm64: vgic: APRn uaccess support.

[Christoffer Dall]

On Fri, Sep 01, 2017 at 12:10:59PM +0800, wanghaibin wrote:
> On 2017/9/1 4:33, Christoffer Dall wrote:
> 
> > Hi Wanghaibin,
> > 
> > On Wed, Aug 23, 2017 at 09:05:22AM +0800, wanghaibin wrote:
> >> v3: Coding style fix.
> >>     Add the valid APRn access check which Marc proposed. 
> >>
> >> v2: Split the patch again to make it easier for review
> >>     some fixes were proposed by Marc
> > 
> > Usually we put the changelog at the end of the description, before the
> > diff state.  I suggest you have a look at other patch series on the
> > kvmarm list or on the ARM linux mailing list and see how most people do
> > it.
> 
> 
> OK, Pay attention next time.
> 
> Thanks.
> 
> > 
> >>
> >> v1: the problem describe:
> >> In the case (GICv2 on GICv3 migration), I did the test on my board as follow:
> >> vm boot => migrate to destination => shutdown at destination => start at destination 
> >> => migrate back to source ... go round and begin again;
> >>
> >> I tested many times, but unlucky, there maybe failed by accident when shutdown the vm 
> >> at destination. (GICv3 on GICv3 migration, 1000+ times, That's OK).
> >> while failed,  we can watch the interrupts in the vm, some vcpus of the vm can not 
> >> receive the virt timer interrupt. And, these vcpus will 100% with top tool at host.
> >>
> >> vgic_state debug file at destination shows(a active virt timer interrupt) that:
> >> VCPU 0 TYP   ID TGT_ID PLAEHC     HWID   TARGET SRC PRI VCPU_ID
> >> ---------------------------------------------------------------
> >>             ....................
> >>        PPI   25      0 000001        0        1   0 160      -1
> >>        PPI   26      0 000001        0        1   0 160      -1
> >>        PPI   27      0 011111       27        1   0 160       0
> >>
> >>
> >> I added log to print ICH* registers for VCPU0 at destination:
> >> **HCR:0x1, VMCR:0xf0ec0001,SRE:0x0, ELRSR:0xe**
> >> -----AP0R:0: 0x0------
> >> -----AP0R:1: 0x0------
> >> -----AP0R:2: 0x0------
> >> -----AP0R:3: 0x0------
> >> -----AP1R:0: 0x0------
> >> -----AP1R:1: 0x0------
> >> -----AP1R:2: 0x0------
> >> -----AP1R:3: 0x0------
> >> -----LR:0: 0xa0a0001b0000001b------
> >> -----LR:1: 0x0------
> >> -----LR:2: 0x0------
> >> -----LR:3: 0x0------
> >>
> >> and the ICH_AP1R0 value is 0x10000 at source.
> >>
> >> At present, QEMU have supproted GICC_APRn put/set interface for emulated GICv2,
> >> and kvm does not support the uaccess interface. This patchset try to support this.
> >>
> > 
> > So I feel like this series is horribly complicated for what it does, and
> > does things in the reverse order.  If you really want to take your
> > appraoch, it would be much nicer if you first changed existing functions
> > and added infrastructure, and then in the end wired it up to the user
> > space ABI.  In other words, reverse your patches.
> 
> 
> No problem. The patch order can be adjusted if you feel necessary (this
> depends on the results of the simpler patch discussion).
> 
> > 
> > However, I think I have a simpler approach to solving this.  Please have
> > a look at this:
> > 
> > commit 1d49c5ef047a2218379aa170d9a3bdd39cd70e3a (HEAD -> gicv2-apr-uaccess)
> > Author: Christoffer Dall <cdall@linaro.org>
> > Date:   Thu Aug 31 22:24:25 2017 +0200
> > 
> >     KVM: arm/arm64: Support uaccess of GICC_APRn
> >     
> >     When migrating guests around we need to know the active priorities to
> >     ensure functional virtual interrupt prioritization by the GIC.
> >     
> >     This commit clarifies the API and how active priorities of interrupts in
> >     different groups are represented, and implements the accessor functions
> >     for the uaccess register range.
> >     
> >     We live with a slight layering violation in accessing GICv3 data
> >     structures from vgic-mmio-v2.c, because anything else just adds too much
> >     complexity for us to deal with (it's not like there's a benefit
> >     elsewhere in the code of an intermediate representation as is the case
> >     with the VMCR).  We accept this, because while doing v3 processing from
> >     a file named something-v2.c can look strange at first, this really is
> >     specific to dealing with the user space interface for something that
> >     looks like a GICv2.
> >
> 
> 
> I have different opinions here.
> 
> Form Marc's proposed, I guess we take the vgic-v2.c/ vgic-v3.c as the hardware abstraction
> layer for GICH_* / ICH_* registers. These files provide a series of methods to interactive with
> registers (not only vgic_vmcr, such as vgic_hcr, vgic_elrsr, vgic_lr), these registers only can
> be changed by the provided methods (eq: vgic_v2/v3_fold/populate_lr_state,  vgic_v2/v3_set_underflow)
> and finally, these methods are invoking by the uniform interface (vgic_set_underflow,
> vgic_populate/fold_lr, these uniform interfaces invoked the corresponding method through HW probed info)
> 
> In a word, I think we should design the high cohesion and low coupling code, and we've been doing this too.
> We should strictly restrict other modules or files to know or access the vgic_v2/v3_cpu_if or
> its registers (Unfortunately, vgic_sre and vgic_aprn are breaking the rules currently, the patch 4 fix
> vgicv3 sys access, and this patchset design followed this rule).
> 
> My understanding is as mentioned above, maybe not correct.
> 

There are no strict layering definitions, exported ABIs or even APIs
here.  We split things into multiple files to make the code readable and
easy to maintain and understand.

Comparing the diff state (6 files, 126 inserts, 22 deletes in your
version, 2 files, 45 inserts, 1 delete in my version) speaks to some of
the completexity of the two approaches.

The biggest problem is that it takes me hours to understand your patch
series, which indicates that it's over-designed or fails to convery the
necessity of the complication.

>      
> 
> 
> Put aside the discussion of the abstract layer,  same to Marc's proposed:
> Avoid to allow userspace to save/restore undefined APR register,  that doesn't
> make sense.
> 

I don't see it as a big problem; they will have RAZ/WI semantics towards
the VM, similar to hardware.  The only downside is that you can write
something into registers that are not used to the VM and read back that
value, when accessing from userspace.  However, if we really want to do
this, I'd argue this is much simpler:

commit 5cd35287d086c99859900a3d7630a12c9d549fad
Author: Christoffer Dall <cdall@linaro.org>
Date:   Fri Sep 1 11:41:52 2017 +0200

    KVM: arm/arm64: Extract GICv3 max APRn index calculation
    
    As we are about to access the APRs from the GICv2 uaccess interface,
    make this logic generally available.
    
    Signed-off-by: Christoffer Dall <cdall@linaro.org>

diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c
index 116786d..c77d508 100644
--- a/arch/arm64/kvm/vgic-sys-reg-v3.c
+++ b/arch/arm64/kvm/vgic-sys-reg-v3.c
@@ -208,29 +208,12 @@ static void vgic_v3_access_apr_reg(struct kvm_vcpu *vcpu,
 static bool access_gic_aprn(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 			    const struct sys_reg_desc *r, u8 apr)
 {
-	struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
 	u8 idx = r->Op2 & 3;
 
-	/*
-	 * num_pri_bits are initialized with HW supported values.
-	 * We can rely safely on num_pri_bits even if VM has not
-	 * restored ICC_CTLR_EL1 before restoring APnR registers.
-	 */
-	switch (vgic_v3_cpu->num_pri_bits) {
-	case 7:
-		vgic_v3_access_apr_reg(vcpu, p, apr, idx);
-		break;
-	case 6:
-		if (idx > 1)
-			goto err;
-		vgic_v3_access_apr_reg(vcpu, p, apr, idx);
-		break;
-	default:
-		if (idx > 0)
-			goto err;
-		vgic_v3_access_apr_reg(vcpu, p, apr, idx);
-	}
+	if (idx > vgic_v3_max_apr_idx(vcpu))
+		goto err;
 
+	vgic_v3_access_apr_reg(vcpu, p, apr, idx);
 	return true;
 err:
 	if (!p->is_write)
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index bba7fa2..bf9ceab 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -220,4 +220,20 @@ int vgic_debug_destroy(struct kvm *kvm);
 bool lock_all_vcpus(struct kvm *kvm);
 void unlock_all_vcpus(struct kvm *kvm);
 
+static inline int vgic_v3_max_apr_idx(struct kvm_vcpu *vcpu)
+{
+	struct vgic_cpu *cpu_if = &vcpu->arch.vgic_cpu;
+
+	/*
+	 * num_pri_bits are initialized with HW supported values.
+	 * We can rely safely on num_pri_bits even if VM has not
+	 * restored ICC_CTLR_EL1 before restoring APnR registers.
+	 */
+	switch (cpu_if->num_pri_bits) {
+	case 7: return 3;
+	case 6: return 1;
+	default: return 0;
+	}
+}
+
 #endif



commit 660efe48259ebe83e6f93cba0a184327cebf8e82 (HEAD -> gicv2-apr-uaccess)
Author: Christoffer Dall <cdall@linaro.org>
Date:   Thu Aug 31 22:24:25 2017 +0200

    KVM: arm/arm64: Support uaccess of GICC_APRn
    
    When migrating guests around we need to know the active priorities to
    ensure functional virtual interrupt prioritization by the GIC.
    
    This commit clarifies the API and how active priorities of interrupts in
    different groups are represented, and implements the accessor functions
    for the uaccess register range.
    
    We live with a slight layering violation in accessing GICv3 data
    structures from vgic-mmio-v2.c, because anything else just adds too much
    complexity for us to deal with (it's not like there's a benefit
    elsewhere in the code of an intermediate representation as is the case
    with the VMCR).  We accept this, because while doing v3 processing from
    a file named something-v2.c can look strange at first, this really is
    specific to dealing with the user space interface for something that
    looks like a GICv2.
    
    Signed-off-by: Christoffer Dall <cdall@linaro.org>

diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
index b2f60ca..b3ce126 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
@@ -83,6 +83,11 @@ Groups:
 
     Bits for undefined preemption levels are RAZ/WI.
 
+    Note that this differs from a CPU's view of the APRs on hardware in which
+    a GIC without the security extensions expose group 0 and group 1 active
+    priorities in separate register groups, whereas we show a combined view
+    similar to GICv2's GICH_APR.
+
     For historical reasons and to provide ABI compatibility with userspace we
     export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask
     field in the lower 5 bits of a word, meaning that userspace must always
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 37522e6..b3d4a10 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -303,6 +303,51 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu,
 	vgic_set_vmcr(vcpu, &vmcr);
 }
 
+static unsigned long vgic_mmio_read_apr(struct kvm_vcpu *vcpu,
+					gpa_t addr, unsigned int len)
+{
+	int n; /* which APRn is this */
+
+	n = (addr >> 2) & 0x3;
+
+	if (kvm_vgic_global_state.type == VGIC_V2) {
+		/* GICv2 hardware systems support max. 32 groups */
+		if (n != 0)
+			return 0;
+		return vcpu->arch.vgic_cpu.vgic_v2.vgic_apr;
+	} else {
+		struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+		if (n > vgic_v3_max_apr_idx(vcpu))
+			return 0;
+		/* GICv3 only uses ICH_AP1Rn for memory mapped (GICv2) guests */
+		return vgicv3->vgic_ap1r[n];
+	}
+}
+
+static void vgic_mmio_write_apr(struct kvm_vcpu *vcpu,
+				gpa_t addr, unsigned int len,
+				unsigned long val)
+{
+	int n; /* which APRn is this */
+
+	n = (addr >> 2) & 0x3;
+
+	if (kvm_vgic_global_state.type == VGIC_V2) {
+		/* GICv2 hardware systems support max. 32 groups */
+		if (n != 0)
+			return;
+		vcpu->arch.vgic_cpu.vgic_v2.vgic_apr = val;
+	} else {
+		struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+		if (n > vgic_v3_max_apr_idx(vcpu))
+			return;
+		/* GICv3 only uses ICH_AP1Rn for memory mapped (GICv2) guests */
+		vgicv3->vgic_ap1r[n] = val;
+	}
+}
+
 static const struct vgic_register_region vgic_v2_dist_registers[] = {
 	REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL,
 		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
@@ -364,7 +409,7 @@ static const struct vgic_register_region vgic_v2_cpu_registers[] = {
 		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH(GIC_CPU_ACTIVEPRIO,
-		vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
+		vgic_mmio_read_apr, vgic_mmio_write_apr, 16,
 		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH(GIC_CPU_IDENT,
 		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,


Thanks,
-Christoffer

Re: [PATCH v3 0/4] kvm: arm/arm64: vgic: APRn uaccess support.

[Marc Zyngier]

On 01/09/17 10:44, Christoffer Dall wrote:
> On Fri, Sep 01, 2017 at 12:10:59PM +0800, wanghaibin wrote:
>> On 2017/9/1 4:33, Christoffer Dall wrote:
>>
>>> Hi Wanghaibin,
>>>
>>> On Wed, Aug 23, 2017 at 09:05:22AM +0800, wanghaibin wrote:
>>>> v3: Coding style fix.
>>>>     Add the valid APRn access check which Marc proposed. 
>>>>
>>>> v2: Split the patch again to make it easier for review
>>>>     some fixes were proposed by Marc
>>>
>>> Usually we put the changelog at the end of the description, before the
>>> diff state.  I suggest you have a look at other patch series on the
>>> kvmarm list or on the ARM linux mailing list and see how most people do
>>> it.
>>
>>
>> OK, Pay attention next time.
>>
>> Thanks.
>>
>>>
>>>>
>>>> v1: the problem describe:
>>>> In the case (GICv2 on GICv3 migration), I did the test on my board as follow:
>>>> vm boot => migrate to destination => shutdown at destination => start at destination 
>>>> => migrate back to source ... go round and begin again;
>>>>
>>>> I tested many times, but unlucky, there maybe failed by accident when shutdown the vm 
>>>> at destination. (GICv3 on GICv3 migration, 1000+ times, That's OK).
>>>> while failed,  we can watch the interrupts in the vm, some vcpus of the vm can not 
>>>> receive the virt timer interrupt. And, these vcpus will 100% with top tool at host.
>>>>
>>>> vgic_state debug file at destination shows(a active virt timer interrupt) that:
>>>> VCPU 0 TYP   ID TGT_ID PLAEHC     HWID   TARGET SRC PRI VCPU_ID
>>>> ---------------------------------------------------------------
>>>>             ....................
>>>>        PPI   25      0 000001        0        1   0 160      -1
>>>>        PPI   26      0 000001        0        1   0 160      -1
>>>>        PPI   27      0 011111       27        1   0 160       0
>>>>
>>>>
>>>> I added log to print ICH* registers for VCPU0 at destination:
>>>> **HCR:0x1, VMCR:0xf0ec0001,SRE:0x0, ELRSR:0xe**
>>>> -----AP0R:0: 0x0------
>>>> -----AP0R:1: 0x0------
>>>> -----AP0R:2: 0x0------
>>>> -----AP0R:3: 0x0------
>>>> -----AP1R:0: 0x0------
>>>> -----AP1R:1: 0x0------
>>>> -----AP1R:2: 0x0------
>>>> -----AP1R:3: 0x0------
>>>> -----LR:0: 0xa0a0001b0000001b------
>>>> -----LR:1: 0x0------
>>>> -----LR:2: 0x0------
>>>> -----LR:3: 0x0------
>>>>
>>>> and the ICH_AP1R0 value is 0x10000 at source.
>>>>
>>>> At present, QEMU have supproted GICC_APRn put/set interface for emulated GICv2,
>>>> and kvm does not support the uaccess interface. This patchset try to support this.
>>>>
>>>
>>> So I feel like this series is horribly complicated for what it does, and
>>> does things in the reverse order.  If you really want to take your
>>> appraoch, it would be much nicer if you first changed existing functions
>>> and added infrastructure, and then in the end wired it up to the user
>>> space ABI.  In other words, reverse your patches.
>>
>>
>> No problem. The patch order can be adjusted if you feel necessary (this
>> depends on the results of the simpler patch discussion).
>>
>>>
>>> However, I think I have a simpler approach to solving this.  Please have
>>> a look at this:
>>>
>>> commit 1d49c5ef047a2218379aa170d9a3bdd39cd70e3a (HEAD -> gicv2-apr-uaccess)
>>> Author: Christoffer Dall <cdall@linaro.org>
>>> Date:   Thu Aug 31 22:24:25 2017 +0200
>>>
>>>     KVM: arm/arm64: Support uaccess of GICC_APRn
>>>     
>>>     When migrating guests around we need to know the active priorities to
>>>     ensure functional virtual interrupt prioritization by the GIC.
>>>     
>>>     This commit clarifies the API and how active priorities of interrupts in
>>>     different groups are represented, and implements the accessor functions
>>>     for the uaccess register range.
>>>     
>>>     We live with a slight layering violation in accessing GICv3 data
>>>     structures from vgic-mmio-v2.c, because anything else just adds too much
>>>     complexity for us to deal with (it's not like there's a benefit
>>>     elsewhere in the code of an intermediate representation as is the case
>>>     with the VMCR).  We accept this, because while doing v3 processing from
>>>     a file named something-v2.c can look strange at first, this really is
>>>     specific to dealing with the user space interface for something that
>>>     looks like a GICv2.
>>>
>>
>>
>> I have different opinions here.
>>
>> Form Marc's proposed, I guess we take the vgic-v2.c/ vgic-v3.c as the hardware abstraction
>> layer for GICH_* / ICH_* registers. These files provide a series of methods to interactive with
>> registers (not only vgic_vmcr, such as vgic_hcr, vgic_elrsr, vgic_lr), these registers only can
>> be changed by the provided methods (eq: vgic_v2/v3_fold/populate_lr_state,  vgic_v2/v3_set_underflow)
>> and finally, these methods are invoking by the uniform interface (vgic_set_underflow,
>> vgic_populate/fold_lr, these uniform interfaces invoked the corresponding method through HW probed info)
>>
>> In a word, I think we should design the high cohesion and low coupling code, and we've been doing this too.
>> We should strictly restrict other modules or files to know or access the vgic_v2/v3_cpu_if or
>> its registers (Unfortunately, vgic_sre and vgic_aprn are breaking the rules currently, the patch 4 fix
>> vgicv3 sys access, and this patchset design followed this rule).
>>
>> My understanding is as mentioned above, maybe not correct.
>>
> 
> There are no strict layering definitions, exported ABIs or even APIs
> here.  We split things into multiple files to make the code readable and
> easy to maintain and understand.
> 
> Comparing the diff state (6 files, 126 inserts, 22 deletes in your
> version, 2 files, 45 inserts, 1 delete in my version) speaks to some of
> the completexity of the two approaches.
> 
> The biggest problem is that it takes me hours to understand your patch
> series, which indicates that it's over-designed or fails to convery the
> necessity of the complication.
> 
>>      
>>
>>
>> Put aside the discussion of the abstract layer,  same to Marc's proposed:
>> Avoid to allow userspace to save/restore undefined APR register,  that doesn't
>> make sense.
>>
> 
> I don't see it as a big problem; they will have RAZ/WI semantics towards
> the VM, similar to hardware.  The only downside is that you can write
> something into registers that are not used to the VM and read back that
> value, when accessing from userspace.  However, if we really want to do
> this, I'd argue this is much simpler:
> 
> commit 5cd35287d086c99859900a3d7630a12c9d549fad
> Author: Christoffer Dall <cdall@linaro.org>
> Date:   Fri Sep 1 11:41:52 2017 +0200
> 
>     KVM: arm/arm64: Extract GICv3 max APRn index calculation
>     
>     As we are about to access the APRs from the GICv2 uaccess interface,
>     make this logic generally available.
>     
>     Signed-off-by: Christoffer Dall <cdall@linaro.org>
> 
> diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c
> index 116786d..c77d508 100644
> --- a/arch/arm64/kvm/vgic-sys-reg-v3.c
> +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c
> @@ -208,29 +208,12 @@ static void vgic_v3_access_apr_reg(struct kvm_vcpu *vcpu,
>  static bool access_gic_aprn(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  			    const struct sys_reg_desc *r, u8 apr)
>  {
> -	struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
>  	u8 idx = r->Op2 & 3;
>  
> -	/*
> -	 * num_pri_bits are initialized with HW supported values.
> -	 * We can rely safely on num_pri_bits even if VM has not
> -	 * restored ICC_CTLR_EL1 before restoring APnR registers.
> -	 */
> -	switch (vgic_v3_cpu->num_pri_bits) {
> -	case 7:
> -		vgic_v3_access_apr_reg(vcpu, p, apr, idx);
> -		break;
> -	case 6:
> -		if (idx > 1)
> -			goto err;
> -		vgic_v3_access_apr_reg(vcpu, p, apr, idx);
> -		break;
> -	default:
> -		if (idx > 0)
> -			goto err;
> -		vgic_v3_access_apr_reg(vcpu, p, apr, idx);
> -	}
> +	if (idx > vgic_v3_max_apr_idx(vcpu))
> +		goto err;
>  
> +	vgic_v3_access_apr_reg(vcpu, p, apr, idx);
>  	return true;
>  err:
>  	if (!p->is_write)
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index bba7fa2..bf9ceab 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -220,4 +220,20 @@ int vgic_debug_destroy(struct kvm *kvm);
>  bool lock_all_vcpus(struct kvm *kvm);
>  void unlock_all_vcpus(struct kvm *kvm);
>  
> +static inline int vgic_v3_max_apr_idx(struct kvm_vcpu *vcpu)
> +{
> +	struct vgic_cpu *cpu_if = &vcpu->arch.vgic_cpu;
> +
> +	/*
> +	 * num_pri_bits are initialized with HW supported values.
> +	 * We can rely safely on num_pri_bits even if VM has not
> +	 * restored ICC_CTLR_EL1 before restoring APnR registers.
> +	 */
> +	switch (cpu_if->num_pri_bits) {
> +	case 7: return 3;
> +	case 6: return 1;
> +	default: return 0;
> +	}
> +}
> +
>  #endif
> 
> 
> 
> commit 660efe48259ebe83e6f93cba0a184327cebf8e82 (HEAD -> gicv2-apr-uaccess)
> Author: Christoffer Dall <cdall@linaro.org>
> Date:   Thu Aug 31 22:24:25 2017 +0200
> 
>     KVM: arm/arm64: Support uaccess of GICC_APRn
>     
>     When migrating guests around we need to know the active priorities to
>     ensure functional virtual interrupt prioritization by the GIC.
>     
>     This commit clarifies the API and how active priorities of interrupts in
>     different groups are represented, and implements the accessor functions
>     for the uaccess register range.
>     
>     We live with a slight layering violation in accessing GICv3 data
>     structures from vgic-mmio-v2.c, because anything else just adds too much
>     complexity for us to deal with (it's not like there's a benefit
>     elsewhere in the code of an intermediate representation as is the case
>     with the VMCR).  We accept this, because while doing v3 processing from
>     a file named something-v2.c can look strange at first, this really is
>     specific to dealing with the user space interface for something that
>     looks like a GICv2.
>     
>     Signed-off-by: Christoffer Dall <cdall@linaro.org>
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> index b2f60ca..b3ce126 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> @@ -83,6 +83,11 @@ Groups:
>  
>      Bits for undefined preemption levels are RAZ/WI.
>  
> +    Note that this differs from a CPU's view of the APRs on hardware in which
> +    a GIC without the security extensions expose group 0 and group 1 active
> +    priorities in separate register groups, whereas we show a combined view
> +    similar to GICv2's GICH_APR.
> +
>      For historical reasons and to provide ABI compatibility with userspace we
>      export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask
>      field in the lower 5 bits of a word, meaning that userspace must always
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 37522e6..b3d4a10 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -303,6 +303,51 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu,
>  	vgic_set_vmcr(vcpu, &vmcr);
>  }
>  
> +static unsigned long vgic_mmio_read_apr(struct kvm_vcpu *vcpu,
> +					gpa_t addr, unsigned int len)
> +{
> +	int n; /* which APRn is this */
> +
> +	n = (addr >> 2) & 0x3;
> +
> +	if (kvm_vgic_global_state.type == VGIC_V2) {
> +		/* GICv2 hardware systems support max. 32 groups */
> +		if (n != 0)
> +			return 0;
> +		return vcpu->arch.vgic_cpu.vgic_v2.vgic_apr;
> +	} else {
> +		struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> +		if (n > vgic_v3_max_apr_idx(vcpu))
> +			return 0;
> +		/* GICv3 only uses ICH_AP1Rn for memory mapped (GICv2) guests */
> +		return vgicv3->vgic_ap1r[n];
> +	}
> +}
> +
> +static void vgic_mmio_write_apr(struct kvm_vcpu *vcpu,
> +				gpa_t addr, unsigned int len,
> +				unsigned long val)
> +{
> +	int n; /* which APRn is this */
> +
> +	n = (addr >> 2) & 0x3;
> +
> +	if (kvm_vgic_global_state.type == VGIC_V2) {
> +		/* GICv2 hardware systems support max. 32 groups */
> +		if (n != 0)
> +			return;
> +		vcpu->arch.vgic_cpu.vgic_v2.vgic_apr = val;
> +	} else {
> +		struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> +		if (n > vgic_v3_max_apr_idx(vcpu))
> +			return;
> +		/* GICv3 only uses ICH_AP1Rn for memory mapped (GICv2) guests */
> +		vgicv3->vgic_ap1r[n] = val;
> +	}
> +}
> +
>  static const struct vgic_register_region vgic_v2_dist_registers[] = {
>  	REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL,
>  		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
> @@ -364,7 +409,7 @@ static const struct vgic_register_region vgic_v2_cpu_registers[] = {
>  		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_LENGTH(GIC_CPU_ACTIVEPRIO,
> -		vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
> +		vgic_mmio_read_apr, vgic_mmio_write_apr, 16,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_LENGTH(GIC_CPU_IDENT,
>  		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
> 
For the record, I find this last solution quite elegant. Yes, the slight
violation of the v2/v3 separation is a bit annoying, but I can
understand this code in about 30 seconds as opposed to 30 minutes for
the original series.

So for these two patches:

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

Wanghaibin: Could you please give it a go and let us know if this solves
the issue for your particular test case?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 0/4] kvm: arm/arm64: vgic: APRn uaccess support.

[Christoffer Dall]

Hi Wanghaibin,

On Fri, Sep 01, 2017 at 11:44:38AM +0200, Christoffer Dall wrote:
> On Fri, Sep 01, 2017 at 12:10:59PM +0800, wanghaibin wrote:
> > On 2017/9/1 4:33, Christoffer Dall wrote:
> > 
> > > Hi Wanghaibin,
> > > 
> > > On Wed, Aug 23, 2017 at 09:05:22AM +0800, wanghaibin wrote:
> > >> v3: Coding style fix.
> > >>     Add the valid APRn access check which Marc proposed. 
> > >>
> > >> v2: Split the patch again to make it easier for review
> > >>     some fixes were proposed by Marc
> > > 
> > > Usually we put the changelog at the end of the description, before the
> > > diff state.  I suggest you have a look at other patch series on the
> > > kvmarm list or on the ARM linux mailing list and see how most people do
> > > it.
> > 
> > 
> > OK, Pay attention next time.
> > 
> > Thanks.
> > 
> > > 
> > >>
> > >> v1: the problem describe:
> > >> In the case (GICv2 on GICv3 migration), I did the test on my board as follow:
> > >> vm boot => migrate to destination => shutdown at destination => start at destination 
> > >> => migrate back to source ... go round and begin again;
> > >>
> > >> I tested many times, but unlucky, there maybe failed by accident when shutdown the vm 
> > >> at destination. (GICv3 on GICv3 migration, 1000+ times, That's OK).
> > >> while failed,  we can watch the interrupts in the vm, some vcpus of the vm can not 
> > >> receive the virt timer interrupt. And, these vcpus will 100% with top tool at host.
> > >>
> > >> vgic_state debug file at destination shows(a active virt timer interrupt) that:
> > >> VCPU 0 TYP   ID TGT_ID PLAEHC     HWID   TARGET SRC PRI VCPU_ID
> > >> ---------------------------------------------------------------
> > >>             ....................
> > >>        PPI   25      0 000001        0        1   0 160      -1
> > >>        PPI   26      0 000001        0        1   0 160      -1
> > >>        PPI   27      0 011111       27        1   0 160       0
> > >>
> > >>
> > >> I added log to print ICH* registers for VCPU0 at destination:
> > >> **HCR:0x1, VMCR:0xf0ec0001,SRE:0x0, ELRSR:0xe**
> > >> -----AP0R:0: 0x0------
> > >> -----AP0R:1: 0x0------
> > >> -----AP0R:2: 0x0------
> > >> -----AP0R:3: 0x0------
> > >> -----AP1R:0: 0x0------
> > >> -----AP1R:1: 0x0------
> > >> -----AP1R:2: 0x0------
> > >> -----AP1R:3: 0x0------
> > >> -----LR:0: 0xa0a0001b0000001b------
> > >> -----LR:1: 0x0------
> > >> -----LR:2: 0x0------
> > >> -----LR:3: 0x0------
> > >>
> > >> and the ICH_AP1R0 value is 0x10000 at source.
> > >>
> > >> At present, QEMU have supproted GICC_APRn put/set interface for emulated GICv2,
> > >> and kvm does not support the uaccess interface. This patchset try to support this.
> > >>
> > > 
> > > So I feel like this series is horribly complicated for what it does, and
> > > does things in the reverse order.  If you really want to take your
> > > appraoch, it would be much nicer if you first changed existing functions
> > > and added infrastructure, and then in the end wired it up to the user
> > > space ABI.  In other words, reverse your patches.
> > 
> > 
> > No problem. The patch order can be adjusted if you feel necessary (this
> > depends on the results of the simpler patch discussion).
> > 
> > > 
> > > However, I think I have a simpler approach to solving this.  Please have
> > > a look at this:
> > > 
> > > commit 1d49c5ef047a2218379aa170d9a3bdd39cd70e3a (HEAD -> gicv2-apr-uaccess)
> > > Author: Christoffer Dall <cdall@linaro.org>
> > > Date:   Thu Aug 31 22:24:25 2017 +0200
> > > 
> > >     KVM: arm/arm64: Support uaccess of GICC_APRn
> > >     
> > >     When migrating guests around we need to know the active priorities to
> > >     ensure functional virtual interrupt prioritization by the GIC.
> > >     
> > >     This commit clarifies the API and how active priorities of interrupts in
> > >     different groups are represented, and implements the accessor functions
> > >     for the uaccess register range.
> > >     
> > >     We live with a slight layering violation in accessing GICv3 data
> > >     structures from vgic-mmio-v2.c, because anything else just adds too much
> > >     complexity for us to deal with (it's not like there's a benefit
> > >     elsewhere in the code of an intermediate representation as is the case
> > >     with the VMCR).  We accept this, because while doing v3 processing from
> > >     a file named something-v2.c can look strange at first, this really is
> > >     specific to dealing with the user space interface for something that
> > >     looks like a GICv2.
> > >
> > 
> > 
> > I have different opinions here.
> > 
> > Form Marc's proposed, I guess we take the vgic-v2.c/ vgic-v3.c as the hardware abstraction
> > layer for GICH_* / ICH_* registers. These files provide a series of methods to interactive with
> > registers (not only vgic_vmcr, such as vgic_hcr, vgic_elrsr, vgic_lr), these registers only can
> > be changed by the provided methods (eq: vgic_v2/v3_fold/populate_lr_state,  vgic_v2/v3_set_underflow)
> > and finally, these methods are invoking by the uniform interface (vgic_set_underflow,
> > vgic_populate/fold_lr, these uniform interfaces invoked the corresponding method through HW probed info)
> > 
> > In a word, I think we should design the high cohesion and low coupling code, and we've been doing this too.
> > We should strictly restrict other modules or files to know or access the vgic_v2/v3_cpu_if or
> > its registers (Unfortunately, vgic_sre and vgic_aprn are breaking the rules currently, the patch 4 fix
> > vgicv3 sys access, and this patchset design followed this rule).
> > 
> > My understanding is as mentioned above, maybe not correct.
> > 
> 
> There are no strict layering definitions, exported ABIs or even APIs
> here.  We split things into multiple files to make the code readable and
> easy to maintain and understand.
> 
> Comparing the diff state (6 files, 126 inserts, 22 deletes in your
> version, 2 files, 45 inserts, 1 delete in my version) speaks to some of
> the completexity of the two approaches.
> 
> The biggest problem is that it takes me hours to understand your patch
> series, which indicates that it's over-designed or fails to convery the
> necessity of the complication.
> 
> >      
> > 
> > 
> > Put aside the discussion of the abstract layer,  same to Marc's proposed:
> > Avoid to allow userspace to save/restore undefined APR register,  that doesn't
> > make sense.
> > 
> 
> I don't see it as a big problem; they will have RAZ/WI semantics towards
> the VM, similar to hardware.  The only downside is that you can write
> something into registers that are not used to the VM and read back that
> value, when accessing from userspace.  However, if we really want to do
> this, I'd argue this is much simpler:
> 
> commit 5cd35287d086c99859900a3d7630a12c9d549fad
> Author: Christoffer Dall <cdall@linaro.org>
> Date:   Fri Sep 1 11:41:52 2017 +0200
> 
>     KVM: arm/arm64: Extract GICv3 max APRn index calculation
>     
>     As we are about to access the APRs from the GICv2 uaccess interface,
>     make this logic generally available.
>     
>     Signed-off-by: Christoffer Dall <cdall@linaro.org>
> 
> diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c
> index 116786d..c77d508 100644
> --- a/arch/arm64/kvm/vgic-sys-reg-v3.c
> +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c
> @@ -208,29 +208,12 @@ static void vgic_v3_access_apr_reg(struct kvm_vcpu *vcpu,
>  static bool access_gic_aprn(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  			    const struct sys_reg_desc *r, u8 apr)
>  {
> -	struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
>  	u8 idx = r->Op2 & 3;
>  
> -	/*
> -	 * num_pri_bits are initialized with HW supported values.
> -	 * We can rely safely on num_pri_bits even if VM has not
> -	 * restored ICC_CTLR_EL1 before restoring APnR registers.
> -	 */
> -	switch (vgic_v3_cpu->num_pri_bits) {
> -	case 7:
> -		vgic_v3_access_apr_reg(vcpu, p, apr, idx);
> -		break;
> -	case 6:
> -		if (idx > 1)
> -			goto err;
> -		vgic_v3_access_apr_reg(vcpu, p, apr, idx);
> -		break;
> -	default:
> -		if (idx > 0)
> -			goto err;
> -		vgic_v3_access_apr_reg(vcpu, p, apr, idx);
> -	}
> +	if (idx > vgic_v3_max_apr_idx(vcpu))
> +		goto err;
>  
> +	vgic_v3_access_apr_reg(vcpu, p, apr, idx);
>  	return true;
>  err:
>  	if (!p->is_write)
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index bba7fa2..bf9ceab 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -220,4 +220,20 @@ int vgic_debug_destroy(struct kvm *kvm);
>  bool lock_all_vcpus(struct kvm *kvm);
>  void unlock_all_vcpus(struct kvm *kvm);
>  
> +static inline int vgic_v3_max_apr_idx(struct kvm_vcpu *vcpu)
> +{
> +	struct vgic_cpu *cpu_if = &vcpu->arch.vgic_cpu;
> +
> +	/*
> +	 * num_pri_bits are initialized with HW supported values.
> +	 * We can rely safely on num_pri_bits even if VM has not
> +	 * restored ICC_CTLR_EL1 before restoring APnR registers.
> +	 */
> +	switch (cpu_if->num_pri_bits) {
> +	case 7: return 3;
> +	case 6: return 1;
> +	default: return 0;
> +	}
> +}
> +
>  #endif
> 
> 
> 
> commit 660efe48259ebe83e6f93cba0a184327cebf8e82 (HEAD -> gicv2-apr-uaccess)
> Author: Christoffer Dall <cdall@linaro.org>
> Date:   Thu Aug 31 22:24:25 2017 +0200
> 
>     KVM: arm/arm64: Support uaccess of GICC_APRn
>     
>     When migrating guests around we need to know the active priorities to
>     ensure functional virtual interrupt prioritization by the GIC.
>     
>     This commit clarifies the API and how active priorities of interrupts in
>     different groups are represented, and implements the accessor functions
>     for the uaccess register range.
>     
>     We live with a slight layering violation in accessing GICv3 data
>     structures from vgic-mmio-v2.c, because anything else just adds too much
>     complexity for us to deal with (it's not like there's a benefit
>     elsewhere in the code of an intermediate representation as is the case
>     with the VMCR).  We accept this, because while doing v3 processing from
>     a file named something-v2.c can look strange at first, this really is
>     specific to dealing with the user space interface for something that
>     looks like a GICv2.
>     
>     Signed-off-by: Christoffer Dall <cdall@linaro.org>
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> index b2f60ca..b3ce126 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> @@ -83,6 +83,11 @@ Groups:
>  
>      Bits for undefined preemption levels are RAZ/WI.
>  
> +    Note that this differs from a CPU's view of the APRs on hardware in which
> +    a GIC without the security extensions expose group 0 and group 1 active
> +    priorities in separate register groups, whereas we show a combined view
> +    similar to GICv2's GICH_APR.
> +
>      For historical reasons and to provide ABI compatibility with userspace we
>      export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask
>      field in the lower 5 bits of a word, meaning that userspace must always
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 37522e6..b3d4a10 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -303,6 +303,51 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu,
>  	vgic_set_vmcr(vcpu, &vmcr);
>  }
>  
> +static unsigned long vgic_mmio_read_apr(struct kvm_vcpu *vcpu,
> +					gpa_t addr, unsigned int len)
> +{
> +	int n; /* which APRn is this */
> +
> +	n = (addr >> 2) & 0x3;
> +
> +	if (kvm_vgic_global_state.type == VGIC_V2) {
> +		/* GICv2 hardware systems support max. 32 groups */
> +		if (n != 0)
> +			return 0;
> +		return vcpu->arch.vgic_cpu.vgic_v2.vgic_apr;
> +	} else {
> +		struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> +		if (n > vgic_v3_max_apr_idx(vcpu))
> +			return 0;
> +		/* GICv3 only uses ICH_AP1Rn for memory mapped (GICv2) guests */
> +		return vgicv3->vgic_ap1r[n];
> +	}
> +}
> +
> +static void vgic_mmio_write_apr(struct kvm_vcpu *vcpu,
> +				gpa_t addr, unsigned int len,
> +				unsigned long val)
> +{
> +	int n; /* which APRn is this */
> +
> +	n = (addr >> 2) & 0x3;
> +
> +	if (kvm_vgic_global_state.type == VGIC_V2) {
> +		/* GICv2 hardware systems support max. 32 groups */
> +		if (n != 0)
> +			return;
> +		vcpu->arch.vgic_cpu.vgic_v2.vgic_apr = val;
> +	} else {
> +		struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> +		if (n > vgic_v3_max_apr_idx(vcpu))
> +			return;
> +		/* GICv3 only uses ICH_AP1Rn for memory mapped (GICv2) guests */
> +		vgicv3->vgic_ap1r[n] = val;
> +	}
> +}
> +
>  static const struct vgic_register_region vgic_v2_dist_registers[] = {
>  	REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL,
>  		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
> @@ -364,7 +409,7 @@ static const struct vgic_register_region vgic_v2_cpu_registers[] = {
>  		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_LENGTH(GIC_CPU_ACTIVEPRIO,
> -		vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
> +		vgic_mmio_read_apr, vgic_mmio_write_apr, 16,
>  		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_LENGTH(GIC_CPU_IDENT,
>  		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
> 
> 

I'd appreciate a tested-by on these patches with your configuration.

Thanks,
-Christoffer

Re: [PATCH v3 0/4] kvm: arm/arm64: vgic: APRn uaccess support.

[wanghaibin]

On 2017/9/5 1:27, Christoffer Dall wrote:

> Hi Wanghaibin,
> 
> On Fri, Sep 01, 2017 at 11:44:38AM +0200, Christoffer Dall wrote:
>> On Fri, Sep 01, 2017 at 12:10:59PM +0800, wanghaibin wrote:
>>> On 2017/9/1 4:33, Christoffer Dall wrote:
>>>
>>>> Hi Wanghaibin,
>>>>
>>>> On Wed, Aug 23, 2017 at 09:05:22AM +0800, wanghaibin wrote:
>>>>> v3: Coding style fix.
>>>>>     Add the valid APRn access check which Marc proposed. 
>>>>>
>>>>> v2: Split the patch again to make it easier for review
>>>>>     some fixes were proposed by Marc
>>>>
>>>> Usually we put the changelog at the end of the description, before the
>>>> diff state.  I suggest you have a look at other patch series on the
>>>> kvmarm list or on the ARM linux mailing list and see how most people do
>>>> it.
>>>
>>>
>>> OK, Pay attention next time.
>>>
>>> Thanks.
>>>
>>>>
>>>>>
>>>>> v1: the problem describe:
>>>>> In the case (GICv2 on GICv3 migration), I did the test on my board as follow:
>>>>> vm boot => migrate to destination => shutdown at destination => start at destination 
>>>>> => migrate back to source ... go round and begin again;
>>>>>
>>>>> I tested many times, but unlucky, there maybe failed by accident when shutdown the vm 
>>>>> at destination. (GICv3 on GICv3 migration, 1000+ times, That's OK).
>>>>> while failed,  we can watch the interrupts in the vm, some vcpus of the vm can not 
>>>>> receive the virt timer interrupt. And, these vcpus will 100% with top tool at host.
>>>>>
>>>>> vgic_state debug file at destination shows(a active virt timer interrupt) that:
>>>>> VCPU 0 TYP   ID TGT_ID PLAEHC     HWID   TARGET SRC PRI VCPU_ID
>>>>> ---------------------------------------------------------------
>>>>>             ....................
>>>>>        PPI   25      0 000001        0        1   0 160      -1
>>>>>        PPI   26      0 000001        0        1   0 160      -1
>>>>>        PPI   27      0 011111       27        1   0 160       0
>>>>>
>>>>>
>>>>> I added log to print ICH* registers for VCPU0 at destination:
>>>>> **HCR:0x1, VMCR:0xf0ec0001,SRE:0x0, ELRSR:0xe**
>>>>> -----AP0R:0: 0x0------
>>>>> -----AP0R:1: 0x0------
>>>>> -----AP0R:2: 0x0------
>>>>> -----AP0R:3: 0x0------
>>>>> -----AP1R:0: 0x0------
>>>>> -----AP1R:1: 0x0------
>>>>> -----AP1R:2: 0x0------
>>>>> -----AP1R:3: 0x0------
>>>>> -----LR:0: 0xa0a0001b0000001b------
>>>>> -----LR:1: 0x0------
>>>>> -----LR:2: 0x0------
>>>>> -----LR:3: 0x0------
>>>>>
>>>>> and the ICH_AP1R0 value is 0x10000 at source.
>>>>>
>>>>> At present, QEMU have supproted GICC_APRn put/set interface for emulated GICv2,
>>>>> and kvm does not support the uaccess interface. This patchset try to support this.
>>>>>
>>>>
>>>> So I feel like this series is horribly complicated for what it does, and
>>>> does things in the reverse order.  If you really want to take your
>>>> appraoch, it would be much nicer if you first changed existing functions
>>>> and added infrastructure, and then in the end wired it up to the user
>>>> space ABI.  In other words, reverse your patches.
>>>
>>>
>>> No problem. The patch order can be adjusted if you feel necessary (this
>>> depends on the results of the simpler patch discussion).
>>>
>>>>
>>>> However, I think I have a simpler approach to solving this.  Please have
>>>> a look at this:
>>>>
>>>> commit 1d49c5ef047a2218379aa170d9a3bdd39cd70e3a (HEAD -> gicv2-apr-uaccess)
>>>> Author: Christoffer Dall <cdall@linaro.org>
>>>> Date:   Thu Aug 31 22:24:25 2017 +0200
>>>>
>>>>     KVM: arm/arm64: Support uaccess of GICC_APRn
>>>>     
>>>>     When migrating guests around we need to know the active priorities to
>>>>     ensure functional virtual interrupt prioritization by the GIC.
>>>>     
>>>>     This commit clarifies the API and how active priorities of interrupts in
>>>>     different groups are represented, and implements the accessor functions
>>>>     for the uaccess register range.
>>>>     
>>>>     We live with a slight layering violation in accessing GICv3 data
>>>>     structures from vgic-mmio-v2.c, because anything else just adds too much
>>>>     complexity for us to deal with (it's not like there's a benefit
>>>>     elsewhere in the code of an intermediate representation as is the case
>>>>     with the VMCR).  We accept this, because while doing v3 processing from
>>>>     a file named something-v2.c can look strange at first, this really is
>>>>     specific to dealing with the user space interface for something that
>>>>     looks like a GICv2.
>>>>
>>>
>>>
>>> I have different opinions here.
>>>
>>> Form Marc's proposed, I guess we take the vgic-v2.c/ vgic-v3.c as the hardware abstraction
>>> layer for GICH_* / ICH_* registers. These files provide a series of methods to interactive with
>>> registers (not only vgic_vmcr, such as vgic_hcr, vgic_elrsr, vgic_lr), these registers only can
>>> be changed by the provided methods (eq: vgic_v2/v3_fold/populate_lr_state,  vgic_v2/v3_set_underflow)
>>> and finally, these methods are invoking by the uniform interface (vgic_set_underflow,
>>> vgic_populate/fold_lr, these uniform interfaces invoked the corresponding method through HW probed info)
>>>
>>> In a word, I think we should design the high cohesion and low coupling code, and we've been doing this too.
>>> We should strictly restrict other modules or files to know or access the vgic_v2/v3_cpu_if or
>>> its registers (Unfortunately, vgic_sre and vgic_aprn are breaking the rules currently, the patch 4 fix
>>> vgicv3 sys access, and this patchset design followed this rule).
>>>
>>> My understanding is as mentioned above, maybe not correct.
>>>
>>
>> There are no strict layering definitions, exported ABIs or even APIs
>> here.  We split things into multiple files to make the code readable and
>> easy to maintain and understand.
>>
>> Comparing the diff state (6 files, 126 inserts, 22 deletes in your
>> version, 2 files, 45 inserts, 1 delete in my version) speaks to some of
>> the completexity of the two approaches.
>>
>> The biggest problem is that it takes me hours to understand your patch
>> series, which indicates that it's over-designed or fails to convery the
>> necessity of the complication.
>>
>>>      
>>>
>>>
>>> Put aside the discussion of the abstract layer,  same to Marc's proposed:
>>> Avoid to allow userspace to save/restore undefined APR register,  that doesn't
>>> make sense.
>>>
>>
>> I don't see it as a big problem; they will have RAZ/WI semantics towards
>> the VM, similar to hardware.  The only downside is that you can write
>> something into registers that are not used to the VM and read back that
>> value, when accessing from userspace.  However, if we really want to do
>> this, I'd argue this is much simpler:
>>
>> commit 5cd35287d086c99859900a3d7630a12c9d549fad
>> Author: Christoffer Dall <cdall@linaro.org>
>> Date:   Fri Sep 1 11:41:52 2017 +0200
>>
>>     KVM: arm/arm64: Extract GICv3 max APRn index calculation
>>     
>>     As we are about to access the APRs from the GICv2 uaccess interface,
>>     make this logic generally available.
>>     
>>     Signed-off-by: Christoffer Dall <cdall@linaro.org>
>>
>> diff --git a/arch/arm64/kvm/vgic-sys-reg-v3.c b/arch/arm64/kvm/vgic-sys-reg-v3.c
>> index 116786d..c77d508 100644
>> --- a/arch/arm64/kvm/vgic-sys-reg-v3.c
>> +++ b/arch/arm64/kvm/vgic-sys-reg-v3.c
>> @@ -208,29 +208,12 @@ static void vgic_v3_access_apr_reg(struct kvm_vcpu *vcpu,
>>  static bool access_gic_aprn(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>>  			    const struct sys_reg_desc *r, u8 apr)
>>  {
>> -	struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
>>  	u8 idx = r->Op2 & 3;
>>  
>> -	/*
>> -	 * num_pri_bits are initialized with HW supported values.
>> -	 * We can rely safely on num_pri_bits even if VM has not
>> -	 * restored ICC_CTLR_EL1 before restoring APnR registers.
>> -	 */
>> -	switch (vgic_v3_cpu->num_pri_bits) {
>> -	case 7:
>> -		vgic_v3_access_apr_reg(vcpu, p, apr, idx);
>> -		break;
>> -	case 6:
>> -		if (idx > 1)
>> -			goto err;
>> -		vgic_v3_access_apr_reg(vcpu, p, apr, idx);
>> -		break;
>> -	default:
>> -		if (idx > 0)
>> -			goto err;
>> -		vgic_v3_access_apr_reg(vcpu, p, apr, idx);
>> -	}
>> +	if (idx > vgic_v3_max_apr_idx(vcpu))
>> +		goto err;
>>  
>> +	vgic_v3_access_apr_reg(vcpu, p, apr, idx);
>>  	return true;
>>  err:
>>  	if (!p->is_write)
>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>> index bba7fa2..bf9ceab 100644
>> --- a/virt/kvm/arm/vgic/vgic.h
>> +++ b/virt/kvm/arm/vgic/vgic.h
>> @@ -220,4 +220,20 @@ int vgic_debug_destroy(struct kvm *kvm);
>>  bool lock_all_vcpus(struct kvm *kvm);
>>  void unlock_all_vcpus(struct kvm *kvm);
>>  
>> +static inline int vgic_v3_max_apr_idx(struct kvm_vcpu *vcpu)
>> +{
>> +	struct vgic_cpu *cpu_if = &vcpu->arch.vgic_cpu;
>> +
>> +	/*
>> +	 * num_pri_bits are initialized with HW supported values.
>> +	 * We can rely safely on num_pri_bits even if VM has not
>> +	 * restored ICC_CTLR_EL1 before restoring APnR registers.
>> +	 */
>> +	switch (cpu_if->num_pri_bits) {
>> +	case 7: return 3;
>> +	case 6: return 1;
>> +	default: return 0;
>> +	}
>> +}
>> +
>>  #endif
>>
>>
>>
>> commit 660efe48259ebe83e6f93cba0a184327cebf8e82 (HEAD -> gicv2-apr-uaccess)
>> Author: Christoffer Dall <cdall@linaro.org>
>> Date:   Thu Aug 31 22:24:25 2017 +0200
>>
>>     KVM: arm/arm64: Support uaccess of GICC_APRn
>>     
>>     When migrating guests around we need to know the active priorities to
>>     ensure functional virtual interrupt prioritization by the GIC.
>>     
>>     This commit clarifies the API and how active priorities of interrupts in
>>     different groups are represented, and implements the accessor functions
>>     for the uaccess register range.
>>     
>>     We live with a slight layering violation in accessing GICv3 data
>>     structures from vgic-mmio-v2.c, because anything else just adds too much
>>     complexity for us to deal with (it's not like there's a benefit
>>     elsewhere in the code of an intermediate representation as is the case
>>     with the VMCR).  We accept this, because while doing v3 processing from
>>     a file named something-v2.c can look strange at first, this really is
>>     specific to dealing with the user space interface for something that
>>     looks like a GICv2.
>>     
>>     Signed-off-by: Christoffer Dall <cdall@linaro.org>
>>
>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
>> index b2f60ca..b3ce126 100644
>> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
>> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
>> @@ -83,6 +83,11 @@ Groups:
>>  
>>      Bits for undefined preemption levels are RAZ/WI.
>>  
>> +    Note that this differs from a CPU's view of the APRs on hardware in which
>> +    a GIC without the security extensions expose group 0 and group 1 active
>> +    priorities in separate register groups, whereas we show a combined view
>> +    similar to GICv2's GICH_APR.
>> +
>>      For historical reasons and to provide ABI compatibility with userspace we
>>      export the GICC_PMR register in the format of the GICH_VMCR.VMPriMask
>>      field in the lower 5 bits of a word, meaning that userspace must always
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> index 37522e6..b3d4a10 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> @@ -303,6 +303,51 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu,
>>  	vgic_set_vmcr(vcpu, &vmcr);
>>  }
>>  
>> +static unsigned long vgic_mmio_read_apr(struct kvm_vcpu *vcpu,
>> +					gpa_t addr, unsigned int len)
>> +{
>> +	int n; /* which APRn is this */
>> +
>> +	n = (addr >> 2) & 0x3;
>> +
>> +	if (kvm_vgic_global_state.type == VGIC_V2) {
>> +		/* GICv2 hardware systems support max. 32 groups */
>> +		if (n != 0)
>> +			return 0;
>> +		return vcpu->arch.vgic_cpu.vgic_v2.vgic_apr;
>> +	} else {
>> +		struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
>> +
>> +		if (n > vgic_v3_max_apr_idx(vcpu))
>> +			return 0;
>> +		/* GICv3 only uses ICH_AP1Rn for memory mapped (GICv2) guests */
>> +		return vgicv3->vgic_ap1r[n];
>> +	}
>> +}
>> +
>> +static void vgic_mmio_write_apr(struct kvm_vcpu *vcpu,
>> +				gpa_t addr, unsigned int len,
>> +				unsigned long val)
>> +{
>> +	int n; /* which APRn is this */
>> +
>> +	n = (addr >> 2) & 0x3;
>> +
>> +	if (kvm_vgic_global_state.type == VGIC_V2) {
>> +		/* GICv2 hardware systems support max. 32 groups */
>> +		if (n != 0)
>> +			return;
>> +		vcpu->arch.vgic_cpu.vgic_v2.vgic_apr = val;
>> +	} else {
>> +		struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
>> +
>> +		if (n > vgic_v3_max_apr_idx(vcpu))
>> +			return;
>> +		/* GICv3 only uses ICH_AP1Rn for memory mapped (GICv2) guests */
>> +		vgicv3->vgic_ap1r[n] = val;
>> +	}
>> +}
>> +
>>  static const struct vgic_register_region vgic_v2_dist_registers[] = {
>>  	REGISTER_DESC_WITH_LENGTH(GIC_DIST_CTRL,
>>  		vgic_mmio_read_v2_misc, vgic_mmio_write_v2_misc, 12,
>> @@ -364,7 +409,7 @@ static const struct vgic_register_region vgic_v2_cpu_registers[] = {
>>  		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
>>  		VGIC_ACCESS_32bit),
>>  	REGISTER_DESC_WITH_LENGTH(GIC_CPU_ACTIVEPRIO,
>> -		vgic_mmio_read_raz, vgic_mmio_write_wi, 16,
>> +		vgic_mmio_read_apr, vgic_mmio_write_apr, 16,
>>  		VGIC_ACCESS_32bit),
>>  	REGISTER_DESC_WITH_LENGTH(GIC_CPU_IDENT,
>>  		vgic_mmio_read_vcpuif, vgic_mmio_write_vcpuif, 4,
>>
>>
> 
> I'd appreciate a tested-by on these patches with your configuration.


Of course yes, I will test these two patches and give out the feedback.

Thanks.

> 
> Thanks,
> -Christoffer
> 
> .
>