Re: [OpenBIOS] [PATCH 2/2] arch/ppc/qemu: simplify MMU exception handlers

[Alexander Graf]

On 28.05.14 21:07, BALATON Zoltan wrote:
> Remove duplicated code from the handlers for DSI and ISI exceptions.
>
> Signed-off-by: BALATON Zoltan

I can't say I'm a big fan of this patch. Jumping from one handler into 
another is a big red flag to me. If you really think it's worth to 
consolidate these 3 instructions, please create a separate 
call_exception_handler function that you call from the DSI and ISI handler.

Alex
> openbios-devel/arch/ppc/qemu/start.S | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/openbios-devel/arch/ppc/qemu/start.S b/openbios-devel/arch/ppc/qemu/start.S
> index ae2fd53..5aa8c62 100644
> --- a/openbios-devel/arch/ppc/qemu/start.S
> +++ b/openbios-devel/arch/ppc/qemu/start.S
> @@ -359,18 +359,14 @@ VECTOR( 0x2200, "ISI_64" ):
>   real_dsi:
>    EXCEPTION_PREAMBLE
>    LOAD_REG_FUNC(r3, dsi_exception)
> mtctr	r3
> bctrl
> b exception_return
>
> b call_exception_handler
>
> real_isi:
> EXCEPTION_PREAMBLE
> LOAD_REG_FUNC(r3, isi_exception)
>
> +call_exception_handler:
>    mtctr	r3
>    bctrl
> b exception_return
>
> -exception_return:
>    EXCEPTION_EPILOGUE
>
> GLOBL(__vectors_end):

[PATCH 2/2] arch/ppc/qemu: simplify MMU exception handlers

[BALATON Zoltan]

On Thu, 29 May 2014, Alexander Graf wrote:
> On 28.05.14 21:07, BALATON Zoltan wrote:
> > Remove duplicated code from the handlers for DSI and ISI exceptions.
> >
> > Signed-off-by: BALATON Zoltan
>
> I can't say I'm a big fan of this patch. Jumping from one handler into 
> another is a big red flag to me. If you really think it's worth to 
> consolidate these 3 instructions, please create a separate 
> call_exception_handler function that you call from the DSI and ISI handler.

This is assembly and these are not functions but labels. Jumping to the 
next instruction (like the b exception_return before the exception_return: 
label) is silly.

Regards,
BALATON Zoltan
> Alex
> > openbios-devel/arch/ppc/qemu/start.S | 8 ++------
> >   1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/openbios-devel/arch/ppc/qemu/start.S 
> > b/openbios-devel/arch/ppc/qemu/start.S
> > index ae2fd53..5aa8c62 100644
> > --- a/openbios-devel/arch/ppc/qemu/start.S
> > +++ b/openbios-devel/arch/ppc/qemu/start.S
> > @@ -359,18 +359,14 @@ VECTOR( 0x2200, "ISI_64" ):
> >   real_dsi:
> >    EXCEPTION_PREAMBLE
> >    LOAD_REG_FUNC(r3, dsi_exception)
> > mtctr	r3
> > bctrl
> > b exception_return
> > b call_exception_handler
> > real_isi:
> > EXCEPTION_PREAMBLE
> > LOAD_REG_FUNC(r3, isi_exception)
> >
> > +call_exception_handler:
> >    mtctr	r3
> >    bctrl
> > b exception_return
> >
> > -exception_return:
> >    EXCEPTION_EPILOGUE
> >     GLOBL(__vectors_end):

[PATCH 2/2] arch/ppc/qemu: simplify MMU exception handlers

[Alexander Graf]

On 29.05.14 01:09, BALATON Zoltan wrote:
> On Thu, 29 May 2014, Alexander Graf wrote:
> > On 28.05.14 21:07, BALATON Zoltan wrote:
> > > Remove duplicated code from the handlers for DSI and ISI exceptions.
> > >
> > > Signed-off-by: BALATON Zoltan
> >
> > I can't say I'm a big fan of this patch. Jumping from one handler 
> > into another is a big red flag to me. If you really think it's worth 
> > to consolidate these 3 instructions, please create a separate 
> > call_exception_handler function that you call from the DSI and ISI 
> > handler.
>
> This is assembly and these are not functions but labels. Jumping to 
> the next instruction (like the b exception_return before the 
> exception_return: label) is silly.

Have you ever tried to maintain assembly code that wasn't trying really 
really hard to give you a good level of readability? :)

Alex

[PATCH 2/2] arch/ppc/qemu: simplify MMU exception handlers

[BALATON Zoltan]

On Thu, 29 May 2014, Alexander Graf wrote:
> On 29.05.14 01:09, BALATON Zoltan wrote:
> > On Thu, 29 May 2014, Alexander Graf wrote:
> > > On 28.05.14 21:07, BALATON Zoltan wrote:
> > > > Remove duplicated code from the handlers for DSI and ISI exceptions.
> > > >
> > > > Signed-off-by: BALATON Zoltan
> > >
> > > I can't say I'm a big fan of this patch. Jumping from one handler into 
> > > another is a big red flag to me. If you really think it's worth to 
> > > consolidate these 3 instructions, please create a separate 
> > > call_exception_handler function that you call from the DSI and ISI 
> > > handler.
> >
> > This is assembly and these are not functions but labels. Jumping to the 
> > next instruction (like the b exception_return before the exception_return: 
> > label) is silly.
>
> Have you ever tried to maintain assembly code that wasn't trying really 
> really hard to give you a good level of readability? :)

No, but for me long code with repeating parts is less readable than 
shorter one with less repetitions. Hence I thought it could worth removing 
these repetitions in code that is so close together so it becomes shorter.

But it's your decision. If you don't like this patch don't take it.

Regards,
BALATON Zoltan