On Fri, 24 Aug 2012, Konrad Rzeszutek Wilk wrote:
His goal was to document the semantics of the call. We all want to cleanNo. His logic to deal with the semantics started to imply wrong and
up the mess of extra calls that don't make sense (remember the
write_msr_safe one?) and the first step is get some of the calls
documented so that we know if some of these calls can be moved around
for refactoring. Attilio went then beyond that being enthuastic about
this and wrote logic to deal with the description of the semantics.
In part this would help the refactoring as it would catch runtime
issues.
silly semantics in the first place. What's the point of making a
function deal with A != B, where A is required to be equal to B. We do
not add special cases for stuff which cannot happen neither on
baremetal nor on XEN. Period.
That is at odds with what Peter would like to have fixed:I'm not against documentation. I'm against wrong documentation, wrong
(from
http://lists.linux-foundation.org/pipermail/ksummit-2012-discuss/2012-June/000070.html)
"
Hooks and notifiers are a form of "COME FROM" programming, and they
make it very hard to reason about the code. The only way that that
can be reasonably mitigated is by having the exact semantics of a
hook or notifier -- the preconditions, postconditions, and other
invariants -- carefully documented. Experience has shown that in
practice that happens somewhere between rarely and never.
Hooks that terminate into hypercalls or otherwise are empty in the
"normal" flow are particularly problematic, as it is trivial for a
mainstream developer to break them.
"
and silly semantics and pointless code which tries to deal with cases which
are just wrong to begin with.
I looked at the whole pgt_buf_* mess and it's amazingly stupid. We
could avoid all that dance and make all of that pgt_buf_* stuff static
and provide proper accessor functions and hand start, end, top to the
reserve function instead of fiddling with global variables all over
the place. That'd be a real cleanup and progress.
But we can't do that easily. And why? Because XEN is making magic
decisions based on those globals in mask_rw_pte().
/*
* If the new pfn is within the range of the newly allocated
* kernel pagetable, and it isn't being mapped into an
* early_ioremap fixmap slot as a freshly allocated page, make sure
* it is RO.
*/
if (((!is_early_ioremap_ptep(ptep)&&
pfn>= pgt_buf_start&& pfn< pgt_buf_top)) ||
(is_early_ioremap_ptep(ptep)&& pfn != (pgt_buf_end - 1)))
This comment along with the implementation is really a master piece of
obfuscation. Let's see what this is doing. RO is enforced when:
This is not an early ioreamp AND
pfn>= pgt_buf_start&& pfn< pgt_buf_top
So why is this checking pgt_buf_top? The early stuff is installed
within pgt_buf_start and pgt_buf_end. Anything which is>=
pgt_buf_end at this point is completely wrong.
Now the second check is even more interesting:
If this is an early ioremap AND
pfn != (pgt_buf_end -1 )
then it's forced RO as well.
So this checks whether the early ioremap is happening on the last
allocated pfn from the pgt_buf range.
OMG, really great design! And the comment above that if() obfuscation
is not really helping much.
If anything is missing a semantic documentation and analysis then
definitely code like this which is just a cobbled together steaming
pile of ....