Re: [PATCH RFC v2 09/12] xen/arm: Data abort exception (R/W) mem_events.

[Julien Grall <julien.grall@linaro.org>]

Hello Tamas,

I've not yet finished to reviewed entirely this patch but I'm at least 
send thoses comments.

On 27/08/14 10:06, Tamas K Lengyel wrote:
>   /*
>    * Lookup the MFN corresponding to a domain's PFN.
>    *
>    * There are no processor functions to do a stage 2 only lookup therefore we
>    * do a a software walk.
> + *
> + * [IN]:  d      Domain
> + * [IN]:  paddr  IPA
> + * [IN]:  a      (Optional) Update PTE access permission

It's very confusing to update the access permission in p2m_lookup. Why 
didn't you add a new function?

Hence, you only update the PTE here and not the radix tree.

[..]

>       {
>           ASSERT(pte.p2m.type != p2m_invalid);
>           maddr = (pte.bits & PADDR_MASK & mask) | (paddr & ~mask);
> +        ASSERT(mfn_valid(maddr>>PAGE_SHIFT));
> +
> +        if ( a )
> +        {
> +            p2m_set_permission(&pte, pte.p2m.type, *a);
> +
> +            /* Only write the PTE if the access permissions changed */

Does this happen often? I'm wondering if we could always write the pte 
no matter if the permission as changed or not.

If it happen often, maybe just checking the access type has changed 
would be a good solution?

> +            if(pte.p2m.read != pte_loc->p2m.read

Coding style

if ( ... )

> +               || pte.p2m.write != pte_loc->p2m.write
> +               || pte.p2m.xn != pte_loc->p2m.xn)
> +            {
> +                p2m_write_pte(pte_loc, pte, 1);
> +            }
> +        }
> +

>           *t = pte.p2m.type;
>       }
>
> @@ -208,8 +308,6 @@ done:
>       if (first) unmap_domain_page(first);
>
>   err:
> -    spin_unlock(&p2m->lock);
> -
>       return maddr;
>   }
>
> @@ -228,7 +326,7 @@ int p2m_pod_decrease_reservation(struct domain *d,
>   }
>
>   static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
> -                               p2m_type_t t)
> +                               p2m_type_t t, p2m_access_t a)
>   {
>       paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
>       /* sh, xn and write bit will be defined in the following switches
> @@ -258,37 +356,7 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
>           break;
>       }
>
> -    switch (t)
> -    {
> -    case p2m_ram_rw:
> -        e.p2m.xn = 0;
> -        e.p2m.write = 1;
> -        break;
> -
> -    case p2m_ram_ro:
> -        e.p2m.xn = 0;
> -        e.p2m.write = 0;
> -        break;
> -
> -    case p2m_iommu_map_rw:
> -    case p2m_map_foreign:
> -    case p2m_grant_map_rw:
> -    case p2m_mmio_direct:
> -        e.p2m.xn = 1;
> -        e.p2m.write = 1;
> -        break;
> -
> -    case p2m_iommu_map_ro:
> -    case p2m_grant_map_ro:
> -    case p2m_invalid:
> -        e.p2m.xn = 1;
> -        e.p2m.write = 0;
> -        break;
> -
> -    case p2m_max_real_type:
> -        BUG();
> -        break;
> -    }
> +    p2m_set_permission(&e, t, a);
>
>       ASSERT(!(pa & ~PAGE_MASK));
>       ASSERT(!(pa & ~PADDR_MASK));
> @@ -298,13 +366,6 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
>       return e;
>   }
>
> -static inline void p2m_write_pte(lpae_t *p, lpae_t pte, bool_t flush_cache)
> -{
> -    write_pte(p, pte);
> -    if ( flush_cache )
> -        clean_xen_dcache(*p);
> -}
> -
>   /*
>    * Allocate a new page table page and hook it in via the given entry.
>    * apply_one_level relies on this returning 0 on success
> @@ -346,7 +407,7 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
>            for ( i=0 ; i < LPAE_ENTRIES; i++ )
>            {
>                pte = mfn_to_p2m_entry(base_pfn + (i<<(level_shift-LPAE_SHIFT)),
> -                                    MATTR_MEM, t);
> +                                    MATTR_MEM, t, p2m->default_access);
>
>                /*
>                 * First and second level super pages set p2m.table = 0, but
> @@ -366,7 +427,7 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
>
>       unmap_domain_page(p);
>
> -    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid);
> +    pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid, p2m->default_access);
>
>       p2m_write_pte(entry, pte, flush_cache);
>
> @@ -498,7 +559,7 @@ static int apply_one_level(struct domain *d,
>               page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0);
>               if ( page )
>               {
> -                pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t);
> +                pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a);
>                   if ( level < 3 )
>                       pte.p2m.table = 0;
>                   p2m_write_pte(entry, pte, flush_cache);
> @@ -533,7 +594,7 @@ static int apply_one_level(struct domain *d,
>                (level == 3 || !p2m_table(orig_pte)) )
>           {
>               /* New mapping is superpage aligned, make it */
> -            pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t);
> +            pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t, a);
>               if ( level < 3 )
>                   pte.p2m.table = 0; /* Superpage entry */
>
> @@ -640,6 +701,7 @@ static int apply_one_level(struct domain *d,
>
>           memset(&pte, 0x00, sizeof(pte));
>           p2m_write_pte(entry, pte, flush_cache);
> +        radix_tree_delete(&p2m->mem_access_settings, paddr_to_pfn(*addr));
>
>           *addr += level_size;
>
> @@ -1048,7 +1110,10 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
>
>   unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
>   {
> -    paddr_t p = p2m_lookup(d, pfn_to_paddr(gpfn), NULL);
> +    paddr_t p;

Missing blank line after the declaration block.

> +    spin_lock(&d->arch.p2m.lock);
> +    p = p2m_lookup(d, pfn_to_paddr(gpfn), NULL, NULL);
> +    spin_unlock(&d->arch.p2m.lock);
>       return p >> PAGE_SHIFT;
>   }
>
> @@ -1080,6 +1145,241 @@ err:
>       return page;
>   }
>
> +int p2m_mem_access_check(paddr_t gpa, vaddr_t gla,
> +                          bool_t access_r, bool_t access_w, bool_t access_x,
> +                          bool_t ptw)

