Merge Slite with main branch, for discussion. by WenyuanShao · Pull Request #445 · gwsystems/composite

WenyuanShao · 2022-03-01T04:46:00Z

Summary of this Pull Request (PR)

Add description here.

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):

(Specify @<github.com username(s)> of the reviewers. Ex: @user1, @user2)

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):

micro_booter
unit_pingpong
unit_schedtests
...(add others here)

* Have SCB capability and resource working. * TODO: DCB capabilities to work. * TODO: Fix the API around SCB frontier. * SCB address in a component will be the start of the heap pointer and the INIT DCB (initial dcb caps that are used when creating the INIT threads in those components) are next to SCB and statically set to be NUM_CPU number of pages. This is the idea to fix their addresses and avoid passing in component_information structure.

gparmer

I'm posting this for now. Still working my way through. You did a very good job pulling this out. I'm mainly providing context, and provide you feedback for where areas of concern might be. Think of this as stream-of-consciousness, not as having very actionable feedback.

gparmer · 2022-03-01T20:44:04Z

src/components/interface/capmgr/capmgr.h

 asndcap_t capmgr_asnd_key_create(cos_channelkey_t key);
 asndcap_t COS_STUB_DECL(capmgr_asnd_key_create)(cos_channelkey_t key);

+int capmgr_thd_migrate(thdid_t tid, thdcap_t tc, cpuid_t core);


Yikes. Not sure we have this at all. Might be dead code?

gparmer · 2022-03-01T22:20:35Z

src/components/interface/capmgr/stubs/c_stub.c

 }

-COS_CLIENT_STUB(thdcap_t, capmgr_initthd_create, spdid_t child, thdid_t *tid)
+COS_CLIENT_STUB(thdcap_t, capmgr_thd_retrieve, spdid_t child, thdid_t tid, thdid_t *inittid)


If we can avoid pulling this in, we want to. Not a high priority though. Might want to add to the list of "fix later" if we have to pull it in.

gparmer · 2022-03-01T22:22:07Z

src/components/interface/capmgr/stubs/c_stub.c


-	ret = cos_sinv_2rets(uc->cap_no, child, idx, 0, 0, &tid_ret, &unused);
+	ret = cos_sinv_2rets(uc->cap_no, id, 0, 0, 0, &tid_ret, &dcb_ret);
+	*dcb = &dcb_ret;


Isn't this completely broken? We cannot return the address of a stack allocated value.

I'm worried that we aren't getting a compiler warning due to this.

gparmer · 2022-03-01T22:22:33Z

src/components/interface/capmgr/stubs/c_stub.c

+	thdcap_t ret;
+
+	ret = cos_sinv_2rets(uc->cap_no, child, idx, 0, 0, &tid_ret, &dcb_ret);
+	*dcb = &dcb_ret;


gparmer · 2022-03-01T22:23:08Z

src/components/interface/capmgr/stubs/c_stub.c

 	aep->tc   = (tcrcvret >> 16);
 	aep->tid  = tid;

+	*dcb = &dcb_ret;


I have no understanding of why this is working!

gparmer · 2022-03-08T23:24:54Z

src/components/lib/sl/sl.h

+
 static inline int
-sl_thd_activate(struct sl_thd *t, sched_tok_t tok)
+sl_thd_activate(struct sl_thd *t, sched_tok_t tok, tcap_time_t timeout)


Confused by the timeout here. We'll see where thaht goes.

gparmer · 2022-03-08T23:25:21Z

src/components/lib/sl/sl.h

 	}
 }

+static inline int


Yeah, this is the core of the logic for slite. This is very very delicate, and we'll want to try and change it as little as possible. Phani took a lot of time on this, and I completely trust his judgement for where it ended up.

...of course, we'll need to update for x86-64, and provide a default version that always uses the slowpath for architectures for which we don't have a fastpath.

gparmer · 2022-03-08T23:31:30Z

src/components/lib/sl_capmgr/sl_capmgr.c

-sl_thd_aep_alloc_ext(struct cos_defcompinfo *comp, struct sl_thd *sched_thd, thdclosure_index_t idx, int is_aep, int own_tcap, cos_channelkey_t key, microsec_t ipiwin, u32_t ipimax, arcvcap_t *extrcv)
+sl_thd_aep_alloc_ext_dcb(struct cos_defcompinfo *comp, struct sl_thd *sched_thd, thdclosure_index_t idx, int is_aep, int own_tcap, cos_channelkey_t key, dcbcap_t dcap, dcboff_t doff, microsec_t ipiwin, u32_t ipimax, arcvcap_t *extrcv)
+{
+	PRINTC("UNIMPLEMENTED: Using CAPMGR API which should manage the DCB capabilities\n");


You pointed these out before. We'll certainly want to invert the logic. The capmgr should manage the scb/dcbs, and the backend should be easier to integrate with slm

gparmer · 2022-03-08T23:33:00Z

src/components/lib/sl_kernel/sl_raw.c

 	sl_cs_exit();
 }
