[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.