Re: [Xen-devel] [PATCH v2 Altp2m cleanup v3 2/3] Move altp2m specific functions to altp2m files.

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

[PAUL] in-line

Ravi -- please comment on swap comment below.

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@xxxxxxxx] 
Sent: Friday, September 2, 2016 6:31 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 v2 Altp2m cleanup v3 2/3] Move altp2m specific functions to 
altp2m files.

>>> On 19.08.16 at 19:22, <paul.c.lai@xxxxxxxxx> wrote:
> @@ -65,6 +66,50 @@ altp2m_vcpu_destroy(struct vcpu *v)
>          vcpu_unpause(v);
>  }
>  
> +int
> +hvm_altp2m_init( struct domain *d)

Stray blank (more elsewhere). Also I don't think hvm_ is a proper prefix for a 
function placed in this file, i.e. if altp2m_init() is used elsewhere already, 
then altp2m_hvm_init() or perhaps better
altp2m_domain_init() please.

> +{
> +    int rc = 0;
> +    unsigned int i = 0;

Pointless initializers.

> +    /* Init alternate p2m data. */
> +    if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +    {
> +        rc = -ENOMEM;
> +        goto out;
> +    }

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

> +void
> +hvm_altp2m_teardown( struct domain *d) {
> +    unsigned int i = 0;
> +    d->arch.altp2m_active = 0;
> +
> +    if ( d->arch.altp2m_eptp )
> +    {
> +        free_xenheap_page(d->arch.altp2m_eptp);
> +        d->arch.altp2m_eptp = NULL;
> +    }

Please avoid the if() - free_xenheap_page() happily deals with a NULL pointer 
passed to it.

> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +        p2m_teardown(d->arch.altp2m_p2m[i]);

I realize it's been that way in the original code, but wouldn't swapping the 
two things be both more natural and more robust?

[PAUL] Ravi ?

> @@ -501,24 +502,9 @@ int hap_enable(struct domain *d, u32 mode)
>  
>      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 = hvm_altp2m_init(d);
> +        if ( rv != 0 )
> +           goto out;
>      }
>  
>      /* Now let other users see the new mode */ @@ -534,18 +520,7 @@ 
> void hap_final_teardown(struct domain *d)
>      unsigned int i;
>  
>      if ( hvm_altp2m_supported() )
> -    {
> -        d->arch.altp2m_active = 0;
> -
> -        if ( d->arch.altp2m_eptp )
> -        {
> -            free_xenheap_page(d->arch.altp2m_eptp);
> -            d->arch.altp2m_eptp = NULL;
> -        }
> -
> -        for ( i = 0; i < MAX_ALTP2M; i++ )
> -            p2m_teardown(d->arch.altp2m_p2m[i]);
> -    }
> +        hvm_altp2m_teardown(d);

I wonder whether in both cases the hvm_altp2m_supported() would better also be 
moved into the functions.

It looks like the parts above and below this point, except for header file 
adjustments, are completely independent. Please consider splitting into two 
patches.

> --- 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_helper( struct domain *d, unsigned int i)

Please drop the _helper suffix now that there is _ept.

Jan


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