+
+int


I don't think that we'll find much use for all of this migration complexity. It spans most of the abstraction layers.

gparmer · 2022-03-08T23:33:36Z

src/kernel/capinv.c

 #include "include/hw.h"
 #include "include/chal/chal_proto.h"
+#include "include/scb.h"
+//#include "include/dcb.h"


if you can, take the opportunity to clean up stuff like this.

gparmer

OK, made it through. Some additional thoughts.

gparmer · 2022-03-08T23:47:43Z

src/kernel/capinv.c

 		sz   = __captbl_cap2bytes(type);

+		/* don't allow cap copy on SCB/DCB */
+		if (type == CAP_SCB || type == CAP_DCB) return -EINVAL;


Oh, wow, so no delegation at all for these. OK, somewhat surprising. Likely to keep things simple.

gparmer · 2022-03-08T23:48:28Z

src/kernel/capinv.c

+	 * The rest is all co-operative, so if sched_tok in scb page
+	 * increments after someone fetching a tok, then check for that!
+	 *
+	 * FIXME: make sure we're checking the scb of the scheduling component and not in any other component.


this is a worrisome comment

gparmer · 2022-03-08T23:49:30Z

src/kernel/capinv.c


 	if (thd_cap->cpuid != get_cpuid() || thd_cap->cpuid != next->cpuid) return -EINVAL;
+	if (unlikely(thd->dcbinfo && thd->dcbinfo->sp)) {
+		assert((unsigned long)regs->cx == thd->dcbinfo->ip + DCB_IP_KERN_OFF);


These are fine for debugging, but have to go after we get it to work. User-level can cause the kernel to crash trivially with these.

gparmer · 2022-03-08T23:52:33Z

src/kernel/capinv.c

+			ret = cap_kmem_activate(ct, ptcap, kaddr, (unsigned long *)&dcb, &pte);
+			if (ret) cos_throw(err, ret);
+
+			ret = dcb_activate(ct, cap, dcbcap, (vaddr_t)dcb, lid, ptcapin, uaddrin);


OK, great, they are both allocated as kernel memory. DCB is mapped into memory at creation time as well (uaddrin).

gparmer · 2022-03-08T23:54:12Z

src/kernel/include/component.h


 	return 0;

+/*undo_scb:


gparmer · 2022-03-08T23:56:24Z

src/kernel/include/shared/cos_types.h

 	BOOT_CAPTBL_SINV_CAP           = 32,
-	BOOT_CAPTBL_SELF_INITHW_BASE   = 36,
-	BOOT_CAPTBL_SELF_INITTHD_BASE  = 40,
+	BOOT_CAPTBL_SELF_SCB           = 36, /* FIXME: Do we need this? */


Hope you were very careful when merging these!

gparmer · 2022-03-08T23:58:09Z

src/kernel/include/thd.h

 	thd_scheduler_set(thd, thd_current(cli));

-	thd_rcvcap_init(thd);
+	/* TODO: fix the way to specify scheduler in a component! */


This whole thing with init_data feels like a hack that might cause us headaches. Better ideas?

gparmer · 2022-03-08T23:59:13Z

src/kernel/include/thd.h

 	memcpy(&curr->regs, regs, sizeof(struct pt_regs));
 }

+static int


We didn't talk about all of this. The SCB is also used to send interrupt events up to user-level now.

… work

…rking in dcb

phanikishoreg and others added 3 commits February 17, 2022 15:58

Almost finish merge. Capmgr still have some problem.

3dd6a37

further merge sl lib and clean up capmgr

eec3a08

gparmer reviewed Mar 8, 2022

View reviewed changes

WenyuanShao added 4 commits March 13, 2022 23:29

fix kernel_test, part of tcap tests and async endpoints still doesn't…

7c6563d

… work

enable scb creation in capmgr, still working on get dcb management wo…

6a7ae74

…rking in dcb

Push libdcb

1897e96

cleanup environment, and rewind modifications on asnd

6fb75e5

WenyuanShao requested review from betahxy and gparmer April 18, 2022 13:41


		return 0;

		/*undo_scb:

Conversation

WenyuanShao commented Mar 1, 2022

Summary of this Pull Request (PR)

Intent for your PR

Reviewers (Mandatory):

Code Quality

Testing

Uh oh!

gparmer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gparmer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants