Re: [Xen-devel] [PATCH for-4.5 v7 07/21] xen: Relocate p2m_mem_access_check into common and refactor it.

[Tim Deegan <tim@xxxxxxx>]

Hi,

Thanks for the patch.  Tidying this up is a great idea but I'm afraid
it can't be done quite this way. 

At 22:51 +0200 on 17 Sep (1410990670), Tamas K Lengyel wrote:
> The p2m_mem_access_check was used only to automatically promote the
> special-case mem_access rights and to allocate the mem_access message.
> In this patch we abstract the violation check into a common mem_access_check
> function and change its return value to int.
> 
> Return with rc < 0 means the fault wasn't triggered by mem_access permissions.
> Return with rc == 0 it was but rights were not automatically promoted.
> Return with rc > 0 means mem_access rights had been automatically promoted.
> 
> As part of this patch, we relocate the x86/mm/mm-locks.h header into asm
> as to be able to create a generic inlined p2m_gpfn_lock/unlock wrapper
> that each arch can define based on its locking scheme.

I'm afraid not.  mm-locks.h is hidden away in arch/x86/mm
deliberately, to stop p2m internals from leaking out into the rest of
the code and making it harder to reason about.  So the fact that you
had to move it suggests that we need to think about using cleaner
interfaces between the code you're extracting and the rest of the p2m
code...

