Re: [PATCH for-4.5 v8 15/19] xen/arm: Data abort exception (R/W) mem_events.

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

Hello Tamas,

On 09/24/2014 05:27 PM, Tamas K Lengyel wrote:
>     >  /* Put any references on the single 4K page referenced by pte.  TODO:
>     > @@ -553,13 +584,22 @@ static int apply_one_level(struct domain *d,
>     >          if ( p2m_valid(orig_pte) )
>     >              return P2M_ONE_DESCEND;
>     >
>     > -        if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) )
>     > +        if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) &&
>     > +           /* We only create superpages when mem_access is not in use. */
>     > +             (level == 3 || (level < 3 && !p2m->access_in_use)) )
> 
>     Can't this check be moved in is_mapping_aligned? You have nearly the
>     same few lines below.
> 
> 
> Unfortunately not, I already checked and it is used in REMOVE as well in
> which case we would need an exception.. and that wasn't very straight
> forward.

Ok.

> 
>     [..]
> 
>     > +    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, 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);
> 
>     Why the ASSERT? I don't see why we wouldn't want to set permission for
>     this type of page.
> 
> 
> Not sure, this I copied from p2m_lookup. Can it even happen that
> something passes p2m_valid() but have a type of p2m_invalid? I think
> that just signals that something is very wrong.

The ASSERT has been added in p2m_lookup, because p2m_invalid means the
MFN is wrong. Hence, p2m_invalid is only used for page table.

In your case, you don't need to use the MFN. So, IHMO, this ASSERT is
not necessary.

> 
>     > +                 && hypercall_preempt_check() )
>     > +            {
>     > +                rc = progress;
>     > +                goto out;
> 
>     Jumping directly to the label "out" will skip flushing the TLB for the
>     domain. While it wasn't critical until now, partial redo during
>     insertion/allocation or hypercall preemption only for relinquish, the
>     guest may use the wrong permission because the TLB hasn't been flushed.
> 
>     At the same time, it looks like you never request to flush for the
>     MEMACCESS operation (see *flush = true). Does memaccess does a TLB flush
>     somewhere else?
> 
> 
> Yes, at the end of p2m_set_mem_access once all PTEs are updated
> successfully. I guess we could flush the TLB as we are progressing as
> well, it wouldn't hurt.

We should flush the TLB as we are progressing because the guest may
technically continue to run...

>     [..]
> 
>     > +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
>     > +{
>     > +    int rc;
>     > +    bool_t violation;
>     > +    xenmem_access_t xma;
>     > +    mem_event_request_t *req;
>     > +    struct vcpu *v = current;
>     > +    struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
>     > +
>     > +    /* Mem_access is not in use. */
>     > +    if ( !p2m->access_in_use )
>     > +        return true;
> 
>     AFAIU, it's not possible to call this function when mem access is not in
>     use. I would turn this check into an ASSERT.
> 
> 
> It is possible to call this function when mem_access is not in use and
> it is called every time there is a permission fault in the second stage
> translation. This check here just makes sure the function returns as
> fast as possible when not in use.

Oh right, sorry for the noise.

This case made me also think about another possible issue. Permission
are checked in raw_copy_{from,to}_guest_helper during virtual address
translation to a physical address.

As you modified the attribute in the P2M, the copy may failed because of
the lake of permission.

Regards,

-- 
Julien Grall