Re: [Qemu-devel] [PATCH v3 18/39] target/mips: Use env_cpu, env_archcpu

[Aleksandar Markovic]

On May 8, 2019 4:33 PM, "Richard Henderson" 
wrote:
>
> On 5/8/19 1:15 AM, Aleksandar Markovic wrote:
> >
> > On May 8, 2019 2:19 AM, "Richard Henderson" <
richard.hender...@linaro.org
> > > wrote:
> >>
> >>
> >>
> >
> > This commit message doesnˊt explain the reason for the change, and why
is this
> > an improvement. The underlyng reason for distingishing between  env_cpu
and
> > env_archcpu cases is not explained too.
>
> It's certainly explained in the preceeding patches that introduce those
functions.
>
> Are you suggesting that it is beneficial to copy-and-paste a common block
> explanation into 21 commit messages for each of target/foo/?

My objection, as I am the maintainer for MIPS part, is about this very
commit.

If you can't put together a classical standalone commit message which will
be according to our guidelines for writing a good commit message, the
minimum I expect from you is the following commit message:

“Please refer to the commit message(s) for commit(s)  for
details.”

If I were you, I would do the same in all similar cases, but again, at this
moment I am talking about this commit only, and I am insisting on not
allowing empty commit messages for any code I maintain, without exceptions.

Regards,
Aleksandar

>
> r~

Re: [Qemu-devel] [PATCH v3 18/39] target/mips: Use env_cpu, env_archcpu

[Aleksandar Markovic]

On May 8, 2019 4:33 PM, "Richard Henderson" 
wrote:
>
> On 5/8/19 1:15 AM, Aleksandar Markovic wrote:
> >
> > On May 8, 2019 2:19 AM, "Richard Henderson" <
richard.hender...@linaro.org
> > > wrote:
> >>
> >>
> >>
> >
> > This commit message doesnˊt explain the reason for the change, and why
is this
> > an improvement. The underlyng reason for distingishing between  env_cpu
and
> > env_archcpu cases is not explained too.
>
> It's certainly explained in the preceeding patches that introduce those
functions.
>

A commit (code+message) should be as standalone as possible, and one should
not be forced to resort to reverse-engineering and perusing mailing list or
patchwork in order to reveal its true meaning in another commit message
altogether.

Thanks,
Aleksandar

> Are you suggesting that it is beneficial to copy-and-paste a common block
> explanation into 21 commit messages for each of target/foo/?
>
>
> r~

Re: [Qemu-devel] [PATCH v3 18/39] target/mips: Use env_cpu, env_archcpu

[Aleksandar Markovic]

