Re: [PATCH v4 11/16] xen/arm: Data abort exception (R/W) mem_events.

[Julien Grall <julien.grall@linaro.org>]

Hi Tamas,

On 05/09/14 01:58, Tamas K Lengyel wrote:
>   #define P2M_ONE_DESCEND        0
>   #define P2M_ONE_PROGRESS_NOP   0x1
>   #define P2M_ONE_PROGRESS       0x10
> @@ -502,6 +568,14 @@ static int apply_one_level(struct domain *d,
>                   pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a);
>                   if ( level < 3 )
>                       pte.p2m.table = 0;
> +
> +                if ( p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a) < 0 )
> +                {
> +                    /* If we fail to save the setting in the Radix tree, we
> +                     * need to clear the PTE mem_access permissions. */
> +                    p2m_set_permission(&pte, pte.p2m.type, p2m_access_rwx);
> +                }
> +

[..]

> @@ -538,6 +612,13 @@ static int apply_one_level(struct domain *d,
>               if ( level < 3 )
>                   pte.p2m.table = 0; /* Superpage entry */
>
> +            if ( p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a) < 0 )
> +            {
> +                /* If we fail to save the setting in the Radix tree, we
> +                 * need to clear the PTE mem_access permissions. */
> +                p2m_set_permission(&pte, pte.p2m.type, p2m_access_rwx);
> +            }
> +

Can't this be abstract to another helper?

Also is it acceptable from mem access POV to unconditionally set the 
access to rwx when an error occured? Shouldn't you report an error?

> +static int p2m_set_entry(struct domain *d, paddr_t paddr, p2m_access_t p2ma)
> +{

As I said on the previous version, I still think this can be merge in 
apply_p2m_changes without any difficulties.

The main reason is to avoid increasing the number of function that have 
a common pattern to use the p2m. Also apply_p2m_changes have some 
interesting performance improvement (such as avoid map/unmap every time 
page table).

Finally, it will make life simpler when 4-Level page table will be 
supported (see Ian's series [1])

[..]

> +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, struct npfec npfec)

const struct npfect

[..]

> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 019991f..7eb875a 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1852,13 +1852,38 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,

[..]

> +    switch ( dabt.dfsc )
> +    {
> +    case DABT_DFSC_PERMISSION_1:
> +    case DABT_DFSC_PERMISSION_2:
> +    case DABT_DFSC_PERMISSION_3:
> +    {
> +        struct npfec npfec = {

const as you won't modify it later.

Regards,

[1] http://lists.xen.org/archives/html/xen-devel/2014-09/msg00596.html

-- 
Julien Grall