Re: [Xen-devel] [PATCH Altp2m cleanup v4 3/4] Move altp2m specific functions to altp2m files.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.09.16 at 00:04, <paul.c.lai@xxxxxxxxx> wrote:
> @@ -65,6 +66,56 @@ altp2m_vcpu_destroy(struct vcpu *v)
>          vcpu_unpause(v);
>  }
>  
> +int
> +altp2m_domain_init(struct domain *d)
> +{
> +    int rc;
> +    unsigned int i;
> +
> +    if ( !hvm_altp2m_supported() )
> +    {
> +        rc = 0;
> +        goto out;
> +    }
> +    /* Init alternate p2m data. */
> +    if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +    {
> +        rc = -ENOMEM;
> +        goto out;
> +    }
> +
> +    for ( i = 0; i < MAX_EPTP; i++ )
> +        d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    {
> +        rc = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> +        if ( rc != 0 )
> +           goto out;
> +    }
> +
> +    d->arch.altp2m_active = 0;
> + out:
> +    return rc;
> +}

I don't follow: I did specifically ask to avoid goto when where the
label is there's just a single statement (return in this case).

> +void
> +altp2m_domain_teardown(struct domain *d)
> +{
> +    unsigned int i;
> +
> +    if ( !hvm_altp2m_supported() )
> +     return;

Bad tab indentation.

> @@ -499,27 +500,7 @@ int hap_enable(struct domain *d, u32 mode)
>             goto out;
>      }
>  
> -    if ( hvm_altp2m_supported() )
> -    {
> -        /* Init alternate p2m data */
> -        if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> -        {
> -            rv = -ENOMEM;
> -            goto out;
> -        }
> -
> -        for ( i = 0; i < MAX_EPTP; i++ )
> -            d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> -
> -        for ( i = 0; i < MAX_ALTP2M; i++ )
> -        {
> -            rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> -            if ( rv != 0 )
> -               goto out;
> -        }
> -
> -        d->arch.altp2m_active = 0;
> -    }
> +    rv = altp2m_domain_init(d);
>  
>      /* Now let other users see the new mode */
>      d->arch.paging.mode = mode | PG_HAP_enable;

I don't think you should reach the following statement(s) when your
function returned an error. Even if this might be benign now, this is
easy to overlook if later more code gets added near the end of the
function.

Also I wonder whether in an error case there's not a possibility for
memory to be leaked. That wouldn't be a problem your patch
introduces, but I think you could have noticed and fixed this as
you touch the code anyway.

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1329,6 +1329,45 @@ void setup_ept_dump(void)
>      register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0);
>  }
>  
> +void p2m_init_altp2m_ept( struct domain *d, unsigned int i)

Another instance of you not having corrected what was previously
pointed out: There's a stray blank here. And even if I hadn't said
there are multiple instances of this, you should still apply such
comments to all of your series. And I only now notice that this e.g.
also applies to the suggested re-ordering of operations in
altp2m_domain_teardown(). I think I'm going to stop reviewing
this series here: Please make sure you address all review comments
(either verbally or by code adjustment) before submitting a new
version (or in extreme cases, like due to lack of feedback, point out
open issues right after the first --- separator).

Jan


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