On 29/01/2019 16:33, Andrew Jones wrote:
> On Fri, Jan 25, 2019 at 10:46:56AM +0100, Christoffer Dall wrote:
>> From: Marc Zyngier <marc.zyngier@xxxxxxx>
>>
>> Failing to properly reset system registers is pretty bad. But not
>> quite as bad as bringing the whole machine down... So warn loudly,
>> but slightly more gracefully.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> Acked-by: Christoffer Dall <christoffer.dall@xxxxxxx>
>> ---
>> arch/arm/kvm/coproc.c | 4 ++--
>> arch/arm64/kvm/sys_regs.c | 4 ++--
>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
>> index 222c1635bc7a..e8bd288fd5be 100644
>> --- a/arch/arm/kvm/coproc.c
>> +++ b/arch/arm/kvm/coproc.c
>> @@ -1450,6 +1450,6 @@ void kvm_reset_coprocs(struct kvm_vcpu *vcpu)
>> reset_coproc_regs(vcpu, table, num);
>>
>> for (num = 1; num < NR_CP15_REGS; num++)
>> - if (vcpu_cp15(vcpu, num) == 0x42424242)
>> - panic("Didn't reset vcpu_cp15(vcpu, %zi)", num);
>> + WARN(vcpu_cp15(vcpu, num) == 0x42424242,
>> + "Didn't reset vcpu_cp15(vcpu, %zi)", num);
>> }
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 86096774abcd..4f067545c7d2 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -2609,6 +2609,6 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
>> reset_sys_reg_descs(vcpu, table, num);
>>
>> for (num = 1; num < NR_SYS_REGS; num++)
>> - if (__vcpu_sys_reg(vcpu, num) == 0x4242424242424242)
>> - panic("Didn't reset __vcpu_sys_reg(%zi)", num);
>> + WARN(__vcpu_sys_reg(vcpu, num) == 0x4242424242424242,
>> + "Didn't reset __vcpu_sys_reg(%zi)\n", num);
>> }
>> --
>> 2.18.0
>>
>
> If we only get halfway through resetting, then we'll get a warn splat,
> complete with a backtrace, for each register. Should we do something
> like the following instead?
>
> for (num = 1; num < NR_SYS_REGS; num++)
> if (__vcpu_sys_reg(vcpu, num) == 0x4242424242424242)
> failed++;
> WARN(failed, "Didn't reset %d system registers", failed);
And doing so we'd loose the important bit of information, which is the
position in the table that doesn't get initialized.
The vcpu reset issue is rare enough that nobody noticed it yet (I only
reproduced it twice), and having it to scream 200 times is not really a
concern. What I want to catch is the case where someone has added a new
sysreg in the table, and has failed to provide a working init function.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
