Re: [Xen-devel] [PATCH v2 11/25] arm/altp2m: Add HVMOP_altp2m_destroy_p2m.

[Julien Grall <julien.grall@xxxxxxx>]

Hello Sergej,

On 01/08/16 18:10, 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.
> ---
>  xen/arch/arm/altp2m.c        | 50 ++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/hvm.c           |  2 +-
>  xen/include/asm-arm/altp2m.h |  4 ++++
>  3 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
> index c22d2e4..80ed553 100644
> --- a/xen/arch/arm/altp2m.c
> +++ b/xen/arch/arm/altp2m.c
> @@ -212,6 +212,56 @@ 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_vttbr[idx] != INVALID_VTTBR )
> +    {
> +        p2m = d->arch.altp2m_p2m[idx];
> +
> +        if ( !_atomic_read(p2m->active_vcpus) )
> +        {
> +            read_lock(&p2m->lock);
Please avoid to use open read_lock and use the p2m_*lock helpers. Also 
read lock does not prevent multiple thread to access the p2m at the same 
time.
> +
> +            p2m_flush_table(p2m);
> +
> +            /*
> +             * Reset VTTBR.
> +             *
> +             * Note that VMID is not freed so that it can be reused later.
> +             */
> +            p2m->vttbr.vttbr = INVALID_VTTBR;
> +            d->arch.altp2m_vttbr[idx] = INVALID_VTTBR;
> +
> +            read_unlock(&p2m->lock);
Why did you decide to reset the p2m rather than free it? The code would 
be simpler with the latter.
> +
> +            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 063a06b..df29cdc 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -125,7 +125,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 3ecae27..afa1580 100644
> --- a/xen/include/asm-arm/altp2m.h
> +++ b/xen/include/asm-arm/altp2m.h
> @@ -60,4 +60,8 @@ int altp2m_init_next(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