Implementation of fault handler #366

WenyuanShao · 2018-07-23T20:55:14Z

Summary of this Pull Request (PR)

This PR is the implementation of user_level fault handlers. When a fault happens, it will make a sinv call to the user_level fault handler to handle the fault.

Modified the invstk and set a bit into the captbl of the component on the top of the stack to indicate whether this is a sinv call to fault handlers or not.
Created a new type for sinv cap to the fault handlers, so we can get rid of a branch in the fast path.
Expanded the thd_introspect to get the register information in user_level and print these register information when fault happens.
Create an interface for fault handlers so different components can implement fault handlers differently.

Intent for your PR

Choose one (Mandatory):

This PR is for a code-review and is intended to get feedback, but not to be pulled yet.
This PR is mature, and ready to be integrated into the repo.

Reviewers (Mandatory):

@gparmer , @hungry-foolish , @phanikishoreg

Code Quality

As part of this pull request, I've considered the following:

Style:

Comments adhere to the Style Guide (SG)
Spacing adhere's to the SG
Naming adhere's to the SG
All other aspects of the SG are adhered to, or exceptions are justified in this pull request
I have run the auto formatter on my code before submitting this PR (see doc/auto_formatter.md for instructions)

Code Craftsmanship:

I've made an attempt to remove all redundant code
I've considered ways in which my changes might impact existing code, and cleaned it up
I've formatted the code in an effort to make it easier to read (proper error handling, function use, etc...)
I've commented appropriately where code is tricky
I agree that there is no "throw-away" code, and that code in this PR is of high quality

Testing

I've tested the code using the following test programs (provide list here):

-I have run llbooter_pong.sh, llbooter_test.sh, microboot.sh, unit_schedcomp.sh and unit_schedtest.sh tests.

…t, get rid of a branch in the fast path by make sinv cap to fault handler another type.

…the component which starts the invocation.

…hether it is a sinv started by a fault_handler.

gparmer

You're missing one major piece of functionality: I believe we need to make sure that sinv_activate cannot be used on the range of capability addresses used for the fault handlers. Do you agree?

gparmer · 2018-08-19T23:53:58Z

src/components/implementation/no_interface/llbooter/boot_deps.h

