Re: [PATCH v4 1/8] target/mips: Clean up helper.c

[Aleksandar Markovic <aleksandar.m.mail@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4762 bytes --]

On Monday, October 14, 2019, Markus Armbruster <armbru@redhat.com> wrote:

> Aleksandar Markovic <aleksandar.markovic@rt-rk.com> writes:
>
> > From: Aleksandar Markovic <amarkovic@wavecomp.com>
> >
> > Mostly fix errors and warnings reported by 'checkpatch.pl -f'.
> >
> > Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> > ---
> >  target/mips/helper.c | 128 ++++++++++++++++++++++++++++++
> +--------------------
> >  1 file changed, 78 insertions(+), 50 deletions(-)
> >
> > diff --git a/target/mips/helper.c b/target/mips/helper.c
> > index a2b6459..2411a2c 100644
> > --- a/target/mips/helper.c
> > +++ b/target/mips/helper.c
> > @@ -39,8 +39,8 @@ enum {
> >  #if !defined(CONFIG_USER_ONLY)
> >
> >  /* no MMU emulation */
> > -int no_mmu_map_address (CPUMIPSState *env, hwaddr *physical, int *prot,
> > -                        target_ulong address, int rw, int access_type)
> > +int no_mmu_map_address(CPUMIPSState *env, hwaddr *physical, int *prot,
> > +                       target_ulong address, int rw, int access_type)
> >  {
> >      *physical = address;
> >      *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> > @@ -48,26 +48,28 @@ int no_mmu_map_address (CPUMIPSState *env, hwaddr
> *physical, int *prot,
> >  }
> >
> >  /* fixed mapping MMU emulation */
> > -int fixed_mmu_map_address (CPUMIPSState *env, hwaddr *physical, int
> *prot,
> > -                           target_ulong address, int rw, int
> access_type)
> > +int fixed_mmu_map_address(CPUMIPSState *env, hwaddr *physical, int
> *prot,
> > +                          target_ulong address, int rw, int access_type)
> >  {
> >      if (address <= (int32_t)0x7FFFFFFFUL) {
> > -        if (!(env->CP0_Status & (1 << CP0St_ERL)))
> > +        if (!(env->CP0_Status & (1 << CP0St_ERL))) {
> >              *physical = address + 0x40000000UL;
> > -        else
> > +        } else {
> >              *physical = address;
> > -    } else if (address <= (int32_t)0xBFFFFFFFUL)
> > +        }
> > +    } else if (address <= (int32_t)0xBFFFFFFFUL) {
> >          *physical = address & 0x1FFFFFFF;
> > -    else
> > +    } else {
> >          *physical = address;
> > +    }
> >
> >      *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> >      return TLBRET_MATCH;
> >  }
> >
> >  /* MIPS32/MIPS64 R4000-style MMU emulation */
> > -int r4k_map_address (CPUMIPSState *env, hwaddr *physical, int *prot,
> > -                     target_ulong address, int rw, int access_type)
> > +int r4k_map_address(CPUMIPSState *env, hwaddr *physical, int *prot,
> > +                    target_ulong address, int rw, int access_type)
> >  {
> >      uint16_t ASID = env->CP0_EntryHi & env->CP0_EntryHi_ASID_mask;
> >      int i;
> > @@ -99,8 +101,9 @@ int r4k_map_address (CPUMIPSState *env, hwaddr
> *physical, int *prot,
> >              if (rw != MMU_DATA_STORE || (n ? tlb->D1 : tlb->D0)) {
> >                  *physical = tlb->PFN[n] | (address & (mask >> 1));
> >                  *prot = PAGE_READ;
> > -                if (n ? tlb->D1 : tlb->D0)
> > +                if (n ? tlb->D1 : tlb->D0) {
> >                      *prot |= PAGE_WRITE;
> > +                }
> >                  if (!(n ? tlb->XI1 : tlb->XI0)) {
> >                      *prot |= PAGE_EXEC;
> >                  }
> > @@ -130,8 +133,11 @@ static int is_seg_am_mapped(unsigned int am, bool
> eu, int mmu_idx)
> >      int32_t adetlb_mask;
> >
> >      switch (mmu_idx) {
> > -    case 3 /* ERL */:
> > -        /* If EU is set, always unmapped */
> > +    case 3:
> > +        /*
> > +         * ERL
> > +         * If EU is set, always unmapped
> > +         */
> >          if (eu) {
> >              return 0;
> >          }
>
> This changes from the usual way we format switch case comments to an
> unusual way.
>
> If you want to pursue this change, please put it in a separate patch,
> so this one is really about fixing "errors and warnings reported by
> 'checkpatch.pl -f'", as your commit message promises.
>
>

Hi, Markus. Thank you for your response.

There must be some misunderstanding here:

The line:

   case 3 /* ERL */:

generates a checkpatch warning. I don't know why I would put it in a
separate patch, if this patch is about fixing checkpatch warnings. Please
explain.

Secondly, I don't see that this is a usual way we format switch statement.
I found just several cases in the whole QEMU code base (and you claimed in
previous comments that there are thousands).

I am just guessing that you somehow mixed this line with the line:

   case 3: /* ERL */

that would have not generated checkpatch warning.

I don't see any reason to change this patch. Please let me know it you
still think I should do something else. And you are welcome to analyse any
patches of mine.

Aleksandar


> [...]
>
>

[-- Attachment #2: Type: text/html, Size: 6599 bytes --]