Re: [Xen-devel] [PATCH v3 18/38] arm/p2m: Add HVMOP_altp2m_destroy_p2m

[Julien Grall <julien.grall@xxxxxxx>]

Hello Sergej,

On 16/08/2016 23:16, Sergej Proskurin wrote:
> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
> ---
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> ---
> v2: Substituted the call to tlb_flush for p2m_flush_table.
>     Added comments.
>     Cosmetic fixes.
>
> v3: Changed the locking mechanism to "p2m_write_lock" inside the
>     function "altp2m_destroy_by_id".
>
>     Do not flush but rather teardown the altp2m in the function
>     "altp2m_destroy_by_id".
>
>     Exchanged the check "altp2m_vttbr[idx] == INVALID_VTTBR" for
>     "altp2m_p2m[idx] == NULL" in "altp2m_destroy_by_id".
> ---
>  xen/arch/arm/altp2m.c        | 43 +++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/hvm.c           |  2 +-
>  xen/include/asm-arm/altp2m.h |  4 ++++
>  3 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
> index b5d1951..c14ab0b 100644
> --- a/xen/arch/arm/altp2m.c
> +++ b/xen/arch/arm/altp2m.c
> @@ -190,6 +190,49 @@ void altp2m_flush(struct domain *d)
>      altp2m_unlock(d);
>  }
>
> +int altp2m_destroy_by_id(struct domain *d, unsigned int idx)
> +{
> +    struct p2m_domain *p2m;
> +    int rc = -EBUSY;
> +
> +    /*
> +     * The altp2m[0] is considered as the hostp2m and is used as a safe harbor
> +     * to which you can switch as long as altp2m is active. After deactivating
> +     * altp2m, the system switches back to the original hostp2m view. That is,
> +     * altp2m[0] should only be destroyed/flushed/freed, when altp2m is
> +     * deactivated.
> +     */
> +    if ( !idx || idx >= MAX_ALTP2M )
> +        return rc;
> +
> +    domain_pause_except_self(d);
> +
> +    altp2m_lock(d);
> +
> +    if ( d->arch.altp2m_p2m[idx] != NULL )
> +    {
> +        p2m = d->arch.altp2m_p2m[idx];
> +
> +        if ( !_atomic_read(p2m->active_vcpus) )
> +        {
> +            p2m_write_lock(p2m);
I am not sure why you take the lock here. It will not protect you from 
anything as the p2m will get freed just after. So if someone else is 
using it, it will be in big trouble.
> +            p2m_teardown_one(p2m);
> +            p2m_write_unlock(p2m);
> +
> +            xfree(p2m);
> +            d->arch.altp2m_p2m[idx] = NULL;
> +
> +            rc = 0;
> +        }
> +    }
> +
> +    altp2m_unlock(d);
> +
> +    domain_unpause_except_self(d);
> +
> +    return rc;
> +}
> +
>  void altp2m_teardown(struct domain *d)
>  {
>      unsigned int i;
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index a504dfd..df973ef 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -128,7 +128,7 @@ static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>
>      case HVMOP_altp2m_destroy_p2m:
> -        rc = -EOPNOTSUPP;
> +        rc = altp2m_destroy_by_id(d, a.u.view.view);
>          break;
>
>      case HVMOP_altp2m_switch_p2m:
> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
> index 5701012..6074079 100644
> --- a/xen/include/asm-arm/altp2m.h
> +++ b/xen/include/asm-arm/altp2m.h
> @@ -63,4 +63,8 @@ int altp2m_init_next_available(struct domain *d,
>  /* Flush all the alternate p2m's for a domain. */
>  void altp2m_flush(struct domain *d);
>
> +/* Make a specific alternate p2m invalid */
> +int altp2m_destroy_by_id(struct domain *d,
> +                         unsigned int idx);
> +
>  #endif /* __ASM_ARM_ALTP2M_H */
Regards,

--
Julien Grall

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