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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

> @@ -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