[PATCH 0/3] KVM: arm64: Fix userspace access to HW pending state
Eric reported that a Seattle system was pretty unhappy about VM migration, and the trace pointed to a glaring bug in the way the GICv2 emulation code reported the interrupt pending state to userspace for HW interrupts, specially if the interrupt state is per-CPU, as this is the case for the timer... Fixing this actually results in a minor cleanup, followed by a bit of extra hardening so that we can catch further issues in this area without completely taking the system down. Unless someone screams, I plan to take these in as fixes as quickly as possible, with the first patch being an obvious stable candidate. I'd appreciate it if people could verify that VM migration still works correctly for both GICv2 and GICv3. Thanks, M. Marc Zyngier (3): KVM: arm64: Don't read a HW interrupt pending state in user context KVM: arm64: Replace vgic_v3_uaccess_read_pending with vgic_uaccess_read_pending KVM: arm64: Warn if accessing timer pending state outside of vcpu context arch/arm64/kvm/arch_timer.c| 3 +++ arch/arm64/kvm/vgic/vgic-mmio-v2.c | 4 +-- arch/arm64/kvm/vgic/vgic-mmio-v3.c | 40 ++ arch/arm64/kvm/vgic/vgic-mmio.c| 19 +++--- arch/arm64/kvm/vgic/vgic-mmio.h| 3 +++ 5 files changed, 26 insertions(+), 43 deletions(-) -- 2.34.1 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH 3/3] KVM: arm64: Warn if accessing timer pending state outside of vcpu context
A recurrent bug in the KVM/arm64 code base consists in trying to access the timer pending state outside of the vcpu context, which makes zero sense (the pending state only exists when the vcpu is loaded). In order to avoid more embarassing crashes and catch the offenders red-handed, add a warning to kvm_arch_timer_get_input_level() and return the state as non-pending. This avoids taking the system down, and still helps tracking down silly bugs. Signed-off-by: Marc Zyngier --- arch/arm64/kvm/arch_timer.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c index 5290ca5db663..bb24a76b4224 100644 --- a/arch/arm64/kvm/arch_timer.c +++ b/arch/arm64/kvm/arch_timer.c @@ -1230,6 +1230,9 @@ bool kvm_arch_timer_get_input_level(int vintid) struct kvm_vcpu *vcpu = kvm_get_running_vcpu(); struct arch_timer_context *timer; + if (WARN(!vcpu, "No vcpu context!\n")) + return false; + if (vintid == vcpu_vtimer(vcpu)->irq.irq) timer = vcpu_vtimer(vcpu); else if (vintid == vcpu_ptimer(vcpu)->irq.irq) -- 2.34.1 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH 2/3] KVM: arm64: Replace vgic_v3_uaccess_read_pending with vgic_uaccess_read_pending
Now that GICv2 has a proper userspace accessor for the pending state,
switch GICv3 over to it, dropping the local version.
Signed-off-by: Marc Zyngier
---
arch/arm64/kvm/vgic/vgic-mmio-v3.c | 40 ++
1 file changed, 2 insertions(+), 38 deletions(-)
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index f7aa7bcd6fb8..f15e29cc63ce 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -353,42 +353,6 @@ static unsigned long vgic_mmio_read_v3_idregs(struct
kvm_vcpu *vcpu,
return 0;
}
-static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
- gpa_t addr, unsigned int len)
-{
- u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
- u32 value = 0;
- int i;
-
- /*
-* pending state of interrupt is latched in pending_latch variable.
-* Userspace will save and restore pending state and line_level
-* separately.
-* Refer to Documentation/virt/kvm/devices/arm-vgic-v3.rst
-* for handling of ISPENDR and ICPENDR.
-*/
- for (i = 0; i < len * 8; i++) {
- struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
- bool state = irq->pending_latch;
-
- if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
- int err;
-
- err = irq_get_irqchip_state(irq->host_irq,
- IRQCHIP_STATE_PENDING,
- );
- WARN_ON(err);
- }
-
- if (state)
- value |= (1U << i);
-
- vgic_put_irq(vcpu->kvm, irq);
- }
-
- return value;
-}
-
static int vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val)
@@ -666,7 +630,7 @@ static const struct vgic_register_region
vgic_v3_dist_registers[] = {
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISPENDR,
vgic_mmio_read_pending, vgic_mmio_write_spending,
- vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1,
+ vgic_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR,
vgic_mmio_read_pending, vgic_mmio_write_cpending,
@@ -750,7 +714,7 @@ static const struct vgic_register_region
vgic_v3_rd_registers[] = {
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_LENGTH_UACCESS(SZ_64K + GICR_ISPENDR0,
vgic_mmio_read_pending, vgic_mmio_write_spending,
- vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4,
+ vgic_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_LENGTH_UACCESS(SZ_64K + GICR_ICPENDR0,
vgic_mmio_read_pending, vgic_mmio_write_cpending,
--
2.34.1
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH 1/3] KVM: arm64: Don't read a HW interrupt pending state in user context
Since 5bfa685e62e9 ("KVM: arm64: vgic: Read HW interrupt pending state
from the HW"), we're able to source the pending bit for an interrupt
that is stored either on the physical distributor or on a device.
However, this state is only available when the vcpu is loaded,
and is not intended to be accessed from userspace. Unfortunately,
the GICv2 emulation doesn't provide specific userspace accessors,
and we fallback with the ones that are intended for the guest,
with fatal consequences.
Add a new vgic_uaccess_read_pending() accessor for userspace
to use, build on top of the existing vgic_mmio_read_pending().
Reported-by: Eric Auger
Signed-off-by: Marc Zyngier
Fixes: 5bfa685e62e9 ("KVM: arm64: vgic: Read HW interrupt pending state from
the HW")
---
arch/arm64/kvm/vgic/vgic-mmio-v2.c | 4 ++--
arch/arm64/kvm/vgic/vgic-mmio.c| 19 ---
arch/arm64/kvm/vgic/vgic-mmio.h| 3 +++
3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v2.c
b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
index 77a67e9d3d14..e070cda86e12 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v2.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
@@ -429,11 +429,11 @@ static const struct vgic_register_region
vgic_v2_dist_registers[] = {
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET,
vgic_mmio_read_pending, vgic_mmio_write_spending,
- NULL, vgic_uaccess_write_spending, 1,
+ vgic_uaccess_read_pending, vgic_uaccess_write_spending, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR,
vgic_mmio_read_pending, vgic_mmio_write_cpending,
- NULL, vgic_uaccess_write_cpending, 1,
+ vgic_uaccess_read_pending, vgic_uaccess_write_cpending, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
vgic_mmio_read_active, vgic_mmio_write_sactive,
diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
index 49837d3a3ef5..dc8c52487e47 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio.c
@@ -226,8 +226,9 @@ int vgic_uaccess_write_cenable(struct kvm_vcpu *vcpu,
return 0;
}
-unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
-gpa_t addr, unsigned int len)
+static unsigned long __read_pending(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len,
+ bool is_user)
{
u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
u32 value = 0;
@@ -248,7 +249,7 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
IRQCHIP_STATE_PENDING,
);
WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
- } else if (vgic_irq_is_mapped_level(irq)) {
+ } else if (!is_user && vgic_irq_is_mapped_level(irq)) {
val = vgic_get_phys_line_level(irq);
} else {
val = irq_is_pending(irq);
@@ -263,6 +264,18 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
return value;
}
+unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
+gpa_t addr, unsigned int len)
+{
+ return __read_pending(vcpu, addr, len, false);
+}
+
+unsigned long vgic_uaccess_read_pending(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len)
+{
+ return __read_pending(vcpu, addr, len, true);
+}
+
static bool is_vgic_v2_sgi(struct kvm_vcpu *vcpu, struct vgic_irq *irq)
{
return (vgic_irq_is_sgi(irq->intid) &&
diff --git a/arch/arm64/kvm/vgic/vgic-mmio.h b/arch/arm64/kvm/vgic/vgic-mmio.h
index 3fa696f198a3..6082d4b66d39 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio.h
+++ b/arch/arm64/kvm/vgic/vgic-mmio.h
@@ -149,6 +149,9 @@ int vgic_uaccess_write_cenable(struct kvm_vcpu *vcpu,
unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len);
+unsigned long vgic_uaccess_read_pending(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len);
+
void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val);
--
2.34.1
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH] KVM: arm64: fix the inconsistent indenting
Fix the inconsistent indenting in function flush_context. Fix the following smatch warnings: arch/arm64/kvm/vmid.c:62 flush_context() warn: inconsistent indenting Reported-by: kernel test robot Signed-off-by: sunliming --- arch/arm64/kvm/vmid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/kvm/vmid.c b/arch/arm64/kvm/vmid.c index 8d5f0506fd87..d78ae63d7c15 100644 --- a/arch/arm64/kvm/vmid.c +++ b/arch/arm64/kvm/vmid.c @@ -66,7 +66,7 @@ static void flush_context(void) * the next context-switch, we broadcast TLB flush + I-cache * invalidation over the inner shareable domain on rollover. */ -kvm_call_hyp(__kvm_flush_vm_context); + kvm_call_hyp(__kvm_flush_vm_context); } static bool check_update_reserved_vmid(u64 vmid, u64 newvmid) -- 2.25.1 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/3] KVM: arm64: Don't read a HW interrupt pending state in user context
Marc,
On 6/2/22 10:30, Marc Zyngier wrote:
> Since 5bfa685e62e9 ("KVM: arm64: vgic: Read HW interrupt pending state
> from the HW"), we're able to source the pending bit for an interrupt
> that is stored either on the physical distributor or on a device.
>
> However, this state is only available when the vcpu is loaded,
> and is not intended to be accessed from userspace. Unfortunately,
> the GICv2 emulation doesn't provide specific userspace accessors,
> and we fallback with the ones that are intended for the guest,
> with fatal consequences.
>
> Add a new vgic_uaccess_read_pending() accessor for userspace
> to use, build on top of the existing vgic_mmio_read_pending().
>
> Reported-by: Eric Auger
> Signed-off-by: Marc Zyngier
> Fixes: 5bfa685e62e9 ("KVM: arm64: vgic: Read HW interrupt pending state from
> the HW")
Feel free to add my
Tested-by: Eric Auger
Thanks!
Eric
> ---
> arch/arm64/kvm/vgic/vgic-mmio-v2.c | 4 ++--
> arch/arm64/kvm/vgic/vgic-mmio.c| 19 ---
> arch/arm64/kvm/vgic/vgic-mmio.h| 3 +++
> 3 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> index 77a67e9d3d14..e070cda86e12 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> @@ -429,11 +429,11 @@ static const struct vgic_register_region
> vgic_v2_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET,
> vgic_mmio_read_pending, vgic_mmio_write_spending,
> - NULL, vgic_uaccess_write_spending, 1,
> + vgic_uaccess_read_pending, vgic_uaccess_write_spending, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR,
> vgic_mmio_read_pending, vgic_mmio_write_cpending,
> - NULL, vgic_uaccess_write_cpending, 1,
> + vgic_uaccess_read_pending, vgic_uaccess_write_cpending, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
> vgic_mmio_read_active, vgic_mmio_write_sactive,
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
> index 49837d3a3ef5..dc8c52487e47 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
> @@ -226,8 +226,9 @@ int vgic_uaccess_write_cenable(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> -unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
> - gpa_t addr, unsigned int len)
> +static unsigned long __read_pending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len,
> + bool is_user)
> {
> u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> u32 value = 0;
> @@ -248,7 +249,7 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu
> *vcpu,
> IRQCHIP_STATE_PENDING,
> );
> WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
> - } else if (vgic_irq_is_mapped_level(irq)) {
> + } else if (!is_user && vgic_irq_is_mapped_level(irq)) {
> val = vgic_get_phys_line_level(irq);
> } else {
> val = irq_is_pending(irq);
> @@ -263,6 +264,18 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu
> *vcpu,
> return value;
> }
>
> +unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + return __read_pending(vcpu, addr, len, false);
> +}
> +
> +unsigned long vgic_uaccess_read_pending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + return __read_pending(vcpu, addr, len, true);
> +}
> +
> static bool is_vgic_v2_sgi(struct kvm_vcpu *vcpu, struct vgic_irq *irq)
> {
> return (vgic_irq_is_sgi(irq->intid) &&
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.h b/arch/arm64/kvm/vgic/vgic-mmio.h
> index 3fa696f198a3..6082d4b66d39 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio.h
> +++ b/arch/arm64/kvm/vgic/vgic-mmio.h
> @@ -149,6 +149,9 @@ int vgic_uaccess_write_cenable(struct kvm_vcpu *vcpu,
> unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
>gpa_t addr, unsigned int len);
>
> +unsigned long vgic_uaccess_read_pending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len);
> +
> void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val);
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/3] KVM: arm64: Don't read a HW interrupt pending state in user context
On 6/2/22 10:30, Marc Zyngier wrote:
> Since 5bfa685e62e9 ("KVM: arm64: vgic: Read HW interrupt pending state
> from the HW"), we're able to source the pending bit for an interrupt
> that is stored either on the physical distributor or on a device.
>
> However, this state is only available when the vcpu is loaded,
> and is not intended to be accessed from userspace. Unfortunately,
> the GICv2 emulation doesn't provide specific userspace accessors,
> and we fallback with the ones that are intended for the guest,
> with fatal consequences.
>
> Add a new vgic_uaccess_read_pending() accessor for userspace
> to use, build on top of the existing vgic_mmio_read_pending().
>
> Reported-by: Eric Auger
> Signed-off-by: Marc Zyngier
> Fixes: 5bfa685e62e9 ("KVM: arm64: vgic: Read HW interrupt pending state from
> the HW")
Reviewed-by: Eric Auger
Eric
> ---
> arch/arm64/kvm/vgic/vgic-mmio-v2.c | 4 ++--
> arch/arm64/kvm/vgic/vgic-mmio.c| 19 ---
> arch/arm64/kvm/vgic/vgic-mmio.h| 3 +++
> 3 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> index 77a67e9d3d14..e070cda86e12 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> @@ -429,11 +429,11 @@ static const struct vgic_register_region
> vgic_v2_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET,
> vgic_mmio_read_pending, vgic_mmio_write_spending,
> - NULL, vgic_uaccess_write_spending, 1,
> + vgic_uaccess_read_pending, vgic_uaccess_write_spending, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR,
> vgic_mmio_read_pending, vgic_mmio_write_cpending,
> - NULL, vgic_uaccess_write_cpending, 1,
> + vgic_uaccess_read_pending, vgic_uaccess_write_cpending, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
> vgic_mmio_read_active, vgic_mmio_write_sactive,
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
> index 49837d3a3ef5..dc8c52487e47 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
> @@ -226,8 +226,9 @@ int vgic_uaccess_write_cenable(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> -unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
> - gpa_t addr, unsigned int len)
> +static unsigned long __read_pending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len,
> + bool is_user)
> {
> u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> u32 value = 0;
> @@ -248,7 +249,7 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu
> *vcpu,
> IRQCHIP_STATE_PENDING,
> );
> WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
> - } else if (vgic_irq_is_mapped_level(irq)) {
> + } else if (!is_user && vgic_irq_is_mapped_level(irq)) {
> val = vgic_get_phys_line_level(irq);
> } else {
> val = irq_is_pending(irq);
> @@ -263,6 +264,18 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu
> *vcpu,
> return value;
> }
>
> +unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + return __read_pending(vcpu, addr, len, false);
> +}
> +
> +unsigned long vgic_uaccess_read_pending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + return __read_pending(vcpu, addr, len, true);
> +}
> +
> static bool is_vgic_v2_sgi(struct kvm_vcpu *vcpu, struct vgic_irq *irq)
> {
> return (vgic_irq_is_sgi(irq->intid) &&
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.h b/arch/arm64/kvm/vgic/vgic-mmio.h
> index 3fa696f198a3..6082d4b66d39 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio.h
> +++ b/arch/arm64/kvm/vgic/vgic-mmio.h
> @@ -149,6 +149,9 @@ int vgic_uaccess_write_cenable(struct kvm_vcpu *vcpu,
> unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
>gpa_t addr, unsigned int len);
>
> +unsigned long vgic_uaccess_read_pending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len);
> +
> void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val);
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 3/3] KVM: arm64: Warn if accessing timer pending state outside of vcpu context
Hi Marc, On 6/2/22 10:30, Marc Zyngier wrote: > A recurrent bug in the KVM/arm64 code base consists in trying to > access the timer pending state outside of the vcpu context, which > makes zero sense (the pending state only exists when the vcpu > is loaded). > > In order to avoid more embarassing crashes and catch the offenders > red-handed, add a warning to kvm_arch_timer_get_input_level() and > return the state as non-pending. This avoids taking the system down, > and still helps tracking down silly bugs. > > Signed-off-by: Marc Zyngier Reviewed-by: Eric Auger Eric > --- > arch/arm64/kvm/arch_timer.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c > index 5290ca5db663..bb24a76b4224 100644 > --- a/arch/arm64/kvm/arch_timer.c > +++ b/arch/arm64/kvm/arch_timer.c > @@ -1230,6 +1230,9 @@ bool kvm_arch_timer_get_input_level(int vintid) > struct kvm_vcpu *vcpu = kvm_get_running_vcpu(); > struct arch_timer_context *timer; > > + if (WARN(!vcpu, "No vcpu context!\n")) > + return false; > + > if (vintid == vcpu_vtimer(vcpu)->irq.irq) > timer = vcpu_vtimer(vcpu); > else if (vintid == vcpu_ptimer(vcpu)->irq.irq) ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 2/3] KVM: arm64: Replace vgic_v3_uaccess_read_pending with vgic_uaccess_read_pending
Hi Marc,
On 6/2/22 10:30, Marc Zyngier wrote:
> Now that GICv2 has a proper userspace accessor for the pending state,
> switch GICv3 over to it, dropping the local version.
>
> Signed-off-by: Marc Zyngier
> ---
> arch/arm64/kvm/vgic/vgic-mmio-v3.c | 40 ++
> 1 file changed, 2 insertions(+), 38 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index f7aa7bcd6fb8..f15e29cc63ce 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -353,42 +353,6 @@ static unsigned long vgic_mmio_read_v3_idregs(struct
> kvm_vcpu *vcpu,
> return 0;
> }
>
> -static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
> - gpa_t addr, unsigned int len)
> -{
> - u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> - u32 value = 0;
> - int i;
> -
> - /*
> - * pending state of interrupt is latched in pending_latch variable.
> - * Userspace will save and restore pending state and line_level
> - * separately.
> - * Refer to Documentation/virt/kvm/devices/arm-vgic-v3.rst
> - * for handling of ISPENDR and ICPENDR.
Don't know if you want a derivative of this comment in
vgic_uaccess_read_pending()?
> - */
> - for (i = 0; i < len * 8; i++) {
> - struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> - bool state = irq->pending_latch;
> -
> - if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
> - int err;
> -
in __read_pending(), irq->irq_lock is hold which looks safer at 1st
sight. If potentially fixing something this can be documented in the
commit msg.
> - err = irq_get_irqchip_state(irq->host_irq,
> - IRQCHIP_STATE_PENDING,
> - );
> - WARN_ON(err);
> - }
> -
in __read_pending(), irq_is_pending(irq) is used instead of
irq->pending_latch. for level sensitive IRQ this is not identical. This
may also deserve some comment. The nuance may be related to the above
comment.
Thanks
Eric
> - if (state)
> - value |= (1U << i);
> -
> - vgic_put_irq(vcpu->kvm, irq);
> - }
> -
> - return value;
> -}
> -
> static int vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
>gpa_t addr, unsigned int len,
>unsigned long val)
> @@ -666,7 +630,7 @@ static const struct vgic_register_region
> vgic_v3_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISPENDR,
> vgic_mmio_read_pending, vgic_mmio_write_spending,
> - vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1,
> + vgic_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR,
> vgic_mmio_read_pending, vgic_mmio_write_cpending,
> @@ -750,7 +714,7 @@ static const struct vgic_register_region
> vgic_v3_rd_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH_UACCESS(SZ_64K + GICR_ISPENDR0,
> vgic_mmio_read_pending, vgic_mmio_write_spending,
> - vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4,
> + vgic_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH_UACCESS(SZ_64K + GICR_ICPENDR0,
> vgic_mmio_read_pending, vgic_mmio_write_cpending,
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
