Re: [OpenBIOS] [PATCH 2/2] arch/ppc/qemu: simplify MMU exception handlers
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 <balaton@eik.bme.hu>
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):
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 <balaton@eik.bme.hu>
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):
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 <balaton@eik.bme.hu>
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
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 <balaton@eik.bme.hu>
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
participants (2)
-
Alexander Graf -
BALATON Zoltan