Re: [Xen-devel] [PATCH Altp2m cleanup 2/3 v12 2/3] Altp2m cleanup: cleaning up partial memory allocations in hap_enable().

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.11.16 at 00:45, <paul.c.lai@xxxxxxxxx> wrote:
> @@ -488,43 +489,44 @@ int hap_enable(struct domain *d, u32 mode)
>      /* allocate P2m table */
>      if ( mode & PG_translate )
>      {
> +        /* p2m_alloc_table() #1 */
>          rv = p2m_alloc_table(p2m_get_hostp2m(d));
>          if ( rv != 0 )
>              goto out;
>      }
>  
>      for (i = 0; i < MAX_NESTEDP2M; i++) {
> +        /* nested p2m_alloc_table() #2 */

I don't consider these comments particularly useful.

>          rv = p2m_alloc_table(d->arch.nested_p2m[i]);
>          if ( rv != 0 )
>             goto out;
>      }
>  
> -    if ( hvm_altp2m_supported() )
> -    {
> -        /* Init alternate p2m data */
> -        if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> -        {
> -            rv = -ENOMEM;
> -            goto out;
> -        }
> +    if ( (rv = altp2m_domain_init(d)) < 0 )
> +        goto out;

Hmm, so you at once switch to the new function here. I think I've
been quite clear on previous rounds that the bug fix should be
first in the series, to aid potential backporting. That said, the
description still does not make clear that there is any issue here
in the first place. I can only re-iterate: Whether there's a leak to
be fixed here depends on how this function and any error
cleanup up the call stack leading here interact. And in order to
determine whether the patch here is a bug fix or mere cleanup
(and maybe unnecessary, due to only making the code bigger
and hence [slightly] harder to read), this is imperative to know.
I think you understand that the decision whether to backport
such a change also depends on knowing whether it fixes an
actual bug.

> -        for ( i = 0; i < MAX_EPTP; i++ )
> -            d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> +    /* Now let other users see the new mode */
> +    d->arch.paging.mode = mode | PG_HAP_enable;
>  
> -        for ( i = 0; i < MAX_ALTP2M; i++ )
> -        {
> -            rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> -            if ( rv != 0 )
> -               goto out;
> -        }
> +    domain_unpause(d);
> +    return rv;
>  
> -        d->arch.altp2m_active = 0;
> + out:
> +    /* Unravel the partial nested p2m_alloc_table() #2 above */
> +    for ( i = 0; i < MAX_NESTEDP2M; i++ )
> +    {
> +        p2m_teardown(d->arch.nested_p2m[i]);
>      }

Unnecessary braces.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel