Re: [PATCH v2 19/25] arm/altp2m: Add altp2m_propagate_change.

[Julien Grall <julien.grall@arm.com>]

Hello Sergej,

On 01/08/16 18:10, Sergej Proskurin wrote:
> This commit introduces the function "altp2m_propagate_change" that is
> responsible to propagate changes applied to the host's p2m to a specific
> or even all altp2m views. In this way, Xen can in-/decrease the guest's
> physmem at run-time without leaving the altp2m views with
> stalled/invalid entries.
>
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/altp2m.c        | 75 ++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/p2m.c           | 14 +++++++++
>  xen/include/asm-arm/altp2m.h |  9 ++++++
>  xen/include/asm-arm/p2m.h    |  5 +++
>  4 files changed, 103 insertions(+)
>
> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
> index f98fd73..f3c1cff 100644
> --- a/xen/arch/arm/altp2m.c
> +++ b/xen/arch/arm/altp2m.c
> @@ -133,6 +133,81 @@ out:
>      return rc;
>  }
>
> +static inline void altp2m_reset(struct p2m_domain *p2m)
> +{
> +    read_lock(&p2m->lock);

Again read_lock does not protect you against concurrent access. Only 
against someone else update the page table.

This should be p2m_write_lock.

> +
> +    p2m_flush_table(p2m);
> +    p2m_flush_tlb(p2m);

altp2m_reset may be called on a p2m used by a running vCPU. What this 
code does is:
     1) clearing root page table
     2) free intermediate page table
     3) invalidate the TLB

Until step 3, the other TLBs may contain entries pointing the 
intermediate page table. But they were freed and could therefore be 
re-used for another purpose. So step 2 and 3 should be inverted.

I will re-iterate same message as in the previous series. Please have a 
think about the locking and memory ordering of all this series. I found 
a lot of race condition and I may have miss someone. If you have a doubt 
don't hesitate to ask.

> +
> +    p2m->lowest_mapped_gfn = INVALID_GFN;
> +    p2m->max_mapped_gfn = _gfn(0);
> +
> +    read_unlock(&p2m->lock);
> +}
> +
> +void altp2m_propagate_change(struct domain *d,
> +                             gfn_t sgfn,
> +                             unsigned long nr,
> +                             mfn_t smfn,
> +                             uint32_t mask,
> +                             p2m_type_t p2mt,
> +                             p2m_access_t p2ma)
> +{
> +    struct p2m_domain *p2m;
> +    mfn_t m;
> +    unsigned int i;
> +    unsigned int reset_count = 0;
> +    unsigned int last_reset_idx = ~0;
> +
> +    if ( !altp2m_active(d) )

This is not safe. d->arch.altp2m_active maybe be turn off just after you 
read. Maybe you want to protect it with altp2m_lock.

> +        return;
> +
> +    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];
> +
> +        m = p2m_lookup_attr(p2m, sgfn, NULL, NULL, NULL, NULL);

What is the benefits to check if it already exists in the altp2m?

> +
> +        /* Check for a dropped page that may impact this altp2m. */
> +        if ( (mfn_eq(smfn, INVALID_MFN) || p2mt == p2m_invalid) &&

Why the check to p2mt against p2m_invalid?

> +             gfn_x(sgfn) >= gfn_x(p2m->lowest_mapped_gfn) &&
> +             gfn_x(sgfn) <= gfn_x(p2m->max_mapped_gfn) )
> +        {
> +            if ( !reset_count++ )
> +            {
> +                altp2m_reset(p2m);
> +                last_reset_idx = i;
> +            }
> +            else
> +            {
> +                /* At least 2 altp2m's impacted, so reset everything. */

So if you remove a 4KB page in more than 2 altp2m, you will flush all 
the p2m. This sounds really more time consuming (you have to free all 
the intermediate page table) than removing a single 4KB page.

> +                for ( i = 0; i < MAX_ALTP2M; i++ )
> +                {
> +                    if ( i == last_reset_idx ||
> +                         d->arch.altp2m_vttbr[i] == INVALID_VTTBR )
> +                        continue;
> +
> +                    p2m = d->arch.altp2m_p2m[i];
> +                    altp2m_reset(p2m);
> +                }
> +                goto out;
> +            }
> +        }
> +        else if ( !mfn_eq(m, INVALID_MFN) )
> +            modify_altp2m_range(d, p2m, sgfn, nr, smfn,
> +                                mask, p2mt, p2ma);

