On Wed, Jan 30, 2019 at 10:56:19AM +0000, Marc Zyngier wrote:
> 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.
OK, Christoffer also said that information was useful. Would any following
registers also be useful? Or should it be something like
for (num = 1; num < NR_SYS_REGS; num++) {
WARN(__vcpu_sys_reg(vcpu, num) == 0x4242424242424242,
"Didn't reset __vcpu_sys_reg(%zi)\n", num);
break;
}
to ensure the first one, the most important one, is there, and that it
doesn't get pushed out of the buffer by hundreds of more lines?
Thanks,
drew
