Re: [PATCH v5 12/17] xen/arm: Data abort exception (R/W) mem_events.

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

Hello Tamas,

On 12/09/14 13:48, Tamas K Lengyel wrote:
>
>
> On Fri, Sep 12, 2014 at 10:35 PM, Julien Grall <julien.grall@linaro.org
> <mailto:julien.grall@linaro.org>> wrote:
>
>     Hello Tamas,
>
>     On 12/09/14 01:46, Tamas K Lengyel wrote:
>
>                                 /* New mapping is superpage aligned,
>         make it */
>                                 pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT,
>                  mattr, t, a);
>                                 if ( level < 3 )
>                  @@ -663,6 +737,7 @@ static int apply_one_level(struct
>         domain *d,
>
>                             memset(&pte, 0x00, sizeof(pte));
>                             p2m_write_pte(entry, pte, flush_cache);
>                  +        radix_tree_delete(&p2m->mem_____access_settings,
>                  paddr_to_pfn(*addr));
>
>                             *addr += level_size;
>                             *maddr += level_size;
>                  @@ -707,6 +782,53 @@ 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)++;
>
>
>              Why increment by 1? You the PTE doesn't contain valid
>         mapping you
>              want to skip the whole level range. ie:
>
>              *addr += level_size;
>
>
>         It doesn't make a difference, apply_p2m_changes is called with
>         start=paddr, end=paddr+1 from a separate loop. So just
>         incrementing it
>         by one or a whole level achieves the same effect, that is, the
>         apply_p2m_changes loop breaks.
>
>
>     Actually it makes a lots of difference. If you increment by 1 the
>     address, you will call up to level_size time your code before
>     effectively going to the next level entry.
>
>     This function can be called with *multiple page*.
>
>
> It can be, but it isn't. It makes no difference form my perspective so
> I'm fine with it either way.

Even in your case, this code will be called too often, and then make 
your code slower. So please fix it, the other part of this function is 
skipping the whole level in this case. There is no reason to decide to 
not use it here.

> It is a bit less efficient for sure. However, the continuation setup for
> mem_access is in common/mem_access for XENMEM_access_op_set_access. In
> order to make use of apply_p2m_change to setup continuation I would have
> to remove that code from common and make it arch specific in both ARM
> and x86. Is that really a good idea just to have this optimization in ARM?

It's even far less efficient, mainly when you are applying on a batch of 
PFN like xenaccess will do.

The common code won't be changed. I'm just asking to move the check in 
apply_p2m_changes. It should not be a big deal.

Regards,

-- 
Julien Grall