Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-03-06 at 22:24 +0100, Tamas K Lengyel wrote:
> This patch enables to store, set, check and deliver LPAE R/W mem_events.
> As the LPAE PTE's lack enough available software programmable bits,
> we store the permissions in a Radix tree. The tree is only looked at if
> mem_access_enabled is turned on.

But it is maintained/updated regardless, is that deliberate?
> +static int p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned long pfn,
> +                                     p2m_access_t a)
> +{
> +    int rc;
> +
> +    if ( p2m_access_rwx == a )
> +    {
> +        if ( p2m->mem_access_enabled )

In particular this is gated, but the rest of the function appears not to
be, which seems inconsistent...

> +            radix_tree_delete(&p2m->mem_access_settings, pfn);
> +
> +        return 0;
> +    }
> +
> +    rc = radix_tree_insert(&p2m->mem_access_settings, pfn,
> +                           radix_tree_int_to_ptr(a));
> +    if ( rc == -EEXIST )
> +    {
> +        /* If a setting existed already, change it to the new one */
> +        radix_tree_replace_slot(
> +            radix_tree_lookup_slot(
> +                &p2m->mem_access_settings, pfn),
> +            radix_tree_int_to_ptr(a));
> +        rc = 0;
> +    }
> +
> +    return rc;
> +}
> +
>  enum p2m_operation {
>      INSERT,
>      ALLOCATE,
>      REMOVE,
>      RELINQUISH,
>      CACHEFLUSH,
> +    MEMACCESS,
>  };
>  
>  /* Put any references on the single 4K page referenced by pte.  TODO:
> @@ -560,13 +592,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->mem_access_enabled)) )

I don't think we can get away with adding this check to
is_mapping_aligned (it's used elsewhere), but perhaps you could wrap
this condition in a helper to use in both places.

mapping_allowed_at_level(p2m, level) or some such.

> -           /* We do not handle replacing an existing table with a superpage */
> -             (level == 3 || !p2m_table(orig_pte)) )
> +           /* We do not handle replacing an existing table with a superpage
> +            * or when mem_access is in use. */
> +             (level == 3 || (!p2m_table(orig_pte) && !p2m->mem_access_enabled)) )

Actually, this is very subtly different isn't it. Can it be unified? If
not then ignore the helper idea.

> @@ -760,6 +807,47 @@ static int apply_one_level(struct domain *d,
>              *addr += PAGE_SIZE;
>              return P2M_ONE_PROGRESS_NOP;
>          }
> +
> +    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;

What is this about? Just clobbering an invalid PTE?

> @@ -783,6 +871,8 @@ static int apply_p2m_changes(struct domain *d,
>      unsigned int cur_root_table = ~0;
>      unsigned int cur_offset[4] = { ~0, ~0, ~0, ~0 };
>      unsigned int count = 0;
> +    const unsigned long sgfn = paddr_to_pfn(start_gpaddr),
> +                        egfn = paddr_to_pfn(end_gpaddr);
>      bool_t flush = false;
>      bool_t flush_pt;
>  
> @@ -912,6 +1006,12 @@ static int apply_p2m_changes(struct domain *d,
>      rc = 0;
>  
>  out:
> +    if ( flush )
> +    {
> +        flush_tlb_domain(d);
> +        iommu_iotlb_flush(d, sgfn, egfn - sgfn);
> +    }

Is moving the flush out of the loop an independent bug fix? If so please
do in a separate commit with a rationale in the commit log. If it is
somehow related to the changes here then please mention it in this
commit log, since it's a bit subtle.

> +
>      if ( rc < 0 && ( op == INSERT || op == ALLOCATE ) &&
>           addr != start_gpaddr )
>      {
> @@ -1281,6 +1381,254 @@ void __init setup_virt_paging(void)
>      smp_call_function(setup_virt_paging_one, (void *)val, 1);
>  }
>  
> +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)

This is different to the current x86 prototype, is that due to your
other cleanup series?

> +{
> +    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->mem_access_enabled )
> +        return true;
> +
> +    rc = p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xma);
> +    if ( rc )
> +        return true;
> +
> +    /* Now check for mem_access violation. */
> +    switch ( xma )
> +    {
> +    case XENMEM_access_rwx:
> +        violation = false;
> +        break;
> +    case XENMEM_access_rw:
> +        violation = npfec.insn_fetch;
> +        break;
> +    case XENMEM_access_wx:
> +        violation = npfec.read_access;
> +        break;
> +    case XENMEM_access_rx:
> +    case XENMEM_access_rx2rw:
> +        violation = npfec.write_access;
> +        break;
> +    case XENMEM_access_x:
> +        violation = npfec.read_access || npfec.write_access;
> +        break;
> +    case XENMEM_access_w:
> +        violation = npfec.read_access || npfec.insn_fetch;
> +        break;
> +    case XENMEM_access_r:
> +        violation = npfec.write_access || npfec.insn_fetch;
> +        break;
> +    default:
> +    case XENMEM_access_n:
> +    case XENMEM_access_n2rwx:
> +        violation = true;
> +        break;
> +    }
> +
> +    if ( !violation )
> +        return true;


The preceding section looks pretty similar to the guits of x86's
p2m_mem_event_emulate_check, can they be combined?

> +
> +    /* First, handle rx2rw and n2rwx conversion automatically. */
> +    if ( npfec.write_access && xma == XENMEM_access_rx2rw )
> +    {
> +        rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
> +                                0, ~0, XENMEM_access_rw);
> +        return false;
> +    }
> +    else if ( xma == XENMEM_access_n2rwx )
> +    {
> +        rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
> +                                0, ~0, XENMEM_access_rwx);
> +    }

This looks like a bit of p2m_mem_access_check, can it be made common?

