Re: [Xen-devel] [PATCH 1/2] x86/mm: factor out the code for shattering an l3 PTE

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/12/2019 15:53, Hongyan Xia wrote:
> map_pages_to_xen and modify_xen_mappings are performing almost exactly
> the same operations when shattering an l3 PTE, the only difference
> being whether we want to flush.
>
> Signed-off-by: Hongyan Xia <hongyxia@xxxxxxxxxx>

Just for reviewing purposes, how does this relate to your other posted
series.  Is it independent, a prerequisite, or does it depend on that
series?

> ---
>  xen/arch/x86/mm.c | 85 ++++++++++++++++++++++-------------------------
>  1 file changed, 40 insertions(+), 45 deletions(-)
>
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 7d4dd80a85..42aaaa1083 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5151,6 +5151,43 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v)
>                           flush_area_local((const void *)v, f) : \
>                           flush_area_all((const void *)v, f))
>  
> +/* Shatter an l3 entry and populate l2. If virt is passed in, also do flush. 
> */
> +static void shatter_l3e(l3_pgentry_t *pl3e, l2_pgentry_t *l2t,
> +        unsigned long virt, bool locking)
> +{
> +    unsigned int i;
> +    l3_pgentry_t ol3e = *pl3e;
> +
> +    for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
> +        l2e_write(l2t + i,
> +                  l2e_from_pfn(l3e_get_pfn(ol3e) +
> +                               (i << PAGETABLE_ORDER),
> +                               l3e_get_flags(ol3e)));

The PTE macros are especially poor for generated asm, and in cases like
this, I'd like to improve things.

In particular, I believe the following code has identical behaviour:

l2_pgentry_t nl2e = l2e_from_intpte(l3e_get_intpte(ol3e));

for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++, nl2e.l2 += PAGETABLE_ORDER )
    l2e_write(l2t + i, nl2e);

(although someone please double check my logic) and rather better asm
generation.  (I also expect there to be some discussion on whether using
n2le.l2 directly is something we'd want to start doing.)

> +
> +    if ( locking )
> +        spin_lock(&map_pgdir_lock);
> +    if ( (l3e_get_flags(ol3e) & _PAGE_PRESENT) &&
> +         (l3e_get_flags(ol3e) & _PAGE_PSE) )

There is a subtle difference between the original code, and the
refactored code, and it depends on the memory barrier from spin_lock().

Previously, it was re-read from memory after the lock, whereas now it is
likely the stale version from before map_pgdir was locked.

Either you can go back to the old version and use *pl3e, or
alternatively use:

    if ( locking )
        spin_lock(&map_pgdir_lock);
    ol3e = ACCESS_ONCE(*pl3e);
    if ( ...

to make it clear that a reread from memory is required.

> +    {
> +        l3e_write_atomic(pl3e, l3e_from_mfn(virt_to_mfn(l2t),

This would probably generate better asm by using the maddr variants so
we don't have a double shift.

> +                                            __PAGE_HYPERVISOR));
> +        l2t = NULL;
> +    }
> +    if ( locking )
> +        spin_unlock(&map_pgdir_lock);
> +    if ( virt )
> +    {
> +        unsigned int flush_flags =
> +            FLUSH_TLB | FLUSH_ORDER(2 * PAGETABLE_ORDER);
> +
> +        if ( (l3e_get_flags(ol3e) & _PAGE_GLOBAL) )
> +                flush_flags |= FLUSH_TLB_GLOBAL;
> +        flush_area(virt, flush_flags);
> +    }
> +    if ( l2t )
> +        free_xen_pagetable(l2t);

This surely needs to NULL out l2t, just so the caller doesn't get any
clever ideas and ends up with a use-after-free?

~Andrew

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