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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.08.2020 15:08, Julien Grall wrote:
> Hi Jan,
> 
> On 18/08/2020 12:30, Jan Beulich wrote:
>> 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().
> 
> You make it sounds easy to be aware, however there are two implementations of 
> vmap_to_mfn() (one per arch). Even looking at the x86 version, it is not 
> obvious there is a restriction.

I didn't mean to make it sound like this - I agree it's not an obvious
restriction.

> So I am a bit concerned of the "misuse" of the function. This could possibly 
> be documented. Although, I am not entirely happy to restrict the use because 
> of x86.

Unless the underlying issue gets fixed, we need _some_ form of bodge.
I'm not really happy with the BUG_ON() as proposed by Hongyan. You're
not really happy with my first proposed alternative, and you didn't
comment on the 2nd one (still kept in context below). Not sure what
to do: Throw dice?

>> 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.
> 
> It may not always be convenient to keep the MFN in hand for the duration of 
> the mapping.
> 
> An example was discussed in [1]. Roughly, the code would look like:
> 
> acpi_os_alloc_memory(...)
> {
>      mfn = alloc_boot_pages(...);
>      vmap(mfn, ...);
> }
> 
> 
> acpi_os_free_memory(...)
> {
>     mfn = vmap_to_mfn(...);
>     vunmap(...);
> 
>     init_boot_pages(mfn, ...);
> }

To a certain degree I'd consider this an abuse of the interface, but
yes, I see your point (and I was aware that there are possible cases
where keeping the MFN(s) in hand may be undesirable). Then again
there's very little use of vmap_to_mfn() in the first place, so I
didn't think it was very likely that a problematic case appeared
until the proper fixing of the issue.

Jan

>>>>> 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
>>
> 
> [1] 
> <a71d1903267b84afdb0e54fa2ac55540ab2f9357.1588278317.git.hongyxia@xxxxxxxxxx>
>