* [PATCH 0/4] kvm: arm/arm64: vgic: vGICv2 GICC_APRn uaccess support.
@ 2017-07-05 11:23 wanghaibin
2017-07-05 11:23 ` [PATCH 1/4] kvm: arm/arm64: vgic-v2: Add GICH_APR accessors for GICv2 wanghaibin
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: wanghaibin @ 2017-07-05 11:23 UTC (permalink / raw)
To: cdall, marc.zyngier, kvmarm; +Cc: wu.wubin
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.
BTW: patch 1 is untested cause I don't have the GICv2 hardware board on hand, sorry.
wanghaibin (4):
kvm: arm/arm64: vgic-v2: Add GICH_APR accessors for GICv2
kvm: arm/arm64: vgic-v3: add ICH_AP[01]Rn accessors for GICv3
kvm: arm/arm64: vgic: Implement the vGICv2 GICC_APRn uaccess
interface.
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 | 21 +++++++++++++++++++++
virt/kvm/arm/vgic/vgic-v3.c | 20 ++++++++++++++++++++
virt/kvm/arm/vgic/vgic.h | 7 +++++++
6 files changed, 101 insertions(+), 22 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/4] kvm: arm/arm64: vgic-v2: Add GICH_APR accessors for GICv2
2017-07-05 11:23 [PATCH 0/4] kvm: arm/arm64: vgic: vGICv2 GICC_APRn uaccess support wanghaibin
@ 2017-07-05 11:23 ` wanghaibin
2017-07-06 16:13 ` Marc Zyngier
2017-07-05 11:23 ` [PATCH 2/4] kvm: arm/arm64: vgic-v3: add ICH_AP[01]Rn accessors for GICv3 wanghaibin
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: wanghaibin @ 2017-07-05 11:23 UTC (permalink / raw)
To: cdall, marc.zyngier, kvmarm; +Cc: wu.wubin
For GICv2, there are at most 5 priority bits are implemented in
GICH_LR<n>.Priority, so we only need to be concerned with GICH_APR0.
The other GICH_APRn access can be treated as raz/wi.
Attention: This patch is untest!
Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
---
virt/kvm/arm/vgic/vgic-v2.c | 21 +++++++++++++++++++++
virt/kvm/arm/vgic/vgic.h | 2 ++
2 files changed, 23 insertions(+)
diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index e4187e5..11d3b73 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -172,6 +172,27 @@ 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;
+ else
+ WARN_ON(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 bba7fa2..8791b9a 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/4] kvm: arm/arm64: vgic-v3: add ICH_AP[01]Rn accessors for GICv3
2017-07-05 11:23 [PATCH 0/4] kvm: arm/arm64: vgic: vGICv2 GICC_APRn uaccess support wanghaibin
2017-07-05 11:23 ` [PATCH 1/4] kvm: arm/arm64: vgic-v2: Add GICH_APR accessors for GICv2 wanghaibin
@ 2017-07-05 11:23 ` wanghaibin
2017-07-06 16:18 ` Marc Zyngier
2017-07-05 11:23 ` [PATCH 3/4] kvm: arm/arm64: vgic: Implement the vGICv2 GICC_APRn uaccess interface wanghaibin
2017-07-05 11:23 ` [PATCH 4/4] kvm: arm/arm64: vgic: clean up vGICv3 ICC_APRn sysreg uaccess wanghaibin
3 siblings, 1 reply; 11+ messages in thread
From: wanghaibin @ 2017-07-05 11:23 UTC (permalink / raw)
To: cdall, marc.zyngier, kvmarm; +Cc: wu.wubin
Compared to GICv2 hardware, GICv3 support ICH_AP[01]Rn for Group [01]
interrupts.
Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
---
virt/kvm/arm/vgic/vgic-v3.c | 20 ++++++++++++++++++++
virt/kvm/arm/vgic/vgic.h | 2 ++
2 files changed, 22 insertions(+)
diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 030248e..361da71 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 apr, u32 idx, u32 val)
+{
+ struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
+
+ if (apr)
+ cpu_if->vgic_ap1r[idx] = val;
+ else
+ cpu_if->vgic_ap0r[idx] = val;
+}
+
+u32 vgic_v3_get_apr(struct kvm_vcpu *vcpu, u8 apr, u32 idx)
+{
+ struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
+
+ if (apr)
+ 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 8791b9a..91d66ec 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 apr, u32 idx, u32 val);
+u32 vgic_v3_get_apr(struct kvm_vcpu *vcpu, u8 apr, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/4] kvm: arm/arm64: vgic: Implement the vGICv2 GICC_APRn uaccess interface.
2017-07-05 11:23 [PATCH 0/4] kvm: arm/arm64: vgic: vGICv2 GICC_APRn uaccess support wanghaibin
2017-07-05 11:23 ` [PATCH 1/4] kvm: arm/arm64: vgic-v2: Add GICH_APR accessors for GICv2 wanghaibin
2017-07-05 11:23 ` [PATCH 2/4] kvm: arm/arm64: vgic-v3: add ICH_AP[01]Rn accessors for GICv3 wanghaibin
@ 2017-07-05 11:23 ` wanghaibin
2017-07-06 16:53 ` Marc Zyngier
2017-07-05 11:23 ` [PATCH 4/4] kvm: arm/arm64: vgic: clean up vGICv3 ICC_APRn sysreg uaccess wanghaibin
3 siblings, 1 reply; 11+ messages in thread
From: wanghaibin @ 2017-07-05 11:23 UTC (permalink / raw)
To: cdall, marc.zyngier, kvmarm; +Cc: wu.wubin
Access GICV_APRn hardware register, SPEC says:
When System register access is disabled for EL2, these registers access GICH_APR<n>,
and all active priorities for virtual machines are held in GICH_APR<n> regardless of
interrupt group.
When System register access is enabled for EL2, these registers access ICH_AP1R<n>_EL2,
and all active priorities for virtual machines are held in ICH_AP1R<n>_EL2 regardless
of interrupt group.
Accordingly, the following two cases are implemented:
(1) GICv2 on GICv2, uaccess GICC_APRn info from GICH_APRn, and,
(2) GICv2 on GICv3, uaccess GICC_APRn info from ICH_AP1Rn.
Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
---
virt/kvm/arm/vgic/vgic-mmio-v2.c | 22 ++++++++++++++++++++--
virt/kvm/arm/vgic/vgic-mmio.c | 28 ++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic.h | 3 +++
3 files changed, 51 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..176b93f 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, 0, 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, 0, 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..42fe00d 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -454,6 +454,34 @@ static int match_region(const void *key, const void *elt)
sizeof(regions[0]), match_region);
}
+void vgic_set_apr(struct kvm_vcpu *vcpu, u8 apr, 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);
+ else
+ vgic_v3_set_apr(vcpu, apr, idx, val);
+ }
+}
+
+u32 vgic_get_apr(struct kvm_vcpu *vcpu, u8 apr, 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);
+ else
+ return vgic_v3_get_apr(vcpu, apr, idx);
+ }
+}
+
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 91d66ec..3cdc448 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -213,6 +213,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, u8 apr, u32 idx, u32 val);
+u32 vgic_get_apr(struct kvm_vcpu *vcpu, u8 apr, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/4] kvm: arm/arm64: vgic: clean up vGICv3 ICC_APRn sysreg uaccess
2017-07-05 11:23 [PATCH 0/4] kvm: arm/arm64: vgic: vGICv2 GICC_APRn uaccess support wanghaibin
` (2 preceding siblings ...)
2017-07-05 11:23 ` [PATCH 3/4] kvm: arm/arm64: vgic: Implement the vGICv2 GICC_APRn uaccess interface wanghaibin
@ 2017-07-05 11:23 ` wanghaibin
3 siblings, 0 replies; 11+ messages in thread
From: wanghaibin @ 2017-07-05 11:23 UTC (permalink / raw)
To: cdall, marc.zyngier, kvmarm; +Cc: wu.wubin
The vGICv3 ICC_APRn sysreg uaccess should call the APR access common
interface.
Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
---
arch/arm64/kvm/vgic-sys-reg-v3.c | 25 +++++--------------------
1 file changed, 5 insertions(+), 20 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)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] kvm: arm/arm64: vgic-v2: Add GICH_APR accessors for GICv2
2017-07-05 11:23 ` [PATCH 1/4] kvm: arm/arm64: vgic-v2: Add GICH_APR accessors for GICv2 wanghaibin
@ 2017-07-06 16:13 ` Marc Zyngier
2017-07-07 8:37 ` wanghaibin
0 siblings, 1 reply; 11+ messages in thread
From: Marc Zyngier @ 2017-07-06 16:13 UTC (permalink / raw)
To: wanghaibin, cdall, kvmarm; +Cc: wu.wubin
On 05/07/17 12:23, wanghaibin wrote:
> For GICv2, there are at most 5 priority bits are implemented in
> GICH_LR<n>.Priority, so we only need to be concerned with GICH_APR0.
> The other GICH_APRn access can be treated as raz/wi.
What is this "other" GICH_APRn?
>
> Attention: This patch is untest!
>
> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
> ---
> virt/kvm/arm/vgic/vgic-v2.c | 21 +++++++++++++++++++++
> virt/kvm/arm/vgic/vgic.h | 2 ++
> 2 files changed, 23 insertions(+)
>
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index e4187e5..11d3b73 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -172,6 +172,27 @@ 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;
> + else
> + WARN_ON(val);
If treated as WI, why do you WARN here? Also, given that there is only
one register for the active priorities, I don't really see the point in
having this "idx" parameter.
> +}
> +
> +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;
> +}
> +
> +
Extra whitespace.
> 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 bba7fa2..8791b9a 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);
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] kvm: arm/arm64: vgic-v3: add ICH_AP[01]Rn accessors for GICv3
2017-07-05 11:23 ` [PATCH 2/4] kvm: arm/arm64: vgic-v3: add ICH_AP[01]Rn accessors for GICv3 wanghaibin
@ 2017-07-06 16:18 ` Marc Zyngier
2017-07-07 9:22 ` wanghaibin
0 siblings, 1 reply; 11+ messages in thread
From: Marc Zyngier @ 2017-07-06 16:18 UTC (permalink / raw)
To: wanghaibin, cdall, kvmarm; +Cc: wu.wubin
On 05/07/17 12:23, wanghaibin wrote:
> Compared to GICv2 hardware, GICv3 support ICH_AP[01]Rn for Group [01]
> interrupts.
This doesn't describe what the patch is trying to achieve.
>
> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
> ---
> virt/kvm/arm/vgic/vgic-v3.c | 20 ++++++++++++++++++++
> virt/kvm/arm/vgic/vgic.h | 2 ++
> 2 files changed, 22 insertions(+)
>
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 030248e..361da71 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 apr, u32 idx, u32 val)
The "apr" parameter is actually the group number, and should be exposed
as such.
> +{
> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + if (apr)
> + cpu_if->vgic_ap1r[idx] = val;
> + else
> + cpu_if->vgic_ap0r[idx] = val;
How do you guarantee that you're not accessing outside of this GIC's
preemption capability?
> +}
> +
> +u32 vgic_v3_get_apr(struct kvm_vcpu *vcpu, u8 apr, u32 idx)
> +{
> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + if (apr)
> + 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 8791b9a..91d66ec 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 apr, u32 idx, u32 val);
> +u32 vgic_v3_get_apr(struct kvm_vcpu *vcpu, u8 apr, 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 smells funny...
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] kvm: arm/arm64: vgic: Implement the vGICv2 GICC_APRn uaccess interface.
2017-07-05 11:23 ` [PATCH 3/4] kvm: arm/arm64: vgic: Implement the vGICv2 GICC_APRn uaccess interface wanghaibin
@ 2017-07-06 16:53 ` Marc Zyngier
2017-07-07 8:28 ` wanghaibin
0 siblings, 1 reply; 11+ messages in thread
From: Marc Zyngier @ 2017-07-06 16:53 UTC (permalink / raw)
To: wanghaibin, cdall, kvmarm; +Cc: wu.wubin
On 05/07/17 12:23, wanghaibin wrote:
> Access GICV_APRn hardware register, SPEC says:
>
> When System register access is disabled for EL2, these registers access GICH_APR<n>,
> and all active priorities for virtual machines are held in GICH_APR<n> regardless of
> interrupt group.
> When System register access is enabled for EL2, these registers access ICH_AP1R<n>_EL2,
> and all active priorities for virtual machines are held in ICH_AP1R<n>_EL2 regardless
> of interrupt group.
>
> Accordingly, the following two cases are implemented:
> (1) GICv2 on GICv2, uaccess GICC_APRn info from GICH_APRn, and,
> (2) GICv2 on GICv3, uaccess GICC_APRn info from ICH_AP1Rn.
>
> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
> ---
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 22 ++++++++++++++++++++--
> virt/kvm/arm/vgic/vgic-mmio.c | 28 ++++++++++++++++++++++++++++
> virt/kvm/arm/vgic/vgic.h | 3 +++
> 3 files changed, 51 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..176b93f 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, 0, 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, 0, 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,
This 16 feels wrong. I now see why you had this index in the first
patch. I think it begs the questions of what we're emulating. Are we
showing a virtual GICv2 with only 5 bits of priority? Or something else?
> 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..42fe00d 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -454,6 +454,34 @@ static int match_region(const void *key, const void *elt)
> sizeof(regions[0]), match_region);
> }
>
> +void vgic_set_apr(struct kvm_vcpu *vcpu, u8 apr, u32 idx, u32 val)
Same remark as before about "apr".
> +{
> + 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);
> + else
> + vgic_v3_set_apr(vcpu, apr, idx, val);
> + }
> +}
> +
> +u32 vgic_get_apr(struct kvm_vcpu *vcpu, u8 apr, 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);
> + else
> + return vgic_v3_get_apr(vcpu, apr, idx);
> + }
> +}
> +
> 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 91d66ec..3cdc448 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -213,6 +213,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, u8 apr, u32 idx, u32 val);
> +u32 vgic_get_apr(struct kvm_vcpu *vcpu, u8 apr, 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);
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] kvm: arm/arm64: vgic: Implement the vGICv2 GICC_APRn uaccess interface.
2017-07-06 16:53 ` Marc Zyngier
@ 2017-07-07 8:28 ` wanghaibin
0 siblings, 0 replies; 11+ messages in thread
From: wanghaibin @ 2017-07-07 8:28 UTC (permalink / raw)
To: Marc Zyngier; +Cc: cdall, kvmarm, wu.wubin
On 2017/7/7 0:53, Marc Zyngier wrote:
> On 05/07/17 12:23, wanghaibin wrote:
>> Access GICV_APRn hardware register, SPEC says:
>>
>> When System register access is disabled for EL2, these registers access GICH_APR<n>,
>> and all active priorities for virtual machines are held in GICH_APR<n> regardless of
>> interrupt group.
>> When System register access is enabled for EL2, these registers access ICH_AP1R<n>_EL2,
>> and all active priorities for virtual machines are held in ICH_AP1R<n>_EL2 regardless
>> of interrupt group.
>>
>> Accordingly, the following two cases are implemented:
>> (1) GICv2 on GICv2, uaccess GICC_APRn info from GICH_APRn, and,
>> (2) GICv2 on GICv3, uaccess GICC_APRn info from ICH_AP1Rn.
>>
>> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
>> ---
>> virt/kvm/arm/vgic/vgic-mmio-v2.c | 22 ++++++++++++++++++++--
>> virt/kvm/arm/vgic/vgic-mmio.c | 28 ++++++++++++++++++++++++++++
>> virt/kvm/arm/vgic/vgic.h | 3 +++
>> 3 files changed, 51 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..176b93f 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, 0, 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, 0, 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,
>
> This 16 feels wrong. I now see why you had this index in the first
> patch. I think it begs the questions of what we're emulating. Are we
> showing a virtual GICv2 with only 5 bits of priority? Or something else?
Hi, Marc:
At present, Yes, I think it's only 5 bits of priority to show for GICv2.
But, the QEMU gicc apr put/get access code just like:
/* s->apr[n][cpu] -> GICC_APRn */
for (i = 0; i < 4; i++) {
reg = s->apr[i][cpu];
kvm_gicc_access(s, 0xd0 + i * 4, cpu, ®, true);
}
It get/put all gicc apr registers (the spec implement GICC_APR<n> (n = 0 - 3) to provides information about
interrupt active priorities), So I think the 16 is correct.
BTW: I notice that we show the 5 priority bits by VGIC_PRI_BITS to filter emulate write GICD_IPRIORITYRn bits (vgic_mmio_write_priority)
both emulate GICv2 and emulate GICv3.
Should we cancel this limit when (GICv2 on GICv3) and (GICv3 on GICv3) ?
I think we can show the bits which the hardware ICH_VTR_EL2.PRIbits support when the hardware is GICv3 to avoid priority bits lost.
Thanks.
>
>> 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..42fe00d 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
>> @@ -454,6 +454,34 @@ static int match_region(const void *key, const void *elt)
>> sizeof(regions[0]), match_region);
>> }
>>
>> +void vgic_set_apr(struct kvm_vcpu *vcpu, u8 apr, u32 idx, u32 val)
>
> Same remark as before about "apr".
>
>> +{
>> + 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);
>> + else
>> + vgic_v3_set_apr(vcpu, apr, idx, val);
>> + }
>> +}
>> +
>> +u32 vgic_get_apr(struct kvm_vcpu *vcpu, u8 apr, 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);
>> + else
>> + return vgic_v3_get_apr(vcpu, apr, idx);
>> + }
>> +}
>> +
>> 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 91d66ec..3cdc448 100644
>> --- a/virt/kvm/arm/vgic/vgic.h
>> +++ b/virt/kvm/arm/vgic/vgic.h
>> @@ -213,6 +213,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, u8 apr, u32 idx, u32 val);
>> +u32 vgic_get_apr(struct kvm_vcpu *vcpu, u8 apr, 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);
>>
>
> Thanks,
>
> M.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] kvm: arm/arm64: vgic-v2: Add GICH_APR accessors for GICv2
2017-07-06 16:13 ` Marc Zyngier
@ 2017-07-07 8:37 ` wanghaibin
0 siblings, 0 replies; 11+ messages in thread
From: wanghaibin @ 2017-07-07 8:37 UTC (permalink / raw)
To: Marc Zyngier; +Cc: cdall, kvmarm, wu.wubin
On 2017/7/7 0:13, Marc Zyngier wrote:
> On 05/07/17 12:23, wanghaibin wrote:
>> For GICv2, there are at most 5 priority bits are implemented in
>> GICH_LR<n>.Priority, so we only need to be concerned with GICH_APR0.
>> The other GICH_APRn access can be treated as raz/wi.
>
> What is this "other" GICH_APRn?
>
>>
>> Attention: This patch is untest!
>>
>> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
>> ---
>> virt/kvm/arm/vgic/vgic-v2.c | 21 +++++++++++++++++++++
>> virt/kvm/arm/vgic/vgic.h | 2 ++
>> 2 files changed, 23 insertions(+)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
>> index e4187e5..11d3b73 100644
>> --- a/virt/kvm/arm/vgic/vgic-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-v2.c
>> @@ -172,6 +172,27 @@ 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;
>> + else
>> + WARN_ON(val);
>
> If treated as WI, why do you WARN here? Also, given that there is only
> one register for the active priorities, I don't really see the point in
> having this "idx" parameter.
>
About idx parameter, the patch3 will reply.
About WARN_ON, GICv2 on GICv2, we just show 5 priority bits currently.
If uaccess set apr[1/2/3] with non-zero value, there must be something wrong, here take a warning.
Of course, this can be deleted.
Thanks.
>> +}
>> +
>> +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;
>> +}
>> +
>> +
>
> Extra whitespace.
will fix.
Thanks.
>
>> 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 bba7fa2..8791b9a 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);
>>
>
> Thanks,
>
> M.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] kvm: arm/arm64: vgic-v3: add ICH_AP[01]Rn accessors for GICv3
2017-07-06 16:18 ` Marc Zyngier
@ 2017-07-07 9:22 ` wanghaibin
0 siblings, 0 replies; 11+ messages in thread
From: wanghaibin @ 2017-07-07 9:22 UTC (permalink / raw)
To: Marc Zyngier; +Cc: cdall, kvmarm, wu.wubin
On 2017/7/7 0:18, Marc Zyngier wrote:
> On 05/07/17 12:23, wanghaibin wrote:
>> Compared to GICv2 hardware, GICv3 support ICH_AP[01]Rn for Group [01]
>> interrupts.
>
> This doesn't describe what the patch is trying to achieve.
Will fix.
>
>>
>> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
>> ---
>> virt/kvm/arm/vgic/vgic-v3.c | 20 ++++++++++++++++++++
>> virt/kvm/arm/vgic/vgic.h | 2 ++
>> 2 files changed, 22 insertions(+)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>> index 030248e..361da71 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 apr, u32 idx, u32 val)
>
> The "apr" parameter is actually the group number, and should be exposed
> as such.
>
Will fix.
>> +{
>> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
>> +
>> + if (apr)
>> + cpu_if->vgic_ap1r[idx] = val;
>> + else
>> + cpu_if->vgic_ap0r[idx] = val;
>
> How do you guarantee that you're not accessing outside of this GIC's
> preemption capability?
>
Only GICC_APRn mmio uaccess for vGICv2 and ICC_APRn uaccess sysreg uaccess will calculate
and pass the idx parameter to here, and these action can guarantee the idx only can be set 0,1,2,3.
Thanks.
>> +}
>> +
>> +u32 vgic_v3_get_apr(struct kvm_vcpu *vcpu, u8 apr, u32 idx)
>> +{
>> + struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
>> +
>> + if (apr)
>> + 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 8791b9a..91d66ec 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 apr, u32 idx, u32 val);
>> +u32 vgic_v3_get_apr(struct kvm_vcpu *vcpu, u8 apr, 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-07-07 9:22 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-05 11:23 [PATCH 0/4] kvm: arm/arm64: vgic: vGICv2 GICC_APRn uaccess support wanghaibin
2017-07-05 11:23 ` [PATCH 1/4] kvm: arm/arm64: vgic-v2: Add GICH_APR accessors for GICv2 wanghaibin
2017-07-06 16:13 ` Marc Zyngier
2017-07-07 8:37 ` wanghaibin
2017-07-05 11:23 ` [PATCH 2/4] kvm: arm/arm64: vgic-v3: add ICH_AP[01]Rn accessors for GICv3 wanghaibin
2017-07-06 16:18 ` Marc Zyngier
2017-07-07 9:22 ` wanghaibin
2017-07-05 11:23 ` [PATCH 3/4] kvm: arm/arm64: vgic: Implement the vGICv2 GICC_APRn uaccess interface wanghaibin
2017-07-06 16:53 ` Marc Zyngier
2017-07-07 8:28 ` wanghaibin
2017-07-05 11:23 ` [PATCH 4/4] kvm: arm/arm64: vgic: clean up vGICv3 ICC_APRn sysreg uaccess wanghaibin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox