lkml.org 
[lkml]   [2012]   [Aug]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 1/2] XEN/X86: Improve semantic support for x86_init.mapping.pagetable_reserve
On Fri, 24 Aug 2012, Konrad Rzeszutek Wilk wrote:
> His goal was to document the semantics of the call. We all want to clean
> 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.

No. His logic to deal with the semantics started to imply wrong and
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:
> (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.
> "

I'm not against documentation. I'm against wrong documentation, wrong
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 ....

Thanks,

tglx





\
 
 \ /
  Last update: 2012-08-24 15:41    [W:0.064 / U:0.404 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site