[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 01/20] target/mips: Clean up helper.c
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v2 01/20] target/mips: Clean up helper.c |
Date: |
Fri, 27 Sep 2019 10:55:33 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
On 9/27/19 6:32 AM, Aleksandar Markovic wrote:
> 25.09.2019. 17.53, "Philippe Mathieu-Daudé" <address@hidden
> <mailto:address@hidden>> је написао/ла:
>>
>> On 9/25/19 2:45 PM, Aleksandar Markovic wrote:
>> > From: Aleksandar Markovic <address@hidden
> <mailto:address@hidden>>
>> >
>> > Mostly fix errors and warnings reported by 'checkpatch.pl
> <http://checkpatch.pl> -f'.
>> >
>> > Signed-off-by: Aleksandar Markovic <address@hidden
> <mailto: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.
Fair enough :D
You are free to question and Nack them.
Regards,
Phil.
- Re: [PATCH v2 04/20] target/mips: Clean up mips-defs.h, (continued)
- [PATCH v2 10/20] target/mips: msa: Unroll loops and demacro <BMNZ|BMZ|BSEL>.V, Aleksandar Markovic, 2019/09/25
- [PATCH v2 02/20] target/mips: Clean up internal.h, Aleksandar Markovic, 2019/09/25
- [PATCH v2 06/20] target/mips: Clean up translate.c, Aleksandar Markovic, 2019/09/25
- [PATCH v2 07/20] target/mips: msa: Split helpers for <NLOC|NLZC>.<B|H|W|D>, Aleksandar Markovic, 2019/09/25
- [PATCH v2 09/20] target/mips: msa: Split helpers for BINS<L|R>.<B|H|W|D>, Aleksandar Markovic, 2019/09/25
- [PATCH v2 01/20] target/mips: Clean up helper.c, Aleksandar Markovic, 2019/09/25
- Re: [PATCH v2 01/20] target/mips: Clean up helper.c, Philippe Mathieu-Daudé, 2019/09/25
- Re: [PATCH v2 01/20] target/mips: Clean up helper.c, Aleksandar Markovic, 2019/09/27
- Re: [PATCH v2 01/20] target/mips: Clean up helper.c,
Philippe Mathieu-Daudé <=
- Re: [PATCH v2 01/20] target/mips: Clean up helper.c, Markus Armbruster, 2019/09/27
- Re: [PATCH v2 01/20] target/mips: Clean up helper.c, Peter Maydell, 2019/09/27
- Re: [PATCH v2 01/20] target/mips: Clean up helper.c, Aleksandar Markovic, 2019/09/27
- Re: [PATCH v2 01/20] target/mips: Clean up helper.c, Markus Armbruster, 2019/09/28
[PATCH v2 08/20] target/mips: msa: Split helpers for PCNT.<B|H|W|D>, Aleksandar Markovic, 2019/09/25
[PATCH v2 11/20] target/mips: msa: Split helpers for B<CLR|NEG|SEL>.<B|H|W|D>, Aleksandar Markovic, 2019/09/25
[PATCH v2 20/20] target/mips: msa: Move helpers for <AND|NOR|OR|XOR>.V, Aleksandar Markovic, 2019/09/25
[PATCH v2 19/20] target/mips: msa: Simplify and move helper for MOVE.V, Aleksandar Markovic, 2019/09/25
[PATCH v2 17/20] target/mips: msa: Split helpers for DIV_<S|U>.<B|H|W|D>, Aleksandar Markovic, 2019/09/25
[PATCH v2 14/20] target/mips: msa: Split helpers for CEQ.<B|H|W|D>, Aleksandar Markovic, 2019/09/25