On May 8, 2019 11:53 PM, "Philippe Mathieu-Daudé"  wrote:
>
> Hi Richard, Aleksandar.
>
> On 5/8/19 4:32 PM, Richard Henderson wrote:
> > On 5/8/19 1:15 AM, Aleksandar Markovic wrote:
> >>
> >> On May 8, 2019 2:19 AM, "Richard Henderson" <
richard.hender...@linaro.org
> >> > wrote:
> >>>
> >>>
> >>>
> >>
> >> This commit message doesnˊt explain the reason for the change, and why
is this
> >> an improvement. The underlyng reason for distingishing between
env_cpu and
> >> env_archcpu cases is not explained too.
> >
> > It's certainly explained in the preceeding patches that introduce those
functions.
> >
> > Are you suggesting that it is beneficial to copy-and-paste a common
block
> > explanation into 21 commit messages for each of target/foo/?
>
>
> *) Richard:
>
> I tried to put myself in Aleksandar shoes. I believe Aleksandar is
> worried about his MIPS maintainer duty, wanting to Ack-by this patch.
>
> It is true that out of the context of the series, it is hard to see what
> is the problem you try to solve.
>
> You could copy/paste the explanation you used previously,
> with s/$arch/mips/:
>
> "Cleanup in the boilerplate that each target must define."
>
> "Combined uses of CPU(mips_env_get_cpu()) were failures to use
> the more proper, ENV_GET_CPU macro, now replaced by env_cpu."
>
> Now to clearly understand this patch we still need to look at the
> previous two arch-generic patches
> - "cpu: Replace ENV_GET_CPU with env_cpu" and
> - "cpu: Introduce env_archcpu".
>
> Also, it is tedious to copy/paste the same explanation, but thinking of
> forks or stable branch that cherry-pick not all but some commits of a
> series, it might be useful.
>
> Another guess is Aleksandar might have looked at the series cover, which
> is not well explained as your v2:
> https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07635.html
> I think you mistakenly copied the v1 blurb instead of the v2 one.
>
> So at some point I can understand Aleksandar frustation.
>
>
> *) Aleksandar:
>
> This series fall under the "Overall Guest CPU cores (TCG)" section
> maintained by Richard and Paolo. I think you have to see this series as
> a whole to understand the benefits of it.
>
> With the same reasoning, I believe you shouldn't worry to not give your
> Ack if you don't feel comfortable.
>
> I think Richard sent this v3 to simply address comments raised by the
> previous reviewer during v1/v2, where there was some discussions: I took
> it as "this is the last round before getting merged" (unless someone
> object).
>
> It is hard to make everybody happy on a such big project, with so many
> areas, lines of code, people, culture, etc... I believe we all try to
> give our best, neither the commiters nor the reviewers are perfect, but
> slowly we help this project to improve :)
>
>
> Best regards,
>
> Phil.

Richard, Philippe,

A commit message along the line that Philippe put together would be OK.

I can talk about this commit only - if other submaintainers are fine with
empty commit messages in key files for their target, that is their
business. I am certainly opposed to any empty commit messages in MIPS
files, and please, Richard, include a decent commit message for this
commit. I don't think I am asking much.

Thanks,
Aleksandar

Re: [Qemu-devel] [PATCH v3 18/39] target/mips: Use env_cpu, env_archcpu

[Philippe Mathieu-Daudé]

Hi Richard, Aleksandar.

On 5/8/19 4:32 PM, Richard Henderson wrote:
> On 5/8/19 1:15 AM, Aleksandar Markovic wrote:
>>
>> On May 8, 2019 2:19 AM, "Richard Henderson" > > wrote:
>>>
>>>
>>>
>>
>> This commit message doesnˊt explain the reason for the change, and why is 
>> this
>> an improvement. The underlyng reason for distingishing between  env_cpu and
>> env_archcpu cases is not explained too.
> 
> It's certainly explained in the preceeding patches that introduce those 
> functions.
> 
> Are you suggesting that it is beneficial to copy-and-paste a common block
> explanation into 21 commit messages for each of target/foo/?


*) Richard:

I tried to put myself in Aleksandar shoes. I believe Aleksandar is
worried about his MIPS maintainer duty, wanting to Ack-by this patch.

It is true that out of the context of the series, it is hard to see what
is the problem you try to solve.

You could copy/paste the explanation you used previously,
with s/$arch/mips/:

"Cleanup in the boilerplate that each target must define."

"Combined uses of CPU(mips_env_get_cpu()) were failures to use
the more proper, ENV_GET_CPU macro, now replaced by env_cpu."

Now to clearly understand this patch we still need to look at the
previous two arch-generic patches
- "cpu: Replace ENV_GET_CPU with env_cpu" and
- "cpu: Introduce env_archcpu".

Also, it is tedious to copy/paste the same explanation, but thinking of
forks or stable branch that cherry-pick not all but some commits of a
series, it might be useful.

Another guess is Aleksandar might have looked at the series cover, which
is not well explained as your v2:
https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07635.html
I think you mistakenly copied the v1 blurb instead of the v2 one.

So at some point I can understand Aleksandar frustation.


*) Aleksandar:

This series fall under the "Overall Guest CPU cores (TCG)" section
maintained by Richard and Paolo. I think you have to see this series as
a whole to understand the benefits of it.

With the same reasoning, I believe you shouldn't worry to not give your
Ack if you don't feel comfortable.