> +
> +    /* Otherwise, check if there is a memory event listener, and send the message along */
> +    if ( !mem_event_check_ring(&v->domain->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, v->domain->domain_id);
> +            domain_crash(v->domain);
> +        }
> +        else
> +        {
> +            /* n2rwx was already handled */
> +            if ( xma != XENMEM_access_n2rwx )
> +            {
> +                /* A listener is not required, so clear the access
> +                 * restrictions. */
> +                rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
> +                                        0, ~0, XENMEM_access_rwx);
> +            }
> +        }
> +
> +        /* No need to reinject */
> +        return false;
> +    }

And this

> +    req = xzalloc(mem_event_request_t);
> +    if ( req )
> +    {
> +        req->reason = MEM_EVENT_REASON_VIOLATION;
> +        if ( xma != XENMEM_access_n2rwx )
> +            req->flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
> +        req->gfn = gpa >> PAGE_SHIFT;
> +        req->offset =  gpa & ((1 << PAGE_SHIFT) - 1);
> +        req->gla = gla;
> +        req->gla_valid = npfec.gla_valid;
> +        req->access_r = npfec.read_access;
> +        req->access_w = npfec.write_access;
> +        req->access_x = npfec.insn_fetch;
> +        if ( npfec_kind_in_gpt == npfec.kind )
> +            req->fault_in_gpt = 1;
> +        if ( npfec_kind_with_gla == npfec.kind )
> +            req->fault_with_gla = 1;
> +        req->vcpu_id = v->vcpu_id;
> +
> +        mem_access_send_req(v->domain, req);
> +        xfree(req);
> +    }
> +
> +    /* Pause the current VCPU */
> +    if ( xma != XENMEM_access_n2rwx )
> +        mem_event_vcpu_pause(v);
> +
> +    return false;
> +}
> +
> +/* Set access type for a region of pfns.
> + * If start_pfn == -1ul, sets the default access type */
> +long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
> +                        uint32_t start, uint32_t mask, xenmem_access_t access)

and this function is nearly identical to the x86 one too.

> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    p2m_access_t a;
> +    long rc = 0;
> +
> +    static const p2m_access_t memaccess[] = {
> +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
> +        ACCESS(n),
> +        ACCESS(r),
> +        ACCESS(w),
> +        ACCESS(rw),
> +        ACCESS(x),
> +        ACCESS(rx),
> +        ACCESS(wx),
> +        ACCESS(rwx),
> +        ACCESS(rx2rw),
> +        ACCESS(n2rwx),
> +#undef ACCESS
> +    };
> +
> +    switch ( access )
> +    {
> +    case 0 ... ARRAY_SIZE(memaccess) - 1:
> +        a = memaccess[access];
> +        break;
> +    case XENMEM_access_default:
> +        a = p2m->default_access;
> +        break;
> +    default:
> +        return -EINVAL;
> +    }
> +
> +    /*
> +     * Flip mem_access_enabled to true when a permission is set, as to prevent
> +     * allocating or inserting super-pages.
> +     */
> +    p2m->mem_access_enabled = true;
> +
> +    /* If request to set default access. */
> +    if ( pfn == ~0ul )
> +    {
> +        p2m->default_access = a;
> +        return 0;
> +    }
> +
> +    rc = apply_p2m_changes(d, MEMACCESS,
> +                           pfn_to_paddr(pfn+start), pfn_to_paddr(pfn+nr),
> +                           0, MATTR_MEM, mask, 0, a);
> +    if ( rc < 0 )
> +        return rc;
> +    else if ( rc > 0 )
> +        return start + rc;
> +
> +    return 0;
> +}
> +
> +int p2m_get_mem_access(struct domain *d, unsigned long gpfn,
> +                       xenmem_access_t *access)
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    void *i;
> +    unsigned int index;
> +
> +    static const xenmem_access_t memaccess[] = {

Would be nice not to have this static const thing twice in .rodata.

> +#define ACCESS(ac) [p2m_access_##ac] = XENMEM_access_##ac
> +            ACCESS(n),
> +            ACCESS(r),
> +            ACCESS(w),
> +            ACCESS(rw),
> +            ACCESS(x),
> +            ACCESS(rx),
> +            ACCESS(wx),
> +            ACCESS(rwx),
> +            ACCESS(rx2rw),
> +            ACCESS(n2rwx),
> +#undef ACCESS
> +    };
> +
> +    /* If no setting was ever set, just return rwx. */
> +    if ( !p2m->mem_access_enabled )
> +    {
> +        *access = XENMEM_access_rwx;
> +        return 0;
> +    }
> +
> +    /* If request to get default access */
> +    if ( gpfn == ~0ull )

We should have a suitable constant for this, I think, INVALID_MFN looks
like the one.

> +    {
> +        *access = memaccess[p2m->default_access];
> +        return 0;
> +    }
> +
> +    spin_lock(&p2m->lock);
> +    i = radix_tree_lookup(&p2m->mem_access_settings, gpfn);
> +    spin_unlock(&p2m->lock);
> +
> +    if ( !i )
> +    {
> +        /*
> +         * No setting was found in the Radix tree. Check if the
> +         * entry exists in the page-tables.
> +         */
> +        paddr_t maddr = p2m_lookup(d, gpfn << PAGE_SHIFT, NULL);
> +        if ( INVALID_PADDR == maddr )
> +            return -ESRCH;
> +
> +        /* If entry exists then its rwx. */

it's, please

> +        *access = XENMEM_access_rwx;
> +    }
> +    else
> +    {
> +        /* Setting was found in the Radix tree. */
> +        index = radix_tree_ptr_to_int(i);
> +        if ( index >= ARRAY_SIZE(memaccess) )
> +            return -ERANGE;
> +
> +        *access = memaccess[index];
> +    }
> +
> +    return 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C

Ian.