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

[Hongyan Xia <hx242@xxxxxxx>]

On Mon, 2020-11-30 at 13:50 +0100, Jan Beulich wrote:
> On 30.11.2020 13:13, Hongyan Xia wrote:
> > On Tue, 2020-08-18 at 18:16 +0200, Jan Beulich wrote:
> > [...]
> > 
> > Actually I did not propose the BUG_ON() fix. I was just in favor of
> > it
> > when Jan presented it as a choice in v7.
> > 
> > The reason I am in favor of it is that even without it, the
> > existing
> > x86 code already BUG_ON() when vmap has a superpage anyway, so it's
> > not
> > like other alternatives behave any differently for superpages. I am
> > also not sure about returning INVALID_MFN because if
> > virt_to_xen_l1e()
> > really returns NULL, then we are calling vmap_to_mfn() on a non-
> > vmap
> > address (not even populated) which frankly deserves at least
> > ASSERT().
> > So, I don't think BUG_ON() is a bad idea for now before
> > vmap_to_mfn()
> > supports superpages.
> > 
> > Of course, we could use MAP_SMALL_PAGES to avoid the whole problem,
> > but
> > Arm may not be happy. After a quick chat with Julien, how about
> > having
> > ARCH_VMAP_FLAGS and only small pages for x86?
> 
> Possibly, albeit this will then make it look less like a bodge and
> more like we would want to keep it this way. How difficult would it
> be to actually make the thing work with superpages? Is it more than
> simply
> (pseudocode, potentially needed locking omitted):
> 
> vmap_to_mfn(va) {
>     pl1e = virt_to_xen_l1e(va);
>     if ( pl1e )
>         return l1e_get_mfn(*pl1e);
>     pl2e = virt_to_xen_l2e(va);
>     if ( pl2e )
>         return l2e_get_mfn(*pl2e) + suitable_bits(va);
>     return l3e_get_mfn(*virt_to_xen_l3e(va)) + suitable_bits(va);
> }
> 
> (assuming virt_to_xen_l<N>e() would be returning NULL in such a
> case)?

The sad part is that instead of returning NULL, such functions BUG_ON()
when there is a superpage, hence why this solution was not considered.
Changing the logic from BUG_ON to returning NULL might not be
straightforward, since so far the callers assume NULL means -ENOMEM and
not anything else.

> Not very efficient, but not needed anywhere anyway - the sole user of
> the construct is domain_page_map_to_mfn(), which maps only individual
> pages. (An even better option would be to avoid the recurring walk of
> the higher levels by using only virt_to_xen_l3e() and then doing the
> remaining steps "by hand". This would at once allow to avoid the here
> unwanted / unneeded checking for whether page tables need
> allocating.)

The "even better option" looks more promising to me, and is what I want
to go forward with. At any rate, this fix grows larger than intended
and I want to send this as an individual patch. Any objections?

Hongyan