Re: [PATCH v3 33/38] arm/p2m: Add altp2m paging mechanism

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

Hello Sergej,

On 16/08/16 23:17, Sergej Proskurin wrote:
> This commit adds the function "altp2m_lazy_copy" implementing the altp2m
> paging mechanism. The function "altp2m_lazy_copy" lazily copies the
> hostp2m's mapping into the currently active altp2m view on 2nd stage
> translation faults on instruction or data access.
>
> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> ---
> v3: Cosmetic fixes.
>
>     Locked hostp2m in the function "altp2m_lazy_copy" to avoid a mapping
>     being changed in hostp2m before it has been inserted into the
>     valtp2m view.
>
>     Removed unnecessary calls to "p2m_mem_access_check" in the functions
>     "do_trap_instr_abort_guest" and "do_trap_data_abort_guest" after a
>     translation fault has been handled by the function
>     "altp2m_lazy_copy".
>
>     Adapted "altp2m_lazy_copy" to return the value "true" if the
>     encountered translation fault encounters a valid entry inside of the
>     currently active altp2m view. If multiple vcpu's are using the same
>     altp2m, it is likely that both generate a translation fault, whereas
>     the first one will be already handled by "altp2m_lazy_copy". With
>     this change the 2nd vcpu will retry accessing the faulting address.
>
>     Changed order of altp2m checking and MMIO emulation within the
>     function "do_trap_data_abort_guest".  Now, altp2m is checked and
>     handled only if the MMIO does not have to be emulated.
>
>     Changed the function prototype of "altp2m_lazy_copy".  This commit
>     removes the unnecessary struct p2m_domain* from the previous
>     function prototype.  Also, this commit removes the unnecessary
>     argument gva.  Finally, this commit changes the address of the
>     function parameter gpa from paddr_t to gfn_t and renames it to gfn.
>
>     Moved the altp2m handling mechanism into a separate function
>     "try_handle_altp2m".
>
>     Moved the functions "p2m_altp2m_check" and
>     "altp2m_switch_vcpu_altp2m_by_id" out of this patch.
>
>     Moved applied code movement into a separate patch.
> ---
>  xen/arch/arm/altp2m.c        | 62 ++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/traps.c         | 35 +++++++++++++++++++++++++
>  xen/include/asm-arm/altp2m.h |  5 ++++
>  3 files changed, 102 insertions(+)
>
> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
> index 11272e9..2009bad 100644
> --- a/xen/arch/arm/altp2m.c
> +++ b/xen/arch/arm/altp2m.c
> @@ -165,6 +165,68 @@ out:
>      return rc;
>  }
>
> +/*
> + * The function altp2m_lazy_copy returns "false" on error.  The return value
> + * "true" signals that either the mapping has been successfully lazy-copied
> + * from the hostp2m to the currently active altp2m view or that the altp2m view
> + * holds already a valid mapping. The latter is the case if multiple vcpu's
> + * using the same altp2m view generate a translation fault that is led back in
> + * both cases to the same mapping and the first fault has been already handled.
> + */
> +bool_t altp2m_lazy_copy(struct vcpu *v,
> +                        gfn_t gfn,
> +                        struct npfec npfec)

Please don't introduce parameters that are not used at all.

