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.
The ASSERT has been added in p2m_lookup, because p2m_invalid means the
>
> [..]
>
> > + 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.
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