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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.10.16 at 01:40, <paul.c.lai@xxxxxxxxx> wrote:
> @@ -66,6 +67,63 @@ 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 ( d == NULL )
> +        return 0;

I'm sorry for not having paid attention before, but why do you add
this check?

> @@ -493,32 +494,25 @@ int hap_enable(struct domain *d, u32 mode)
>              goto out;
>      }
>  
> -    for (i = 0; i < MAX_NESTEDP2M; i++) {
> +    for (i = 0; i < MAX_NESTEDP2M; i++)
> +    {

If you correct coding style issues, then please deal with all of them
at once. Albeit this code is unrelated to altp2m, so maybe you'd
better leave it alone in this patch.

>          rv = p2m_alloc_table(d->arch.nested_p2m[i]);
>          if ( rv != 0 )
>             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 )
> +        for (i = 0; i < MAX_NESTEDP2M; i++)

As said on v8: Coding style (you've moved the opening brace, but
you didn't insert blanks).

>          {
> -            rv = -ENOMEM;
> -            goto out;
> +            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;
>      }

Repeating my v8 comment: "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." While it looks like the final part of that may
have been taken care of, may I once again remind you that it is a
waste of everyone's time unless _all_ comments given on a prior
version get addressed either verbally or in the next submitted
version? In particular (but please don't again take this as the only
thing needing changing/explaining) the call to hap_teardown()
looks unmotivated. Don't you mean to just undo the earlier
hap_set_allocation()? And then - did you check that cleanup is
(a) necessary here in the first place and (b) is now complete? I
ask because e.g. the nested p2m allocation loop doesn't appear
to be doing any cleanup either. It looks like this is wrong too,
but if you fix it you need to (1) confirm for yourself the change
is needed and (2) reason about the change in the commit message.
And it may well be that this again would better not be done here,
but in a prerequisite (and thus backportable) patch.

Jan


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