> +{
> +    struct domain *d = v->domain;
> +    struct p2m_domain *hp2m = p2m_get_hostp2m(d), *ap2m = NULL;
> +    p2m_type_t p2mt;
> +    p2m_access_t p2ma;
> +    mfn_t mfn;
> +    unsigned int page_order;
> +    int rc;
> +
> +    ap2m = altp2m_get_altp2m(v);
> +    if ( ap2m == NULL)
> +        return false;
> +
> +    /* Check if entry is part of the altp2m view. */
> +    mfn = p2m_lookup_attr(ap2m, gfn, NULL, NULL, NULL);
> +    if ( !mfn_eq(mfn, INVALID_MFN) )
> +        /*
> +         * If multiple vcpu's are using the same altp2m, it is likely that both

s/vcpu's/vcpus/

> +         * generate a translation fault, whereas the first one will be handled
> +         * successfully and the second will encounter a valid mapping that has
> +         * already been added as a result of the previous translation fault.
> +         * In this case, the 2nd vcpu need to retry accessing the faulting

s/need/needs/

> +         * address.
> +         */
> +        return true;
> +
> +    /*
> +     * Lock hp2m to prevent the hostp2m to change a mapping before it is added
> +     * to the altp2m view.
> +     */
> +    p2m_read_lock(hp2m);
> +
> +    /* Check if entry is part of the host p2m view. */
> +    mfn = p2m_lookup_attr(hp2m, gfn, &p2mt, &p2ma, &page_order);
> +    if ( mfn_eq(mfn, INVALID_MFN) )
> +        goto out;
> +
> +    rc = modify_altp2m_entry(ap2m, gfn, mfn, p2mt, p2ma, page_order);
> +    if ( rc )
> +    {
> +        gdprintk(XENLOG_ERR, "altp2m[%d] failed to set entry for %#"PRI_gfn" -> %#"PRI_mfn"\n",

p2midx is unsigned so this should be %u.

> +                 altp2m_vcpu(v).p2midx, gfn_x(gfn), mfn_x(mfn));
> +        domain_crash(hp2m->domain);

You already have the domain in hand with 'd'.

> +    }
> +
> +out:
> +    p2m_read_unlock(hp2m);
> +
> +    return true;
> +}
> +
>  static inline void altp2m_reset(struct p2m_domain *p2m)
>  {
>      p2m_write_lock(p2m);
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 0bf1653..a4c923c 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -48,6 +48,8 @@
>  #include <asm/vgic.h>
>  #include <asm/cpuerrata.h>
>
> +#include <asm/altp2m.h>
> +
>  /* The base of the stack must always be double-word aligned, which means
>   * that both the kernel half of struct cpu_user_regs (which is pushed in
>   * entry.S) and struct cpu_info (which lives at the bottom of a Xen
> @@ -2397,6 +2399,24 @@ static inline bool hpfar_is_valid(bool s1ptw, uint8_t fsc)
>      return s1ptw || (fsc == FSC_FLT_TRANS && !check_workaround_834220());
>  }
>
> +static bool_t try_handle_altp2m(struct vcpu *v,
> +                                paddr_t gpa,
> +                                struct npfec npfec)

I am not convinced about the usefulness of this function.

> +{
> +    bool_t rc = false;
> +
> +    if ( altp2m_active(v->domain) )

I might be worth to include an unlikely in altp2m_active to tell the 
compiler that altp2m is not the main path.

> +        /*
> +         * Copy the mapping of the faulting address into the currently
> +         * active altp2m view. Return true on success or if the particular
> +         * mapping has already been lazily copied to the currently active
> +         * altp2m view by another vcpu. Return false otherwise.
> +         */
> +        rc = altp2m_lazy_copy(v, _gfn(paddr_to_pfn(gpa)), npfec);
> +
> +    return rc;
> +}
> +
>  static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>                                        const union hsr hsr)
>  {
> @@ -2445,6 +2465,14 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>          break;
>      case FSC_FLT_TRANS:
>          /*
> +         * The guest shall retry accessing the page if the altp2m handler
> +         * succeeds. Otherwise, we continue injecting an instruction abort
> +         * exception.
> +         */
> +        if ( try_handle_altp2m(current, gpa, npfec) )
> +            return;

I would have expected that altp2m is the last thing we want to check in 
the abort handler. I.e after p2m_lookup.

> +
> +        /*
>           * The PT walk may have failed because someone was playing
>           * with the Stage-2 page table. Walk the Stage-2 PT to check
>           * if the entry exists. If it's the case, return to the guest
> @@ -2547,6 +2575,13 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>          }
>
>          /*
> +         * The guest shall retry accessing the page if the altp2m handler
> +         * succeeds. Otherwise, we continue injecting a data abort exception.
> +         */
> +        if ( try_handle_altp2m(current, info.gpa, npfec) )
> +            return;

Ditto here.

> +
> +        /*
>           * The PT walk may have failed because someone was playing
>           * with the Stage-2 page table. Walk the Stage-2 PT to check
>           * if the entry exists. If it's the case, return to the guest
> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
> index ef80829..8e40c45 100644
> --- a/xen/include/asm-arm/altp2m.h
> +++ b/xen/include/asm-arm/altp2m.h
> @@ -84,6 +84,11 @@ int altp2m_set_mem_access(struct domain *d,
>                            p2m_access_t a,
>                            gfn_t gfn);
>
> +/* Alternate p2m paging mechanism. */
> +bool_t altp2m_lazy_copy(struct vcpu *v,
> +                        gfn_t gfn,
> +                        struct npfec npfec);
> +
>  /* Propagates changes made to hostp2m to affected altp2m views. */
>  int altp2m_propagate_change(struct domain *d,
>                              gfn_t sgfn,
>

Regards,

-- 
Julien Grall

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