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

[Sergej Proskurin <proskurin@sec.in.tum.de>]

Hi Julien,


On 08/04/2016 03:50 PM, Julien Grall wrote:
> 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.
>

The implemented paging mechanism also covers permission issues, which is
the reason why I chose the term violation. By this, I mean that the
implementation covers traps to Xen occured due to 2nd stage permission
violations (as altp2m view's might have set different access permissions
to trap on). Also, the implementation covers translation faults due to
not present entries in the active altp2m view, as well.

> 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?
>

No. Every time, the altp2m's configuration lets the guest trap into Xen
due to a lack of memory access permissions (e.g., on execution of a rw
page), we fill the associated fields in the req buffer in
mem_access_check so that the management domain receives the information
required to understand what kind of altp2m violation just happened.
Based on this information, it might decide what to do next (perform
additional checks or simply change the altp2m view to continue guest
execution).

>>
>> 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.
>

Ok.

>> +
>> +    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.
>

Ok.

>>  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
>

True. Thank you.

>> +                        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.
>

True. Another leftover from the x86 implementation. I will change that.

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

Thanks.

>> +    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.
>

Ok.

>> +
>> +    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.
>

In the last patch series, I proposed to lock the hp2m->lock
(write_lock), which is not a good solution at this point, as it would
potentially create lots of contention on hp2m. Also, we would need to
export __p2m_lookup or remove the lock from p2m_lookup_attr, which is
not a good solution either.

The addition of the altp2m_lock could help in this case: If a page would
be removed from the hostp2m before we added it in the altp2m view, the
propagate function would need to wait until lazy_copy would finish and
eventually remove it from the altp2m view. But on the other hand, it
would highly decrease the performance on a multi core system.

If I understand that correctly, a better solution would be to use a
p2m_read_lock(hp2m) as we would still allow reading but not writing (in
case the hp2m gets entries removed in apply_p2m_changes). That is, I
would set it right before p2m_lookup_attr(hp2m, ...) and release it
right after modify_altp2m_entry. This solution would not present a
bottleneck on the lazy_copy mechanism, and simultaneously prevent hp2m
from changing. What do you think?

>> +
>> +    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.
>

Ok.

>> +    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.
>

x86 leftover. I will change that.

>> +                (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.
>

Another x86 leftover. I will change that.

>> +        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.
>

This function is used in ./xen/common/vm_event.c. Since the function is
used from a common place and already named p2m_* I did not want to pull
it out of p2m.c (and have a p2m_* file/function prefix in altp2m.c).
However, I could move the function to altp2m.c rename it globally (also
in the x86 implementation). Or, I could simply move it to altp2m despite
the name. What would you prefer?

>>  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.
>

Ok.

>>
>>      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.
>

Ok.

>> +             * 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.
>

Right. I have experienced that the test systems jump into the case
FSC_FLT_TRANS, right after I have lazily-copied the page into the
associated altp2m view. Not sure what the issue might be here.

>> +
>> +            /* 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.

I am not sure whether I understood your request. Could you be more
specific please? What excatly shall be moved where?

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

I will try to answer this question after I had a look at [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.
>

Same issue as above.

>> +
>> +            /* 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?
>

I really important reason: I moved it simply because int(FSC_FLT_TRANS)
< int(FSC_FLT_PERM). I can move it back if you like.

>>      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
>

Best regards,
~Sergej


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