The 2 arguments lines should be aligned to p. IOW, you need to remove 
one space on the both lines.

Also, as the function return always 0 or 1 I would use a bool_t for the 
return value.

> +{
> +    struct vcpu *v = current;
> +    mem_event_request_t *req = NULL;
> +    xenmem_access_t xma;
> +    bool_t violation;
> +    int rc;
> +
> +    /* If we have no listener, nothing to do */
> +    if( !mem_event_check_ring( &v->domain->mem_event->access ) )

The spaces in the inner () are not necessary.

Also, don't you miss to check p2m->access_required?

> +    {
> +        return 1;
> +    }
> +
> +    rc = p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xma);
> +    if ( rc )
> +        return rc;
> +
> +    switch (xma)

switch ( ... )

> +    {
> +        default:

It looks like all the possible case of the enum has been defined below. 
Why do you define default as XENMEM_access_n?

> +        case XENMEM_access_n:

The "case" is usually aligned to "{". Such as

{
case ...:

> +            violation = access_r || access_w || access_x;

Silly question, where doess access_* comes from? I can't find any 
definition with CTAGS.

[..]

> +    if (!violation)

if ( ... )

> +        return 1;
> +
> +    req = xzalloc(mem_event_request_t);
> +    if ( req )
> +    {
> +        req->reason = MEM_EVENT_REASON_VIOLATION;
> +        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 = 1;
> +        req->access_r = access_r;
> +        req->access_w = access_w;
> +        req->access_x = access_x;
> +        req->vcpu_id = v->vcpu_id;
> +
> +        mem_event_vcpu_pause(v);
> +        mem_access_send_req(v->domain, req);
> +
> +        xfree(req);
> +
> +        return 0;
> +    }

Ignoring the access when Xen fails to allocate req sounds strange. 
Shouldn't you at least print a warning?

> +
> +    return 1;
> +}

[..]

> +int p2m_get_mem_access(struct domain *d, unsigned long gpfn,
> +                       xenmem_access_t *access)
> +{

[..]

> +    if ( (unsigned) index >= ARRAY_SIZE(memaccess) )
> +        return -ERANGE;
> +
> +    *access =  memaccess[ (unsigned) index];

Spurious space at after [ ?

Regards,

-- 
Julien Grall