Re: [PATCH for-4.5 v6 13/17] xen/arm: Data abort exception (R/W) mem_events.

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2014-09-15 at 16:02 +0200, Tamas K Lengyel wrote:

> -        if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) )
> +        if ( level < 3 && p2m_access_rwx != a )
> +        {
> +            /* We create only 4k pages when mem_access is in use. */

I wonder if it might turn out cleaner to integrate this check into
is_mapping_aligned (which really is more of a "can we use a superpage"
function).

i.e.
        /* mem access cannot use super pages */
        if ( a != p2m_access_rwx && level_size != THIRD_SIZE )
                return false;

> +        }
> +        else if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) )
>          {
>              struct page_info *page;
>  
>              page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0);
>              if ( page )
>              {
> +                if ( 3 == level )

Please write the conditionals the other way around.

> +                {
> +                    rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a);
> +                    if ( rc < 0 )
> +                    {
> +                        free_domheap_page(page);
> +                        return rc;
> +                    }

> +                }
> +                else
> +                {
> +                    a = p2m_access_rwx;
> +                }

You have this else clause twice, I think you could pull it up to the
head of the function, or perhaps even into the caller.
> @@ -627,15 +741,11 @@ static int apply_one_level(struct domain *d,
>                   * and descend.
>                   */
>                  *flush = true;
> -                rc = p2m_create_table(d, entry,
> -                                      level_shift - PAGE_SHIFT, flush_cache);
> +                rc = p2m_shatter_page(d, entry, level, level_shift, flush_cache);
> +

Please keep the error handling if snuggled against the function, (i.e.
drop the additional blank line) here and in at least one other place
which you've changed.

> @@ -704,6 +815,49 @@ static int apply_one_level(struct domain *d,
>              *addr += PAGE_SIZE;
>              return P2M_ONE_PROGRESS_NOP;
>          }
> +
> +    case MEMACCESS:
> +        if ( level < 3 )
> +        {
> +            if ( !p2m_valid(orig_pte) )
> +            {
> +                *addr += level_size;
> +                return P2M_ONE_PROGRESS_NOP;
> +            }
> +
> +            /* Shatter large pages as we descend */
> +            if ( p2m_mapping(orig_pte) )
> +            {
> +                rc = p2m_shatter_page(d, entry, level, level_shift, flush_cache);
> +
> +                if ( rc < 0 )
> +                    return rc;
> +            } /* else: an existing table mapping -> descend */
> +
> +            return P2M_ONE_DESCEND;
> +        }
> +        else
> +        {
> +            pte = orig_pte;
> +
> +            if ( !p2m_table(pte) )
> +                pte.bits = 0;
> +
> +            if ( p2m_valid(pte) )
> +            {
> +                ASSERT(pte.p2m.type != p2m_invalid);
> +
> +                rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a);
> +                if ( rc < 0 )
> +                    return rc;
> +
> +                p2m_set_permission(&pte, pte.p2m.type, a);

I think this function can always make use of pte.p2m.type itself rather
than receiving it as a parameter. The other caller passes "t" but has
already assigned that to pte.p2m.type as well.
>  
> -    rc = gva_to_ipa(info.gva, &info.gpa);
> -    if ( rc == -EFAULT )
> +    switch ( dabt.dfsc )
> +    {
> +    case DABT_DFSC_PERMISSION_1:
> +    case DABT_DFSC_PERMISSION_2:
> +    case DABT_DFSC_PERMISSION_3:

Eventually this will need to handle level 0 too. Would it work to mask
out the level bits and check the remainder against the common bit
pattern?

> +/* Data abort data fetch status codes */
> +enum dabt_dfsc {
> +    DABT_DFSC_ADDR_SIZE_0       = 0b000000,

Unfortunately I think 0b... is a gcc extension and not standard C
(please correct me if I'm wrong). In which case we should probably avoid
it and use hex instead.

Actually, isn't this partially duplicating the existing FSC_* defines?
We should either use those here or move the existing users over to the
new scheme.

Ian.