[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Julien,


On 09/12/2016 04:18 PM, Julien Grall wrote:
> 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@xxxxxxxxxxxxx>
>> ---
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> Cc: Julien Grall <julien.grall@xxxxxxx>
>> ---
>> 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.
>

It was a stupid mistake from my part. Thanks for noticing.

>> +{
>> +    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/
>

Ok.

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

Ok.

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

Ok.

>> +                 altp2m_vcpu(v).p2midx, gfn_x(gfn), mfn_x(mfn));
>> +        domain_crash(hp2m->domain);
>
> You already have the domain in hand with 'd'.
>

Thanks.

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

Your call. However, I believe it is better to have the altp2m handling
routine at one place.

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

Done.

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

Alright, I will reorder both calls, thank you.

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

Same here, thanks.

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

Cheers,
~Sergej


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.