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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.10.16 at 22:32, <paul.c.lai@xxxxxxxxx> wrote:
> @@ -499,26 +500,21 @@ 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 )
> +        /* Unravel the partial p2m_alloc_table() above */
> +        for ( i = 0; i < MAX_NESTEDP2M; i++ )
>          {
> -            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;
> -        }
> +        /* Unravel the hap_set_allocation(d, 256, NULL) above */
> +        paging_lock(d);
> +        hap_set_allocation(d, 0, NULL);
> +        ASSERT(d->arch.paging.hap.p2m_pages == 0);
> +        paging_unlock(d);
>  
> -        d->arch.altp2m_active = 0;
> +        goto out;
>      }
>  
>      /* Now let other users see the new mode */

As said before, I don't think this is the right way to approach this.
For one, this error handling fix - if it is really needed, proof of
which is sadly _still_ missing from the patch description - should
be a separate patch (to allow backporting). And then it should be
complete - right now you undo one of the earlier p2m_alloc_table()
instances, but not the other, and you adjust only the altp2m error
paths, but neither the PG_translate nor the nested ones. And if
you deal with the whole issue at once (again - provided this needs
fixing in the first place, i.e. the cleanup doesn't occur by other,
implicit means), it will likely become clear that central error handling
produces easier to read/maintain code than repeated instances of
partial cleanup. The question really is whether one of the
existing teardown functions can't fulfill this purpose (or can't be
easily made do so).

> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -561,6 +561,9 @@ void ept_p2m_uninit(struct p2m_domain *p2m);
>  void ept_walk_table(struct domain *d, unsigned long gfn);
>  bool_t ept_handle_misconfig(uint64_t gpa);
>  void setup_ept_dump(void);
> +void p2m_init_altp2m_ept(struct domain *d, unsigned int i);
> +/* Locate an alternate p2m by its EPTP */
> +unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp);

The movement of p2m_find_altp2m_by_eptp()'s declaration
appears to belong into patch 2. And the addition of
p2m_init_altp2m_ept()'s one very certainly has to go there.

Jan

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