On Tue, Jan 29, 2019 at 05:33:37PM +0100, 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);
>
I don't care stongly whichever way we do it, but when we actually saw
this, it seemed useful to see which system register was not initialized.
Thanks for the review.
Christoffer
