Re: [PATCH v2 20/25] arm/altp2m: Add altp2m paging mechanism.

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

Hello Sergej,

On 01/08/16 18:10, 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 violations on instruction or data access. Every altp2m
> violation generates a vm_event.

I think you want to "translation fault" and not "violation". The latter 
looks more a permission issue whilst it is not the case here.

However I am not sure what you mean by "every altp2m violation generates 
a vm_event". Do you mean the userspace will be aware of it?

>
> 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        |  86 ++++++++++++++++++++++++++++++
>  xen/arch/arm/p2m.c           |   6 +++
>  xen/arch/arm/traps.c         | 124 +++++++++++++++++++++++++++++++------------
>  xen/include/asm-arm/altp2m.h |  15 ++++--
>  xen/include/asm-arm/p2m.h    |   6 +--
>  5 files changed, 196 insertions(+), 41 deletions(-)
>
> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c
> index f3c1cff..78fc1d5 100644
> --- a/xen/arch/arm/altp2m.c
> +++ b/xen/arch/arm/altp2m.c
> @@ -33,6 +33,32 @@ struct p2m_domain *altp2m_get_altp2m(struct vcpu *v)
>      return v->domain->arch.altp2m_p2m[index];
>  }
>
> +bool_t altp2m_switch_vcpu_altp2m_by_id(struct vcpu *v, unsigned int idx)
> +{
> +    struct domain *d = v->domain;
> +    bool_t rc = 0;

Please use true/false rather than 0/1.

> +
> +    if ( idx >= MAX_ALTP2M )
> +        return rc;
> +
> +    altp2m_lock(d);
> +
> +    if ( d->arch.altp2m_vttbr[idx] != INVALID_VTTBR )
> +    {
> +        if ( idx != vcpu_altp2m(v).p2midx )
> +        {
> +            atomic_dec(&altp2m_get_altp2m(v)->active_vcpus);
> +            vcpu_altp2m(v).p2midx = idx;
> +            atomic_inc(&altp2m_get_altp2m(v)->active_vcpus);
> +        }
> +        rc = 1;
> +    }
> +
> +    altp2m_unlock(d);
> +
> +    return rc;
> +}
> +

You implement 2 distinct features in this patch which make really 
difficult to read and they are not all described in the commit message:

  * Implementation of altp2m_switch_vcpu_altp2m_by_id and p2m_alp2m_check
  * Implementation of lazy copy when receiving a data abort

So please split it.

>  int altp2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx)
>  {
>      struct vcpu *v;
> @@ -133,6 +159,66 @@ out:
>      return rc;
>  }
>
> +bool_t altp2m_lazy_copy(struct vcpu *v,
> +                        paddr_t gpa,
> +                        unsigned long gva,

this should be vaddr_t

> +                        struct npfec npfec,
> +                        struct p2m_domain **ap2m)

Why do you need the parameter ap2m? None of the callers make use of it 
except setting it.

> +{
> +    struct domain *d = v->domain;
> +    struct p2m_domain *hp2m = p2m_get_hostp2m(v->domain);

p2m_get_hostp2m(d);

> +    p2m_type_t p2mt;
> +    xenmem_access_t xma;
> +    gfn_t gfn = _gfn(paddr_to_pfn(gpa));
> +    mfn_t mfn;
> +    unsigned int level;
> +    int rc = 0;

Please use true/false rather than 0/1. Also this should be bool_t.

> +
> +    static const p2m_access_t memaccess[] = {
> +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
> +        ACCESS(n),
> +        ACCESS(r),
> +        ACCESS(w),
> +        ACCESS(rw),
> +        ACCESS(x),
> +        ACCESS(rx),
> +        ACCESS(wx),
> +        ACCESS(rwx),
> +        ACCESS(rx2rw),
> +        ACCESS(n2rwx),
> +#undef ACCESS
> +    };
> +
> +    *ap2m = altp2m_get_altp2m(v);
> +    if ( *ap2m == NULL)
> +        return 0;
> +
> +    /* Check if entry is part of the altp2m view */
> +    mfn = p2m_lookup_attr(*ap2m, gfn, NULL, NULL, NULL, NULL);
> +    if ( !mfn_eq(mfn, INVALID_MFN) )
> +        goto out;
> +
> +    /* Check if entry is part of the host p2m view */
> +    mfn = p2m_lookup_attr(hp2m, gfn, &p2mt, &level, NULL, &xma);
> +    if ( mfn_eq(mfn, INVALID_MFN) )
> +        goto out;

This is quite racy. The page could be removed from the host p2m by the 
time you have added it in the altp2m because you have no lock.

> +
> +    rc = modify_altp2m_entry(d, *ap2m, gpa, pfn_to_paddr(mfn_x(mfn)), level,
> +                             p2mt, memaccess[xma]);

Please avoid to mix bool and int even though today we have implicitly 
conversion.

> +    if ( rc )
> +    {
> +        gdprintk(XENLOG_ERR, "failed to set entry for %lx -> %lx p2m %lx\n",
> +                (unsigned long)gpa, (unsigned long)(paddr_to_pfn(mfn_x(mfn))),

By using (unsigned long) you will truncate the address on ARM32 because 
we are able to support up to 40 bits address.

Also why do you print the full address? The guest physical address may 
not be page-aligned so it will confuse the user.

> +                (unsigned long)*ap2m);

It does not seem really helpful to print the pointer here. You will not 
be able to exploit it when reading the log. Also this should be printed 
with "%p" and not using a cast.

> +        domain_crash(hp2m->domain);
> +    }
> +
> +    rc = 1;
> +
> +out:
> +    return rc;
> +}
> +
>  static inline void altp2m_reset(struct p2m_domain *p2m)
>  {
>      read_lock(&p2m->lock);
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 31810e6..bee8be7 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1812,6 +1812,12 @@ void __init setup_virt_paging(void)
>      smp_call_function(setup_virt_paging_one, (void *)val, 1);
>  }
>
> +void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
> +{
> +    if ( altp2m_active(v->domain) )
> +        altp2m_switch_vcpu_altp2m_by_id(v, idx);
> +}
> +

