|
| From: | Aleksandar Markovic |
| Subject: | Re: [PATCH v2 01/20] target/mips: Clean up helper.c |
| Date: | Fri, 27 Sep 2019 06:32:52 +0200 |
25.09.2019. 17.53, "Philippe Mathieu-Daudé" <address@hidden> је написао/ла:
>
> On 9/25/19 2:45 PM, Aleksandar Markovic wrote:
> > From: Aleksandar Markovic <address@hidden>
> >
> > Mostly fix errors and warnings reported by 'checkpatch.pl -f'.
> >
> > Signed-off-by: Aleksandar Markovic <address@hidden>
> > ---
> > target/mips/helper.c | 132 +++++++++++++++++++++++++++++++--------------------
> > 1 file changed, 80 insertions(+), 52 deletions(-)
> >
> > diff --git a/target/mips/helper.c b/target/mips/helper.c
> > index a2b6459..3dd1aae 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
> > + */
>
> My IDE show the current form nicer when the switch is folded.
>
> Are these comment really bothering checkpatch?
>
While being sintaxically correct, interleaving comments and code in a single code line is considered a bad practice by many.
> > if (eu) {
> > return 0;
> > }
> > @@ -204,9 +210,9 @@ static int get_segctl_physical_address(CPUMIPSState *env, hwaddr *physical,
> > pa & ~(hwaddr)segmask);
> > }
> >
> > -static int get_physical_address (CPUMIPSState *env, hwaddr *physical,
> > - int *prot, target_ulong real_address,
> > - int rw, int access_type, int mmu_idx)
> > +static int get_physical_address(CPUMIPSState *env, hwaddr *physical,
> > + int *prot, target_ulong real_address,
> > + int rw, int access_type, int mmu_idx)
> > {
> > /* User mode can only access useg/xuseg */
> > #if defined(TARGET_MIPS64)
> > @@ -252,14 +258,15 @@ static int get_physical_address (CPUMIPSState *env, hwaddr *physical,
> > } else {
> > segctl = env->CP0_SegCtl2 >> 16;
> > }
> > - ret = get_segctl_physical_address(env, physical, prot, real_address, rw,
> > - access_type, mmu_idx, segctl,
> > - 0x3FFFFFFF);
> > + ret = get_segctl_physical_address(env, physical, prot,
> > + real_address, rw, access_type,
> > + mmu_idx, segctl, 0x3FFFFFFF);
> > #if defined(TARGET_MIPS64)
> > } else if (address < 0x4000000000000000ULL) {
> > /* xuseg */
> > if (UX && address <= (0x3FFFFFFFFFFFFFFFULL & env->SEGMask)) {
> > - ret = env->tlb->map_address(env, physical, prot, real_address, rw, access_type);
> > + ret = env->tlb->map_address(env, physical, prot,
> > + real_address, rw, access_type);
> > } else {
> > ret = TLBRET_BADADDR;
> > }
> > @@ -267,7 +274,8 @@ static int get_physical_address (CPUMIPSState *env, hwaddr *physical,
> > /* xsseg */
> > if ((supervisor_mode || kernel_mode) &&
> > SX && address <= (0x7FFFFFFFFFFFFFFFULL & env->SEGMask)) {
> > - ret = env->tlb->map_address(env, physical, prot, real_address, rw, access_type);
> > + ret = env->tlb->map_address(env, physical, prot,
> > + real_address, rw, access_type);
> > } else {
> > ret = TLBRET_BADADDR;
> > }
> > @@ -307,7 +315,8 @@ static int get_physical_address (CPUMIPSState *env, hwaddr *physical,
> > /* xkseg */
> > if (kernel_mode && KX &&
> > address <= (0xFFFFFFFF7FFFFFFFULL & env->SEGMask)) {
> > - ret = env->tlb->map_address(env, physical, prot, real_address, rw, access_type);
> > + ret = env->tlb->map_address(env, physical, prot,
> > + real_address, rw, access_type);
> > } else {
> > ret = TLBRET_BADADDR;
> > }
> > @@ -328,8 +337,10 @@ static int get_physical_address (CPUMIPSState *env, hwaddr *physical,
> > access_type, mmu_idx,
> > env->CP0_SegCtl0 >> 16, 0x1FFFFFFF);
> > } else {
> > - /* kseg3 */
> > - /* XXX: debug segment is not emulated */
> > + /*
> > + * kseg3
> > + * XXX: debug segment is not emulated
> > + */
>
> Ditto.
>
> > ret = get_segctl_physical_address(env, physical, prot, real_address, rw,
> > access_type, mmu_idx,
> > env->CP0_SegCtl0, 0x1FFFFFFF);
> > @@ -515,9 +526,9 @@ static void raise_mmu_exception(CPUMIPSState *env, target_ulong address,
> > #if defined(TARGET_MIPS64)
> > env->CP0_EntryHi &= env->SEGMask;
> > env->CP0_XContext =
> > - /* PTEBase */ (env->CP0_XContext & ((~0ULL) << (env->SEGBITS - 7))) |
> > - /* R */ (extract64(address, 62, 2) << (env->SEGBITS - 9)) |
> > - /* BadVPN2 */ (extract64(address, 13, env->SEGBITS - 13) << 4);
> > + (env->CP0_XContext & ((~0ULL) << (env->SEGBITS - 7))) | /* PTEBase */
> > + (extract64(address, 62, 2) << (env->SEGBITS - 9)) | /* R */
> > + (extract64(address, 13, env->SEGBITS - 13) << 4); /* BadVPN2 */
> > #endif
> > cs->exception_index = exception;
> > env->error_code = error_code;
> > @@ -945,7 +956,8 @@ bool mips_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> > }
> >
> > #ifndef CONFIG_USER_ONLY
> > -hwaddr cpu_mips_translate_address(CPUMIPSState *env, target_ulong address, int rw)
> > +hwaddr cpu_mips_translate_address(CPUMIPSState *env, target_ulong address,
> > + int rw)
> > {
> > hwaddr physical;
> > int prot;
> > @@ -1005,7 +1017,7 @@ static const char * const excp_names[EXCP_LAST + 1] = {
> > };
> > #endif
> >
> > -target_ulong exception_resume_pc (CPUMIPSState *env)
> > +target_ulong exception_resume_pc(CPUMIPSState *env)
> > {
> > target_ulong bad_pc;
> > target_ulong isa_mode;
> > @@ -1013,8 +1025,10 @@ target_ulong exception_resume_pc (CPUMIPSState *env)
> > isa_mode = !!(env->hflags & MIPS_HFLAG_M16);
> > bad_pc = env->active_tc.PC | isa_mode;
> > if (env->hflags & MIPS_HFLAG_BMASK) {
> > - /* If the exception was raised from a delay slot, come back to
> > - the jump. */
> > + /*
> > + * If the exception was raised from a delay slot, come back to
> > + * the jump.
> > + */
> > bad_pc -= (env->hflags & MIPS_HFLAG_B16 ? 2 : 4);
> > }
> >
> > @@ -1022,14 +1036,14 @@ target_ulong exception_resume_pc (CPUMIPSState *env)
> > }
> >
> > #if !defined(CONFIG_USER_ONLY)
> > -static void set_hflags_for_handler (CPUMIPSState *env)
> > +static void set_hflags_for_handler(CPUMIPSState *env)
> > {
> > /* Exception handlers are entered in 32-bit mode. */
> > env->hflags &= ~(MIPS_HFLAG_M16);
> > /* ...except that microMIPS lets you choose. */
> > if (env->insn_flags & ASE_MICROMIPS) {
> > - env->hflags |= (!!(env->CP0_Config3
> > - & (1 << CP0C3_ISA_ON_EXC))
> > + env->hflags |= (!!(env->CP0_Config3 &
> > + (1 << CP0C3_ISA_ON_EXC))
> > << MIPS_HFLAG_M16_SHIFT);
> > }
> > }
> > @@ -1096,10 +1110,12 @@ void mips_cpu_do_interrupt(CPUState *cs)
> > switch (cs->exception_index) {
> > case EXCP_DSS:
> > env->CP0_Debug |= 1 << CP0DB_DSS;
> > - /* Debug single step cannot be raised inside a delay slot and
> > - resume will always occur on the next instruction
> > - (but we assume the pc has always been updated during
> > - code translation). */
> > + /*
> > + * Debug single step cannot be raised inside a delay slot and
> > + * resume will always occur on the next instruction
> > + * (but we assume the pc has always been updated during
> > + * code translation).
> > + */
> > env->CP0_DEPC = env->active_tc.PC | !!(env->hflags & MIPS_HFLAG_M16);
> > goto enter_debug_mode;
> > case EXCP_DINT:
> > @@ -1111,7 +1127,8 @@ void mips_cpu_do_interrupt(CPUState *cs)
> > case EXCP_DBp:
> > env->CP0_Debug |= 1 << CP0DB_DBp;
> > /* Setup DExcCode - SDBBP instruction */
> > - env->CP0_Debug = (env->CP0_Debug & ~(0x1fULL << CP0DB_DEC)) | 9 << CP0DB_DEC;
> > + env->CP0_Debug = (env->CP0_Debug & ~(0x1fULL << CP0DB_DEC)) |
> > + (9 << CP0DB_DEC);
> > goto set_DEPC;
> > case EXCP_DDBS:
> > env->CP0_Debug |= 1 << CP0DB_DDBS;
> > @@ -1132,8 +1149,9 @@ void mips_cpu_do_interrupt(CPUState *cs)
> > env->hflags |= MIPS_HFLAG_DM | MIPS_HFLAG_CP0;
> > env->hflags &= ~(MIPS_HFLAG_KSU);
> > /* EJTAG probe trap enable is not implemented... */
> > - if (!(env->CP0_Status & (1 << CP0St_EXL)))
> > + if (!(env->CP0_Status & (1 << CP0St_EXL))) {
> > env->CP0_Cause &= ~(1U << CP0Ca_BD);
> > + }
> > env->active_tc.PC = env->exception_base + 0x480;
> > set_hflags_for_handler(env);
> > break;
> > @@ -1159,8 +1177,9 @@ void mips_cpu_do_interrupt(CPUState *cs)
> > }
> > env->hflags |= MIPS_HFLAG_CP0;
> > env->hflags &= ~(MIPS_HFLAG_KSU);
> > - if (!(env->CP0_Status & (1 << CP0St_EXL)))
> > + if (!(env->CP0_Status & (1 << CP0St_EXL))) {
> > env->CP0_Cause &= ~(1U << CP0Ca_BD);
> > + }
> > env->active_tc.PC = env->exception_base;
> > set_hflags_for_handler(env);
> > break;
> > @@ -1176,12 +1195,16 @@ void mips_cpu_do_interrupt(CPUState *cs)
> > uint32_t pending = (env->CP0_Cause & CP0Ca_IP_mask) >> CP0Ca_IP;
> >
> > if (env->CP0_Config3 & (1 << CP0C3_VEIC)) {
> > - /* For VEIC mode, the external interrupt controller feeds
> > - * the vector through the CP0Cause IP lines. */
> > + /*
> > + * For VEIC mode, the external interrupt controller feeds
> > + * the vector through the CP0Cause IP lines.
> > + */
> > vector = pending;
> > } else {
> > - /* Vectored Interrupts
> > - * Mask with Status.IM7-IM0 to get enabled interrupts. */
> > + /*
> > + * Vectored Interrupts
> > + * Mask with Status.IM7-IM0 to get enabled interrupts.
> > + */
> > pending &= (env->CP0_Status >> CP0St_IM) & 0xff;
> > /* Find the highest-priority interrupt. */
> > while (pending >>= 1) {
> > @@ -1354,7 +1377,8 @@ void mips_cpu_do_interrupt(CPUState *cs)
> >
> > env->active_tc.PC += offset;
> > set_hflags_for_handler(env);
> > - env->CP0_Cause = (env->CP0_Cause & ~(0x1f << CP0Ca_EC)) | (cause << CP0Ca_EC);
> > + env->CP0_Cause = (env->CP0_Cause & ~(0x1f << CP0Ca_EC)) |
> > + (cause << CP0Ca_EC);
> > break;
> > default:
> > abort();
> > @@ -1390,7 +1414,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)
> > +void r4k_invalidate_tlb(CPUMIPSState *env, int idx, int use_extra)
> > {
> > CPUState *cs = env_cpu(env);
> > r4k_tlb_t *tlb;
> > @@ -1400,16 +1424,20 @@ void r4k_invalidate_tlb (CPUMIPSState *env, int idx, int use_extra)
> > target_ulong mask;
> >
> > tlb = &env->tlb->mmu.r4k.tlb[idx];
> > - /* The qemu TLB is flushed when the ASID changes, so no need to
> > - flush these entries again. */
> > + /*
> > + * The qemu TLB is flushed when the ASID changes, so no need to
> > + * flush these entries again.
> > + */
> > if (tlb->G == 0 && tlb->ASID != ASID) {
> > return;
> > }
> >
> > if (use_extra && env->tlb->tlb_in_use < MIPS_TLB_MAX) {
> > - /* For tlbwr, we can shadow the discarded entry into
> > - a new (fake) TLB entry, as long as the guest can not
> > - tell that it's there. */
> > + /*
> > + * For tlbwr, we can shadow the discarded entry into
> > + * a new (fake) TLB entry, as long as the guest can not
> > + * tell that it's there.
> > + */
> > env->tlb->mmu.r4k.tlb[env->tlb->tlb_in_use] = *tlb;
> > env->tlb->tlb_in_use++;
> > return;
> >
>
> Except 2 comments, OK for the rest.
>
> Another patch that makes rebasing very painful :(
>
It would be fantastic if you apply the same reasoning to your patches, that spread all over the code base, and happen so frequently, and certainly create enormously more rebasing problems for multitude of people than this patch or series does.
Yours, Aleksandar
>
| [Prev in Thread] | Current Thread | [Next in Thread] |