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

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

[wanghaibin]

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

v1: the problem describe:
https://lists.cs.columbia.edu/pipermail/kvmarm/2017-July/026295.html

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      | 20 ++++++++++++++++++++
 virt/kvm/arm/vgic/vgic.h         |  7 +++++++
 6 files changed, 98 insertions(+), 22 deletions(-)

-- 
1.8.3.1

[PATCH v2 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 63e0bbd..832f403 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,
@@ -361,8 +378,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 1c17b2a..deed781 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -454,6 +454,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 v2 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 deed781..f6f3681 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -456,10 +456,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 v2 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.

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

diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index f6f3681..3b648a7 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -456,14 +456,26 @@ static int match_region(const void *key, const void *elt)
 
 void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
 {
+	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)
 {
+	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 030248e..da40681 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -156,6 +156,26 @@ void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr)
 	vcpu->arch.vgic_cpu.vgic_v3.vgic_lr[lr] = 0;
 }
 
+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 (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 (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 v2 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 6260b69..0f7fbcd 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 832f403..176b93f 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 3b648a7..79a5eac 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -454,7 +454,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;
 
@@ -463,10 +463,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;
 
@@ -475,9 +477,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 v2 0/4] kvm: arm/arm64: vgic: APRn uaccess support.

[Christoffer Dall]

Hi Wanghaibin,

On Mon, Jul 17, 2017 at 06:23:28PM +0800, wanghaibin wrote:
> v2: Split the patch again to make it easier for review
>     some fixes were proposed by Marc
> 
> v1: the problem describe:
> https://lists.cs.columbia.edu/pipermail/kvmarm/2017-July/026295.html

please also include the same description in the cover letter for future
revisions of this patch series.  People may not remember the context
(like me) or others may jump in as reviewers at a later time, and having
to go back and look at an old posting via a link is not very friendly to
the review process.

Thanks,
-Christoffer

> 
> 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      | 20 ++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic.h         |  7 +++++++
>  6 files changed, 98 insertions(+), 22 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Marc Zyngier]

On Mon, Jul 17 2017 at  6:23:31 pm BST, wanghaibin <wanghaibin.wang@huawei.com> wrote:
> 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.
>
> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
> ---
>  virt/kvm/arm/vgic/vgic-mmio.c | 12 ++++++++++++
>  virt/kvm/arm/vgic/vgic-v3.c   | 20 ++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic.h      |  2 ++
>  3 files changed, 34 insertions(+)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index f6f3681..3b648a7 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -456,14 +456,26 @@ static int match_region(const void *key, const void *elt)
>  
>  void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
>  {
> +	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 {

Coding style.

> +		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)
>  {
> +	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 {

Same here.

> +		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 030248e..da40681 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -156,6 +156,26 @@ void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr)
>  	vcpu->arch.vgic_cpu.vgic_v3.vgic_lr[lr] = 0;
>  }
>  
> +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 (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 (group)
> +		return cpu_if->vgic_ap1r[idx];
> +	else
> +		return cpu_if->vgic_ap0r[idx];
> +}
> +

How do we ensure that these APRs are even valid? We can have anything
from 5 to 7 bits of preemption, and thus 1 to 4 APRs. The rest is
UNDEFined.

>  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);

Thanks,

	M.
-- 
Jazz is not dead, it just smell funny.

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

[wanghaibin]

On 2017/7/25 19:25, Marc Zyngier wrote:

> On Mon, Jul 17 2017 at  6:23:31 pm BST, wanghaibin <wanghaibin.wang@huawei.com> wrote:
>> 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.
>>
>> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-mmio.c | 12 ++++++++++++
>>  virt/kvm/arm/vgic/vgic-v3.c   | 20 ++++++++++++++++++++
>>  virt/kvm/arm/vgic/vgic.h      |  2 ++
>>  3 files changed, 34 insertions(+)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>> index f6f3681..3b648a7 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>> @@ -456,14 +456,26 @@ static int match_region(const void *key, const void *elt)
>>  
>>  void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
>>  {
>> +	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 {
> 
> Coding style.


Will fix.

Thanks

> 
>> +		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)
>>  {
>> +	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 {
> 
> Same here.
> 
>> +		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 030248e..da40681 100644
>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>> @@ -156,6 +156,26 @@ void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr)
>>  	vcpu->arch.vgic_cpu.vgic_v3.vgic_lr[lr] = 0;
>>  }
>>  
>> +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 (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 (group)
>> +		return cpu_if->vgic_ap1r[idx];
>> +	else
>> +		return cpu_if->vgic_ap0r[idx];
>> +}
>> +
> 
> How do we ensure that these APRs are even valid? We can have anything
> from 5 to 7 bits of preemption, and thus 1 to 4 APRs. The rest is
> UNDEFined.


Sorry, Marc, I can't follow you.

Do you suspect that an undefined APR register(implemented by ICH_VTR_EL2.PRIbits) will be accessed here?

This code just access the soft vgic_ap[0,1]r structure, and I notice that, this structure saved the hardware
APRn info under the constraint of ICH_VTR_EL2.PREbits implemented when save the vcpu content. the soft vgic_ap[0,1]r
structure will keep 0 value when the hw APRn UNDEFined.

Thanks.

> 
>>  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);
> 
> Thanks,
> 
> 	M.

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

