Re: [PATCH v4 1/2] XEN/X86: Improve semantic support for x86_init.mapping.pagetable_reserve

[Thomas Gleixner <tglx@linutronix.de>]

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