On Wed, Sep 24, 2014 at 6:51 PM, Julien Grall <julien.grall@linaro.org>
wrote:

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

Ack, will remove it.


>
> >
> >     > +                 && 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...
>

Hm, I think the guest is always paused while mem_access is being set via
memop but sure, it can't hurt.


>
> >     [..]
> >
> >     > +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.
>

I'm not entire sure what you mean. Can you elaborate?

Thanks,
Tamas


>
> Regards,
>
> --
> Julien Grall
>