Merge i386 and amd64 mtrr drivers.
ClosedPublic
Actions

Authored by kib on Feb 17 2017, 11:54 AM.

Details

Reviewers

jhb
royger

Commits

rS313898: Merge i386 and amd64 mtrr drivers.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kib updated this revision to Diff 25307.Feb 17 2017, 11:54 AM

kib retitled this revision from to Merge i386 and amd64 mtrr drivers..

kib updated this object.

kib edited the test plan for this revision. (Show Details)

kib added reviewers: jhb, royger.

kib set the repository for this revision to rS FreeBSD src repository - subversion.

Herald added subscribers: rpokala, imp. · View Herald TranscriptFeb 17 2017, 11:54 AM

kib added inline comments.Feb 17 2017, 11:55 AM

sys/x86/x86/x86_mem.c
733 ↗	(On Diff #25307)	I think that this comment could be safely removed. I am not sure what is meant by 'locking gate', but it seems that it describes an algorithm implemented by smp_rendezvous().

Thanks for doing this (I was planning to do it this weekend), LGTM. Just a minor comment regarding a repeated code chunk. In any case feel free to commit with or without that fixed, it's a general improvement after all.

sys/x86/x86/x86_mem.c
705 ↗	(On Diff #25307)	This seems to be duplicated above, maybe place it in a function?
733 ↗	(On Diff #25307)	That's basically what _rendezvous does AFAIK, so I agree that the comment can be removed.
751 ↗	(On Diff #25307)	Do you know why this check is needed? AFAIK the MTRR cpuid flag should be enough.

This revision is now accepted and ready to land.Feb 17 2017, 12:08 PM

kib marked 2 inline comments as done.Feb 17 2017, 12:23 PM

kib added inline comments.

sys/x86/x86/x86_mem.c
751 ↗	(On Diff #25307)	I can only guess. It seems that code precautiosly checks that it only handles known hardware. Also see the cpu_vendor_id check below. Since mtrrs are architectural, I agree that the check looks excessive.

Move dmap split code into function and call it instead of duplicating. Remove stale comment.

This revision now requires review to proceed.Feb 17 2017, 12:24 PM

I will wait for D9630 in HEAD.

D9630 is now committed, feel free to push this.

sys/x86/x86/x86_mem.c
751 ↗	(On Diff #25307)	Let's fix that in a different commit though, in case there are regressions and we need to back it out.

This revision is now accepted and ready to land.Feb 17 2017, 12:49 PM

After merge of 313871. Also removed second instance of the comment.

This revision now requires review to proceed.Feb 17 2017, 1:17 PM

LGTM

This revision is now accepted and ready to land.Feb 17 2017, 3:41 PM

jhb accepted this revision.Feb 17 2017, 7:16 PM

jhb edited edge metadata.

jhb added inline comments.

sys/modules/mem/Makefile
6 ↗	(On Diff #25311)	Yet another place that will be simplified if we change MACHINE_CPUARCH on i386 and amd64 to x86.
sys/x86/x86/x86_mem.c
312 ↗	(On Diff #25311)	Do we even need this #ifdef now? smp_rendezvous() works in !SMP kernels.
719 ↗	(On Diff #25311)	This #ifdef can go away as well.