x86-64 PCID and PMU Support #446

evanstella · 2022-03-09T20:09:49Z

Summary of this Pull Request (PR)

This PR is for an initial review of PCID support. This is not ready to be pulled but will need to be integrated with the namespace implementation that is being developed right now.

This PR adds support for PCID, a kernel API for using PCID for an ASID implementation, as well as PMU programming support for Intel's hardware PMU.

Particularly, I'd like your opinion on:

the kernel ASID API and whether it makes sense
The updated context switch code (chal_pgtbl_update())
(less important) The PMU programming API and whether this is actually an addition we would want to add to the kernel API

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

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
All test outlined in the x86-64 patch

gparmer

Looks mostly great. Some comments that are mainly stream of thought.

gparmer · 2022-03-10T15:06:08Z

composition_scripts/unit_pmu.toml

@@ -0,0 +1,36 @@
+[system]
+description = "Simplest system with both capability manager and scheduler to test shared memory implementation"


focus is now on PMU testing, right?

gparmer · 2022-03-10T15:06:46Z

src/components/implementation/no_interface/llbooter/llbooter.c

@@ -514,6 +514,7 @@ cos_init(void)
 {
 	booter_init();
 	comps_init();
+


Does your editor auto-insert blank lines at the end of functions? Very strange behavior.

gparmer · 2022-03-10T15:07:22Z

src/components/implementation/tests/unit_pingshmem/ping.c

@@ -19,6 +19,16 @@ char *pong_test_strings[] = {

 shm_bm_t shm;

+static unsigned long 
+rdpmc (unsigned long cntr)


no space between fn name and (.

I think that the question is: where should we put this function?

We'll need to consider that it will need to have a generic version (that just returns 0) for architectures that don't support it.

Yes, need to figure out what a higher level abstraction would look like

gparmer · 2022-03-10T15:16:46Z

src/components/implementation/tests/unit_pmu/pmu_test.c

+	cos_pmu_enable_fixed_counter(BOOT_CAPTBL_SELF_INITHW_BASE, 0);
+	cos_pmu_enable_fixed_counter(BOOT_CAPTBL_SELF_INITHW_BASE, 1);
+	/* enable architecture specific counter events (reference https://perfmon-events.intel.com/) */
+	cos_pmu_program_event_counter(BOOT_CAPTBL_SELF_INITHW_BASE, 0, 0x49, 0x0E);


I like the power and flexibility of this -- the direct programming of the PMUs. For that reason, we likely want this implementation.

I don't like how this can't really be fit into a capabiliyt-based system were we can delegate in a controlled fashion out the ability to program a subset of the capabilities, including limiting the # of counters set (since the hardware has a limit). A problem for a future us.

Thats actually how KVM does it from my understanding; they delegate a subset of the counters that the guest can use.

gparmer · 2022-03-10T15:19:28Z

src/components/implementation/tests/unit_pmu/pmu_test.c

+	char *buf;
+	int  i;
+
+	buf = (char *)memmgr_heap_page_allocn(NUM_PAGES);


check return value, at least with assert.

gparmer · 2022-03-10T17:41:38Z

src/platform/i386/chal_cpu.h

@@ -120,7 +121,7 @@ chal_cpu_init(void)
 #if defined(__x86_64__)
 	u32_t low = 0, high = 0;

-	chal_cpu_cr4_set(cr4 | CR4_PSE | CR4_PGE);
+	chal_cpu_cr4_set(cr4 | CR4_PSE | CR4_PGE | CR4_PCE | CR4_PCIDE);


Check in CPUID if the feature exists on the processor? Bomb out if it doesnt?

gparmer · 2022-03-10T17:43:07Z

src/platform/i386/chal_pgtbl.c

-
+#if defined(__i386__)	
+	/* x86-32 does not support PCID :( */
+	if (asid != 0) return -EINVAL;


maybe it is better to just proceed. x86-32 can pass in any asids it wants. The system will still be functionally correct. So it might be better to simply have this code for 32 be the same. In the kernel, we just ignore the value.

We don't want to have to maintain differnet client code for 32 bit vs. 64.

gparmer · 2022-03-10T17:44:48Z

src/platform/x86_64/chal_pmu.c

+extern u8_t processor_num_pmc;
+
+static inline void
+wrmsr(u32_t msr_addr, u32_t in_lo, u32_t in_hi)


Are these already in chal_cpu?

gparmer · 2022-03-10T17:45:54Z

src/platform/x86_64/chal_pmu.c

+    /* fixed counter enable is in the high dword */
+    perf_global_ctrl_hi |= 1 << cntr;
+    /* enable OS and USR mode counting of event, maybe this should be made optional? */
+    perf_fixed_ctrl_lo  |= (IA32_FIXED_CTR_CTRL_USR_EN | IA32_FIXED_CTR_CTRL_OS_EN) << (cntr * 4); 


Would be useful to be able to watch only kernel mode events for microoptimization in the kernel. But I don't like having to be considered in the API. What you have here is probably sufficient for now.

gparmer · 2022-03-10T17:48:48Z

src/platform/i386/chal/chal_plat.h

+	asm volatile("invpcid %0, %1" : : "m"(desc), "r"(type) : "memory");
+}
+
+static inline unsigned long


I don't see where this and __write_cr3 are used. __write* I can see being used to switch page-tables. but __readcr3?

gparmer · 2022-03-10T17:50:27Z

You do need to add logic into pgtbl_deactivate to remove the asid from the per-cpu table. We need to discuss this as we might be deactivating on a different core than that on which the component executed.

evanstella added 7 commits February 28, 2022 12:02

pmu stuff

e789001

pmu implementation

21f7ad6

asid pgtbl switch implementation

dbdeeb2

add asid to cosdefkernel

8ebb521

formatting

4db0690

remove erroneous printc

0947676

remove tlb miss counting from benchmark

e75a549

gparmer reviewed Mar 10, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x86-64 PCID and PMU Support #446

x86-64 PCID and PMU Support #446

evanstella commented Mar 9, 2022

gparmer left a comment

gparmer Mar 10, 2022

gparmer Mar 10, 2022

gparmer Mar 10, 2022

gparmer Mar 10, 2022

evanstella Mar 10, 2022

gparmer Mar 10, 2022

evanstella Mar 10, 2022

gparmer Mar 10, 2022

gparmer Mar 10, 2022

gparmer Mar 10, 2022

gparmer Mar 10, 2022

gparmer Mar 10, 2022

gparmer Mar 10, 2022

gparmer commented Mar 10, 2022

		@@ -0,0 +1,36 @@
		[system]
		description = "Simplest system with both capability manager and scheduler to test shared memory implementation"

x86-64 PCID and PMU Support #446

Are you sure you want to change the base?

x86-64 PCID and PMU Support #446

Conversation

evanstella commented Mar 9, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gparmer commented Mar 10, 2022