[wanghaibin]

On 2017/7/21 21:27, Christoffer Dall wrote:

> Hi Wanghaibin,
> 
> On Mon, Jul 17, 2017 at 06:23:28PM +0800, wanghaibin wrote:
>> v2: Split the patch again to make it easier for review
>>     some fixes were proposed by Marc
>>
>> v1: the problem describe:
>> https://lists.cs.columbia.edu/pipermail/kvmarm/2017-July/026295.html
> 
> please also include the same description in the cover letter for future
> revisions of this patch series.  People may not remember the context
> (like me) or others may jump in as reviewers at a later time, and having
> to go back and look at an old posting via a link is not very friendly to
> the review process.
> 


OK, I'll do it next time.

This problem describe as follows:

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.

Thanks.

> Thanks,
> -Christoffer
> 
>>
>> 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      | 20 ++++++++++++++++++++
>>  virt/kvm/arm/vgic/vgic.h         |  7 +++++++
>>  6 files changed, 98 insertions(+), 22 deletions(-)
>>
>> -- 
>> 1.8.3.1
>>
>>
>> _______________________________________________
>> kvmarm mailing list
>> kvmarm@lists.cs.columbia.edu
>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 
> .
> 

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

[Marc Zyngier]

On 08/08/17 14:01, wanghaibin wrote:
> On 2017/7/25 19:25, Marc Zyngier wrote:
> 
>> On Mon, Jul 17 2017 at  6:23:31 pm BST, wanghaibin <wanghaibin.wang@huawei.com> wrote:
>>> 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.
>>>
>>> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
>>> ---
>>>  virt/kvm/arm/vgic/vgic-mmio.c | 12 ++++++++++++
>>>  virt/kvm/arm/vgic/vgic-v3.c   | 20 ++++++++++++++++++++
>>>  virt/kvm/arm/vgic/vgic.h      |  2 ++
>>>  3 files changed, 34 insertions(+)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
>>> index f6f3681..3b648a7 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>>> @@ -456,14 +456,26 @@ static int match_region(const void *key, const void *elt)
>>>  
>>>  void vgic_set_apr(struct kvm_vcpu *vcpu, u32 idx, u32 val)
>>>  {
>>> +	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 {
>>
>> Coding style.
> 
> 
> Will fix.
> 
> Thanks
> 
>>
>>> +		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)
>>>  {
>>> +	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 {
>>
>> Same here.
>>
>>> +		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 030248e..da40681 100644
>>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>>> @@ -156,6 +156,26 @@ void vgic_v3_clear_lr(struct kvm_vcpu *vcpu, int lr)
>>>  	vcpu->arch.vgic_cpu.vgic_v3.vgic_lr[lr] = 0;
>>>  }
>>>  
>>> +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 (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 (group)
>>> +		return cpu_if->vgic_ap1r[idx];
>>> +	else
>>> +		return cpu_if->vgic_ap0r[idx];
>>> +}
>>> +
>>
>> How do we ensure that these APRs are even valid? We can have anything
>> from 5 to 7 bits of preemption, and thus 1 to 4 APRs. The rest is
>> UNDEFined.
> 
> 
> Sorry, Marc, I can't follow you.
> 
> Do you suspect that an undefined APR register(implemented by ICH_VTR_EL2.PRIbits) will be accessed here?

Two implementations may not implement the same number of PRIbits. But
you seem to allow userspace to save/restore them, even if that doesn't
make sense.

> This code just access the soft vgic_ap[0,1]r structure, and I notice that, this structure saved the hardware
> APRn info under the constraint of ICH_VTR_EL2.PREbits implemented when save the vcpu content. the soft vgic_ap[0,1]r
> structure will keep 0 value when the hw APRn UNDEFined.

Yes, but userspace should be aware of this constraint, and access
register that cannot be used on this implementation.

To summarize, you need to be consistent. Something that cannot be used
for the guest shouldn't be available to userspace either.

Thanks,

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