I think Richard sent this v3 to simply address comments raised by the
previous reviewer during v1/v2, where there was some discussions: I took
it as "this is the last round before getting merged" (unless someone
object).

It is hard to make everybody happy on a such big project, with so many
areas, lines of code, people, culture, etc... I believe we all try to
give our best, neither the commiters nor the reviewers are perfect, but
slowly we help this project to improve :)


Best regards,

Phil.

Re: [Qemu-devel] [PATCH v3 18/39] target/mips: Use env_cpu, env_archcpu

[Richard Henderson]

On 5/8/19 1:15 AM, Aleksandar Markovic wrote:
> 
> On May 8, 2019 2:19 AM, "Richard Henderson"  > wrote:
>>
>>
>>
> 
> This commit message doesnˊt explain the reason for the change, and why is this
> an improvement. The underlyng reason for distingishing between  env_cpu and
> env_archcpu cases is not explained too.

It's certainly explained in the preceeding patches that introduce those 
functions.

Are you suggesting that it is beneficial to copy-and-paste a common block
explanation into 21 commit messages for each of target/foo/?


r~

Re: [Qemu-devel] [PATCH v3 18/39] target/mips: Use env_cpu, env_archcpu

[Aleksandar Markovic]

On May 8, 2019 2:19 AM, "Richard Henderson" 
wrote:
>
>
>

This commit message doesnˊt explain the reason for the change, and why is
this an improvement. The underlyng reason for distingishing between
env_cpu and env_archcpu cases is not explained too.

Thanks,
Aleksandar

