Re: [Xen-devel] [PATCH 2/4] x86/mm: rename and tidy create_pae_xen_mappings()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/12/2019 14:19, Jan Beulich wrote:
> After dad74b0f9e ("i386: fix handling of Xen entries in final L2 page
> table") and the removal of 32-bit support the function doesn't modify
> state anymore, and hence its name has been misleading. Change its name,
> constify parameters and a local variable, and make it return bool.
>
> Also drop the call to it from mod_l3_entry(): The function explicitly
> disallows 32-bit domains to modify slot 3. Furthermore, re-checking
> of slot 3 when other slots change has needlessly disallowed doing this
> together with making some L2 table recursively link back to an L2 used
> in some L3's 3rd slot (because of the checking of the type ref count to
> be 1). (Note that allowing dynamic changes of L3 entries in the way we
> do is bogus anyway, as that's not how L3s behave in the native case:
> They get re-evaluated only upon CR3 reloads.)

I'm having a hard time following this.  I agree that dropping the call
from mod_l3_entry() is safe, but I cant parse the second sentence.

As for the final sentence, I'm not sure that is actually true.  In
particular, AMD NPT says that the PDPTRs may actually behave like L3's
in 4-level paging, rather than how legacy PAE used to work.

> As a result of this we no longer need to play games to get at the start
> of the L3 table. Leave an ASSERT() though, just in case.

I'd drop the assert().  The sole caller is internal, and we would have
major problems if any of mm.c operated on misaligned pointers.  (I did
double check all of the MM code following whichever XSA the misaligned
grant map by PTE was.)

>
> Further move a BUG_ON() such that in the common case its condition
> wouldn't need evaluating.
>
> Finally, since we're at it, move init_xen_pae_l2_slots() next to the
> renamed function, as they really belong together (in fact
> init_xen_pae_l2_slots() was [indirectly] broken out of this function).
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> We could go further here and delete the function altogether: There are
> no linear mappings in a PGT_pae_xen_l2 table anymore (this was on 32-bit
> only). The corresponding conditional in mod_l3_entry() could then go
> away as well (or, more precisely, would need to be replaced by correct
> handling of 3rd slot updates). This would mean that a 32-bit guest
> functioning on new Xen may fail to work on older (possibly 32-bit) Xen.

Given that PV guests can't do anything differently or they'd stop
working on pre-4.14, I don't think removing this restriction is helpful.

It would be worth explicitly commenting that it is a legacy restriction
however.  (I could have sworn I submitted a patch to do this, but I
can't locate it in my tree.)

>
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1414,22 +1414,23 @@ static int promote_l1_table(struct page_
>      return ret;
>  }
>  
> -static int create_pae_xen_mappings(struct domain *d, l3_pgentry_t *pl3e)
> +static bool pae_xen_mappings_check(const struct domain *d,
> +                                   const l3_pgentry_t *pl3e)
>  {
> -    struct page_info *page;
> -    l3_pgentry_t     l3e3;
> +    const struct page_info *page;
> +    l3_pgentry_t l3e3;
>  
>      if ( !is_pv_32bit_domain(d) )
> -        return 1;
> +        return true;

There is now a single caller.  By repositioning the caller in
promote_l3_table(), this condition can be dropped, which removes one
eval_nospec() from the mix (seeing as these are expensive now.)

>  
> -    pl3e = (l3_pgentry_t *)((unsigned long)pl3e & PAGE_MASK);
> +    ASSERT(!((unsigned long)pl3e & ~PAGE_MASK));
>  
>      /* 3rd L3 slot contains L2 with Xen-private mappings. It *must* exist. */
>      l3e3 = pl3e[3];
>      if ( !(l3e_get_flags(l3e3) & _PAGE_PRESENT) )
>      {
>          gdprintk(XENLOG_WARNING, "PAE L3 3rd slot is empty\n");

Also, by repositioning, it becomes clear that the _PAGE_PRESENT check is
duplicated, and this printk() is now dead code.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel