[PATCH 0/3] arm/arm64: KVM: Fix !irqchip_in_kernel() handling

[PATCH 0/3] arm/arm64: KVM: Fix !irqchip_in_kernel() handling

[Marc Zyngier]

It is quite obvious that the non kernel-irqchip has been bitrotting
for a while now. Pavel has been doing some work to address this, but
it turns out that there is more fun to be had...

This series picks up one of Pavel's series (which is an obvious fix),
and adds another fix for both arm and arm64.

With these two patches applied, I get a working system as long as I
don't need timers - which is expected.

Tested on X-Gene.

Marc Zyngier (2):
  arm64: KVM: Disable virtual timer even if the guest is not using it
  arm: KVM: Disable virtual timer even if the guest is not using it

Pavel Fedin (1):
  arm/arm64: KVM: vgic: Check for !irqchip_in_kernel() when mapping
    resources

 arch/arm/kvm/arm.c             | 2 +-
 arch/arm/kvm/interrupts_head.S | 6 ++++--
 arch/arm64/kvm/hyp.S           | 4 ++--
 3 files changed, 7 insertions(+), 5 deletions(-)

-- 
2.1.4

[PATCH 1/3] arm/arm64: KVM: vgic: Check for !irqchip_in_kernel() when mapping resources

[Marc Zyngier]

From: Pavel Fedin <p.fedin@samsung.com>

Until b26e5fdac43c ("arm/arm64: KVM: introduce per-VM ops"),
kvm_vgic_map_resources() used to include a check on irqchip_in_kernel(),
and vgic_v2_map_resources() still has it.

But now vm_ops are not initialized until we call kvm_vgic_create().
Therefore kvm_vgic_map_resources() can being called without a VGIC,
and we die because vm_ops.map_resources is NULL.

Fixing this restores QEMU's kernel-irqchip=off option to a working state,
allowing to use GIC emulation in userspace.

Fixes: b26e5fdac43c ("arm/arm64: KVM: introduce per-VM ops")
Cc: stable@vger.kernel.org
Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
[maz: reworked commit message]
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index ce404a5..dc017ad 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -446,7 +446,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
 	 * Map the VGIC hardware resources before running a vcpu the first
 	 * time on this VM.
 	 */
