Xen project Mailing List

On Wed, Sep 24, 2014 at 4:43 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:

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.

Ack, forgot to move it back after I abstracted p2m_access_t into p2m-common.

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

Ack.

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

Thanks for looking into it, that was my initial assumption as well. However, as the radix tree API lacks an is_empty() check, it is still somewhat nice to have this separate boolean for the checks in ALLOCATE/INSERT.

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

Regards,

--
Julien Grall

Thanks!
Tamas

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