From: Markus Armbruster <armbru@redhat.com>
References: <cover.1432501932.git.crosthwaite.peter@gmail.com>
	<a68f13aedf088eca8021956b980ced6599e26077.1432501932.git.crosthwaite.peter@gmail.com>
	<87lhfjk7w3.fsf@blackfin.pond.sub.org>
	<CAEgOgz5PkZoN5xfJszi1+jp457gdumDyk+-KvmwaAH-qaRzy0w@mail.gmail.com>
Date: Wed, 17 Jun 2015 08:42:20 +0200
In-Reply-To: <CAEgOgz5PkZoN5xfJszi1+jp457gdumDyk+-KvmwaAH-qaRzy0w@mail.gmail.com>
	(Peter Crosthwaite's message of "Tue, 16 Jun 2015 22:39:05 -0700")
Message-ID: <87eglaj0oj.fsf@blackfin.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH v2 1/2] monitor: Split mon_get_cpu fn to
	remove ENV_GET_CPU
To: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>, "qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>, Luiz Capitulino <lcapitulino@redhat.com>, Peter Crosthwaite <crosthwaitepeter@gmail.com>, Andreas =?utf-8?Q?F=C3=A4rber?= <afaerber@suse.de>, Richard Henderson <rth@twiddle.net>

Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> On Tue, Jun 16, 2015 at 8:09 AM, Markus Armbruster <armbru@redhat.com> wr=
ote:
>> Peter Crosthwaite <crosthwaitepeter@gmail.com> writes:
>>
>>> The monitor currently has one helper, mon_get_cpu() which will return
>>> an env pointer. The target specific users of this API want an env, but
>>> all the target agnostic users really just want the cpu pointer. These
>>> users then need to use the target-specifically defined ENV_GET_CPU to
>>> navigate back up to the CPU from the ENV. Split the API for the two
>>> uses cases to remove all need for ENV_GET_CPU.
>>>
>>> Reviewed-by: Richard Henderson <rth@twiddle.net>
>>> Reviewed-by: Andreas F=C3=A4rber <afaerber@suse.de>
>>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>>> ---
>>> Changed since v1:
>>> s/mon_get_env/mon_get_cpu_env (Andreas review)
>>> Avoid C99 declaration (RH review)
>>> ---
>>>  monitor.c | 65
>>> ++++++++++++++++++++++++++++-----------------------------------
>>>  1 file changed, 29 insertions(+), 36 deletions(-)
>>>
>> [...]
>>> @@ -1208,16 +1203,14 @@ static void monitor_printc(Monitor *mon, int c)
>>>  static void memory_dump(Monitor *mon, int count, int format, int wsize,
>>>                          hwaddr addr, int is_physical)
>>>  {
>>> -    CPUArchState *env;
>>>      int l, line_size, i, max_digits, len;
>>>      uint8_t buf[16];
>>>      uint64_t v;
>>>
>>>      if (format =3D=3D 'i') {
>>> -        int flags;
>>> -        flags =3D 0;
>>> -        env =3D mon_get_cpu();
>>> +        int flags =3D 0;
>>>  #ifdef TARGET_I386
>>> +        CPUArchState *env =3D mon_get_cpu_env();
>>>          if (wsize =3D=3D 2) {
>>>              flags =3D 1;
>>>          } else if (wsize =3D=3D 4) {
>>> @@ -1238,10 +1231,11 @@ static void memory_dump(Monitor *mon, int count=
, int format, int wsize,
>>>          }
>>>  #endif
>>>  #ifdef TARGET_PPC
>>> +        CPUArchState *env =3D mon_get_cpu_env();
>>>          flags =3D msr_le << 16;
>>>          flags |=3D env->bfd_mach;
>>>  #endif
>>> -        monitor_disas(mon, env, addr, count, is_physical, flags);
>>> +        monitor_disas(mon, mon_get_cpu_env(), addr, count, is_physical=
, flags);
>>>          return;
>>>      }
>>>
>>> @@ -1280,8 +1274,7 @@ static void memory_dump(Monitor *mon, int count, =
int format, int wsize,
>>>          if (is_physical) {
>>>              cpu_physical_memory_read(addr, buf, l);
>>>          } else {
>>> -            env =3D mon_get_cpu();
>>> -            if (cpu_memory_rw_debug(ENV_GET_CPU(env), addr, buf, l, 0)=
 < 0) {
>>> +            if (cpu_memory_rw_debug(mon_get_cpu(), addr, buf, l, 0) < =
0) {
>>>                  monitor_printf(mon, " Cannot access memory\n");
>>>                  break;
>>>              }
>>
>> You call mon_get_cpu_env() and mon_get_cpu() multiple times in this
>> function, and declare CPUArchState *env twice (which rang my shadowing
>> alarm bell; fortunately the two are under mutually exclusive #ifdef).
>> Why not simply do
>>
>>     CPUState *cpu =3D mon_get_cpu();
>
> This we can do easily and the choice of the existing code is pure
> personal preference. I generally prefer inline calls to trivial
> getters for a low number of call sites as I think code is slightly
> easier to read when you don't have to do a local variable lookup on
> where all the function args are coming from.

Point taken.

The getter isn't quite trivial, though: cpu_synchronize_state().

>>     CPUArchState *env =3D mon_get_cpu_env();
>>
>
> So the multiple decl of env is now needed to avoid an unused variable
> werror for non-x86-non-PPC builds when considering the change in P2.
> Not strictly needed until P2, but doing it this way straight up
> slightly minimises churn. The ENV_GET_CPU removal and the change in P2
> remove the only two unconditional uses of env forcing this variable to
> go local to its #ifdef usages. It also has the advantage that only
> those two arches use the env at all. Envlessness is a good thing for
> common code so prefer to leave the multi-defs in target specific code
> with a plan to get rid of them one by one with X86/PPC specific
> patches that operate solely on their respective #ifdef sections.
>
> FWIW, the follow up to this series adds the infrastructure needed for
> getting rid of these #ifdef sections (once I muster the courage to
> patch X86 and PPC):
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg04742.html
>
>> right at the beginning and be done with it?
>>
>> Not a big deal, I'm willing to take this patch through my tree as is.
>>
>
> Let me know if you need respin for the first change. I can turn it

Your choice.  As I said, I'm willing to take it as is.

> around quickly, i'd really like to get this one through as its
> blocking a lot of the multi-arch work!

Aiming for a pull request this week.