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

[Peter Crosthwaite]

On Tue, Jun 16, 2015 at 11:42 PM, Markus Armbruster <address@hidden> wrote:
> 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.
>

Thanks, as it is queued I will leave it and I have made a note in my
todos to follow this up.

Regards,
Peter

>> 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.
>