Re: [Xen-devel] [PATCH v5 09/17] xen/arm: p2m type definitions and changes

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

On Thu, Sep 11, 2014 at 10:25 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:

Hi Tamas,

You skipped my comments/questions on v4.

On 10/09/14 06:28, Tamas K Lengyel wrote:

diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 08ce07b..b4ca86d 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -2,6 +2,9 @@

[..]

+#include <public/memory.h>
+#include <public/mem_event.h>

Why do you need those 2 includes?

<public/memory.h> might not be necessary in this patch yet, but it will be required for defining the functions that mem_access will call passing xenmem_access_t access. I can move the header include into the next patch in the series if that is cleaner. The mem_event header is not actually required so I'll remove it.

 

  #include <xen/p2m-common.h>

@@ -11,6 +14,48 @@ struct domain;

  extern void memory_type_changed(struct domain *);

+/* List of possible type for each page in the p2m entry.
+ * The number of available bit per page in the pte for this purpose is 4 bits.
+ * So it's possible to only have 16 fields. If we run out of value in the
+ * future, it's possible to use higher value for pseudo-type and don't store
+ * them in the p2m entry.
+ */
+typedef enum {
+    p2m_invalid = 0,    /* Nothing mapped here */
+    p2m_ram_rw,         /* Normal read/write guest RAM */
+    p2m_ram_ro,         /* Read-only; writes are silently dropped */
+    p2m_mmio_direct,    /* Read/write mapping of genuine MMIO area */
+    p2m_map_foreign,    /* Ram pages from foreign domain */
+    p2m_grant_map_rw,   /* Read/write grant mapping */
+    p2m_grant_map_ro,   /* Read-only grant mapping */
+    /* The types below are only used to decide the page attribute in the P2M */
+    p2m_iommu_map_rw,   /* Read/write iommu mapping */
+    p2m_iommu_map_ro,   /* Read-only iommu mapping */
+    p2m_max_real_type,  /* Types after this won't be store in the p2m */
+} p2m_type_t;

Any reason to move the enum earlier? If not, I would prefer to keep at the same place. It will be easier with git-blame to find when a new type has been added.

Stylistically it made more sense to have p2m_type_t and p2m_access_t together (as it is on the x86 as well). And they are defined here as the p2m_domain does have a field now defining default_access with p2m_access_t.

 

+/*
+ * Additional access types, which are used to further restrict
+ * the permissions given by the p2m_type_t memory type. Violations
+ * caused by p2m_access_t restrictions are sent to the mem_event
+ * interface.
+ *
+ * The access permissions are soft state: when any ambigious change of page

ambiguous.

Copy-pasta but will fix.

 

[..]

+    /* Default P2M access type for each page in the the domain: new pages,
+     * swapped in pages, cleared pages, and pages that are ambiquously

Did you intend to mean ambiguously rather than ambiquously?

Copy-pasta again but will fix. Maybe in a separate patch where I fix it here and in the x86 side as well?

 

[..]

+/* mem_event and mem_access are supported on all ARM */

I don't find "all ARM" clear. I would replace by "any ARM guest"

Ack.

Thanks!
Tamas

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