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

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

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@linaro.org
> <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.

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

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

Regards,

-- 
Julien Grall