I am not sure why this function lives here.

>  bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
>  {
>      int rc;
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 12be7c9..628abd7 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
> @@ -2403,35 +2405,64 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>      int rc;
>      register_t gva = READ_SYSREG(FAR_EL2);
>      uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
> +    struct vcpu *v = current;
> +    struct domain *d = v->domain;
> +    struct p2m_domain *p2m = NULL;
> +    paddr_t gpa;
> +
> +    if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
> +        gpa = get_faulting_ipa(gva);
> +    else
> +    {
> +        /*
> +         * Flush the TLB to make sure the DTLB is clear before
> +         * doing GVA->IPA translation. If we got here because of
> +         * an entry only present in the ITLB, this translation may
> +         * still be inaccurate.
> +         */
> +        flush_tlb_local();
> +
> +        rc = gva_to_ipa(gva, &gpa, GV2M_READ);
> +        if ( rc == -EFAULT )
> +            return; /* Try again */
> +    }

This code movement should really be a separate patch.

>
>      switch ( fsc )
>      {
> +    case FSC_FLT_TRANS:
> +    {
> +        if ( altp2m_active(d) )
> +        {
> +            const struct npfec npfec = {
> +                .insn_fetch = 1,
> +                .gla_valid = 1,
> +                .kind = hsr.iabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
> +            };
> +
> +            /*
> +             * Copy the entire page of the failing instruction into the

I think "page" is misleading here. altp2m_lazy_copy is able to copy a 
superpage mapping also.

> +             * currently active altp2m view.
> +             */
> +            if ( altp2m_lazy_copy(v, gpa, gva, npfec, &p2m) )
> +                return;
> +
> +            rc = p2m_mem_access_check(gpa, gva, npfec);

Why do you call p2m_mem_access_check here? If you are here it is for a 
translation fault which you handle via altp2m_lazy_copy.

> +
> +            /* Trap was triggered by mem_access, work here is done */
> +            if ( !rc )
> +                return;
> +        }
> +
> +        break;
> +    }
>      case FSC_FLT_PERM:
>      {
> -        paddr_t gpa;
>          const struct npfec npfec = {
>              .insn_fetch = 1,
>              .gla_valid = 1,
>              .kind = hsr.iabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
>          };
>
> -        if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
> -            gpa = get_faulting_ipa(gva);
> -        else
> -        {
> -            /*
> -             * Flush the TLB to make sure the DTLB is clear before
> -             * doing GVA->IPA translation. If we got here because of
> -             * an entry only present in the ITLB, this translation may
> -             * still be inaccurate.
> -             */
> -            flush_tlb_local();
> -
> -            rc = gva_to_ipa(gva, &gpa, GV2M_READ);
> -            if ( rc == -EFAULT )
> -                return; /* Try again */
> -        }
> -
>          rc = p2m_mem_access_check(gpa, gva, npfec);
>
>          /* Trap was triggered by mem_access, work here is done */
> @@ -2451,6 +2482,8 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>      int rc;
>      mmio_info_t info;
>      uint8_t fsc = hsr.dabt.dfsc & ~FSC_LL_MASK;
> +    struct vcpu *v = current;
> +    struct p2m_domain *p2m = NULL;
>
>      info.dabt = dabt;
>  #ifdef CONFIG_ARM_32
> @@ -2459,7 +2492,7 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>      info.gva = READ_SYSREG64(FAR_EL2);
>  #endif
>
> -    if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
> +    if ( hpfar_is_valid(hsr.dabt.s1ptw, fsc) )
>          info.gpa = get_faulting_ipa(info.gva);
>      else
>      {
> @@ -2470,23 +2503,31 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>
>      switch ( fsc )
>      {
> -    case FSC_FLT_PERM:
> +    case FSC_FLT_TRANS:
>      {
> -        const struct npfec npfec = {
> -            .read_access = !dabt.write,
> -            .write_access = dabt.write,
> -            .gla_valid = 1,
> -            .kind = dabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
> -        };
> +        if ( altp2m_active(current->domain) )

I would much prefer to this checking altp2m only if the MMIO was not 
emulated (so moving the code afterwards). This will avoid to add 
overhead when access the virtual interrupt controller.

Also, how would that fit with break-before-make sequence introduced in [1]?

> +        {
> +            const struct npfec npfec = {
> +                .read_access = !dabt.write,
> +                .write_access = dabt.write,
> +                .gla_valid = 1,
> +                .kind = dabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
> +            };
>
> -        rc = p2m_mem_access_check(info.gpa, info.gva, npfec);
> +            /*
> +             * Copy the entire page of the failing data access into the
> +             * currently active altp2m view.
> +             */
> +            if ( altp2m_lazy_copy(v, info.gpa, info.gva, npfec, &p2m) )
> +                return;
> +
> +            rc = p2m_mem_access_check(info.gpa, info.gva, npfec);

Similar question here.

> +
> +            /* Trap was triggered by mem_access, work here is done */
> +            if ( !rc )
> +                return;
> +        }
>
> -        /* Trap was triggered by mem_access, work here is done */
> -        if ( !rc )
> -            return;
> -        break;
> -    }
> -    case FSC_FLT_TRANS:
>          if ( dabt.s1ptw )
>              goto bad_data_abort;
>
> @@ -2515,6 +2556,23 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>              return;
>          }
>          break;
> +    }
> +    case FSC_FLT_PERM:
> +    {
> +        const struct npfec npfec = {
> +            .read_access = !dabt.write,
> +            .write_access = dabt.write,
> +            .gla_valid = 1,
> +            .kind = dabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
> +        };
> +
> +        rc = p2m_mem_access_check(info.gpa, info.gva, npfec);
> +
> +        /* Trap was triggered by mem_access, work here is done */
> +        if ( !rc )
> +            return;
> +        break;
> +    }

Why did you move the case handling FSC_FLT_PERM?

>      default:
>          gprintk(XENLOG_WARNING, "Unsupported DFSC: HSR=%#x DFSC=%#x\n",
>                  hsr.bits, dabt.dfsc);
> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h
> index 9aeb7d6..8bfbc6a 100644
> --- a/xen/include/asm-arm/altp2m.h
> +++ b/xen/include/asm-arm/altp2m.h
> @@ -38,9 +38,7 @@ static inline bool_t altp2m_active(const struct domain *d)
>  /* Alternate p2m VCPU */
>  static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
>  {
> -    /* Not implemented on ARM, should not be reached. */
> -    BUG();
> -    return 0;
> +    return vcpu_altp2m(v).p2midx;
>  }
>
>  int altp2m_init(struct domain *d);
> @@ -52,6 +50,10 @@ void altp2m_vcpu_destroy(struct vcpu *v);
>  /* Get current alternate p2m table. */
>  struct p2m_domain *altp2m_get_altp2m(struct vcpu *v);
>
> +/* Switch alternate p2m for a single vcpu. */
> +bool_t altp2m_switch_vcpu_altp2m_by_id(struct vcpu *v,
> +                                       unsigned int idx);
> +
>  /* Switch alternate p2m for entire domain */
>  int altp2m_switch_domain_altp2m_by_id(struct domain *d,
>                                        unsigned int idx);
> @@ -81,6 +83,13 @@ 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,
> +                        paddr_t gpa,
> +                        unsigned long gla,
> +                        struct npfec npfec,
> +                        struct p2m_domain **ap2m);
> +
>  /* Propagates changes made to hostp2m to affected altp2m views. */
>  void altp2m_propagate_change(struct domain *d,
>                               gfn_t sgfn,
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 59186c9..16e33ca 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -145,11 +145,7 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
>      /* Not supported on ARM. */
>  }
>
> -static inline
> -void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
> -{
> -    /* Not supported on ARM. */
> -}
> +void p2m_altp2m_check(struct vcpu *v, uint16_t idx);
>
>  /* Initialise vmid allocator */
>  void p2m_vmid_allocator_init(void);
>

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg02957.html

-- 
Julien Grall

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