Messages in this thread | | | Date | Fri, 24 Aug 2012 15:00:14 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [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
| |