Differential D17257

amd64: macroify copyin/copyout and provide erms variants
ClosedPublic
Actions

Authored by mjg on Sep 20 2018, 10:39 AM.

Details

Reviewers

kib

Commits

rS338837: amd64: macroify copyin/copyout and provide erms variants

Summary

The patch deduplicates all the code with a set of common macros.

I don't know what to do with pre-existing comments, they interfere with current macro use.

There will be further changes to slightly optimize this code.

Test Plan

compared assembly before and after. booted no smap, no erms + no smap, erms + smap, erms boxes.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

mjg created this revision.Sep 20 2018, 10:39 AM

Herald added a subscriber: imp. · View Herald TranscriptSep 20 2018, 10:39 AM

Harbormaster completed remote builds in B19694: Diff 48259.Sep 20 2018, 10:39 AM

mjg edited the summary of this revision. (Show Details)Sep 20 2018, 10:42 AM

Now that I uploaded the patch I see a bug for the case I did not boot: SMAP without erms - it can exit with smap disabled. Will update the patch later, but feedback on approach taken is appreciated.

fixed the smap+noerms case. forced a smap+erms capable box to use it, no problems.

I have a global opinion about this that would also handle the comments issue.

When I worked on PTI patch, I initially used C preprocessor for the macros, but it quickly came out of control. Comments were the minor problem. Much bigger issue is the inability to edit the asm without much pain due to backslashes, and most important, the high distance between code fragment in the macro and its use in the actual code. So I ended up switching to the asm macros instead.

WIth asm macros, you can write single macro parametrized by ERMS/SMAP and whatever additional tunables. Then tunable would conditionally disable part of the code. Benefits are that you write normal asm, readers see normal asm, and the tuning when some feature is disabled in the instantiation, is easy to read. Look at amd64/include/asmacros.h for examples.

sys/amd64/amd64/copyout.c
169 ↗	(On Diff #48260)	spaces around '\|'.

rework the patch to use arm macros

I don't see how to produce necessary function names. All concatenation examples I found use passed values, none select a part of the name based on a value. Thus the somewhat ugly repetition of name and params.

kib accepted this revision.Sep 20 2018, 4:04 PM

kib added inline comments.

sys/amd64/amd64/support.S
386 ↗	(On Diff #48270)	Are you sure you want to remove this ?

This revision is now accepted and ready to land.Sep 20 2018, 4:04 PM

mjg marked an inline comment as done.Sep 20 2018, 5:13 PM

mjg added inline comments.

sys/amd64/amd64/support.S
386 ↗	(On Diff #48270)	No, it was meant to stay here temporarily. copyin_fault and copyout_fault are identical and next I'll propose a change to both unify them in copy_fault and move the fault itself to the end of the file.