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

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

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@xxxxxxxxxx
<mailto:julien.grall@xxxxxxxxxx>> 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,

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


Julien Grall

Xen-devel mailing list



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