Re: [Xen-devel] [RFC PATCH v2 5/8] arm/mem_access: Add software guest-page-table walk

[Julien Grall <julien.grall@xxxxxxx>]

Hi Sergej,

On 06/01/2017 04:18 PM, Sergej Proskurin wrote:
> The function p2m_mem_access_check_and_get_page in mem_access.c translates a gva
> to an ipa by means of the hardware functionality of the ARM architecture. This
> is implemented in the function gva_to_ipa. If mem_access is active,
> hardware-based gva to ipa translation might fail, as gva_to_ipa uses the
> guest's translation tables, access to which might be restricted by the active
> VTTBR. To address this issue, in this commit we add a software-based
> guest-page-table walk, which will be used by the function
> p2m_mem_access_check_and_get_page perform the gva to ipa translation in
> software in one of the following commits.
>
> Note: This function p2m_walk_gpt assumes that the domain, the gva of
> which is to be translated, is running on the currently active vCPU. To
> walk the guest's page table on a different vCPU, the following registers
> would need to be loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.
>
> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
> ---
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> ---
> v2: Rename p2m_gva_to_ipa to p2m_walk_gpt and move it to p2m.c.
>
>      Move the functionality responsible for walking long-descriptor based
>      translation tables out of the function p2m_walk_gpt. Also move out the
>      long-descriptor based translation out of this commit.
>
>      Change function parameters in order to return access access rights to a
>      requested gva.
>
>      Cosmetic fixes.
> ---
>   xen/arch/arm/p2m.c        | 58 +++++++++++++++++++++++++++++++++++++++++++++++
I know I suggested to move in p2m.c. Looking at the diff stat, this will 
increase quite a lot p2m.c which is already big.
How about introducing a file guest_walk.c which contain the new functions?

>   xen/include/asm-arm/p2m.h |  6 +++++
>   2 files changed, 64 insertions(+)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 752e948070..0337d83581 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1548,6 +1548,64 @@ void __init setup_virt_paging(void)
>   }
>   
>   /*
> + * The function __p2m_walk_gpt_sd translates a given GVA into an IPA using the
> + * short-descriptor translation table format in software. This function assumes
> + * that the domain is running on the currently active vCPU. To walk the guest's
> + * page table on a different vCPU, the following registers would need to be
> + * loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.
> + */
> +static int __p2m_walk_gpt_sd(struct p2m_domain *p2m,
The __ are not necessary here. You don't use the name p2m_walk_gpt_sd 
anywhere.
> +                             vaddr_t gva, paddr_t *ipa,
> +                             unsigned int *perm_ro)
I am a bit confused with perm_ro. Will you only return 0/1? If so it 
should be a bool.
But we likely want to know more permission such as the execution bit...

> +{
> +    /* Not implemented yet. */
> +    return -EFAULT;
> +}
> +
> +/*
> + * The function __p2m_walk_gpt_ld translates a given GVA into an IPA using the
> + * long-descriptor translation table format in software. This function assumes
> + * that the domain is running on the currently active vCPU. To walk the guest's
> + * page table on a different vCPU, the following registers would need to be
> + * loaded: TCR_EL1, TTBR0_EL1, TTBR1_EL1, and SCTLR_EL1.
> + */
> +static int __p2m_walk_gpt_ld(struct p2m_domain *p2m,
Ditto.

> +                             vaddr_t gva, paddr_t *ipa,
> +                             unsigned int *perm_ro)
> +{
> +    /* Not implemented yet. */
> +    return -EFAULT;
> +}
> +
> +int p2m_walk_gpt(struct p2m_domain *p2m, vaddr_t gva,
So you mix 2 things, p2m and gpt. P2M is used to refer to stage-2 page 
table. Whilst gpt stands I guess for guest page table.
The name gpt is not very used in Xen and would prefer a clearer name 
such as the x86 one "guest_walk_tables".
> +                 paddr_t *ipa, unsigned int *perm_ro)
> +{
> +    uint32_t sctlr = READ_SYSREG(SCTLR_EL1);
> +    register_t tcr = READ_SYSREG(TCR_EL1);
> +#ifdef CONFIG_ARM_64
> +    struct domain *d = p2m->domain;
> +#endif
The only place use *d is in the is_32bit_domain, so no need to add some 
#ifdef and define the variable.
> +
> +    /* If the MMU is disabled, there is no need to translate the gva. */
> +    if ( !(sctlr & SCTLR_M) )
> +    {
> +        *ipa = gva;
Why *perm_ro is not set here?

> +
> +        return 0;
> +    }
> +
> +#ifdef CONFIG_ARM_64
> +    if ( is_32bit_domain(d) )
> +#endif
is_32bit_domain exists for 32-bit Xen. So not need to have this #ifdef.

> +    {
> +        if ( !(tcr & TTBCR_EAE) )
> +            return __p2m_walk_gpt_sd(p2m, gva, ipa, perm_ro);
> +    }
> +
> +    return __p2m_walk_gpt_ld(p2m, gva, ipa, perm_ro);
> +}
> +
> +/*
>    * Local variables:
>    * mode: C
>    * c-file-style: "BSD"
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 18c57f936e..cc9f4bf225 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -264,6 +264,12 @@ void guest_physmap_remove_page(struct domain *d,
>   
>   mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn);
>   
> +/* Walk the guest's page tables in software. */
> +int p2m_walk_gpt(struct p2m_domain *p2m,
> +                 vaddr_t gva,
> +                 paddr_t *ipa,
> +                 unsigned int *perm_ro);
> +
>   /*
>    * Populate-on-demand
>    */
Cheers,

--
Julien Grall

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