Re: [Qemu-devel] [PATCH v2 1/2] monitor: Split mon_get_cpu fn to remove ENV_GET_CPU

[Markus Armbruster]

Peter Crosthwaite <address@hidden> writes:

> On Tue, Jun 16, 2015 at 8:09 AM, Markus Armbruster <address@hidden> wrote:
>> Peter Crosthwaite <address@hidden> 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 <address@hidden>
>>> Reviewed-by: Andreas Färber <address@hidden>
>>> Signed-off-by: Peter Crosthwaite <address@hidden>
>>> ---
>>> 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 == 'i') {
>>> -        int flags;
>>> -        flags = 0;
>>> -        env = mon_get_cpu();
>>> +        int flags = 0;
>>>  #ifdef TARGET_I386
>>> +        CPUArchState *env = mon_get_cpu_env();
>>>          if (wsize == 2) {
>>>              flags = 1;
>>>          } else if (wsize == 4) {
>>> @@ -1238,10 +1231,11 @@ static void memory_dump(Monitor *mon, int count, 
>>> int format, int wsize,
>>>          }
>>>  #endif
>>>  #ifdef TARGET_PPC
>>> +        CPUArchState *env = mon_get_cpu_env();
>>>          flags = msr_le << 16;
>>>          flags |= 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 = 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 = 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 = 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.