Re: [Xen-devel] [PATCH v1 Altp2m cleanup 3/3] Making altp2m struct dynamically allocated.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.06.16 at 18:04, <paul.c.lai@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5228,7 +5228,7 @@ static int do_altp2m_op(
>  
>      if ( (a.cmd != HVMOP_altp2m_get_domain_state) &&
>           (a.cmd != HVMOP_altp2m_set_domain_state) &&
> -         !d->arch.altp2m_active )
> +         ! altp2m_active(d) )

Stray blank.

> @@ -5262,11 +5262,11 @@ static int do_altp2m_op(
>              break;
>          }
>  
> -        ostate = d->arch.altp2m_active;
> -        d->arch.altp2m_active = !!a.u.domain_state.state;
> +        ostate = altp2m_active(d);
> +     set_altp2m_active(d, !!a.u.domain_state.state);

Bogus tab indentation.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -193,12 +193,15 @@ static void p2m_teardown_altp2m(struct domain *d)
>  
>      for ( i = 0; i < MAX_ALTP2M; i++ )
>      {
> -        if ( !d->arch.altp2m_p2m[i] )
> +        if ( !d->arch.altp2m->altp2m_p2m[i] )
>              continue;
> -        p2m = d->arch.altp2m_p2m[i];
> +        p2m = d->arch.altp2m->altp2m_p2m[i];
>          p2m_free_one(p2m);
> -        d->arch.altp2m_p2m[i] = NULL;
> +        d->arch.altp2m->altp2m_p2m[i] = NULL;
>      }
> +
> +    if (d->arch.altp2m) 

Missing blanks.

> +        xfree(d->arch.altp2m);

But the conditional is pointless anyway.

> @@ -206,10 +209,12 @@ static int p2m_init_altp2m(struct domain *d)
>      unsigned int i;
>      struct p2m_domain *p2m;
>  
> -    mm_lock_init(&d->arch.altp2m_list_lock);
> +    d->arch.altp2m = xzalloc(struct altp2m_domain);
> +
> +    mm_lock_init(&d->arch.altp2m->altp2m_list_lock);

Missing error check.

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -274,6 +274,13 @@ struct monitor_write_data {
>      uint64_t cr4;
>  };
>  
> +struct altp2m_domain {
> +    bool_t altp2m_active;
> +    struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
> +    mm_lock_t altp2m_list_lock;
> +    uint64_t *altp2m_eptp;
> +};

No point prefixing all the fields with altp2m_. And also the structure
now doesn't belong here anymore - it should move to e.g. p2m.h.

> @@ -320,10 +327,13 @@ struct arch_domain
>      mm_lock_t nested_p2m_lock;
>  
>      /* altp2m: allow multiple copies of host p2m */
> +    /*
>      bool_t altp2m_active;
>      struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
>      mm_lock_t altp2m_list_lock;
> -    uint64_t *altp2m_eptp;
> +    uint64_t *altp2m_eptp; 
> +    */

What's the purpose of this comment?

Jan


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