Re: [PATCH 2/4] KVM: PPC: Only WARN on invalid emulation

[Gleb Natapov <gleb@redhat.com>]

On Wed, Dec 19, 2012 at 10:59:28AM +0100, Alexander Graf wrote:
> 
> On 19.12.2012, at 10:37, Gleb Natapov wrote:
> 
> > On Wed, Dec 19, 2012 at 12:01:19AM +0100, Alexander Graf wrote:
> >> 
> >> On 18.12.2012, at 23:54, Scott Wood wrote:
> >> 
> >>> On 12/18/2012 06:38:41 AM, Alexander Graf wrote:
> >>>> When we hit an emulation result that we didn't expect, that is an error,
> >>>> but it's nothing that warrants a BUG(), because it can be guest triggered.
> >>>> So instead, let's only WARN() the user that this happened.
> >>>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>> ---
> >>>> arch/powerpc/kvm/powerpc.c |    3 ++-
> >>>> 1 files changed, 2 insertions(+), 1 deletions(-)
> >>>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> >>>> index be83fca..e2225e5 100644
> >>>> --- a/arch/powerpc/kvm/powerpc.c
> >>>> +++ b/arch/powerpc/kvm/powerpc.c
> >>>> @@ -237,7 +237,8 @@ int kvmppc_emulate_mmio(struct kvm_run *run, struct kvm_vcpu *vcpu)
> >>>> 		r = RESUME_HOST;
> >>>> 		break;
> >>>> 	default:
> >>>> -		BUG();
> >>>> +		WARN_ON(1);
> >>>> +		r = RESUME_GUEST;
> >>> 
> >>> Do you have a specific way of a guest triggering this in mind, or is it just being cautious?  The guest probably shouldn't be allowed to spam the kernel log with WARNs either.  Is a traceback even useful here?
> >> 
> >> For debugging, yes. But maybe we would be better off with a trace point. Anyway, a WARN is better than a BUG either way for now.
> >> 
> >> I was able to provoke this by live patching an instruction without flushing the icache, so that the last_inst instruction fetch gets a different instruction from the instruction that resulted in the trap we're currently in.
> >> 
> > If guest can trigger this it better be WARN_ON_ONCE(). Otherwise, as
> > Scott said, guest will be able to spam host kernel log.
> 
> I really think eventually we want a trace point and no WARN at all or all possible EMULATE targets handled, because a guest can legitimately not flush its icache and thus confuse our logic.
> 
> Just consider this patch as a quick fix to make sure we enable people (me) to unload the module still after they hit this case ;). Real fix coming soon.
> 
OK.

--
			Gleb.