* [PATCH] ARM: KVM: Cleanup exception injection
@ 2015-12-14 18:06 ` Marc Zyngier
0 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2015-12-14 18:06 UTC (permalink / raw)
To: kvmarm, linux-arm-kernel; +Cc: David Binderman
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 <dcb314@hotmail.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
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;
- *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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] ARM: KVM: Cleanup exception injection
@ 2015-12-14 18:06 ` Marc Zyngier
0 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2015-12-14 18:06 UTC (permalink / raw)
To: linux-arm-kernel
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 <dcb314@hotmail.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
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;
- *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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ARM: KVM: Cleanup exception injection
2015-12-14 18:06 ` Marc Zyngier
@ 2015-12-16 20:40 ` Christoffer Dall
-1 siblings, 0 replies; 6+ messages in thread
From: Christoffer Dall @ 2015-12-16 20:40 UTC (permalink / raw)
To: Marc Zyngier; +Cc: David Binderman, kvmarm, linux-arm-kernel
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 <dcb314@hotmail.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> 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?
> - *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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: KVM: Cleanup exception injection
@ 2015-12-16 20:40 ` Christoffer Dall
0 siblings, 0 replies; 6+ messages in thread
From: Christoffer Dall @ 2015-12-16 20:40 UTC (permalink / raw)
To: linux-arm-kernel
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 <dcb314@hotmail.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> 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?
> - *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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ARM: KVM: Cleanup exception injection
2015-12-16 20:40 ` Christoffer Dall
@ 2015-12-17 8:59 ` Marc Zyngier
-1 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2015-12-17 8:59 UTC (permalink / raw)
To: Christoffer Dall; +Cc: David Binderman, kvmarm, linux-arm-kernel
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 <dcb314@hotmail.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> 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...
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM: KVM: Cleanup exception injection
@ 2015-12-17 8:59 ` Marc Zyngier
0 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2015-12-17 8:59 UTC (permalink / raw)
To: linux-arm-kernel
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 <dcb314@hotmail.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> 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...
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-12-17 8:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-14 18:06 [PATCH] ARM: KVM: Cleanup exception injection Marc Zyngier
2015-12-14 18:06 ` Marc Zyngier
2015-12-16 20:40 ` Christoffer Dall
2015-12-16 20:40 ` Christoffer Dall
2015-12-17 8:59 ` Marc Zyngier
2015-12-17 8:59 ` Marc Zyngier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.