-	if (unlikely(!vgic_ready(kvm))) {
+	if (unlikely(irqchip_in_kernel(kvm) && !vgic_ready(kvm))) {
 		ret = kvm_vgic_map_resources(kvm);
 		if (ret)
 			return ret;
-- 
2.1.4

RE: [PATCH 1/3] arm/arm64: KVM: vgic: Check for !irqchip_in_kernel() when mapping resources

[Pavel Fedin]

 Hello!

> Until b26e5fdac43c ("arm/arm64: KVM: introduce per-VM ops"),
> kvm_vgic_map_resources() used to include a check on irqchip_in_kernel(),
> and vgic_v2_map_resources() still has it.

 I'm back from vacation and very glad to see myself being useful :)
 What's with the rest of that patch set? Does it need rebase or what ?
 By the way, after this is applied, we can drop useless check inside vgic_v2_map_resources(). 

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

[PATCH 2/3] arm64: KVM: Disable virtual timer even if the guest is not using it

[Marc Zyngier]

When running a guest with the architected timer disabled (with QEMU and
the kernel_irqchip=off option, for example), it is important to make
sure the timer gets turned off. Otherwise, the guest may try to
enable it anyway, leading to a screaming HW interrupt.

The fix is to unconditionally turn off the virtual timer on guest
exit.

Cc: stable@vger.kernel.org
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/hyp.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index 6addf97..38f5434 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -570,8 +570,6 @@ alternative_endif
 	mrs	x3, cntv_ctl_el0
 	and	x3, x3, #3
 	str	w3, [x0, #VCPU_TIMER_CNTV_CTL]
-	bic	x3, x3, #1		// Clear Enable
-	msr	cntv_ctl_el0, x3
 
 	isb
 
@@ -579,6 +577,8 @@ alternative_endif
 	str	x3, [x0, #VCPU_TIMER_CNTV_CVAL]
 
 1:
+	msr	cntv_ctl_el0, xzr
+
 	// Allow physical timer/counter access for the host
 	mrs	x2, cnthctl_el2
 	orr	x2, x2, #3
-- 
2.1.4

Re: [PATCH 2/3] arm64: KVM: Disable virtual timer even if the guest is not using it

[Christoffer Dall]

On Wed, Sep 16, 2015 at 04:58:06PM +0100, Marc Zyngier wrote:
> When running a guest with the architected timer disabled (with QEMU and
> the kernel_irqchip=off option, for example), it is important to make
> sure the timer gets turned off. Otherwise, the guest may try to
> enable it anyway, leading to a screaming HW interrupt.
> 
> The fix is to unconditionally turn off the virtual timer on guest
> exit.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/hyp.S | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index 6addf97..38f5434 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -570,8 +570,6 @@ alternative_endif

The context confuses me; did you happen to base this on your VHE
patches?

>  	mrs	x3, cntv_ctl_el0
>  	and	x3, x3, #3
>  	str	w3, [x0, #VCPU_TIMER_CNTV_CTL]
> -	bic	x3, x3, #1		// Clear Enable
> -	msr	cntv_ctl_el0, x3
>  
>  	isb
>  
> @@ -579,6 +577,8 @@ alternative_endif
>  	str	x3, [x0, #VCPU_TIMER_CNTV_CVAL]
>  
>  1:
> +	msr	cntv_ctl_el0, xzr
> +

We could have a comment here, but ok.

>  	// Allow physical timer/counter access for the host
>  	mrs	x2, cnthctl_el2
>  	orr	x2, x2, #3
> -- 
> 2.1.4
> 

Otherwise:

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

Re: [PATCH 2/3] arm64: KVM: Disable virtual timer even if the guest is not using it

[Marc Zyngier]

On 17/09/15 12:17, Christoffer Dall wrote:
> On Wed, Sep 16, 2015 at 04:58:06PM +0100, Marc Zyngier wrote:
>> When running a guest with the architected timer disabled (with QEMU and
>> the kernel_irqchip=off option, for example), it is important to make
>> sure the timer gets turned off. Otherwise, the guest may try to
>> enable it anyway, leading to a screaming HW interrupt.
>>
>> The fix is to unconditionally turn off the virtual timer on guest
>> exit.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/kvm/hyp.S | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
>> index 6addf97..38f5434 100644
>> --- a/arch/arm64/kvm/hyp.S
>> +++ b/arch/arm64/kvm/hyp.S
>> @@ -570,8 +570,6 @@ alternative_endif
> 
> The context confuses me; did you happen to base this on your VHE
> patches?

No, that's on top of 4.3-rc1, which happens to have this:

[...]
alternative_if_not ARM64_HAS_SYSREG_GIC_CPUIF
	bl	__restore_vgic_v2_state
alternative_else
	bl	__restore_vgic_v3_state
alternative_endif
.endm

.macro save_timer_state
[...]

and for some reason git doesn't use save_timer_state as the context anchor.

>>  	mrs	x3, cntv_ctl_el0
>>  	and	x3, x3, #3
>>  	str	w3, [x0, #VCPU_TIMER_CNTV_CTL]
>> -	bic	x3, x3, #1		// Clear Enable
>> -	msr	cntv_ctl_el0, x3
>>  
>>  	isb
>>  
>> @@ -579,6 +577,8 @@ alternative_endif
>>  	str	x3, [x0, #VCPU_TIMER_CNTV_CVAL]
>>  
>>  1:
>> +	msr	cntv_ctl_el0, xzr
>> +
> 
> We could have a comment here, but ok.

I'll add something.

>>  	// Allow physical timer/counter access for the host
>>  	mrs	x2, cnthctl_el2
>>  	orr	x2, x2, #3
>> -- 
>> 2.1.4
>>
> 
> Otherwise:
> 
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
> 

Thanks!

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

[PATCH 3/3] arm: KVM: Disable virtual timer even if the guest is not using it

[Marc Zyngier]

When running a guest with the architected timer disabled (with QEMU and
the kernel_irqchip=off option, for example), it is important to make
sure the timer gets turned off. Otherwise, the guest may try to
enable it anyway, leading to a screaming HW interrupt.

The fix is to unconditionally turn off the virtual timer on guest
exit.

Cc: stable@vger.kernel.org
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/interrupts_head.S | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S
index 702740d..51a5950 100644
--- a/arch/arm/kvm/interrupts_head.S
+++ b/arch/arm/kvm/interrupts_head.S
@@ -515,8 +515,7 @@ ARM_BE8(rev	r6, r6  )
 
 	mrc	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
 	str	r2, [vcpu, #VCPU_TIMER_CNTV_CTL]
-	bic	r2, #1			@ Clear ENABLE
-	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
+
 	isb
 
 	mrrc	p15, 3, rr_lo_hi(r2, r3), c14	@ CNTV_CVAL
@@ -529,6 +528,9 @@ ARM_BE8(rev	r6, r6  )
 	mcrr	p15, 4, r2, r2, c14	@ CNTVOFF
 
 1:
+	mov	r2, #0			@ Clear ENABLE
+	mcr	p15, 0, r2, c14, c3, 1	@ CNTV_CTL
+
 	@ Allow physical timer/counter access for the host
 	mrc	p15, 4, r2, c14, c1, 0	@ CNTHCTL
 	orr	r2, r2, #(CNTHCTL_PL1PCEN | CNTHCTL_PL1PCTEN)
-- 
2.1.4