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

[Julien Grall <julien@xxxxxxx>]

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.
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.
> 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, ...);
}

> > > > 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>
--
Julien Grall