Xen project Mailing List

On Fri, Sep 12, 2014 at 9:23 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:

H Tamas,

On 12/09/14 01:15, Tamas K Lengyel wrote:

On Thu, Sep 11, 2014 at 10:25 PM, Julien Grall <julien.grall@xxxxxxxxxx
<mailto:julien.grall@linaro.org>> 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.

Yes please.

Ack.

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

If it's only for the style I wouldn't move them.

Anyway, I'll Ian/Stefano decides about this.

Sure.

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

I'm OK for a separate patch fixing x86 side, but there is no reason to fix the spelling for ARM outside this patch.

Ack, that's what I ended up doing already.

Regards,

--
Julien Grall

Thanks!
Tamas

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