Re: [Xen-devel] [PATCH Altp2m cleanup v8] Move altp2m specific functions to altp2m files.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.10.16 at 20:13, <paul.c.lai@xxxxxxxxx> wrote:
> v8 of this patch.
> No change since v4 since we've just focused on patch #1 in this series.

Well, not exactly: I did repeatedly mention that comments made
there should be applied to the other parts of the original series.
Anyway...

> @@ -66,6 +67,62 @@ altp2m_vcpu_destroy(struct vcpu *v)
>  }
>  
>  /*
> + * altp2m_domain_init(struct domain *d))

I don't see the point of repeating the name in the comment here. In
any event there's a stray closing parenthesis.

> + *  allocate and initialize memory for altp2m portion of domain
> + *
> + *  returns < 0 on error
> + *  returns 0 on no operation (success)
> + *  returns > 0 on success

I can't spot any case of this.

> + */
> +int
> +altp2m_domain_init(struct domain *d)
> +{
> +    int rc;
> +    unsigned int i;
> +
> +    if ( d == NULL)

Missing blank.

> +        return 0;
> +
> +    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 )
> +           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;
> +
> +    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]);

As said before - I think you want to switch these two steps around,
even if right now their order if benign. This would (a) mirror the
order used during init (read: doing the inverse here) and (b) make
sure code called out of p2m_teardown() could still access the other
data structure if need be.

> @@ -499,26 +500,17 @@ int hap_enable(struct domain *d, u32 mode)
>             goto out;
>      }
>  
> -    if ( hvm_altp2m_supported() )
> +    if ( (rv = altp2m_domain_init(d)) < 0 )
>      {
> -        /* Init alternate p2m data */
> -        if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> -        {
> -            rv = -ENOMEM;
> -            goto out;
> +        for (i = 0; i < MAX_NESTEDP2M; i++) {

Coding style.

> +            p2m_teardown(d->arch.nested_p2m[i]);
>          }
>  
> -        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;
> -        }
> +        if ( d->arch.paging.hap.total_pages != 0 )
> +            hap_teardown(d, NULL);
>  
> -        d->arch.altp2m_active = 0;
> +        p2m_teardown(p2m_get_hostp2m(d));
> +        goto out;
>      }

The portions of code removed in this hunk went into
altp2m_domain_init(). The additions, however, are unexplained in
the commit message and I'm not convinced they actually properly
deal with the previously missing error cleanup. In particular I don't
see how partial success of altp2m_domain_init() is being dealt with.

Also I continue to be of the opinion that most of the changes below
here could actually be in a separate patch.

> @@ -197,8 +197,8 @@ static void p2m_teardown_altp2m(struct domain *d)
>          if ( !d->arch.altp2m_p2m[i] )
>              continue;
>          p2m = d->arch.altp2m_p2m[i];
> -        d->arch.altp2m_p2m[i] = NULL;
>          p2m_free_one(p2m);
> +        d->arch.altp2m_p2m[i] = NULL;

I think I've been puzzled by this change before, and I continue to
be. If this is a necessary change for something, it should be
mentioned in the commit message.

Jan

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