Re: [PATCH v8 03/15] x86/mm: rewrite virt_to_xen_l*e

[Jan Beulich <jbeulich@suse.com>]

On 13.08.2020 19:22, Julien Grall wrote:
> Hi,
> 
> On 13/08/2020 17:08, Hongyan Xia wrote:
>> On Fri, 2020-08-07 at 16:05 +0200, Jan Beulich wrote:
>>> On 27.07.2020 16:21, Hongyan Xia wrote:
>>>> From: Wei Liu <wei.liu2@citrix.com>
>>>>
>>>> Rewrite those functions to use the new APIs. Modify its callers to
>>>> unmap
>>>> the pointer returned. Since alloc_xen_pagetable_new() is almost
>>>> never
>>>> useful unless accompanied by page clearing and a mapping, introduce
>>>> a
>>>> helper alloc_map_clear_xen_pt() for this sequence.
>>>>
>>>> Note that the change of virt_to_xen_l1e() also requires
>>>> vmap_to_mfn() to
>>>> unmap the page, which requires domain_page.h header in vmap.
>>>>
>>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>>> Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> ---
>>>> Changed in v8:
>>>> - s/virtual address/linear address/.
>>>> - BUG_ON() on NULL return in vmap_to_mfn().
>>>
>>> The justification for this should be recorded in the description. In
>>
>> Will do.
>>
>>> reply to v7 I did even suggest how to easily address the issue you
>>> did notice with large pages, as well as alternative behavior for
>>> vmap_to_mfn().
>>
>> One thing about adding SMALL_PAGES is that vmap is common code and I am
>> not sure if the Arm side is happy with it.
> 
> At the moment, Arm is only using small mapping but I plan to change that soon because we have regions that can be fairly big.
> 
> Regardless that, the issue with vmap_to_mfn() is rather x86 specific. So I don't particularly like the idea to expose such trick in common code.
> 
> Even on x86, I think this is not the right approach. Such band-aid will impact the performance as, assuming superpages are used, it will take longer to map and add pressure on the TLBs.
> 
> I am aware that superpages will be useful for LiveUpdate, but is there any use cases in upstream?

Superpage use by vmalloc() is purely occasional: You'd have to vmalloc()
2Mb or more _and_ the page-wise allocation ought to return 512
consecutive pages in the right order. Getting 512 consecutive pages is
possible in practice, but with the page allocator allocating top-down it
is very unlikely for them to be returned in increasing-sorted order.

> If not, could we just use the BUG_ON() and implement correctly vmap_to_mfn() in a follow-up?

My main concern with the BUG_ON() is that it detects a problem long after
it was introduced (when the mapping was established). I'd rather see a
BUG_ON() added there if use of MAP_SMALL_PAGES is deemed unsuitable.

Jan