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@xxxxxxxxxxx>
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> ---
>> 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...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
