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

[Tamas K Lengyel <tamas.lengyel@zentific.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8135 bytes --]

On Thu, Sep 18, 2014 at 12:39 PM, Tim Deegan <tim@xen.org> wrote:

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

Ack. I think I'll drop this patch for the time being as the refactoring
work required for it to be in common mem_access is quite significant, both
in the x86 and the ARM side. And it would only result in a bit of
code-deduplication. We could pick-it up later if there is a need for it /
value in it.

Thanks,
Tamas

[-- Attachment #1.2: Type: text/html, Size: 9785 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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