I am a bit concerned about this function. We decided to limit the size 
of the mapping to avoid long running memory operations (see XSA-158).

With this function you multiply up to 10 times the duration of the 
operation.

Also, what is modify_altp2m_range has failed?


> +    }
> +
> +out:
> +    altp2m_unlock(d);
> +}
> +
>  static void altp2m_vcpu_reset(struct vcpu *v)
>  {
>      struct altp2mvcpu *av = &vcpu_altp2m(v);
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index e0a7f38..31810e6 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -992,6 +992,7 @@ static int apply_p2m_changes(struct domain *d,
>      const bool_t preempt = !is_idle_vcpu(current);
>      bool_t flush = false;
>      bool_t flush_pt;
> +    bool_t entry_written = false;
>      PAGE_LIST_HEAD(free_pages);
>      struct page_info *pg;
>
> @@ -1112,6 +1113,7 @@ static int apply_p2m_changes(struct domain *d,
>                                    &addr, &maddr, &flush,
>                                    t, a);
>              if ( ret < 0 ) { rc = ret ; goto out; }
> +            if ( ret ) entry_written = 1;

Please don't mix false and 1. This should be true here.

>              count += ret;
>
>              if ( ret != P2M_ONE_PROGRESS_NOP )
> @@ -1208,6 +1210,9 @@ out:
>
>      p2m_write_unlock(p2m);
>
> +    if ( rc >= 0 && entry_written && p2m_is_hostp2m(p2m) )
> +        altp2m_propagate_change(d, sgfn, nr, smfn, mask, t, a);

There are operation which does not require propagation (for instance 
RELINQUISH and CACHEFLUSH).

> +
>      if ( rc < 0 && ( op == INSERT ) &&
>           addr != start_gpaddr )
>      {
> @@ -1331,6 +1336,15 @@ int modify_altp2m_entry(struct domain *d, struct p2m_domain *ap2m,
>      return apply_p2m_changes(d, ap2m, INSERT, gfn, nr, mfn, 0, t, a);
>  }
>
> +int modify_altp2m_range(struct domain *d, struct p2m_domain *ap2m,
> +                        gfn_t sgfn, unsigned long nr, mfn_t smfn,
> +                        uint32_t m, p2m_type_t t, p2m_access_t a)
> +{
> +    ASSERT(p2m_is_altp2m(ap2m));
> +
> +    return apply_p2m_changes(d, ap2m, INSERT, sgfn, nr, smfn, m, t, a);
> +}
> +
>  int p2m_alloc_table(struct p2m_domain *p2m)
>  {
>      unsigned int i;
> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
> index dc41f93..9aeb7d6 100644
> --- a/xen/include/asm-arm/altp2m.h
> +++ b/xen/include/asm-arm/altp2m.h
> @@ -81,4 +81,13 @@ int altp2m_set_mem_access(struct domain *d,
>                            p2m_access_t a,
>                            gfn_t gfn);
>
> +/* Propagates changes made to hostp2m to affected altp2m views. */
> +void altp2m_propagate_change(struct domain *d,
> +                             gfn_t sgfn,
> +                             unsigned long nr,
> +                             mfn_t smfn,
> +                             uint32_t mask,
> +                             p2m_type_t p2mt,
> +                             p2m_access_t p2ma);
> +
>  #endif /* __ASM_ARM_ALTP2M_H */
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 9859ad1..59186c9 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -191,6 +191,11 @@ int modify_altp2m_entry(struct domain *d, struct p2m_domain *p2m,
>                          paddr_t gpa, paddr_t maddr, unsigned int level,
>                          p2m_type_t t, p2m_access_t a);
>
> +/* Modify an altp2m view's range of entries or their attributes. */
> +int modify_altp2m_range(struct domain *d, struct p2m_domain *p2m,
> +                        gfn_t sgfn, unsigned long nr, mfn_t smfn,
> +                        uint32_t mask, p2m_type_t t, p2m_access_t a);
> +
>  /* Clean & invalidate caches corresponding to a region of guest address space */
>  int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr);
>
>

-- 
Julien Grall

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