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

Re: [Xen-devel] [PATCH v4 11/16] xen/arm: Data abort exception (R/W) mem_events.



Hi Tamas,

On 05/09/14 01:58, Tamas K Lengyel wrote:
  #define P2M_ONE_DESCEND        0
  #define P2M_ONE_PROGRESS_NOP   0x1
  #define P2M_ONE_PROGRESS       0x10
@@ -502,6 +568,14 @@ static int apply_one_level(struct domain *d,
                  pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a);
                  if ( level < 3 )
                      pte.p2m.table = 0;
+
+                if ( p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a) < 0 
)
+                {
+                    /* If we fail to save the setting in the Radix tree, we
+                     * need to clear the PTE mem_access permissions. */
+                    p2m_set_permission(&pte, pte.p2m.type, p2m_access_rwx);
+                }
+

[..]

@@ -538,6 +612,13 @@ static int apply_one_level(struct domain *d,
              if ( level < 3 )
                  pte.p2m.table = 0; /* Superpage entry */

+            if ( p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a) < 0 )
+            {
+                /* If we fail to save the setting in the Radix tree, we
+                 * need to clear the PTE mem_access permissions. */
+                p2m_set_permission(&pte, pte.p2m.type, p2m_access_rwx);
+            }
+

Can't this be abstract to another helper?

Also is it acceptable from mem access POV to unconditionally set the access to rwx when an error occured? Shouldn't you report an error?

+static int p2m_set_entry(struct domain *d, paddr_t paddr, p2m_access_t p2ma)
+{

As I said on the previous version, I still think this can be merge in apply_p2m_changes without any difficulties.

The main reason is to avoid increasing the number of function that have a common pattern to use the p2m. Also apply_p2m_changes have some interesting performance improvement (such as avoid map/unmap every time page table).

Finally, it will make life simpler when 4-Level page table will be supported (see Ian's series [1])

[..]

+bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, struct npfec npfec)

const struct npfect

[..]

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 019991f..7eb875a 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1852,13 +1852,38 @@ static void do_trap_data_abort_guest(struct 
cpu_user_regs *regs,

[..]

+    switch ( dabt.dfsc )
+    {
+    case DABT_DFSC_PERMISSION_1:
+    case DABT_DFSC_PERMISSION_2:
+    case DABT_DFSC_PERMISSION_3:
+    {
+        struct npfec npfec = {

const as you won't modify it later.

Regards,

[1] http://lists.xen.org/archives/html/xen-devel/2014-09/msg00596.html

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