+void
+fault_div_by_zero(unsigned long sp, unsigned long ip, unsigned long fault_addr, unsigned long fault_type)
+{
+	PRINTLOG(PRINT_DEBUG, "in div by zero error fault handler, fault happens in component:%u\n\n", cos_inv_token());


@phanikishoreg Why did the PRINTLOG start getting pervasively used? What functionality does it provide? This isn't an API we ever discussed. Not necessarily a problem here, but I'm curious.

gparmer · 2018-08-20T00:01:41Z

src/components/include/hypercall.h

@@ -67,8 +67,8 @@ hypercall_comp_initaep_get(spdid_t spdid, int is_sched, struct cos_aep_info *aep
 	}

 	/* capid_t though is unsigned long, only assuming it occupies 16bits for packing */
-	ret = cos_sinv(BOOT_CAPTBL_SINV_CAP, 0, HYPERCALL_COMP_INITAEP_GET,


What is this old code doing!!!! Is this just as broken as it looks?

gparmer · 2018-08-20T00:03:06Z

src/components/interface/fault_handler/fault_handler.h

+#ifndef FAULT_HANDLER_H
+#define FAULT_HANDLER_H
+
+#include <cos_kernal_api.h>


How does this compile?

Is this tested?

OK, just checked. This isn't part of any DEPENDENCIES, so is essentially untested.

The complexity here is that this is in some way a "virtual" API. In some sense every component should depend on this, so we shouldn't have to list it in DEPENDENCIES. It should be the build system that adds the dependency automatically. However, that dependency doesn't do much at build time...only when the system is composed together via sinvs. So this interface is in a strange place. However, it might be nice to have, if only to document the shape of the API. That said, the code should compile....

gparmer · 2018-08-20T00:12:00Z

src/components/interface/fault_handler/stubs/s_stub.S

+
+.text
+
+cos_asm_server_stub(fault_div_by_zero)


Oddly, these don't make much sense as they can never be used...

gparmer · 2018-08-20T00:28:37Z

src/kernel/capinv.c

@@ -969,8 +970,7 @@ composite_syscall_handler(struct pt_regs *regs)
 	/* fast path: invocation return (avoiding captbl accesses) */
 	if (cap == COS_DEFAULT_RET_CAP) {
 		/* No need to lookup captbl */
-		sret_ret(thd, regs, cos_info);
-		return 0;
+		return sret_ret(thd, regs, cos_info);


Are the semantics around this correct? This is a functional change. Is it fixing a bug? What behavior is it changing?

gparmer · 2018-08-20T01:52:01Z

src/kernel/include/thd.h

-	return thd_invstk_current(thd, ip, sp, cos_info);
+	/* 
+	 * This is the only time we might use the in_fault flag.
+	 * We can reset the captbl after we get the in_fault flag.


again, I'm not sure how useful this comment is without broader context. Perhaps add that broader context into the data-structure (struct) location so that people can understand? I don't see how it is relevant here as this is "outside" of the abstraction.

gparmer · 2018-08-20T01:52:19Z

src/kernel/include/thd.h

@@ -584,6 +637,57 @@ thd_introspect(struct thread *t, unsigned long op, unsigned long *retval)
 	case THD_GET_TID:
 		*retval = t->tid;
 		break;
+	case THD_GET_FAULT_REG1:


Make a macro to do this.

gparmer · 2018-08-20T02:16:19Z

src/platform/i386/exception.c

+	cos_info = thd_invstk_current(curr_thd, &ip, &sp, ci);
+	fh = captbl_lkup(cos_info->captbl, cap);
+	__userregs_sinvargset(regs, regs->sp, regs->ip, fault_addr, cap);
+	/* This is a software fault and should be fixed in the future. */


Add FIXME in the comment.

gparmer · 2018-08-20T02:16:43Z

src/platform/i386/exception.c

+	unsigned long              ip, sp;
+	u32_t                      fault_addr = 0, errcode, eip;
+
+	fault_regs_save (regs, curr_thd);


gparmer · 2018-08-20T02:17:26Z

src/platform/i386/exception.c

 int
 div_by_zero_err_fault_handler(struct pt_regs *regs)
 {
-	print_regs_state(regs);
-	die("FAULT: Divide by Zero Error\n\n");
+	fault_handler_sinv(regs, FAULT_CAPTBL_DIVZERO);


maybe a macro to generate all of these as well?

gparmer · 2018-08-20T02:19:05Z

@WenyuanShao this is a REALLY strong first PR. Some minor issues to clear up, but this is almost ready!

WenyuanShao added 16 commits June 16, 2018 16:13

update the implementation of fault handlers.

762283a

Format change.

e0579b1

bring back the fourth arguement

0f95309

Fix a bug happened in the sret_ret

41be7b0

fix the lack of the code caused by VM crash

370a243

change name of the functions and variables

56feb7c

update the introspect of the thread registers

f9ef836

make the fault_handler an interface

0d3752a

update the interface and add a new API

e36d46f

fix some style issue.

716103b

achitecture based optimization: sp & in_fault now conbined to sp_faul…

b08e377

…t, get rid of a branch in the fast path by make sinv cap to fault handler another type.

roll back and separate sp and in_fault.

80c2ac8

optimize the invstk_entry by combine the in_fault bit with captbl of …

f9863a6

…the component which starts the invocation.

set a bit into the comp_invstk_info of the invstk_entry to indicate w…

ad4a0d8

…hether it is a sinv started by a fault_handler.

fix some style, add some comments

b1da7b2

Add more comments and delete redundant code.

99a5be5

WenyuanShao requested review from gparmer, hungry-foolish and phanikishoreg July 23, 2018 20:55

gparmer requested changes Aug 20, 2018

View reviewed changes

gparmer closed this Aug 20, 2018

WenyuanShao added a commit to WenyuanShao/composite that referenced this pull request Aug 28, 2018

slove some of the problem rasied in the PR gwsystems#366.

e200a66

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of fault handler #366

Implementation of fault handler #366

WenyuanShao commented Jul 23, 2018

gparmer left a comment

gparmer Aug 19, 2018

gparmer Aug 20, 2018

gparmer Aug 20, 2018

gparmer Aug 20, 2018

gparmer Aug 20, 2018

gparmer Aug 20, 2018

gparmer Aug 20, 2018

gparmer Aug 20, 2018

gparmer Aug 20, 2018

gparmer Aug 20, 2018

gparmer Aug 20, 2018

gparmer commented Aug 20, 2018


		.text

		cos_asm_server_stub(fault_div_by_zero)

Implementation of fault handler #366

Implementation of fault handler #366

Conversation

WenyuanShao commented Jul 23, 2018

Summary of this Pull Request (PR)

Intent for your PR

Reviewers (Mandatory):

Code Quality

Testing

gparmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gparmer commented Aug 20, 2018