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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/11/16 23:45, Paul Lai wrote:
> Moving altp2m domain startup and teardown into altp2m_domain_init()
> and altp2m_domain_teardown() respectively.

You're not "moving" the startup into a function unless the new function
appears *and* the old code disappears.

I think it would be better to have the function introduced in the next
patch, so that it's easier to verify that the removed code and the added
code are doing the same thing.

Everything else looks good to me, thanks.

 -George

> Moving hvm_altp2m_supported() check into functions that use it
> for better readability.
> Got rid of stray blanks after open paren after function names.
> Defining _XEN_ASM_X86_P2M_H instead of _XEN_P2M_H for
> xen/include/asm-x86/p2m.h.
> 
> Signed-off-by: Paul Lai <paul.c.lai@xxxxxxxxx>
> ---
>  xen/arch/x86/mm/altp2m.c     | 55 
> ++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/mm/hap/hap.c    | 14 +----------
>  xen/include/asm-x86/altp2m.h |  4 +++-
>  3 files changed, 59 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/altp2m.c b/xen/arch/x86/mm/altp2m.c
> index 930bdc2..317c8f7 100644
> --- a/xen/arch/x86/mm/altp2m.c
> +++ b/xen/arch/x86/mm/altp2m.c
> @@ -17,6 +17,7 @@
>  
>  #include <asm/hvm/support.h>
>  #include <asm/hvm/hvm.h>
> +#include <asm/domain.h>
>  #include <asm/p2m.h>
>  #include <asm/altp2m.h>
>  
> @@ -66,6 +67,60 @@ altp2m_vcpu_destroy(struct vcpu *v)
>  }
>  
>  /*
> + *  allocate and initialize memory for altp2m portion of domain
> + *
> + *  returns < 0 on error
> + *  returns 0 on no operation & success
> + */
> +int
> +altp2m_domain_init(struct domain *d)
> +{
> +    int rc;
> +    unsigned int i;
> +
> +    if ( !hvm_altp2m_supported() )
> +        return 0;
> +
> +    /* Init alternate p2m data. */
> +    if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +        return -ENOMEM;
> +
> +    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 )
> +        {
> +            altp2m_domain_teardown(d);
> +            return rc;
> +        }
> +    }
> +
> +    d->arch.altp2m_active = 0;
> +
> +    return rc;
> +}
> +
> +void
> +altp2m_domain_teardown(struct domain *d)
> +{
> +    unsigned int i;
> +
> +    if ( !hvm_altp2m_supported() )
> +        return;
> +
> +    d->arch.altp2m_active = 0;
> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +        p2m_teardown(d->arch.altp2m_p2m[i]);
> +
> +    free_xenheap_page(d->arch.altp2m_eptp);
> +    d->arch.altp2m_eptp = NULL;
> +}
> +
> +/*
>   * Local variables:
>   * mode: C
>   * c-file-style: "BSD"
> diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> index 3218fa2..7fe6f83 100644
> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -533,19 +533,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]);
> -    }
> +    altp2m_domain_teardown(d);
>  
>      /* Destroy nestedp2m's first */
>      for (i = 0; i < MAX_NESTEDP2M; i++) {
> diff --git a/xen/include/asm-x86/altp2m.h b/xen/include/asm-x86/altp2m.h
> index 64c7618..0090c89 100644
> --- a/xen/include/asm-x86/altp2m.h
> +++ b/xen/include/asm-x86/altp2m.h
> @@ -18,7 +18,6 @@
>  #ifndef __ASM_X86_ALTP2M_H
>  #define __ASM_X86_ALTP2M_H
>  
> -#include <xen/types.h>
>  #include <xen/sched.h>         /* for struct vcpu, struct domain */
>  #include <asm/hvm/vcpu.h>      /* for vcpu_altp2m */
>  
> @@ -38,4 +37,7 @@ static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
>      return vcpu_altp2m(v).p2midx;
>  }
>  
> +int altp2m_domain_init(struct domain *d);
> +void altp2m_domain_teardown(struct domain *d);
> +
>  #endif /* __ASM_X86_ALTP2M_H */
> 


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