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

[Julien Grall <julien.grall@linaro.org>]

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?

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

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

[..]

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

[..]

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

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

Regards,

-- 
Julien Grall