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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.08.2020 12:13, Julien Grall wrote:
> Hi Jan,
> 
> On 18/08/2020 09:49, Jan Beulich wrote:
>> 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@xxxxxxxxxx>
>>>>>>
>>>>>> 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@xxxxxxxxxx>
>>>>>> Signed-off-by: Hongyan Xia <hongyxia@xxxxxxxxxx>
>>>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>>
>>>>>> ---
>>>>>> 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.
> So your assumption here is vmap_to_mfn() can only be called on vmalloc-ed() 
> area. While this may be the case in Xen today, the name clearly suggest it 
> can be called on all vmap-ed region.

No, I don't make this assumption, and I did spell this out in an earlier
reply to Hongyan: Parties using vmap() on a sufficiently large address
range with consecutive MFNs simply have to be aware that they may not
call vmap_to_mfn(). And why would they? They had the MFNs in their hands
at the time of mapping, so no need to (re-)obtain them by looking up the
translation.

>>> 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.
> 
> From what you wrote, I would agree that vmalloc() is unlikely going to use 
> superpages. But this is not going to solve the underlying problem with the 
> rest of the vmap area.
> 
> So are you suggesting to use MAP_SMALL_PAGES for *all* the vmap()?

As per above - no.

Jan