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

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

[PAUL] in line

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@xxxxxxxx] 
Sent: Friday, September 2, 2016 6:47 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 3/3] Making altp2m struct dynamically 
allocated.

>>> On 19.08.16 at 19:22, <paul.c.lai@xxxxxxxxx> wrote:
> Ravi Sahita's dynamically allocated altp2m structs

I think I've asked before: With this and ...

> Signed-off-by: Paul Lai <paul.c.lai@xxxxxxxxx>
> Reviewed-by: Ravi Sahita <ravi.sahita@xxxxxxxxx>

... this - who's the actual author?

[PAUL] Ravi is the actual author.  I thought the commit message would have been 
clear :(

> @@ -5279,11 +5279,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);

The !! shouldn't be needed anymore.

> --- a/xen/arch/x86/mm/altp2m.c
> +++ b/xen/arch/x86/mm/altp2m.c
> @@ -73,23 +73,23 @@ hvm_altp2m_init( struct domain *d)
>      unsigned int i = 0;
>  
>      /* Init alternate p2m data. */
> -    if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +    if ( (d->arch.altp2m->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);
> +        d->arch.altp2m->altp2m_eptp[i] = mfn_x(INVALID_MFN);
>  
>      for ( i = 0; i < MAX_ALTP2M; i++ )
>      {
> -        rc = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> +        rc = p2m_alloc_table(d->arch.altp2m->altp2m_p2m[i]);
>          if ( rc != 0 )
>             goto out;
>      }
>  
> -    d->arch.altp2m_active = 0;
> +    set_altp2m_active(d, 0);

"false" please (also elsewhere).

> --- 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 )
> +        xfree(d->arch.altp2m);

Two problems here: First, xfree() happily deals with NULL being passed. But 
then, such a NULL check clearly should not come _after_ the pointer did already 
get dereferenced. I.e. first of all you need to clarify for yourself whether 
the function can be called with the pointer being NULL.

[PAUL] great catch

> @@ -206,10 +209,14 @@ static int p2m_init_altp2m(struct domain *d)

And then, considering this is the last patch in the series - how come these two 
functions are still in this source file?

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

None of the altp2m_ prefixes here are really useful for anything afaics.

Jan


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