| Message ID | 1344985483-7440-18-git-send-email-agraf@suse.de |
|---|---|
| State | New, archived |
| Headers | show |
On 08/14/2012 06:04 PM, Alexander Graf wrote: > Generic KVM code might want to know whether we are inside guest context > or outside. It also wants to be able to push us out of guest context. > > Add support to the BookE code for the generic vcpu->mode field that describes > the above states. > > Signed-off-by: Alexander Graf <agraf@suse.de> > --- > arch/powerpc/kvm/booke.c | 11 +++++++++++ > 1 files changed, 11 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c > index bcf87fe..70a86c0 100644 > --- a/arch/powerpc/kvm/booke.c > +++ b/arch/powerpc/kvm/booke.c > @@ -501,6 +501,15 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu) > continue; > } > > + if (vcpu->mode == EXITING_GUEST_MODE) { > + r = 1; > + break; > + } > + > + /* Going into guest context! Yay! */ > + vcpu->mode = IN_GUEST_MODE; > + smp_wmb(); > + > break; > } Normally on entry to this function mode should be OUTSIDE_GUEST_MODE, right? How could it possibly be EXITING_GUEST_MODE then, since that only replaces IN_GUEST_MODE? This doesn't match what x86 does with mode on entry. Mode is supposed to be set to IN_GUEST_MODE before requests are checked. I'm not sure what the point of EXITING_GUEST_MODE is at all, compared to just waiting until after interrupts are disabled before setting IN_GUEST_MODE (which we do on ppc, but not on x86 even though it seems like a trivial change), plus the existing ordering between mode and requests. -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 15.08.2012, at 02:17, Scott Wood wrote: > On 08/14/2012 06:04 PM, Alexander Graf wrote: >> Generic KVM code might want to know whether we are inside guest context >> or outside. It also wants to be able to push us out of guest context. >> >> Add support to the BookE code for the generic vcpu->mode field that describes >> the above states. >> >> Signed-off-by: Alexander Graf <agraf@suse.de> >> --- >> arch/powerpc/kvm/booke.c | 11 +++++++++++ >> 1 files changed, 11 insertions(+), 0 deletions(-) >> >> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c >> index bcf87fe..70a86c0 100644 >> --- a/arch/powerpc/kvm/booke.c >> +++ b/arch/powerpc/kvm/booke.c >> @@ -501,6 +501,15 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu) >> continue; >> } >> >> + if (vcpu->mode == EXITING_GUEST_MODE) { >> + r = 1; >> + break; >> + } >> + >> + /* Going into guest context! Yay! */ >> + vcpu->mode = IN_GUEST_MODE; >> + smp_wmb(); >> + >> break; >> } > > Normally on entry to this function mode should be OUTSIDE_GUEST_MODE, > right? How could it possibly be EXITING_GUEST_MODE then, since that > only replaces IN_GUEST_MODE? > > This doesn't match what x86 does with mode on entry. Mode is supposed > to be set to IN_GUEST_MODE before requests are checked. > > I'm not sure what the point of EXITING_GUEST_MODE is at all, compared to > just waiting until after interrupts are disabled before setting > IN_GUEST_MODE (which we do on ppc, but not on x86 even though it seems > like a trivial change), plus the existing ordering between mode and > requests. Well, the only real use case I could find for the mode was the remote vcpu kick. If we're not outside of guest mode, we get an IPI to notify us that requests are outstanding. So I only get us into OUTSIDE_GUEST_MODE when we really exit __vcpu_run, thus are in user space. That doesn't reflect what x86 does, right, but so doesn't our whole loop concept. However, since we might do the vcpu_block in this loop, we will never really get into OUTSIDE_GUEST_MODE, right? Hrm. So what would you suggest? Do all the handle_exit in OUTSIDE_GUEST_MODE scope and then reenter IN_GUEST_MODE in prepare_to_enter? We still wouldn't need an EXITING_GUEST_MODE 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
On 08/14/2012 07:26 PM, Alexander Graf wrote: > > On 15.08.2012, at 02:17, Scott Wood wrote: > >> On 08/14/2012 06:04 PM, Alexander Graf wrote: >>> Generic KVM code might want to know whether we are inside guest context >>> or outside. It also wants to be able to push us out of guest context. >>> >>> Add support to the BookE code for the generic vcpu->mode field that describes >>> the above states. >>> >>> Signed-off-by: Alexander Graf <agraf@suse.de> >>> --- >>> arch/powerpc/kvm/booke.c | 11 +++++++++++ >>> 1 files changed, 11 insertions(+), 0 deletions(-) >>> >>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c >>> index bcf87fe..70a86c0 100644 >>> --- a/arch/powerpc/kvm/booke.c >>> +++ b/arch/powerpc/kvm/booke.c >>> @@ -501,6 +501,15 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu) >>> continue; >>> } >>> >>> + if (vcpu->mode == EXITING_GUEST_MODE) { >>> + r = 1; >>> + break; >>> + } >>> + >>> + /* Going into guest context! Yay! */ >>> + vcpu->mode = IN_GUEST_MODE; >>> + smp_wmb(); >>> + >>> break; >>> } >> >> Normally on entry to this function mode should be OUTSIDE_GUEST_MODE, >> right? How could it possibly be EXITING_GUEST_MODE then, since that >> only replaces IN_GUEST_MODE? >> >> This doesn't match what x86 does with mode on entry. Mode is supposed >> to be set to IN_GUEST_MODE before requests are checked. >> >> I'm not sure what the point of EXITING_GUEST_MODE is at all, compared to >> just waiting until after interrupts are disabled before setting >> IN_GUEST_MODE (which we do on ppc, but not on x86 even though it seems >> like a trivial change), plus the existing ordering between mode and >> requests. > > Well, the only real use case I could find for the mode was the remote > vcpu kick. If we're not outside of guest mode, we get an IPI to > notify us that requests are outstanding. I'm curious why this is done so differently for broadcast requests than for single-cpu requests. > So I only get us into OUTSIDE_GUEST_MODE when we really exit > __vcpu_run, thus are in user space. That doesn't reflect what x86 > does, right, but so doesn't our whole loop concept. OK. We still need to do ordering like x86 does, because otherwise there's a race where we could check requests before the request bit is set, and still have make_all_cpus_request see OUTSIDE_GUEST_MODE and not send an IPI. > However, since we might do the vcpu_block in this loop, we will never > really get into OUTSIDE_GUEST_MODE, right? Not when the guest is just idling, only when it's exited to QEMU. > Hrm. So what would you > suggest? Do all the handle_exit in OUTSIDE_GUEST_MODE scope and then > reenter IN_GUEST_MODE in prepare_to_enter? I suggest leaving this optimization until a need is felt. > We still wouldn't need an EXITING_GUEST_MODE though. Right. -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 08/14/2012 06:04 PM, Alexander Graf wrote: > + /* Going into guest context! Yay! */ > + vcpu->mode = IN_GUEST_MODE; > + smp_wmb(); > + > break; > } What is the wmb protecting against here? -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 15.08.2012, at 03:17, Scott Wood wrote: > On 08/14/2012 07:26 PM, Alexander Graf wrote: >> >> On 15.08.2012, at 02:17, Scott Wood wrote: >> >>> On 08/14/2012 06:04 PM, Alexander Graf wrote: >>>> Generic KVM code might want to know whether we are inside guest context >>>> or outside. It also wants to be able to push us out of guest context. >>>> >>>> Add support to the BookE code for the generic vcpu->mode field that describes >>>> the above states. >>>> >>>> Signed-off-by: Alexander Graf <agraf@suse.de> >>>> --- >>>> arch/powerpc/kvm/booke.c | 11 +++++++++++ >>>> 1 files changed, 11 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c >>>> index bcf87fe..70a86c0 100644 >>>> --- a/arch/powerpc/kvm/booke.c >>>> +++ b/arch/powerpc/kvm/booke.c >>>> @@ -501,6 +501,15 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu) >>>> continue; >>>> } >>>> >>>> + if (vcpu->mode == EXITING_GUEST_MODE) { >>>> + r = 1; >>>> + break; >>>> + } >>>> + >>>> + /* Going into guest context! Yay! */ >>>> + vcpu->mode = IN_GUEST_MODE; >>>> + smp_wmb(); >>>> + >>>> break; >>>> } >>> >>> Normally on entry to this function mode should be OUTSIDE_GUEST_MODE, >>> right? How could it possibly be EXITING_GUEST_MODE then, since that >>> only replaces IN_GUEST_MODE? >>> >>> This doesn't match what x86 does with mode on entry. Mode is supposed >>> to be set to IN_GUEST_MODE before requests are checked. >>> >>> I'm not sure what the point of EXITING_GUEST_MODE is at all, compared to >>> just waiting until after interrupts are disabled before setting >>> IN_GUEST_MODE (which we do on ppc, but not on x86 even though it seems >>> like a trivial change), plus the existing ordering between mode and >>> requests. >> >> Well, the only real use case I could find for the mode was the remote >> vcpu kick. If we're not outside of guest mode, we get an IPI to >> notify us that requests are outstanding. > > I'm curious why this is done so differently for broadcast requests than > for single-cpu requests. > >> So I only get us into OUTSIDE_GUEST_MODE when we really exit >> __vcpu_run, thus are in user space. That doesn't reflect what x86 >> does, right, but so doesn't our whole loop concept. > > OK. We still need to do ordering like x86 does, because otherwise > there's a race where we could check requests before the request bit is > set, and still have make_all_cpus_request see OUTSIDE_GUEST_MODE and not > send an IPI. Could you please send a patch showing what workflow you envision? The code as is should work, just be inefficient at times, right? 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
On 08/15/2012 04:29 AM, Alexander Graf wrote: > > On 15.08.2012, at 03:17, Scott Wood wrote: > >> On 08/14/2012 07:26 PM, Alexander Graf wrote: >>> >>> On 15.08.2012, at 02:17, Scott Wood wrote: >>> >>>> On 08/14/2012 06:04 PM, Alexander Graf wrote: >>>>> Generic KVM code might want to know whether we are inside guest context >>>>> or outside. It also wants to be able to push us out of guest context. >>>>> >>>>> Add support to the BookE code for the generic vcpu->mode field that describes >>>>> the above states. >>>>> >>>>> Signed-off-by: Alexander Graf <agraf@suse.de> >>>>> --- >>>>> arch/powerpc/kvm/booke.c | 11 +++++++++++ >>>>> 1 files changed, 11 insertions(+), 0 deletions(-) >>>>> >>>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c >>>>> index bcf87fe..70a86c0 100644 >>>>> --- a/arch/powerpc/kvm/booke.c >>>>> +++ b/arch/powerpc/kvm/booke.c >>>>> @@ -501,6 +501,15 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu) >>>>> continue; >>>>> } >>>>> >>>>> + if (vcpu->mode == EXITING_GUEST_MODE) { >>>>> + r = 1; >>>>> + break; >>>>> + } >>>>> + >>>>> + /* Going into guest context! Yay! */ >>>>> + vcpu->mode = IN_GUEST_MODE; >>>>> + smp_wmb(); >>>>> + >>>>> break; >>>>> } >>>> >>>> Normally on entry to this function mode should be OUTSIDE_GUEST_MODE, >>>> right? How could it possibly be EXITING_GUEST_MODE then, since that >>>> only replaces IN_GUEST_MODE? >>>> >>>> This doesn't match what x86 does with mode on entry. Mode is supposed >>>> to be set to IN_GUEST_MODE before requests are checked. >>>> >>>> I'm not sure what the point of EXITING_GUEST_MODE is at all, compared to >>>> just waiting until after interrupts are disabled before setting >>>> IN_GUEST_MODE (which we do on ppc, but not on x86 even though it seems >>>> like a trivial change), plus the existing ordering between mode and >>>> requests. >>> >>> Well, the only real use case I could find for the mode was the remote >>> vcpu kick. If we're not outside of guest mode, we get an IPI to >>> notify us that requests are outstanding. >> >> I'm curious why this is done so differently for broadcast requests than >> for single-cpu requests. >> >>> So I only get us into OUTSIDE_GUEST_MODE when we really exit >>> __vcpu_run, thus are in user space. That doesn't reflect what x86 >>> does, right, but so doesn't our whole loop concept. >> >> OK. We still need to do ordering like x86 does, because otherwise >> there's a race where we could check requests before the request bit is >> set, and still have make_all_cpus_request see OUTSIDE_GUEST_MODE and not >> send an IPI. > > Could you please send a patch showing what workflow you envision? I'll try to do this tomorrow. > The code as is should work, just be inefficient at times, right? No, we could fail to send the IPI -- couldn't that result in the guest using a stale TLB entry? -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
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index bcf87fe..70a86c0 100644 --- a/arch/powerpc/kvm/booke.c +++ b/arch/powerpc/kvm/booke.c @@ -501,6 +501,15 @@ static int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu) continue; } + if (vcpu->mode == EXITING_GUEST_MODE) { + r = 1; + break; + } + + /* Going into guest context! Yay! */ + vcpu->mode = IN_GUEST_MODE; + smp_wmb(); + break; } @@ -572,6 +581,8 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu) kvm_guest_exit(); out: + vcpu->mode = OUTSIDE_GUEST_MODE; + smp_wmb(); local_irq_enable(); return ret; }
Generic KVM code might want to know whether we are inside guest context or outside. It also wants to be able to push us out of guest context. Add support to the BookE code for the generic vcpu->mode field that describes the above states. Signed-off-by: Alexander Graf <agraf@suse.de> --- arch/powerpc/kvm/booke.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)