> +int mem_access_check(paddr_t gpa, unsigned long gla, struct npfec npfec)
> +{
> +    bool_t violation;
> +    mfn_t mfn;
> +    p2m_access_t p2ma;
> +    p2m_type_t p2mt;
> +    mem_event_request_t *req;
> +    int rc;
> +    struct vcpu *v = current;
> +    unsigned long gfn = gpa >> PAGE_SHIFT;
> +    struct domain *d = v->domain;
> +    struct p2m_domain* p2m = p2m_get_hostp2m(d);
> +
> +    /* First, handle rx2rw conversion automatically.
> +     * These calls to p2m->set_entry() must succeed: we have the gfn
> +     * locked and just did a successful get_entry(). */
> +    p2m_gpfn_lock(p2m, gfn);
> +    mfn = p2m->get_entry(p2m, gfn, &p2mt, &p2ma, 0, NULL);

Neither of these (direct access to a p2m lock or direct call of a p2m
function pointer) should happen outside the p2m code. 

From an outside caller you _could_ use get_gfn_type_access(p2m, gfn,
&p2mt, &p2ma, 0, NULL) instead (and put_gfn() instead of
p2m_gpfn_unlock()).  But directly calling p2m->set_entry() (or
p2m_set_entry()) is still not OK.

I think it would be better to leave an arch-specific 
"bool_t p2m_mem_access_check(p2m, gfn, npfec, &p2ma)" that does the
lookup and the automatic promotions and returns the access type.  It
should also do the check for missing listeners and automatically reset
p2m entries (i.e. the second call to ->set_entry() below).  Then this
function won't need to take any locks or get_gfn calls at all -- it
will just have the check for non-vioating faults, the call to
p2m_mem_access_check() that might ajdust entries, and the code to
prepare a req.

> +
> +    /* Check if the access is against the permissions. */
> +    switch ( p2ma )
> +    {
> +    case p2m_access_rwx:
> +        violation = 0;
> +        break;
> +    case p2m_access_rw:
> +        violation = npfec.insn_fetch;
> +        break;
> +    case p2m_access_wx:
> +        violation = npfec.read_access;
> +        break;
> +    case p2m_access_rx:
> +    case p2m_access_rx2rw:
> +        violation = npfec.write_access;
> +        break;
> +    case p2m_access_x:
> +        violation = npfec.read_access || npfec.write_access;
> +        break;
> +    case p2m_access_w:
> +        violation = npfec.read_access || npfec.insn_fetch;
> +        break;
> +    case p2m_access_r:
> +        violation = npfec.write_access || npfec.insn_fetch;
> +        break;
> +    case p2m_access_n:
> +    case p2m_access_n2rwx:
> +    default:
> +        violation = npfec.read_access || npfec.write_access || 
> npfec.insn_fetch;
> +        break;
> +    }

In any case this block, which doesn't depend on the p2m contents at
all, should happen before any locks or lookups.  That way you can
avoid taking the locks at all for non-violating faults (as the
original code does).

> +    /* If no violation is found here, it needs to be reinjected. */
> +    if ( !violation )
> +    {
> +        p2m_gpfn_unlock(p2m, gfn);
> +        return -EFAULT;
> +    }
> +
> +    /* Check for automatic setting promotion. */
> +    if ( npfec.write_access && p2ma == p2m_access_rx2rw )
> +    {
> +        rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, 
> p2m_access_rw);
> +        ASSERT(rc == 0);
> +        p2m_gpfn_unlock(p2m, gfn);
> +        return 1;
> +    }
> +    else if ( p2ma == p2m_access_n2rwx )
> +    {
> +        ASSERT(npfec.write_access || npfec.read_access || npfec.insn_fetch);
> +        rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K,
> +                            p2mt, p2m_access_rwx);
> +        ASSERT(rc == 0);
> +    }
> +    p2m_gpfn_unlock(p2m, gfn);
> +
> +    /* Otherwise, check if there is a memory event listener, and send the 
> message along */
> +    if ( !mem_event_check_ring(&d->mem_event->access) )
> +    {
> +        /* No listener */
> +        if ( p2m->access_required )
> +        {
> +            gdprintk(XENLOG_INFO, "Memory access permissions failure, "
> +                                  "no mem_event listener VCPU %d, dom %d\n",
> +                                  v->vcpu_id, d->domain_id);
> +            domain_crash(v->domain);
> +            return -EFAULT;
> +        }
> +        else
> +        {
> +            p2m_gpfn_lock(p2m, gfn);
> +            mfn = p2m->get_entry(p2m, gfn, &p2mt, &p2ma, 0, NULL);
> +            if ( p2ma != p2m_access_n2rwx )
> +
> +            {
> +                /* A listener is not required, so clear the access
> +                 * restrictions.  This set must succeed: we have the
> +                 * gfn locked and just did a successful get_entry(). */
> +                rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K,
> +                                    p2mt, p2m_access_rwx);
> +                ASSERT(rc == 0);
> +            }
> +            p2m_gpfn_unlock(p2m, gfn);
> +            return 1;
> +        }
> +    }
> +
> +    req = xzalloc(mem_event_request_t);
> +    if ( req )
> +    {
> +        req->reason = MEM_EVENT_REASON_VIOLATION;
> +
> +        /* Pause the current VCPU */
> +        if ( p2ma != p2m_access_n2rwx )
> +            req->flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
> +
> +        /* Send request to mem event */
> +        req->gfn = gfn;
> +        req->offset = gpa & ((1 << PAGE_SHIFT) - 1);
> +        req->gla_valid = npfec.gla_valid;
> +        req->gla = gla;
> +        if ( npfec.kind == npfec_kind_with_gla )
> +            req->fault_with_gla = 1;
> +        else if ( npfec.kind == npfec_kind_in_gpt )
> +            req->fault_in_gpt = 1;
> +        req->access_r = npfec.read_access;
> +        req->access_w = npfec.write_access;
> +        req->access_x = npfec.insn_fetch;
> +        req->vcpu_id = v->vcpu_id;
> +
> +        mem_access_send_req(v->domain, req);

The original function returned the req for the caller to send, and the
caller delayed sending it until the end of hvm_hap_nested_page_fault(),
when the last locks are dropped.  You need to keep this behaviour or
the system can deadlock.

I wonder whether we could add an ASSERT_NOT_IN_ATOMIC() at the top of
mem_event_wait_slot() to make this kind of thing easier to spot.  At
the moment it will only fail if it actually has to wait.

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel