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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Fri, Aug 29, 2014 at 11:41 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:

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.

The Radix tree is only updated in the caller in if this function returns 1. In the next iteration of the series I already added a separate function for this, p2m_set_entry which doesn't lock so p2m_lookup will be unchanged.

 

[..]

      {
          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?

It really only happened because of large pages in the table, as I was passing paddr inputs based on 4k page boundaries. I already started working on shattering the large pages as I'm setting the permissions so this will go away in the next iteration.

 

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

Coding style

if ( ... )

Ack.
 

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

Ack.

 

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

I would need to adjust this function in the x86 side as well where I think it may be returning -errno if something is wrong.

 

+{
+    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?

I didn't add it in this iteration yet (and n2rwx, rx2rw are also missing). I have them already in the next iteration which I will be sending in Monday after I go back to my office to reset my hung arndale to test it :-)

 

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

switch ( ... )

Ack.
 

+    {
+        default:

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

Just in case I guess? I'm not entirely certain, this is pretty much just copy-pasta from the x86 side.

 

+        case XENMEM_access_n:

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

{
case ...:

Ack.

 

+            violation = access_r || access_w || access_x;

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

They were bool_t inputs into the function passed from the trap handler in traps.c. In the next iteration they are going away and will be using the new combined struct npfec input from my other patch that just landed in staging.

 

[..]

+    if (!violation)

if ( ... )

Ack.
 

+        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?

I'm not to happy with this either but that's what the x86 side is doing too. I would prefer it retrying to deliver the notification but keeping the offending domain paused in the meanwhile. Not entirely sure how to go about that though as I assume if xzalloc fails we have other things to worry about already.

 

+
+    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 [ ?

Ack.

 

Regards,

--
Julien Grall

Thanks!

Tamas

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