| Message ID | FEA566E6-687E-46DD-9B64-73038645A9E2@suse.de |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Saturday, February 02, 2013 4:09 AM > To: Alexander Graf > Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org > Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest > > On 01/31/2013 06:11:32 PM, Alexander Graf wrote: > > > > On 31.01.2013, at 23:40, Scott Wood wrote: > > > > > 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. > > > > For e500v2 we have 2 fields > > > > * MSR as the guest sees it > > * MSR as we execute when the guest runs > > > > Since we know the MSR when the guest sees it, we can decide what to do > > when we get an unhandled debug interrupt. > > That's not the same thing as making the real MSR[DE] show up in the guest > MSR[DE]. > > There are other problems with sharing -- what happens when both host and guest > try to write to a particular IAC or DAC? > > Also, performance would be pretty awful if the guest has e.g. single stepping in > DBCR0 enabled but MSR[DE]=0, and the host doesn't care about single stepping > (but does want debugging enabled in general). > > > > 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. > > > > Why not? That's the whole point of jumping through user space. > > That's still needed for software breakpoints, which don't rely on the debug > resources. > > > 1) guest exits with debug interrupt > > 2) QEMU gets a debug exit > > 3) QEMU checks in its list whether it belongs to its own debug > > points > > 4) if not, it reinjects the interrupt into the guest > > > > Step 4 is pretty difficult to do when we don't know whether the guest > > is actually capable of handling debug interrupts at that moment. > > Software breakpoints take a Program interrupt rather than a Debug interrupt, > unless MSR[DE]=1 and DBCR0[TRAP]=1. If the guest does not own debug resources > we should always send it to the Program interrupt, so MSR[DE] doesn't matter. > > > > 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. > > > > Well, I'm mostly bouncing ideas here to find a way to express what > > we're trying to say in a way that someone who hasn't read this email > > thread would still understand what's going on :). > > I think it's already straightforward enough if you accept that shared debug > resources aren't supported, and that we are either in a mode where the real > MSR[DE] reflects the guest MSR[DE], or a mode where the real MSR[DE] is always > on in guest mode and the guest MSR[DE] is irrelevant. > > > How about this version? > > > > > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index > > 38a62ef..9929c41 100644 > > --- a/arch/powerpc/kvm/booke.c > > +++ b/arch/powerpc/kvm/booke.c > > @@ -133,6 +133,28 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu > > *vcpu) > > #endif > > } > > > > +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu) { #ifndef > > +CONFIG_KVM_BOOKE_HV > > + /* Synchronize guest's desire to get debug interrupts into > > shadow MSR */ > > + vcpu->arch.shadow_msr &= ~MSR_DE; > > + vcpu->arch.shadow_msr |= vcpu->arch.shared->msr & MSR_DE; #endif > > + > > + /* Force enable debug interrupts when user space wants to debug > > */ > > + if (vcpu->guest_debug) { > > +#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; > > +#else > > + vcpu->arch.shadow_msr |= MSR_DE; > > +#endif > > + } > > +} > > This shows "guest's desire to get debug interrupts" in a context that is not > specifically for !vcpu->guest_debug, which is misleading. > > > + > > /* > > * Helper function for "full" MSR writes. No need to call this if > > only > > * EE/CE/ME/DE/RI are changing. > > @@ -150,6 +172,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 > > new_msr) > > kvmppc_mmu_msr_notify(vcpu, old_msr); > > kvmppc_vcpu_sync_spe(vcpu); > > kvmppc_vcpu_sync_fpu(vcpu); > > + kvmppc_vcpu_sync_debug(vcpu); > > } > > > > static void kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu, > > > > > > My main concern here is that we don't know when to remove MSR_DE again > > from the (shadow) MSR. So how about this one instead? > > Why wouldn't you know this? if (vcpu->guest_debug) { you never remove it } else > { just copy whatever's in guest MSR } I think we are ok with shadow_msr on e500v2 but we can have problem on bookehv where we do not know when to clear MSR_DE in shared->msr. How it works on e500mc: (1) User-space makes ioctl to use debug resource, we set vcpu->guest_debug. (2) Before entering into the guest we check vcpu->guest_debug flag and if set we set MSR_DE in shared->msr. (3) Sometime later user-space releases the debug resource then in ioctl handling will clear vcpu->guest_debug. (4) Now when entering to guest we do not know what to do with MSR_DE in shared->msr as we do now know if guest might have tried to set/clear MSR_DE in between step (2) and step(3). What should be safe thing to do? Can we leave MSR_DE set or clear MSR_DE. If we want to clear MSR_DE then will it be good idea to clear this in step (3) above (in ioctl where we clear vcpu->guest_debug). On e500v2 we have separate shadow_msr and shared->msr so the flow can be: (1) User-space makes ioctl to use debug resource, we set vcpu->guest_debug. (2) Before entering into the guest we check vcpu->guest_debug flag and we set MSR_DE in vcpu->arch.shadow_msr. And if vcpu->guest_debug flag is not set then use vcpu->arch.shared->msr for MSR_DE in shadow_msr. (3) User-space releases the debug resource then in ioctl handling will clear vcpu->guest_debug. (4) Now when entering to guest set shadow_msr as per vcpu->arch.shared->msr. Thanks -Bharat > > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index > > 38a62ef..2676703 100644 > > --- a/arch/powerpc/kvm/booke.c > > +++ b/arch/powerpc/kvm/booke.c > > @@ -142,7 +142,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 > > new_msr) > > u32 old_msr = vcpu->arch.shared->msr; > > > > #ifdef CONFIG_KVM_BOOKE_HV > > - new_msr |= MSR_GS; > > + new_msr |= MSR_GS | MSR_DE; > > #endif > > > > vcpu->arch.shared->msr = new_msr; > > > > > > That would semantically move e500mc to the same logic as e500v2. With > > the main difference that we have no idea what MSR_DE value the guest > > really wanted to have set. > > This would break the case where the guest owns the debug resources. > > > If I read the spec correctly, rfci traps. > > rfdi is the relevant one for e500mc, but yes. > > > So we know the time frame from [inject debug interrupt ... rfci]. > > During that time we know for sure that the guest thinks MSR_DE is 0. > > No, we don't. The guest could have tried to use mtmsr or rfi to enable MSR[DE]. > It could have seen the context it came from was userspace, and scheduled to > another process, etc. > > > Outside of that context, we just have to assume the guest can always > > receive debug interrupts if it configured them. > > No. > > -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Saturday, February 02, 2013 4:09 AM > To: Alexander Graf > Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org > Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest > > On 01/31/2013 06:11:32 PM, Alexander Graf wrote: > > > > On 31.01.2013, at 23:40, Scott Wood wrote: > > > > > 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. > > > > For e500v2 we have 2 fields > > > > * MSR as the guest sees it > > * MSR as we execute when the guest runs > > > > Since we know the MSR when the guest sees it, we can decide what to do > > when we get an unhandled debug interrupt. > > That's not the same thing as making the real MSR[DE] show up in the guest > MSR[DE]. > > There are other problems with sharing -- what happens when both host and guest > try to write to a particular IAC or DAC? > > Also, performance would be pretty awful if the guest has e.g. single stepping in > DBCR0 enabled but MSR[DE]=0, and the host doesn't care about single stepping > (but does want debugging enabled in general). > > > > 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. > > > > Why not? That's the whole point of jumping through user space. > > That's still needed for software breakpoints, which don't rely on the debug > resources. > > > 1) guest exits with debug interrupt > > 2) QEMU gets a debug exit > > 3) QEMU checks in its list whether it belongs to its own debug > > points > > 4) if not, it reinjects the interrupt into the guest > > > > Step 4 is pretty difficult to do when we don't know whether the guest > > is actually capable of handling debug interrupts at that moment. > > Software breakpoints take a Program interrupt rather than a Debug interrupt, > unless MSR[DE]=1 and DBCR0[TRAP]=1. If the guest does not own debug resources > we should always send it to the Program interrupt, so MSR[DE] doesn't matter. > > > > 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. > > > > Well, I'm mostly bouncing ideas here to find a way to express what > > we're trying to say in a way that someone who hasn't read this email > > thread would still understand what's going on :). > > I think it's already straightforward enough if you accept that shared debug > resources aren't supported, and that we are either in a mode where the real > MSR[DE] reflects the guest MSR[DE], or a mode where the real MSR[DE] is always > on in guest mode and the guest MSR[DE] is irrelevant. > > > How about this version? > > > > > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index > > 38a62ef..9929c41 100644 > > --- a/arch/powerpc/kvm/booke.c > > +++ b/arch/powerpc/kvm/booke.c > > @@ -133,6 +133,28 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu > > *vcpu) > > #endif > > } > > > > +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu) { #ifndef > > +CONFIG_KVM_BOOKE_HV > > + /* Synchronize guest's desire to get debug interrupts into > > shadow MSR */ > > + vcpu->arch.shadow_msr &= ~MSR_DE; > > + vcpu->arch.shadow_msr |= vcpu->arch.shared->msr & MSR_DE; #endif > > + > > + /* Force enable debug interrupts when user space wants to debug > > */ > > + if (vcpu->guest_debug) { > > +#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; > > +#else > > + vcpu->arch.shadow_msr |= MSR_DE; > > +#endif > > + } > > +} > > This shows "guest's desire to get debug interrupts" in a context that is not > specifically for !vcpu->guest_debug, which is misleading. > > > + > > /* > > * Helper function for "full" MSR writes. No need to call this if > > only > > * EE/CE/ME/DE/RI are changing. > > @@ -150,6 +172,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 > > new_msr) > > kvmppc_mmu_msr_notify(vcpu, old_msr); > > kvmppc_vcpu_sync_spe(vcpu); > > kvmppc_vcpu_sync_fpu(vcpu); > > + kvmppc_vcpu_sync_debug(vcpu); > > } > > > > static void kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu, > > > > > > My main concern here is that we don't know when to remove MSR_DE again > > from the (shadow) MSR. So how about this one instead? > > Why wouldn't you know this? if (vcpu->guest_debug) { you never remove it } else > { just copy whatever's in guest MSR } Once MSR_DE set because of vcpu->guest_debug, it will remains set even if vcpu->guest_debug is not set, until guest clears msr_de itself. And guest may not clear MSR_DE because it have never set it. Thanks -Bharat > > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index > > 38a62ef..2676703 100644 > > --- a/arch/powerpc/kvm/booke.c > > +++ b/arch/powerpc/kvm/booke.c > > @@ -142,7 +142,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 > > new_msr) > > u32 old_msr = vcpu->arch.shared->msr; > > > > #ifdef CONFIG_KVM_BOOKE_HV > > - new_msr |= MSR_GS; > > + new_msr |= MSR_GS | MSR_DE; > > #endif > > > > vcpu->arch.shared->msr = new_msr; > > > > > > That would semantically move e500mc to the same logic as e500v2. With > > the main difference that we have no idea what MSR_DE value the guest > > really wanted to have set. > > This would break the case where the guest owns the debug resources. > > > If I read the spec correctly, rfci traps. > > rfdi is the relevant one for e500mc, but yes. > > > So we know the time frame from [inject debug interrupt ... rfci]. > > During that time we know for sure that the guest thinks MSR_DE is 0. > > No, we don't. The guest could have tried to use mtmsr or rfi to enable MSR[DE]. > It could have seen the context it came from was userspace, and scheduled to > another process, etc. > > > Outside of that context, we just have to assume the guest can always > > receive debug interrupts if it configured them. > > No. > > -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07.02.2013, at 16:00, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- >> From: Wood Scott-B07421 >> Sent: Saturday, February 02, 2013 4:09 AM >> To: Alexander Graf >> Cc: Bhushan Bharat-R65777; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org >> Subject: Re: [PATCH 8/8] KVM:PPC:booke: Allow debug interrupt injection to guest >> >> On 01/31/2013 06:11:32 PM, Alexander Graf wrote: >>> >>> On 31.01.2013, at 23:40, Scott Wood wrote: >>> >>>> 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. >>> >>> For e500v2 we have 2 fields >>> >>> * MSR as the guest sees it >>> * MSR as we execute when the guest runs >>> >>> Since we know the MSR when the guest sees it, we can decide what to do >>> when we get an unhandled debug interrupt. >> >> That's not the same thing as making the real MSR[DE] show up in the guest >> MSR[DE]. >> >> There are other problems with sharing -- what happens when both host and guest >> try to write to a particular IAC or DAC? >> >> Also, performance would be pretty awful if the guest has e.g. single stepping in >> DBCR0 enabled but MSR[DE]=0, and the host doesn't care about single stepping >> (but does want debugging enabled in general). >> >>>> 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. >>> >>> Why not? That's the whole point of jumping through user space. >> >> That's still needed for software breakpoints, which don't rely on the debug >> resources. >> >>> 1) guest exits with debug interrupt >>> 2) QEMU gets a debug exit >>> 3) QEMU checks in its list whether it belongs to its own debug >>> points >>> 4) if not, it reinjects the interrupt into the guest >>> >>> Step 4 is pretty difficult to do when we don't know whether the guest >>> is actually capable of handling debug interrupts at that moment. >> >> Software breakpoints take a Program interrupt rather than a Debug interrupt, >> unless MSR[DE]=1 and DBCR0[TRAP]=1. If the guest does not own debug resources >> we should always send it to the Program interrupt, so MSR[DE] doesn't matter. >> >>>> 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. >>> >>> Well, I'm mostly bouncing ideas here to find a way to express what >>> we're trying to say in a way that someone who hasn't read this email >>> thread would still understand what's going on :). >> >> I think it's already straightforward enough if you accept that shared debug >> resources aren't supported, and that we are either in a mode where the real >> MSR[DE] reflects the guest MSR[DE], or a mode where the real MSR[DE] is always >> on in guest mode and the guest MSR[DE] is irrelevant. >> >>> How about this version? >>> >>> >>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index >>> 38a62ef..9929c41 100644 >>> --- a/arch/powerpc/kvm/booke.c >>> +++ b/arch/powerpc/kvm/booke.c >>> @@ -133,6 +133,28 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu >>> *vcpu) >>> #endif >>> } >>> >>> +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu) { #ifndef >>> +CONFIG_KVM_BOOKE_HV >>> + /* Synchronize guest's desire to get debug interrupts into >>> shadow MSR */ >>> + vcpu->arch.shadow_msr &= ~MSR_DE; >>> + vcpu->arch.shadow_msr |= vcpu->arch.shared->msr & MSR_DE; #endif >>> + >>> + /* Force enable debug interrupts when user space wants to debug >>> */ >>> + if (vcpu->guest_debug) { >>> +#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; >>> +#else >>> + vcpu->arch.shadow_msr |= MSR_DE; >>> +#endif >>> + } >>> +} >> >> This shows "guest's desire to get debug interrupts" in a context that is not >> specifically for !vcpu->guest_debug, which is misleading. >> >>> + >>> /* >>> * Helper function for "full" MSR writes. No need to call this if >>> only >>> * EE/CE/ME/DE/RI are changing. >>> @@ -150,6 +172,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 >>> new_msr) >>> kvmppc_mmu_msr_notify(vcpu, old_msr); >>> kvmppc_vcpu_sync_spe(vcpu); >>> kvmppc_vcpu_sync_fpu(vcpu); >>> + kvmppc_vcpu_sync_debug(vcpu); >>> } >>> >>> static void kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu, >>> >>> >>> My main concern here is that we don't know when to remove MSR_DE again >>> from the (shadow) MSR. So how about this one instead? >> >> Why wouldn't you know this? if (vcpu->guest_debug) { you never remove it } else >> { just copy whatever's in guest MSR } > > Once MSR_DE set because of vcpu->guest_debug, it will remains set even if vcpu->guest_debug is not set, until guest clears msr_de itself. And guest may not clear MSR_DE because it have never set it. That would create a time frame where the guest maybe doesn't want debug interrupts, but would get them. I'm not sure how bad that would be though. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index 38a62ef..9929c41 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -133,6 +133,28 @@ static void kvmppc_vcpu_sync_fpu(struct kvm_vcpu *vcpu) #endif } +static void kvmppc_vcpu_sync_debug(struct kvm_vcpu *vcpu) +{ +#ifndef CONFIG_KVM_BOOKE_HV + /* Synchronize guest's desire to get debug interrupts into shadow MSR */ + vcpu->arch.shadow_msr &= ~MSR_DE; + vcpu->arch.shadow_msr |= vcpu->arch.shared->msr & MSR_DE; +#endif + + /* Force enable debug interrupts when user space wants to debug */ + if (vcpu->guest_debug) { +#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; +#else + vcpu->arch.shadow_msr |= MSR_DE; +#endif + } +} + /* * Helper function for "full" MSR writes. No need to call this if only * EE/CE/ME/DE/RI are changing. @@ -150,6 +172,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr) kvmppc_mmu_msr_notify(vcpu, old_msr); kvmppc_vcpu_sync_spe(vcpu); kvmppc_vcpu_sync_fpu(vcpu); + kvmppc_vcpu_sync_debug(vcpu); } static void kvmppc_booke_queue_irqprio(struct kvm_vcpu *vcpu, My main concern here is that we don't know when to remove MSR_DE again from the (shadow) MSR. So how about this one instead? diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index 38a62ef..2676703 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -142,7 +142,7 @@ void kvmppc_set_msr(struct kvm_vcpu *vcpu, u32 new_msr) u32 old_msr = vcpu->arch.shared->msr; #ifdef CONFIG_KVM_BOOKE_HV - new_msr |= MSR_GS; + new_msr |= MSR_GS | MSR_DE; #endif vcpu->arch.shared->msr = new_msr;