On Wed, Sep 24, 2014 at 4:43 PM, Julien Grall <julien.grall@linaro.org>
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