Xen project Mailing List

On Wed, Sep 24, 2014 at 6:51 PM, Julien Grall <julien.grall@xxxxxxxxxx> 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

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