| Message ID | 20220815162020.2420093-4-matheus.ferst@eldorado.org.br |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | PowerPC interrupt rework | expand |
Matheus Ferst <matheus.ferst@eldorado.org.br> writes: > Move the interrupt masking logic to a new method, ppc_pending_interrupt, > and only handle the interrupt processing in ppc_hw_interrupt. > > Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br> > --- > target/ppc/excp_helper.c | 228 ++++++++++++++++++++++++--------------- > 1 file changed, 141 insertions(+), 87 deletions(-) > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c <snip> > @@ -1884,15 +1915,38 @@ bool ppc_cpu_exec_interrupt(CPUState *cs, int interrupt_request) > { > PowerPCCPU *cpu = POWERPC_CPU(cs); > CPUPPCState *env = &cpu->env; > + int pending_interrupt; I would give this a simpler name to avoid confusion with the other two pending_interrupt terms. > > - if (interrupt_request & CPU_INTERRUPT_HARD) { > - ppc_hw_interrupt(env); > - if (env->pending_interrupts == 0) { > - cs->interrupt_request &= ~CPU_INTERRUPT_HARD; > - } > - return true; > + if ((interrupt_request & CPU_INTERRUPT_HARD) == 0) { > + return false; > } > - return false; > + It seems we're assuming that after this point we certainly have some pending interrupt... > + pending_interrupt = ppc_pending_interrupt(env); > + if (pending_interrupt == 0) { ...but how then is this able to return 0? Could the function's name be made a bit clearer? Maybe interrupt = ppc_next_pending_interrupt or something to that effect. > + if (env->resume_as_sreset) { > + /* > + * This is a bug ! It means that has_work took us out of halt > + * without anything to deliver while in a PM state that requires > + * getting out via a 0x100 > + * > + * This means we will incorrectly execute past the power management > + * instruction instead of triggering a reset. > + * > + * It generally means a discrepancy between the wakeup conditions in > + * the processor has_work implementation and the logic in this > + * function. > + */ > + cpu_abort(env_cpu(env), > + "Wakeup from PM state but interrupt Undelivered"); This condition is BookS only. Perhaps it would be better to move it inside each of the processor-specific functions. And since you're merging has_work with pending_interrupts, can't you solve that issue earlier? Specifically the "has_work tooks us out of halt..." part. > + } > + return false; > + } > + > + ppc_hw_interrupt(env, pending_interrupt); Some verbs would be nice. ppc_deliver_interrupt? > + if (env->pending_interrupts == 0) { > + cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD); > + } > + return true; > } > > #endif /* !CONFIG_USER_ONLY */
On 15/08/2022 17:09, Fabiano Rosas wrote: > Matheus Ferst <matheus.ferst@eldorado.org.br> writes: > >> Move the interrupt masking logic to a new method, ppc_pending_interrupt, >> and only handle the interrupt processing in ppc_hw_interrupt. >> >> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br> >> --- >> target/ppc/excp_helper.c | 228 ++++++++++++++++++++++++--------------- >> 1 file changed, 141 insertions(+), 87 deletions(-) >> >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > > <snip> > >> @@ -1884,15 +1915,38 @@ bool ppc_cpu_exec_interrupt(CPUState *cs, int interrupt_request) >> { >> PowerPCCPU *cpu = POWERPC_CPU(cs); >> CPUPPCState *env = &cpu->env; >> + int pending_interrupt; > > I would give this a simpler name to avoid confusion with the other two > pending_interrupt terms. > >> >> - if (interrupt_request & CPU_INTERRUPT_HARD) { >> - ppc_hw_interrupt(env); >> - if (env->pending_interrupts == 0) { >> - cs->interrupt_request &= ~CPU_INTERRUPT_HARD; >> - } >> - return true; >> + if ((interrupt_request & CPU_INTERRUPT_HARD) == 0) { >> + return false; >> } >> - return false; >> + > > It seems we're assuming that after this point we certainly have some > pending interrupt... > >> + pending_interrupt = ppc_pending_interrupt(env); >> + if (pending_interrupt == 0) { > > ...but how then is this able to return 0? At this point in the patch series, raising interrupts with ppc_set_irq always sets CPU_INTERRUPT_HARD, so it is possible that all interrupts are masked, and then ppc_pending_interrupt would return zero. > Could the function's name be > made a bit clearer? Maybe interrupt = ppc_next_pending_interrupt or > something to that effect. > Maybe ppc_next_unmasked_interrupt? >> + if (env->resume_as_sreset) { >> + /* >> + * This is a bug ! It means that has_work took us out of halt >> + * without anything to deliver while in a PM state that requires >> + * getting out via a 0x100 >> + * >> + * This means we will incorrectly execute past the power management >> + * instruction instead of triggering a reset. >> + * >> + * It generally means a discrepancy between the wakeup conditions in >> + * the processor has_work implementation and the logic in this >> + * function. >> + */ >> + cpu_abort(env_cpu(env), >> + "Wakeup from PM state but interrupt Undelivered"); > > This condition is BookS only. Perhaps it would be better to move it > inside each of the processor-specific functions. And since you're > merging has_work with pending_interrupts, can't you solve that issue > earlier? Specifically the "has_work tooks us out of halt..." part. > This condition would not be an error in ppc_pending_interrupt because we'll call this method from other places in the following patches, like ppc_set_irq. Maybe we should move it to a "case 0:" in ppc_hw_interrupt? >> + } >> + return false; >> + } >> + >> + ppc_hw_interrupt(env, pending_interrupt); > > Some verbs would be nice. ppc_deliver_interrupt? Agreed. Should we also make processor-specific versions of this method? That way, we could get rid of the calls to ppc_decr_clear_on_delivery and is_book3s_arch2x. > >> + if (env->pending_interrupts == 0) { >> + cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD); >> + } >> + return true; >> } >> >> #endif /* !CONFIG_USER_ONLY */ Thanks, Matheus K. Ferst Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/> Analista de Software Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
"Matheus K. Ferst" <matheus.ferst@eldorado.org.br> writes: > On 15/08/2022 17:09, Fabiano Rosas wrote: >> Matheus Ferst <matheus.ferst@eldorado.org.br> writes: >> >>> Move the interrupt masking logic to a new method, ppc_pending_interrupt, >>> and only handle the interrupt processing in ppc_hw_interrupt. >>> >>> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br> >>> --- >>> target/ppc/excp_helper.c | 228 ++++++++++++++++++++++++--------------- >>> 1 file changed, 141 insertions(+), 87 deletions(-) >>> >>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >> >> <snip> >> >>> @@ -1884,15 +1915,38 @@ bool ppc_cpu_exec_interrupt(CPUState *cs, int interrupt_request) >>> { >>> PowerPCCPU *cpu = POWERPC_CPU(cs); >>> CPUPPCState *env = &cpu->env; >>> + int pending_interrupt; >> >> I would give this a simpler name to avoid confusion with the other two >> pending_interrupt terms. >> >>> >>> - if (interrupt_request & CPU_INTERRUPT_HARD) { >>> - ppc_hw_interrupt(env); >>> - if (env->pending_interrupts == 0) { >>> - cs->interrupt_request &= ~CPU_INTERRUPT_HARD; >>> - } >>> - return true; >>> + if ((interrupt_request & CPU_INTERRUPT_HARD) == 0) { >>> + return false; >>> } >>> - return false; >>> + >> >> It seems we're assuming that after this point we certainly have some >> pending interrupt... >> >>> + pending_interrupt = ppc_pending_interrupt(env); >>> + if (pending_interrupt == 0) { >> >> ...but how then is this able to return 0? > > At this point in the patch series, raising interrupts with ppc_set_irq > always sets CPU_INTERRUPT_HARD, so it is possible that all interrupts > are masked, and then ppc_pending_interrupt would return zero. > >> Could the function's name be >> made a bit clearer? Maybe interrupt = ppc_next_pending_interrupt or >> something to that effect. >> > > Maybe ppc_next_unmasked_interrupt? Could be. Although your suggestion below of moving the pending=0 case into ppc_hw_interrupt already clears up the confusion quite a lot. >>> + if (env->resume_as_sreset) { >>> + /* >>> + * This is a bug ! It means that has_work took us out of halt >>> + * without anything to deliver while in a PM state that requires >>> + * getting out via a 0x100 >>> + * >>> + * This means we will incorrectly execute past the power management >>> + * instruction instead of triggering a reset. >>> + * >>> + * It generally means a discrepancy between the wakeup conditions in >>> + * the processor has_work implementation and the logic in this >>> + * function. >>> + */ >>> + cpu_abort(env_cpu(env), >>> + "Wakeup from PM state but interrupt Undelivered"); >> >> This condition is BookS only. Perhaps it would be better to move it >> inside each of the processor-specific functions. And since you're >> merging has_work with pending_interrupts, can't you solve that issue >> earlier? Specifically the "has_work tooks us out of halt..." part. >> > > This condition would not be an error in ppc_pending_interrupt because > we'll call this method from other places in the following patches, like > ppc_set_irq. Maybe we should move it to a "case 0:" in ppc_hw_interrupt? > Good idea, this condition is very unlikely (impossible?) to happen so there's no need to have it prominent in this function like this. Should it be turned into an assert as well perhaps? >>> + } >>> + return false; >>> + } >>> + >>> + ppc_hw_interrupt(env, pending_interrupt); >> >> Some verbs would be nice. ppc_deliver_interrupt? > > Agreed. Should we also make processor-specific versions of this method? > That way, we could get rid of the calls to ppc_decr_clear_on_delivery > and is_book3s_arch2x. Yes, let's see what it looks like. What interrupts exist for a given CPU along with their implementation specific behavior are data that's linked to the CPU/group of CPUs. In theory we could have these decisions made at build time/CPU selection time and just operate on the smaller subset of data at runtime. The is_book3s_arch2x check can already be removed. After the powerpc_excp functions were split there's nothing handling DOORI; and SDOOR is now only handled by powerpc_books. So we would just be changing the "exception not implemented" message from DOORI to SDOOR. ... which will probably lead to some exploration of why we are generating exceptions in TCG code for interrupts that are never delivered/handled. >> >>> + if (env->pending_interrupts == 0) { >>> + cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD); >>> + } >>> + return true; >>> } >>> >>> #endif /* !CONFIG_USER_ONLY */ > > Thanks, > Matheus K. Ferst > Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/> > Analista de Software > Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index ae9576546f..8690017c70 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -1678,29 +1678,22 @@ void ppc_cpu_do_interrupt(CPUState *cs) powerpc_excp(cpu, cs->exception_index); } -static void ppc_hw_interrupt(CPUPPCState *env) +static int ppc_pending_interrupt(CPUPPCState *env) { - PowerPCCPU *cpu = env_archcpu(env); bool async_deliver; /* External reset */ if (env->pending_interrupts & PPC_INTERRUPT_RESET) { - env->pending_interrupts &= ~PPC_INTERRUPT_RESET; - powerpc_excp(cpu, POWERPC_EXCP_RESET); - return; + return PPC_INTERRUPT_RESET; } /* Machine check exception */ if (env->pending_interrupts & PPC_INTERRUPT_MCK) { - env->pending_interrupts &= ~PPC_INTERRUPT_MCK; - powerpc_excp(cpu, POWERPC_EXCP_MCHECK); - return; + return PPC_INTERRUPT_MCK; } #if 0 /* TODO */ /* External debug exception */ if (env->pending_interrupts & PPC_INTERRUPT_DEBUG) { - env->pending_interrupts &= ~PPC_INTERRUPT_DEBUG; - powerpc_excp(cpu, POWERPC_EXCP_DEBUG); - return; + return PPC_INTERRUPT_DEBUG; } #endif @@ -1718,9 +1711,7 @@ static void ppc_hw_interrupt(CPUPPCState *env) bool hdice = !!(env->spr[SPR_LPCR] & LPCR_HDICE); if ((async_deliver || !FIELD_EX64_HV(env->msr)) && hdice) { /* HDEC clears on delivery */ - env->pending_interrupts &= ~PPC_INTERRUPT_HDECR; - powerpc_excp(cpu, POWERPC_EXCP_HDECR); - return; + return PPC_INTERRUPT_HDECR; } } @@ -1729,8 +1720,7 @@ static void ppc_hw_interrupt(CPUPPCState *env) /* LPCR will be clear when not supported so this will work */ bool hvice = !!(env->spr[SPR_LPCR] & LPCR_HVICE); if ((async_deliver || !FIELD_EX64_HV(env->msr)) && hvice) { - powerpc_excp(cpu, POWERPC_EXCP_HVIRT); - return; + return PPC_INTERRUPT_HVIRT; } } @@ -1742,77 +1732,47 @@ static void ppc_hw_interrupt(CPUPPCState *env) if ((async_deliver && !(heic && FIELD_EX64_HV(env->msr) && !FIELD_EX64(env->msr, MSR, PR))) || (env->has_hv_mode && !FIELD_EX64_HV(env->msr) && !lpes0)) { - if (books_vhyp_promotes_external_to_hvirt(cpu)) { - powerpc_excp(cpu, POWERPC_EXCP_HVIRT); - } else { - powerpc_excp(cpu, POWERPC_EXCP_EXTERNAL); - } - return; + return PPC_INTERRUPT_EXT; } } if (FIELD_EX64(env->msr, MSR, CE)) { /* External critical interrupt */ if (env->pending_interrupts & PPC_INTERRUPT_CEXT) { - powerpc_excp(cpu, POWERPC_EXCP_CRITICAL); - return; + return PPC_INTERRUPT_CEXT; } } if (async_deliver != 0) { /* Watchdog timer on embedded PowerPC */ if (env->pending_interrupts & PPC_INTERRUPT_WDT) { - env->pending_interrupts &= ~PPC_INTERRUPT_WDT; - powerpc_excp(cpu, POWERPC_EXCP_WDT); - return; + return PPC_INTERRUPT_WDT; } if (env->pending_interrupts & PPC_INTERRUPT_CDOORBELL) { - env->pending_interrupts &= ~PPC_INTERRUPT_CDOORBELL; - powerpc_excp(cpu, POWERPC_EXCP_DOORCI); - return; + return PPC_INTERRUPT_CDOORBELL; } /* Fixed interval timer on embedded PowerPC */ if (env->pending_interrupts & PPC_INTERRUPT_FIT) { - env->pending_interrupts &= ~PPC_INTERRUPT_FIT; - powerpc_excp(cpu, POWERPC_EXCP_FIT); - return; + return PPC_INTERRUPT_FIT; } /* Programmable interval timer on embedded PowerPC */ if (env->pending_interrupts & PPC_INTERRUPT_PIT) { - env->pending_interrupts &= ~PPC_INTERRUPT_PIT; - powerpc_excp(cpu, POWERPC_EXCP_PIT); - return; + return PPC_INTERRUPT_PIT; } /* Decrementer exception */ if (env->pending_interrupts & PPC_INTERRUPT_DECR) { - if (ppc_decr_clear_on_delivery(env)) { - env->pending_interrupts &= ~PPC_INTERRUPT_DECR; - } - powerpc_excp(cpu, POWERPC_EXCP_DECR); - return; + return PPC_INTERRUPT_DECR; } if (env->pending_interrupts & PPC_INTERRUPT_DOORBELL) { - env->pending_interrupts &= ~PPC_INTERRUPT_DOORBELL; - if (is_book3s_arch2x(env)) { - powerpc_excp(cpu, POWERPC_EXCP_SDOOR); - } else { - powerpc_excp(cpu, POWERPC_EXCP_DOORI); - } - return; + return PPC_INTERRUPT_DOORBELL; } if (env->pending_interrupts & PPC_INTERRUPT_HDOORBELL) { - env->pending_interrupts &= ~PPC_INTERRUPT_HDOORBELL; - powerpc_excp(cpu, POWERPC_EXCP_SDOOR_HV); - return; + return PPC_INTERRUPT_HDOORBELL; } if (env->pending_interrupts & PPC_INTERRUPT_PERFM) { - env->pending_interrupts &= ~PPC_INTERRUPT_PERFM; - powerpc_excp(cpu, POWERPC_EXCP_PERFM); - return; + return PPC_INTERRUPT_PERFM; } /* Thermal interrupt */ if (env->pending_interrupts & PPC_INTERRUPT_THERM) { - env->pending_interrupts &= ~PPC_INTERRUPT_THERM; - powerpc_excp(cpu, POWERPC_EXCP_THERM); - return; + return PPC_INTERRUPT_THERM; } /* EBB exception */ if (env->pending_interrupts & PPC_INTERRUPT_EBB) { @@ -1822,33 +1782,104 @@ static void ppc_hw_interrupt(CPUPPCState *env) */ if (FIELD_EX64(env->msr, MSR, PR) && (env->spr[SPR_BESCR] & BESCR_GE)) { - env->pending_interrupts &= ~PPC_INTERRUPT_EBB; - - if (env->spr[SPR_BESCR] & BESCR_PMEO) { - powerpc_excp(cpu, POWERPC_EXCP_PERFM_EBB); - } else if (env->spr[SPR_BESCR] & BESCR_EEO) { - powerpc_excp(cpu, POWERPC_EXCP_EXTERNAL_EBB); - } - - return; + return PPC_INTERRUPT_EBB; } } } - if (env->resume_as_sreset) { - /* - * This is a bug ! It means that has_work took us out of halt without - * anything to deliver while in a PM state that requires getting - * out via a 0x100 - * - * This means we will incorrectly execute past the power management - * instruction instead of triggering a reset. - * - * It generally means a discrepancy between the wakeup conditions in the - * processor has_work implementation and the logic in this function. - */ - cpu_abort(env_cpu(env), - "Wakeup from PM state but interrupt Undelivered"); + return 0; +} + +static void ppc_hw_interrupt(CPUPPCState *env, int pending_interrupt) +{ + PowerPCCPU *cpu = env_archcpu(env); + + switch (pending_interrupt) { + case PPC_INTERRUPT_RESET: /* External reset */ + env->pending_interrupts &= ~PPC_INTERRUPT_RESET; + powerpc_excp(cpu, POWERPC_EXCP_RESET); + break; + case PPC_INTERRUPT_MCK: /* Machine check exception */ + env->pending_interrupts &= ~PPC_INTERRUPT_MCK; + powerpc_excp(cpu, POWERPC_EXCP_MCHECK); + break; +#if 0 /* TODO */ + case PPC_INTERRUPT_DEBUG: /* External debug exception */ + env->pending_interrupts &= ~PPC_INTERRUPT_DEBUG; + powerpc_excp(cpu, POWERPC_EXCP_DEBUG); + break; +#endif + + case PPC_INTERRUPT_HDECR: /* Hypervisor decrementer exception */ + /* HDEC clears on delivery */ + env->pending_interrupts &= ~PPC_INTERRUPT_HDECR; + powerpc_excp(cpu, POWERPC_EXCP_HDECR); + break; + case PPC_INTERRUPT_HVIRT: /* Hypervisor virtualization interrupt */ + powerpc_excp(cpu, POWERPC_EXCP_HVIRT); + break; + + case PPC_INTERRUPT_EXT: + if (books_vhyp_promotes_external_to_hvirt(cpu)) { + powerpc_excp(cpu, POWERPC_EXCP_HVIRT); + } else { + powerpc_excp(cpu, POWERPC_EXCP_EXTERNAL); + } + break; + case PPC_INTERRUPT_CEXT: /* External critical interrupt */ + powerpc_excp(cpu, POWERPC_EXCP_CRITICAL); + break; + + case PPC_INTERRUPT_WDT: /* Watchdog timer on embedded PowerPC */ + env->pending_interrupts &= ~PPC_INTERRUPT_WDT; + powerpc_excp(cpu, POWERPC_EXCP_WDT); + break; + case PPC_INTERRUPT_CDOORBELL: + env->pending_interrupts &= ~PPC_INTERRUPT_CDOORBELL; + powerpc_excp(cpu, POWERPC_EXCP_DOORCI); + break; + case PPC_INTERRUPT_FIT: /* Fixed interval timer on embedded PowerPC */ + env->pending_interrupts &= ~PPC_INTERRUPT_FIT; + powerpc_excp(cpu, POWERPC_EXCP_FIT); + break; + case PPC_INTERRUPT_PIT: /* Programmable interval timer on embedded PowerPC */ + env->pending_interrupts &= ~PPC_INTERRUPT_PIT; + powerpc_excp(cpu, POWERPC_EXCP_PIT); + break; + case PPC_INTERRUPT_DECR: /* Decrementer exception */ + if (ppc_decr_clear_on_delivery(env)) { + env->pending_interrupts &= ~PPC_INTERRUPT_DECR; + } + powerpc_excp(cpu, POWERPC_EXCP_DECR); + break; + case PPC_INTERRUPT_DOORBELL: + env->pending_interrupts &= ~PPC_INTERRUPT_DOORBELL; + if (is_book3s_arch2x(env)) { + powerpc_excp(cpu, POWERPC_EXCP_SDOOR); + } else { + powerpc_excp(cpu, POWERPC_EXCP_DOORI); + } + break; + case PPC_INTERRUPT_HDOORBELL: + env->pending_interrupts &= ~PPC_INTERRUPT_HDOORBELL; + powerpc_excp(cpu, POWERPC_EXCP_SDOOR_HV); + break; + case PPC_INTERRUPT_PERFM: + env->pending_interrupts &= ~PPC_INTERRUPT_PERFM; + powerpc_excp(cpu, POWERPC_EXCP_PERFM); + break; + case PPC_INTERRUPT_THERM: /* Thermal interrupt */ + env->pending_interrupts &= ~PPC_INTERRUPT_THERM; + powerpc_excp(cpu, POWERPC_EXCP_THERM); + break; + case PPC_INTERRUPT_EBB: /* EBB exception */ + env->pending_interrupts &= ~PPC_INTERRUPT_EBB; + if (env->spr[SPR_BESCR] & BESCR_PMEO) { + powerpc_excp(cpu, POWERPC_EXCP_PERFM_EBB); + } else if (env->spr[SPR_BESCR] & BESCR_EEO) { + powerpc_excp(cpu, POWERPC_EXCP_EXTERNAL_EBB); + } + break; } } @@ -1884,15 +1915,38 @@ bool ppc_cpu_exec_interrupt(CPUState *cs, int interrupt_request) { PowerPCCPU *cpu = POWERPC_CPU(cs); CPUPPCState *env = &cpu->env; + int pending_interrupt; - if (interrupt_request & CPU_INTERRUPT_HARD) { - ppc_hw_interrupt(env); - if (env->pending_interrupts == 0) { - cs->interrupt_request &= ~CPU_INTERRUPT_HARD; - } - return true; + if ((interrupt_request & CPU_INTERRUPT_HARD) == 0) { + return false; } - return false; + + pending_interrupt = ppc_pending_interrupt(env); + if (pending_interrupt == 0) { + if (env->resume_as_sreset) { + /* + * This is a bug ! It means that has_work took us out of halt + * without anything to deliver while in a PM state that requires + * getting out via a 0x100 + * + * This means we will incorrectly execute past the power management + * instruction instead of triggering a reset. + * + * It generally means a discrepancy between the wakeup conditions in + * the processor has_work implementation and the logic in this + * function. + */ + cpu_abort(env_cpu(env), + "Wakeup from PM state but interrupt Undelivered"); + } + return false; + } + + ppc_hw_interrupt(env, pending_interrupt); + if (env->pending_interrupts == 0) { + cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD); + } + return true; } #endif /* !CONFIG_USER_ONLY */
Move the interrupt masking logic to a new method, ppc_pending_interrupt, and only handle the interrupt processing in ppc_hw_interrupt. Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br> --- target/ppc/excp_helper.c | 228 ++++++++++++++++++++++++--------------- 1 file changed, 141 insertions(+), 87 deletions(-)