From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Subject: Re: [PATCH] ARM: KVM: Cleanup exception injection Date: Thu, 17 Dec 2015 08:59:38 +0000 Message-ID: <5672797A.3050305@arm.com> References: <1450116396-11718-1-git-send-email-marc.zyngier@arm.com> <20151216204018.GF24889@cbox> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id D7137496A4 for ; Thu, 17 Dec 2015 03:57:17 -0500 (EST) Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id JffArYVbZ-PW for ; Thu, 17 Dec 2015 03:57:17 -0500 (EST) Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by mm01.cs.columbia.edu (Postfix) with ESMTP id CE540412A6 for ; Thu, 17 Dec 2015 03:57:16 -0500 (EST) In-Reply-To: <20151216204018.GF24889@cbox> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu To: Christoffer Dall Cc: David Binderman , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org List-Id: kvmarm@lists.cs.columbia.edu On 16/12/15 20:40, Christoffer Dall wrote: > On Mon, Dec 14, 2015 at 06:06:36PM +0000, Marc Zyngier wrote: >> David Binderman reported that the exception injection code had a >> couple of unused variables lingering around. >> >> Upon examination, it looked like this code could do with an >> anticipated spring cleaning, which amounts to deduplicating >> the CPSR/SPSR update. >> >> The spurious variables are removed in the process. >> >> Reported-by: David Binderman >> Signed-off-by: Marc Zyngier >> --- >> arch/arm/kvm/emulate.c | 59 ++++++++++++++++++++------------------------------ >> 1 file changed, 23 insertions(+), 36 deletions(-) >> >> diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c >> index d6c0052..245106e 100644 >> --- a/arch/arm/kvm/emulate.c >> +++ b/arch/arm/kvm/emulate.c >> @@ -275,6 +275,25 @@ static u32 exc_vector_base(struct kvm_vcpu *vcpu) >> return vbar; >> } >> >> +/* Switch to an exception mode, updating both CPSR and SPSR */ >> +static void kvm_update_psr(struct kvm_vcpu *vcpu, unsigned long mode) >> +{ >> + unsigned long cpsr = *vcpu_cpsr(vcpu); >> + u32 sctlr = vcpu->arch.cp15[c1_SCTLR]; >> + >> + *vcpu_cpsr(vcpu) = (cpsr & ~MODE_MASK) | mode; >> + *vcpu_cpsr(vcpu) |= PSR_I_BIT; >> + *vcpu_cpsr(vcpu) &= ~(PSR_IT_MASK | PSR_J_BIT | PSR_E_BIT | PSR_T_BIT); >> + >> + if (sctlr & SCTLR_TE) >> + *vcpu_cpsr(vcpu) |= PSR_T_BIT; >> + if (sctlr & SCTLR_EE) >> + *vcpu_cpsr(vcpu) |= PSR_E_BIT; >> + >> + /* Note: These now point to the mode banked copies */ >> + *vcpu_spsr(vcpu) = cpsr; >> +} >> + >> /** >> * kvm_inject_undefined - inject an undefined exception into the guest >> * @vcpu: The VCPU to receive the undefined exception >> @@ -286,29 +305,13 @@ static u32 exc_vector_base(struct kvm_vcpu *vcpu) >> */ >> void kvm_inject_undefined(struct kvm_vcpu *vcpu) >> { >> - unsigned long new_lr_value; >> - unsigned long new_spsr_value; >> unsigned long cpsr = *vcpu_cpsr(vcpu); >> - u32 sctlr = vcpu->arch.cp15[c1_SCTLR]; >> bool is_thumb = (cpsr & PSR_T_BIT); >> u32 vect_offset = 4; >> u32 return_offset = (is_thumb) ? 2 : 4; >> >> - new_spsr_value = cpsr; >> - new_lr_value = *vcpu_pc(vcpu) - return_offset; >> - >> - *vcpu_cpsr(vcpu) = (cpsr & ~MODE_MASK) | UND_MODE; >> - *vcpu_cpsr(vcpu) |= PSR_I_BIT; >> - *vcpu_cpsr(vcpu) &= ~(PSR_IT_MASK | PSR_J_BIT | PSR_E_BIT | PSR_T_BIT); >> - >> - if (sctlr & SCTLR_TE) >> - *vcpu_cpsr(vcpu) |= PSR_T_BIT; >> - if (sctlr & SCTLR_EE) >> - *vcpu_cpsr(vcpu) |= PSR_E_BIT; >> - >> - /* Note: These now point to UND banked copies */ >> - *vcpu_spsr(vcpu) = cpsr; >> - *vcpu_reg(vcpu, 14) = new_lr_value; >> + kvm_update_psr(vcpu, UND_MODE); >> + *vcpu_reg(vcpu, 14) = *vcpu_pc(vcpu) - return_offset; >> >> /* Branch to exception vector */ >> *vcpu_pc(vcpu) = exc_vector_base(vcpu) + vect_offset; >> @@ -320,30 +323,14 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu) >> */ >> static void inject_abt(struct kvm_vcpu *vcpu, bool is_pabt, unsigned long addr) >> { >> - unsigned long new_lr_value; >> - unsigned long new_spsr_value; >> unsigned long cpsr = *vcpu_cpsr(vcpu); >> - u32 sctlr = vcpu->arch.cp15[c1_SCTLR]; >> bool is_thumb = (cpsr & PSR_T_BIT); >> u32 vect_offset; >> u32 return_offset = (is_thumb) ? 4 : 0; >> bool is_lpae; >> >> - new_spsr_value = cpsr; >> - new_lr_value = *vcpu_pc(vcpu) + return_offset; >> - >> - *vcpu_cpsr(vcpu) = (cpsr & ~MODE_MASK) | ABT_MODE; >> - *vcpu_cpsr(vcpu) |= PSR_I_BIT | PSR_A_BIT; > > seems like you're losing the PSR_A_BIT in kvm_update_psr() now? Ah, nice catch. Indeed, CPSR.A gets set on abort entry, but not on undef. I'll add a check in kvm_update_psr, with a reference to the AArch32.EnterMode() pseudocode from the ARMv8 ARM. > >> - *vcpu_cpsr(vcpu) &= ~(PSR_IT_MASK | PSR_J_BIT | PSR_E_BIT | PSR_T_BIT); >> - >> - if (sctlr & SCTLR_TE) >> - *vcpu_cpsr(vcpu) |= PSR_T_BIT; >> - if (sctlr & SCTLR_EE) >> - *vcpu_cpsr(vcpu) |= PSR_E_BIT; >> - >> - /* Note: These now point to ABT banked copies */ >> - *vcpu_spsr(vcpu) = cpsr; >> - *vcpu_reg(vcpu, 14) = new_lr_value; >> + kvm_update_psr(vcpu, ABT_MODE); >> + *vcpu_reg(vcpu, 14) = *vcpu_pc(vcpu) + return_offset; >> >> if (is_pabt) >> vect_offset = 12; >> -- >> 2.1.4 >> > > Otherwise looks like a welcome cleanup! > > Thanks, > -Christoffer > Thanks, M. -- Jazz is not dead. It just smells funny... From mboxrd@z Thu Jan 1 00:00:00 1970 From: marc.zyngier@arm.com (Marc Zyngier) Date: Thu, 17 Dec 2015 08:59:38 +0000 Subject: [PATCH] ARM: KVM: Cleanup exception injection In-Reply-To: <20151216204018.GF24889@cbox> References: <1450116396-11718-1-git-send-email-marc.zyngier@arm.com> <20151216204018.GF24889@cbox> Message-ID: <5672797A.3050305@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 16/12/15 20:40, Christoffer Dall wrote: > On Mon, Dec 14, 2015 at 06:06:36PM +0000, Marc Zyngier wrote: >> David Binderman reported that the exception injection code had a >> couple of unused variables lingering around. >> >> Upon examination, it looked like this code could do with an >> anticipated spring cleaning, which amounts to deduplicating >> the CPSR/SPSR update. >> >> The spurious variables are removed in the process. >> >> Reported-by: David Binderman >> Signed-off-by: Marc Zyngier >> --- >> arch/arm/kvm/emulate.c | 59 ++++++++++++++++++++------------------------------ >> 1 file changed, 23 insertions(+), 36 deletions(-) >> >> diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c >> index d6c0052..245106e 100644 >> --- a/arch/arm/kvm/emulate.c >> +++ b/arch/arm/kvm/emulate.c >> @@ -275,6 +275,25 @@ static u32 exc_vector_base(struct kvm_vcpu *vcpu) >> return vbar; >> } >> >> +/* Switch to an exception mode, updating both CPSR and SPSR */ >> +static void kvm_update_psr(struct kvm_vcpu *vcpu, unsigned long mode) >> +{ >> + unsigned long cpsr = *vcpu_cpsr(vcpu); >> + u32 sctlr = vcpu->arch.cp15[c1_SCTLR]; >> + >> + *vcpu_cpsr(vcpu) = (cpsr & ~MODE_MASK) | mode; >> + *vcpu_cpsr(vcpu) |= PSR_I_BIT; >> + *vcpu_cpsr(vcpu) &= ~(PSR_IT_MASK | PSR_J_BIT | PSR_E_BIT | PSR_T_BIT); >> + >> + if (sctlr & SCTLR_TE) >> + *vcpu_cpsr(vcpu) |= PSR_T_BIT; >> + if (sctlr & SCTLR_EE) >> + *vcpu_cpsr(vcpu) |= PSR_E_BIT; >> + >> + /* Note: These now point to the mode banked copies */ >> + *vcpu_spsr(vcpu) = cpsr; >> +} >> + >> /** >> * kvm_inject_undefined - inject an undefined exception into the guest >> * @vcpu: The VCPU to receive the undefined exception >> @@ -286,29 +305,13 @@ static u32 exc_vector_base(struct kvm_vcpu *vcpu) >> */ >> void kvm_inject_undefined(struct kvm_vcpu *vcpu) >> { >> - unsigned long new_lr_value; >> - unsigned long new_spsr_value; >> unsigned long cpsr = *vcpu_cpsr(vcpu); >> - u32 sctlr = vcpu->arch.cp15[c1_SCTLR]; >> bool is_thumb = (cpsr & PSR_T_BIT); >> u32 vect_offset = 4; >> u32 return_offset = (is_thumb) ? 2 : 4; >> >> - new_spsr_value = cpsr; >> - new_lr_value = *vcpu_pc(vcpu) - return_offset; >> - >> - *vcpu_cpsr(vcpu) = (cpsr & ~MODE_MASK) | UND_MODE; >> - *vcpu_cpsr(vcpu) |= PSR_I_BIT; >> - *vcpu_cpsr(vcpu) &= ~(PSR_IT_MASK | PSR_J_BIT | PSR_E_BIT | PSR_T_BIT); >> - >> - if (sctlr & SCTLR_TE) >> - *vcpu_cpsr(vcpu) |= PSR_T_BIT; >> - if (sctlr & SCTLR_EE) >> - *vcpu_cpsr(vcpu) |= PSR_E_BIT; >> - >> - /* Note: These now point to UND banked copies */ >> - *vcpu_spsr(vcpu) = cpsr; >> - *vcpu_reg(vcpu, 14) = new_lr_value; >> + kvm_update_psr(vcpu, UND_MODE); >> + *vcpu_reg(vcpu, 14) = *vcpu_pc(vcpu) - return_offset; >> >> /* Branch to exception vector */ >> *vcpu_pc(vcpu) = exc_vector_base(vcpu) + vect_offset; >> @@ -320,30 +323,14 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu) >> */ >> static void inject_abt(struct kvm_vcpu *vcpu, bool is_pabt, unsigned long addr) >> { >> - unsigned long new_lr_value; >> - unsigned long new_spsr_value; >> unsigned long cpsr = *vcpu_cpsr(vcpu); >> - u32 sctlr = vcpu->arch.cp15[c1_SCTLR]; >> bool is_thumb = (cpsr & PSR_T_BIT); >> u32 vect_offset; >> u32 return_offset = (is_thumb) ? 4 : 0; >> bool is_lpae; >> >> - new_spsr_value = cpsr; >> - new_lr_value = *vcpu_pc(vcpu) + return_offset; >> - >> - *vcpu_cpsr(vcpu) = (cpsr & ~MODE_MASK) | ABT_MODE; >> - *vcpu_cpsr(vcpu) |= PSR_I_BIT | PSR_A_BIT; > > seems like you're losing the PSR_A_BIT in kvm_update_psr() now? Ah, nice catch. Indeed, CPSR.A gets set on abort entry, but not on undef. I'll add a check in kvm_update_psr, with a reference to the AArch32.EnterMode() pseudocode from the ARMv8 ARM. > >> - *vcpu_cpsr(vcpu) &= ~(PSR_IT_MASK | PSR_J_BIT | PSR_E_BIT | PSR_T_BIT); >> - >> - if (sctlr & SCTLR_TE) >> - *vcpu_cpsr(vcpu) |= PSR_T_BIT; >> - if (sctlr & SCTLR_EE) >> - *vcpu_cpsr(vcpu) |= PSR_E_BIT; >> - >> - /* Note: These now point to ABT banked copies */ >> - *vcpu_spsr(vcpu) = cpsr; >> - *vcpu_reg(vcpu, 14) = new_lr_value; >> + kvm_update_psr(vcpu, ABT_MODE); >> + *vcpu_reg(vcpu, 14) = *vcpu_pc(vcpu) + return_offset; >> >> if (is_pabt) >> vect_offset = 12; >> -- >> 2.1.4 >> > > Otherwise looks like a welcome cleanup! > > Thanks, > -Christoffer > Thanks, M. -- Jazz is not dead. It just smells funny...