From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48914) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YlEXG-0000eV-Ez for qemu-devel@nongnu.org; Thu, 23 Apr 2015 06:39:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YlEXF-00061L-7P for qemu-devel@nongnu.org; Thu, 23 Apr 2015 06:39:10 -0400 Received: from mail-ie0-x229.google.com ([2607:f8b0:4001:c03::229]:32978) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YlEXF-000616-0b for qemu-devel@nongnu.org; Thu, 23 Apr 2015 06:39:09 -0400 Received: by iecrt8 with SMTP id rt8so57457642iec.0 for ; Thu, 23 Apr 2015 03:39:08 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1429722561-12651-1-git-send-email-greg.bellows@linaro.org> <1429722561-12651-10-git-send-email-greg.bellows@linaro.org> <20150423024929.GC17116@toto> Date: Thu, 23 Apr 2015 20:39:06 +1000 Message-ID: From: "Edgar E. Iglesias" Content-Type: multipart/alternative; boundary=bcaec519611b961a78051461e379 Subject: Re: [Qemu-devel] [PATCH v2 9/9] target-arm: Add WFx instruction trap support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Sergey Fedorov , =?UTF-8?B?QWxleCBCZW5uw6ll?= , QEMU Developers , Greg Bellows --bcaec519611b961a78051461e379 Content-Type: text/plain; charset=UTF-8 On 23/04/2015 6:00 pm, "Peter Maydell" wrote: > > On 23 April 2015 at 03:49, Edgar E. Iglesias wrote: > > On Wed, Apr 22, 2015 at 12:09:21PM -0500, Greg Bellows wrote: > >> void HELPER(wfe)(CPUARMState *env) > >> { > >> CPUState *cs = CPU(arm_env_get_cpu(env)); > >> + int target_el = check_wfx_trap(env, true); > >> > >> /* Don't actually halt the CPU, just yield back to top > >> * level loop > >> */ > >> - cs->exception_index = EXCP_YIELD; > >> + if (target_el) { > >> + cs->exception_index = EXCP_UDEF; > >> + env->exception.syndrome = syn_wfx(1, 0xe, true); > >> + env->exception.target_el = target_el; > >> + env->pc -= 4; > >> + } else { > >> + cs->exception_index = EXCP_YIELD; > >> + } > >> cpu_loop_exit(cs); > >> } > > > > > > Hi Greg, > > > > Before trapping, don't you need to check that the CPU has no actual work? > > e.g: > > if (!cc->has_work(cs) && ..) > > We don't track WFE-wakeup events, so we must always trap. (Well, > I suppose strictly we could also go for "never trap" on the basis > that really our implementation here is pretty much a NOP, but I > think always-trap is closer to reasonable.) > > In theory you could maybe check has_work() for the WFI case, > since doing an EXCP_HLT really should cause us to stop until > has_work is true, but it seems a bit fragile -- could we really > guarantee that nothing would change between this point and > when we went back through the main loop that would change > whether has_work evaluates true or not? I think that it's better > there too to just always take the trap: setting EXCP_HLT is our > "going into a low power state" and so we should take the trap > if we would otherwise have done that. I think functional wise we are OK. The implementation can AFAIK always choose to nop for whatever reason (e.g has_work()). Only when we choose to enter low power, the trap comes into play. Maybe wfe is the most problematic one because it fires more frequently and often when has_work() is true? Cheers, Edgar > > > I think you can still EXCP_YIELD the core if it has work though > > as it's probably a good place to reschedule other CPUs in our > > single-threaded SMP model. > > That is indeed the only reason we do anything with WFE at all > (it used to be a no-op, but yielding gives better performance). > > -- PMM --bcaec519611b961a78051461e379 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On 23/04/2015 6:00 pm, "Peter Maydell" <peter.maydell@linaro.org> wrote:
>
> On 23 April 2015 at 03:49, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > On Wed, Apr 22, 2015 at 12:09:21PM -0500, Greg Bellows wrote:
> >>=C2=A0 void HELPER(wfe)(CPUARMState *env)
> >>=C2=A0 {
> >>=C2=A0 =C2=A0 =C2=A0 CPUState *cs =3D CPU(arm_env_get_cpu(env)= );
> >> +=C2=A0 =C2=A0 int target_el =3D check_wfx_trap(env, true); > >>
> >>=C2=A0 =C2=A0 =C2=A0 /* Don't actually halt the CPU, just = yield back to top
> >>=C2=A0 =C2=A0 =C2=A0 =C2=A0* level loop
> >>=C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> >> -=C2=A0 =C2=A0 cs->exception_index =3D EXCP_YIELD;
> >> +=C2=A0 =C2=A0 if (target_el) {
> >> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 cs->exception_index =3D EXCP_= UDEF;
> >> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 env->exception.syndrome =3D s= yn_wfx(1, 0xe, true);
> >> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 env->exception.target_el =3D = target_el;
> >> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 env->pc -=3D 4;
> >> +=C2=A0 =C2=A0 } else {
> >> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 cs->exception_index =3D EXCP_= YIELD;
> >> +=C2=A0 =C2=A0 }
> >>=C2=A0 =C2=A0 =C2=A0 cpu_loop_exit(cs);
> >>=C2=A0 }
> >
> >
> > Hi Greg,
> >
> > Before trapping, don't you need to check that the CPU has no = actual work?
> > e.g:
> > if (!cc->has_work(cs) && ..)
>
> We don't track WFE-wakeup events, so we must always trap. (Well, > I suppose strictly we could also go for "never trap" on the = basis
> that really our implementation here is pretty much a NOP, but I
> think always-trap is closer to reasonable.)
>
> In theory you could maybe check has_work() for the WFI case,
> since doing an EXCP_HLT really should cause us to stop until
> has_work is true, but it seems a bit fragile -- could we really
> guarantee that nothing would change between this point and
> when we went back through the main loop that would change
> whether has_work evaluates true or not? I think that it's better > there too to just always take the trap: setting EXCP_HLT is our
> "going into a low power state" and so we should take the tra= p
> if we would otherwise have done that.

I think functional wise we are OK.
The implementation can AFAIK always choose to nop for whatever reason (e.g = has_work()). Only when we choose to enter low power, the trap comes into pl= ay.

Maybe wfe is the most problematic one because it fires more = frequently and often when has_work() is true?

Cheers,
Edgar

>
> > I think you can still EXCP_YIELD the core if it has work though > > as it's probably a good place to reschedule other CPUs in our=
> > single-threaded SMP model.
>
> That is indeed the only reason we do anything with WFE at all
> (it used to be a no-op, but yielding gives better performance).
>
> -- PMM

--bcaec519611b961a78051461e379--