Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest

[Scott Wood <scottwood@freescale.com>]

On 01/31/2013 01:20:39 PM, Alexander Graf wrote:
> 
> On 31.01.2013, at 20:05, Alexander Graf wrote:
> 
> >
> > On 31.01.2013, at 19:54, Scott Wood wrote:
> >
> >> On 01/31/2013 12:52:41 PM, Alexander Graf wrote:
> >>> On 31.01.2013, at 19:43, Scott Wood wrote:
> >>>> On 01/31/2013 12:21:07 PM, Alexander Graf wrote:
> >>>>> How about something like this? Then both targets at least suck  
> as much :).
> >>>>
> >>>> I'm not sure that should be the goal...
> >>>>
> >>>>> Thanks to e500mc's awful hardware design, we don't know who  
> sets the MSR_DE bit. Once we forced it onto the guest, we have no  
> change to know whether the guest also set it or not. We could only  
> guess.
> >>>>
> >>>> MSRP[DEP] can prevent the guest from modifying MSR[DE] -- but we  
> still need to set it in the first place.
> >>>>
> >>>> According to ISA V2.06B, the hypervisor should set DBCR0[EDM] to  
> let the guest know that the debug resources are not available, and  
> that "the value of MSR[DE] is not specified and not modifiable".
> >>> So what would the guest do then to tell the hypervisor that it  
> actually wants to know about debug events?
> >>
> >> The guest is out of luck, just as if a JTAG were in use.
> >
> > Hrm.
> >
> > Can we somehow generalize this "out of luck" behavior?
> >
> > Every time we would set or clear an MSR bit in shadow_msr on  
> e500v2, we would instead set or clear it in the real MSR. That way  
> only e500mc is out of luck, but the code would still be shared.

I don't follow.  e500v2 is just as out-of-luck.  The mechanism simply  
does not support sharing debug resources.

What do you mean by "the real MSR"?  The real MSR is shadow_msr, and  
MSR_DE must always be set there if the host is debugging the guest.  As  
for reflecting it into the guest MSR, we could, but I don't really see  
the point.  We're never going to actually send a debug exception to the  
guest when the host owns the debug resources.

Speaking of naming issues, "guest_debug" is very ambiguous...

> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 38a62ef..9bdb845 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -133,6 +133,29 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu  
> *vcpu)
>  #endif
>  }
> 
> +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu)
> +{
> +	u32 is_debug = vcpu->arch.shared->msr & MSR_DE;
> +
> +	/* Force debug to on in guest space when user space wants to  
> debug */
> +	if (vcpu->guest_debug)
> +		is_debug = MSR_DE;
> +
> +#ifdef CONFIG_KVM_BOOKE_HV
> +	/*
> +	 * Since there is no shadow MSR, sync MSR_DE into the guest
> +	 * visible MSR.
> +	 */
> +	vcpu->arch.shared->msr &= ~MSR_DE;
> +	vcpu->arch.shared->msr |= is_debug;
> +#endif
> +
> +#ifndef CONFIG_KVM_BOOKE_HV
> +	vcpu->arch.shadow_msr &= ~MSR_DE;
> +	vcpu->arch.shadow_msr |= is_debug;
> +#endif
> +}

The "&= ~MSR_DE" line is pointless on bookehv, and makes it harder to  
read.  I had to stare at it a while before noticing that you initially  
set is_debug from the guest MSR and that you'd never really clear  
MSR_DE here on bookehv.

-Scott