Re: [Xen-devel] [PATCH v2 09/25] arm/altp2m: Add altp2m table flushing routine.

[Julien Grall <julien.grall@xxxxxxx>]

Hello Sergej,

On 01/08/16 18:10, Sergej Proskurin wrote:
> The current implementation differentiates between flushing and
> destroying altp2m views. This commit adds the function altp2m_flush,
> which allows to flush all or individual altp2m views without destroying
> the entire table. In this way, altp2m views can be reused at a later
> point in time.
>
> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
> ---
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> ---
> v2: Pages in p2m->pages are not cleared in p2m_flush_table anymore.
>     VMID is freed in p2m_free_one.
>     Cosmetic fixes.
> ---
>  xen/arch/arm/altp2m.c        | 38 ++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/altp2m.h |  5 +----
>  xen/include/asm-arm/p2m.h    |  3 +++
>  3 files changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
> index 767f233..e73424c 100644
> --- a/xen/arch/arm/altp2m.c
> +++ b/xen/arch/arm/altp2m.c
> @@ -151,6 +151,44 @@ int altp2m_init(struct domain *d)
>      return 0;
>  }
>
> +void altp2m_flush(struct domain *d)
> +{
> +    unsigned int i;
> +    struct p2m_domain *p2m;
> +
> +    /*
> +     * If altp2m is active, we are not allowed to flush altp2m[0]. This special
> +     * view is considered as the hostp2m as long as altp2m is active.
> +     */
> +    ASSERT(!altp2m_active(d));
Because of the race condition I mentioned in the previous patch (#8), 
this ASSERT may be hit randomly.
> +
> +    altp2m_lock(d);
> +
> +    for ( i = 0; i < MAX_ALTP2M; i++ )
> +    {
> +        if ( d->arch.altp2m_vttbr[i] == INVALID_VTTBR )
> +            continue;
> +
> +        p2m = d->arch.altp2m_p2m[i];
> +
> +        read_lock(&p2m->lock);
You should use p2m_lock rather than re-inventing your own. If p2m_*lock 
helpers need to be exposed, then expose them.
Also, this should be a write_lock otherwise someone else could access 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[i] = INVALID_VTTBR;
> +
> +        read_unlock(&p2m->lock);
I would much prefer if the p2m is fully destroyed rather than 
re-initialized.
> +    }
> +
> +    altp2m_unlock(d);
> +}
> +
>  void altp2m_teardown(struct domain *d)
>  {
>      unsigned int i;
> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
> index a33c740..3ba82a8 100644
> --- a/xen/include/asm-arm/altp2m.h
> +++ b/xen/include/asm-arm/altp2m.h
> @@ -54,9 +54,6 @@ int altp2m_init_by_id(struct domain *d,
>                        unsigned int idx);
>
>  /* Flush all the alternate p2m's for a domain */
> -static inline void altp2m_flush(struct domain *d)
> -{
> -    /* Not yet implemented. */
> -}
> +void altp2m_flush(struct domain *d);
>
>  #endif /* __ASM_ARM_ALTP2M_H */
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index f13f285..32326cb 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -222,6 +222,9 @@ mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn);
>  /* Allocates page table for a p2m. */
>  int p2m_alloc_table(struct p2m_domain *p2m);
>
> +/* Flushes the page table held by the p2m. */
> +void p2m_flush_table(struct p2m_domain *p2m);
> +
>  /* Initialize the p2m structure. */
>  int p2m_init_one(struct domain *d, struct p2m_domain *p2m);
Regards,

--
Julien Grall

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