Re: [PATCH for-4.5 v8 12/19] xen/arm: p2m changes for mem_access support

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

On 09/23/2014 02:14 PM, Tamas K Lengyel wrote:
> +/* 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;

In the context of this patch, this clearly looks a spurious change.

As said on a previous version, moving this enum only for style is not a
good reason. It makes more difficult to use git-blame.

Please avoid to move this enum.

> +
> +/* Look up a GFN and take a reference count on the backing page. */
> +typedef unsigned int p2m_query_t;
> +#define P2M_ALLOC    (1u<<0)   /* Populate PoD and paged-out entries */
> +#define P2M_UNSHARE  (1u<<1)   /* Break CoW sharing */
> +
>  /* Per-p2m-table state */
>  struct p2m_domain {
>      /* Lock that protects updates to the p2m */
> @@ -48,27 +75,20 @@ struct p2m_domain {
>      /* If true, and an access fault comes in and there is no mem_event listener,
>       * pause domain. Otherwise, remove access restrictions. */
>      bool_t access_required;
> -};
>  
> -/* 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;
> +    /* Defines if mem_access is in use for the domain to avoid uneccessary radix

unecessary

> +     * lookups. */
> +    bool_t access_in_use;

Actually, you are using this boolean for more than avoid lookup to the
radix. I would drop the end of the sentence i.e: "to avoid uneccessary
radix lookups".

I gave a look to the radix tree lookups functions. Even if it's not
obvious, those functions have nearly the same cost as checking a boolean
when the radix tree is empty.

It might be interesting to see if we can avoid some if
(p2m->access_in_use) check in the p2m code.

Regards,


-- 
Julien Grall