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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.12.2019 12:48, Hongyan Xia wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5151,6 +5151,51 @@ 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 int shatter_l3e(l3_pgentry_t *pl3e, unsigned long virt, bool locking)
> +{
> +    unsigned int i;
> +    l3_pgentry_t ol3e;
> +    l2_pgentry_t ol2e, *l2t = alloc_xen_pagetable();
> +
> +    if ( l2t == NULL )

Nowadays we seem to prefer !l2t in cases like this one.

> +        return -1;

-ENOMEM please (and then handed on by the caller).

> +    ol3e = *pl3e;

This could be the variable's initializer.

> +    ol2e = l2e_from_intpte(l3e_get_intpte(ol3e));

There's nothing "old" about this L2 entry, so its name would better
be just "l2e" I think.

> +    for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
> +    {
> +        l2e_write(l2t + i, ol2e);
> +        ol2e = l2e_from_intpte(
> +                l2e_get_intpte(ol2e) + (1 << (PAGETABLE_ORDER + 
> PAGE_SHIFT)));

Indentation looks odd here (also further down). If the first argument
of a function call doesn't fit on the line and would also be ugly to
split across lines, what we do is indent it the usual 4 characters
from the function invocation, i.e. in this case

        ol2e = l2e_from_intpte(
                   l2e_get_intpte(ol2e) + (1 << (PAGETABLE_ORDER + 
PAGE_SHIFT)));

and then slightly shorter

        ol2e = l2e_from_intpte(
                   l2e_get_intpte(ol2e) + (PAGE_SIZE << PAGETABLE_ORDER));

Of course, as mentioned before, I'm not overly happy to see type
safety lost in case like this one, where it's not needed like e.g.
further up to convert from L3 to L2 entry.

> +    }
> +    if ( locking )
> +        spin_lock(&map_pgdir_lock);
> +    if ( (l3e_get_flags(*pl3e) & _PAGE_PRESENT) &&
> +         (l3e_get_flags(*pl3e) & _PAGE_PSE) )
> +    {
> +        l3e_write_atomic(pl3e,
> +                l3e_from_paddr((paddr_t)virt_to_maddr(l2t), 
> __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) )

Unnecessary pair of parentheses (which also wasn't there in the
original code).

> +                flush_flags |= FLUSH_TLB_GLOBAL;

Too deep indentation.

> +        flush_area(virt, flush_flags);
> +    }
> +    if ( l2t )
> +        free_xen_pagetable(l2t);
> +
> +    return 0;
> +}

Also please add blank lines between
- L2 population and lock acquire,
- lock release and TLB flush,
- TLB flush and free.

Jan

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