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

>         +                return P2M_ONE_PROGRESS_NOP;
>         +            }
>         +
>         +            /* Shatter large pages as we descend */
>         +            if ( p2m_mapping(orig_pte) )
>         +            {
>         +                rc = p2m_create_table(d, entry,
>         +                                      level_shift - PAGE_SHIFT,
>         flush_cache);
>         +                if ( rc < 0 )
>         +                    return rc;
>         +
>         +                p2m->stats.shattered[level]++;
>         +                p2m->stats.mappings[level]--;
>         +                p2m->stats.mappings[level+1] += LPAE_ENTRIES;
>         +            } /* else: an existing table mapping -> descend */
>         +
>
>
>     This piece of code is exactly the same in INSERT, REMOVE and now
>     MEMACCESS. I would create an helper to shatter and update the stats.
>
>
> Ack.
>
>
>         +            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);
>         +                p2m_write_pte(entry, pte, flush_cache);
>         +            }
>         +
>         +            (*addr)++;
>
>
>     *addr += PAGE_SIZE;
>
>     [..]
>
>
>         +/* Set access type for a region of pfns.
>         + * If start_pfn == -1ul, sets the default access type */
>         +long p2m_set_mem_access(struct domain *d, unsigned long pfn,
>         uint32_t nr,
>         +                        uint32_t start, uint32_t mask,
>         xenmem_access_t access)
>         +{
>         +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>         +    p2m_access_t a;
>         +    long rc = 0;
>         +    paddr_t paddr;
>         +
>         +    static const p2m_access_t memaccess[] = {
>         +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
>         +        ACCESS(n),
>         +        ACCESS(r),
>         +        ACCESS(w),
>         +        ACCESS(rw),
>         +        ACCESS(x),
>         +        ACCESS(rx),
>         +        ACCESS(wx),
>         +        ACCESS(rwx),
>         +#undef ACCESS
>         +    };
>         +
>         +    switch ( access )
>         +    {
>         +    case 0 ... ARRAY_SIZE(memaccess) - 1:
>         +        a = memaccess[access];
>         +        break;
>         +    case XENMEM_access_default:
>         +        a = p2m->default_access;
>         +        break;
>         +    default:
>         +        return -EINVAL;
>         +    }
>         +
>         +    /* If request to set default access */
>         +    if ( pfn == ~0ul )
>         +    {
>         +        p2m->default_access = a;
>         +        return 0;
>         +    }
>         +
>         +    for ( pfn += start; nr > start; ++pfn )
>         +    {
>         +        paddr = pfn_to_paddr(pfn);
>         +        rc = apply_p2m_changes(d, MEMACCESS, paddr, paddr+1, 0,
>         MATTR_MEM, 0, a);
>
>
>     Hmmm... why didn't you call directly apply_p2m_changes with the
>     whole range?
>
>
> Because the hypercall continuation. Setting mem_access permissions needs
> to be preemptible and it has its own separate routine to do that here.
> See http://xenbits.xen.org/xsa/advisory-89.html for more info.

We do have hypercall continuation in apply_p2m_changes (see for 
relinquish). Please do the same for MEMACCESS rather than using your own 
loop.

Hence, with your solution, the p2m lookup is taken/released at each 
loop. This is inefficient.

Regards,

-- 
Julien Grall