Re: [PATCH v2] add locking around certain calls to map_pages_to_xen()

[Keir Fraser <keir.xen@gmail.com>]

On 12/07/2013 13:44, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 12.07.13 at 14:15, Keir Fraser <keir.xen@gmail.com> wrote:
>> On 12/07/2013 09:17, "Jan Beulich" <JBeulich@suse.com> wrote:
>> 
>>> While boot time calls don't need this, run time uses of the function
>>> which may result in L2 page tables getting populated need to be
>>> serialized to avoid two CPUs populating the same L2 (or L3) entry,
>>> overwriting each other's results.
>>> 
>>> This fixes what would seem to be a regression from commit b0581b92
>>> ("x86: make map_domain_page_global() a simple wrapper around vmap()"),
>>> albeit that change only made more readily visible the already existing
>>> issue.
>>> 
>>> The __init addition to memguard_init(), while seemingly unrelated,
>>> helps making obvious that this function's use of map_pages_to_xen() is
>>> a boot time only one.
>> 
>> Why can't the locking be implemented inside map_pages_to_xen()? The
>> requirement is due to implementation details of that function after all.
>> Pushing the synchronisation out to the callers is uglier and more fragile.
> 
> Not all use cases of the function require synchronization, so the
> only kind of synchronization I would see validly adding there
> instead of in the callers would be a mechanism marking a to-be-
> populated non-leaf page table entry as "being processed" first,
> and have racing invocations spin until that state clears. Afaict
> that wouldn't cope with eventual (future) races through
> destroy_xen_mappings() though, and hence I think the proposed
> patch is the better alternative. But if you're fine with ignoring
> that last aspect, I'm okay with going the alternative route.

Is it unsafe to just stick a lock around the guts of map_pages_to_xen(), or
at least the parts that add new page tables?

> Jan
>