Re: [PATCH] x86: use pgd accessors when cloning a pgd range.

["H. Peter Anvin" <hpa@linux.intel.com>]

On 10/27/2010 10:51 AM, Jeremy Fitzhardinge wrote:
>>
>> This is what makes me absolutely hate paravirt with a passion...
>> "let's hid things away in<obscure place>  and make it absolutely
>> impossible to either follow the code flow or figure out what the
>> intended semantics are supposed to be."
>
> Its not really an obscure place; it's where x86-32 does the rest of its
> boot-time pagetable adjustments (like cleaning out the low identity
> maps, etc).  Having those clone_pgd_ranges() floating around in
> setup_arch() is out of place.
>

"Cleaning out the low identity maps" is part of what this patchset 
eliminates.  This is exactly a good reason why paravirt_ops damages the 
kernel -- it makes it impossible to make forward process.

>> (Let not even get me started on how ill-defined the semantics of some
>> of the paravirt operations are.)  In this case, at the most you need a
>> single flag of state... or you could even just ignore this low-level
>> data structure that you will never use in the first place.  Ian's
>> message just mentioned "a failure" and never described in any way what
>> kind of "failure" it was.
>
> It would be a pagefault from Xen preventing a direct write to the pgd
> level of an active pagetable.  At the point in setup_arch() where it
> does the first clone_pgd_range() we're already running on swapper_pg_dir
> and the copy from initial_page_table is outright wrong.
>
> As Ian suggests, we could switch Xen to use initial_page_table at boot
> then move to swapper_pg_dir in the same way native does.

Once the failure was explained, it makes more sense.  Either that or 
just skip this setting if we're already running on swapper_pg_dir.

Let me state this clearly: if Xen is going to continue to live as a 
merged platform, it has to have an obligation to follow changes on the 
native platform.  This is not unique to Xen, but rather a universal rule 
for integrated platforms.  Xen is more widely used than a lot of the 
other minority platforms, which means it legitimately gets allowed more 
slack, but that is moderated by its tremendous invasiveness.

Quite frankly, the single biggest thing you could improve is to improve 
documentation about what you expect in terms of semantics of various 
entry points.  There are a number of cleanups which we currently cannot 
do because they are directly mapped to paravirt_ops which unclear or 
nonsensical semantics.  Having a more explicit description of the design 
space would help there.

paravirt_ops is fundamentally misdesigned as a large monolithic 
driverization layer which combines a lot of unrelated things.  In a 
whole lot of cases it directly duplicates driverization layers already 
in the kernel, meaning we take the cost both in cost clarity and 
performance multiple times.  The patching technology is nice, and it 
would be good to have that available to other platform layers as well, 
but paravirt_ops as it currently sits is going to have to go at some point.

	-hpa