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

["Lai, Paul C" <paul.c.lai@xxxxxxxxx>]

[Paul2] in-line

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@xxxxxxxx] 
Sent: Thursday, September 8, 2016 8:02 AM
To: Lai, Paul C <paul.c.lai@xxxxxxxxx>
Cc: george.dunlap@xxxxxxxxxx; Sahita, Ravi <ravi.sahita@xxxxxxxxx>; xen-devel 
<xen-devel@xxxxxxxxxxxxxxxxxxxx>
Subject: Re: [PATCH Altp2m cleanup v4 3/4] Move altp2m specific functions to 
altp2m files.

>>> 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).

[Paul2]  In the v3 2/3 patch series you said:
   
  [Jan] When the epilogue (after the target label) is just a return statement, 
please avoid using goto.

    [PAUL] I don't see this code in an epilogue (after the target label).  

[Paul2] I didn't get an answer to the question.  After scratching my head, just 
realized you want the 'goto' to become a 'return'.  I will make this change in 
order to pass your review (and the coding style of Xen).  That said, I'm a 
believer in one exit per function -- to me, this is a much cleaner coding 
style.  I'm not trying to start a debate, I'm just explaining how I was 
confused.

> +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.

[Paul2] Good catch

> --- 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).

[Paul2] I did do a sweep for the stray blanks (and other like typos/style 
issues), but your eyes are much better trained.  Will do again.  

[Paul 2] Thanks for the quick feedback.

Jan


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