> Reviewed-by: Philippe Mathieu-Daudé 
> Signed-off-by: Richard Henderson 
> ---
>  target/mips/cpu.h|  5 -
>  hw/intc/mips_gic.c   |  2 +-
>  hw/mips/mips_int.c   |  2 +-
>  linux-user/mips/cpu_loop.c   |  2 +-
>  target/mips/helper.c | 15 +--
>  target/mips/op_helper.c  | 25 +++--
>  target/mips/translate.c  |  3 +--
>  target/mips/translate_init.inc.c |  4 +---
>  8 files changed, 21 insertions(+), 37 deletions(-)
>
> diff --git a/target/mips/cpu.h b/target/mips/cpu.h
> index 31e15834ca..e0645eb1d1 100644
> --- a/target/mips/cpu.h
> +++ b/target/mips/cpu.h
> @@ -1051,11 +1051,6 @@ struct MIPSCPU {
>  CPUMIPSState env;
>  };
>
> -static inline MIPSCPU *mips_env_get_cpu(CPUMIPSState *env)
> -{
> -return container_of(env, MIPSCPU, env);
> -}
> -
>  #define ENV_OFFSET offsetof(MIPSCPU, env)
>
>  void mips_cpu_list(void);
> diff --git a/hw/intc/mips_gic.c b/hw/intc/mips_gic.c
> index 15e6e40f9f..8f509493ea 100644
> --- a/hw/intc/mips_gic.c
> +++ b/hw/intc/mips_gic.c
> @@ -44,7 +44,7 @@ static void mips_gic_set_vp_irq(MIPSGICState *gic, int
vp, int pin)
>GIC_VP_MASK_CMP_SHF;
>  }
>  if (kvm_enabled())  {
> -kvm_mips_set_ipi_interrupt(mips_env_get_cpu(gic->vps[vp].env),
> +kvm_mips_set_ipi_interrupt(env_archcpu(gic->vps[vp].env),
> pin + GIC_CPU_PIN_OFFSET,
> ored_level);
>  } else {
> diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
> index 5ddeb15848..f899f6ceb3 100644
> --- a/hw/mips/mips_int.c
> +++ b/hw/mips/mips_int.c
> @@ -76,7 +76,7 @@ void cpu_mips_irq_init_cpu(MIPSCPU *cpu)
>  qemu_irq *qi;
>  int i;
>
> -qi = qemu_allocate_irqs(cpu_mips_irq_request, mips_env_get_cpu(env),
8);
> +qi = qemu_allocate_irqs(cpu_mips_irq_request, env_archcpu(env), 8);
>  for (i = 0; i < 8; i++) {
>  env->irq[i] = qi[i];
>  }
> diff --git a/linux-user/mips/cpu_loop.c b/linux-user/mips/cpu_loop.c
> index 828137cd84..ac6c6d1504 100644
> --- a/linux-user/mips/cpu_loop.c
> +++ b/linux-user/mips/cpu_loop.c
> @@ -425,7 +425,7 @@ static int do_break(CPUMIPSState *env,
target_siginfo_t *info,
>
>  void cpu_loop(CPUMIPSState *env)
>  {
> -CPUState *cs = CPU(mips_env_get_cpu(env));
> +CPUState *cs = env_cpu(env);
>  target_siginfo_t info;
>  int trapnr;
>  abi_long ret;
> diff --git a/target/mips/helper.c b/target/mips/helper.c
> index c44cdca3b5..1fc0a4ce4b 100644
> --- a/target/mips/helper.c
> +++ b/target/mips/helper.c
> @@ -336,10 +336,8 @@ static int get_physical_address (CPUMIPSState *env,
hwaddr *physical,
>
>  void cpu_mips_tlb_flush(CPUMIPSState *env)
>  {
> -MIPSCPU *cpu = mips_env_get_cpu(env);
> -
>  /* Flush qemu's TLB and discard all shadowed entries.  */
> -tlb_flush(CPU(cpu));
> +tlb_flush(env_cpu(env));
>  env->tlb->tlb_in_use = env->tlb->nb_tlb;
>  }
>
> @@ -401,7 +399,7 @@ void cpu_mips_store_status(CPUMIPSState *env,
target_ulong val)
>  #if defined(TARGET_MIPS64)
>  if ((env->CP0_Status ^ old) & (old & (7 << CP0St_UX))) {
>  /* Access to at least one of the 64-bit segments has been
disabled */
> -tlb_flush(CPU(mips_env_get_cpu(env)));
> +tlb_flush(env_cpu(env));
>  }
>  #endif
>  if (env->CP0_Config3 & (1 << CP0C3_MT)) {
> @@ -446,7 +444,7 @@ void cpu_mips_store_cause(CPUMIPSState *env,
target_ulong val)
>  static void raise_mmu_exception(CPUMIPSState *env, target_ulong address,
>  int rw, int tlb_error)
>  {
> -CPUState *cs = CPU(mips_env_get_cpu(env));
> +CPUState *cs = env_cpu(env);
>  int exception = 0, error_code = 0;
>
>  if (rw == MMU_INST_FETCH) {
> @@ -1400,8 +1398,7 @@ bool mips_cpu_exec_interrupt(CPUState *cs, int
interrupt_request)
>  #if !defined(CONFIG_USER_ONLY)
>  void r4k_invalidate_tlb (CPUMIPSState *env, int idx, int use_extra)
>  {
> -MIPSCPU *cpu = mips_env_get_cpu(env);
> -CPUState *cs;
> +CPUState *cs = env_cpu(env);
>  r4k_tlb_t *tlb;
>  target_ulong addr;
>  target_ulong end;
> @@ -1427,7 +1424,6 @@ void r4k_invalidate_tlb (CPUMIPSState *env, int
idx, int use_extra)
>  /* 1k pages are not supported. */
>  mask = tlb->PageMask | ~(TARGET_PAGE_MASK << 1);
>  if (tlb->V0) {
> -cs = CPU(cpu);
>  addr = tlb->VPN & ~mask;
>  #if defined(TARGET_MIPS64)
>  if (addr >= (0x8000ULL & env->SEGMask)) {
> @@ -1441,7 +1437,6 @@ void r4k_invalidate_tlb (CPUMIPSState *env, int
idx, int use_extra)
>  }
>  }
>  if (tlb->V1) {
> -cs = CPU(cpu);
>