|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC 12/22] xen/arm: p2m: Introduce p2m_get_entry and use it to implement __p2m_lookupo
On Wed, 31 Aug 2016, Julien Grall wrote:
> > > @@ -236,28 +238,99 @@ static lpae_t *p2m_get_root_pointer(struct
> > > p2m_domain *p2m,
> > >
> > > /*
> > > * Lookup the MFN corresponding to a domain's GFN.
> > > + * Lookup mem access in the ratrix tree.
> > > + * The entries associated to the GFN is considered valid.
> > > + */
> > > +static p2m_access_t p2m_mem_access_radix_get(struct p2m_domain *p2m,
> > > gfn_t gfn)
> > > +{
> > > + void *ptr;
> > > +
> > > + if ( !p2m->mem_access_enabled )
> > > + return p2m_access_rwx;
> >
> > Shouldn't this be p2m->default_access?
>
> default_access will always be p2m_access_rwx when memaccess is disabled. It
> will lead to crash a if you try to restrict permission without memaccess.
>
> Note that, this is matching the behavior of __p2m_get_mem_access.
I would like to avoid some places to use p2m_access_rwx and others to
use p2m->default_access. But it is true that in this context both are
fine. This was just a suggestion.
> > > + ptr = radix_tree_lookup(&p2m->mem_access_settings, gfn_x(gfn));
> > > + if ( !ptr )
> > > + return p2m_access_rwx;
> >
> > Same here?
>
> The radix tree will contain all the permission restriction but p2m_access_rwx.
> This is because you may change the default_access whilst memaccess is enabled
> and you don't know what page was restricted with the default access.
>
> Note that this is matching the behavior of p2m_mem_access_radix_set.
>
> >
> >
> > > + else
> > > + return radix_tree_ptr_to_int(ptr);
> > > +}
> > > +
> > > +#define GUEST_TABLE_MAP_FAILED 0
> > > +#define GUEST_TABLE_SUPER_PAGE 1
> > > +#define GUEST_TABLE_NORMAL_PAGE 2
> > > +
> > > +static int p2m_create_table(struct p2m_domain *p2m, lpae_t *entry,
> > > + int level_shift);
> > > +
> > > +/*
> > > + * Take the currently mapped table, find the corresponding GFN entry,
> > > + * and map the next table, if available.
> >
> > It is important to write down that the function also unmaps the previous
> > table.
>
> Will do.
>
> >
> > > *
> > > - * There are no processor functions to do a stage 2 only lookup therefore
> > > we
> > > - * do a a software walk.
> > > + * Return values:
> > > + * GUEST_TABLE_MAP_FAILED: Either read_only was set and the entry
> > > + * was empty, or allocating a new page failed.
> > > + * GUEST_TABLE_NORMAL_PAGE: next level mapped normally
> > > + * GUEST_TABLE_SUPER_PAGE: The next entry points to a superpage.
> > > */
> > > -static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
> > > +static int p2m_next_level(struct p2m_domain *p2m, bool read_only,
> > > + lpae_t **table, unsigned int offset)
> > > {
> > > - struct p2m_domain *p2m = &d->arch.p2m;
> > > - const paddr_t paddr = pfn_to_paddr(gfn_x(gfn));
> > > - const unsigned int offsets[4] = {
> > > - zeroeth_table_offset(paddr),
> > > - first_table_offset(paddr),
> > > - second_table_offset(paddr),
> > > - third_table_offset(paddr)
> > > - };
> > > - const paddr_t masks[4] = {
> > > - ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK
> > > - };
> > > - lpae_t pte, *map;
> > > + lpae_t *entry;
> > > + int ret;
> > > + mfn_t mfn;
> > > +
> > > + entry = *table + offset;
> > > +
> > > + if ( !p2m_valid(*entry) )
> > > + {
> > > + if ( read_only )
> > > + return GUEST_TABLE_MAP_FAILED;
> > > +
> > > + ret = p2m_create_table(p2m, entry, /* not used */ ~0);
> > > + if ( ret )
> > > + return GUEST_TABLE_MAP_FAILED;
> > > + }
> > > +
> > > + /* The function p2m_next_level is never called at the 3rd level */
> > > + if ( p2m_mapping(*entry) )
> > > + return GUEST_TABLE_SUPER_PAGE;
> > > +
> > > + mfn = _mfn(entry->p2m.base);
> > > +
> > > + unmap_domain_page(*table);
> > > + *table = map_domain_page(mfn);
> > > +
> > > + return GUEST_TABLE_NORMAL_PAGE;
> >
> > I am a bit worried about having the same function doing the lookup and
> > creating new tables, especially given it doesn't tell you whether the
> > entry was already there or it was created: the return value is the same
> > in both cases. At the very least the return values should be different.
>
> I don't understand your worry here. Why would you care that the table has been
> allocated or was already existing?
>
> If the caller does not want to allocate table, it can request to browse the
> entry in a read-only mode (see read_only).
To avoid unintentional side effects, such as calling this function for a
lookup but passing read-only as false by mistake. But maybe we just need
to be careful enough in the code reviews, after all this function won't
be called that many times.
> > > +}
> > > +
> > > +/*
> > > + * Get the details of a given gfn.
> > > + *
> > > + * If the entry is present, the associated MFN will be returned and the
> > > + * access and type filled up. The page_order will correspond to the
> > > + * order of the mapping in the page table (i.e it could be a superpage).
> > > + *
> > > + * If the entry is not present, INVALID_MFN will be returned and the
> > > + * page_order will be set according to the order of the invalid range.
> > > + */
> > > +static mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
> > > + p2m_type_t *t, p2m_access_t *a,
> > > + unsigned int *page_order)
> > > +{
> > > + paddr_t addr = pfn_to_paddr(gfn_x(gfn));
> > > + unsigned int level = 0;
> > > + lpae_t entry, *table;
> > > + int rc;
> > > mfn_t mfn = INVALID_MFN;
> > > - paddr_t mask = 0;
> > > p2m_type_t _t;
> > > - unsigned int level;
> > > +
> > > + /* Convenience aliases */
> > > + const unsigned int offsets[4] = {
> > > + zeroeth_table_offset(addr),
> > > + first_table_offset(addr),
> > > + second_table_offset(addr),
> > > + third_table_offset(addr)
> > > + };
> > >
> > > ASSERT(p2m_is_locked(p2m));
> > > BUILD_BUG_ON(THIRD_MASK != PAGE_MASK);
> > > @@ -267,46 +340,75 @@ static mfn_t __p2m_lookup(struct domain *d, gfn_t
> > > gfn, p2m_type_t *t)
> > >
> > > *t = p2m_invalid;
> > >
> > > - map = p2m_get_root_pointer(p2m, gfn);
> > > - if ( !map )
> > > - return INVALID_MFN;
> > > + /* XXX: Check if the mapping is lower than the mapped gfn */
> > >
> > > - ASSERT(P2M_ROOT_LEVEL < 4);
> > > -
> > > - for ( level = P2M_ROOT_LEVEL ; level < 4 ; level++ )
> > > + /* This gfn is higher than the highest the p2m map currently holds */
> > > + if ( gfn_x(gfn) > gfn_x(p2m->max_mapped_gfn) )
> > > {
> > > - mask = masks[level];
> > > -
> > > - pte = map[offsets[level]];
> > > + for ( level = P2M_ROOT_LEVEL; level < 3; level++ )
> > > + {
> > > + if ( (gfn_x(gfn) & (level_masks[level] >> PAGE_SHIFT)) >
> > > + gfn_x(p2m->max_mapped_gfn) )
> > > + break;
> > > + goto out;
> >
> > I am not sure what this loop is for, but it looks wrong.
>
> As mentioned in the description of the function, if the entry is not present,
> the function will return the order of the invalid range.
>
> The loop will find the highest possible order by checking the base of the
> block mapping is greater than the max mapped gfn.
All right, but shouldn't the `goto out` be right after the loop?
Otherwise it is not really a loop :-)
> > > + }
> > > + }
> > >
> > > - if ( level == 3 && !p2m_table(pte) )
> > > - /* Invalid, clobber the pte */
> > > - pte.bits = 0;
> > > - if ( level == 3 || !p2m_table(pte) )
> > > - /* Done */
> > > - break;
> > > + table = p2m_get_root_pointer(p2m, gfn);
> > >
> > > - ASSERT(level < 3);
> > > + /*
> > > + * the table should always be non-NULL because the gfn is below
> > > + * p2m->max_mapped_gfn and the root table pages are always present.
> > > + */
> > > + BUG_ON(table == NULL);
> > >
> > > - /* Map for next level */
> > > - unmap_domain_page(map);
> > > - map = map_domain_page(_mfn(pte.p2m.base));
> > > + for ( level = P2M_ROOT_LEVEL; level < 3; level++ )
> > > + {
> > > + rc = p2m_next_level(p2m, true, &table, offsets[level]);
> > > + if ( rc == GUEST_TABLE_MAP_FAILED )
> > > + goto out_unmap;
> > > + else if ( rc != GUEST_TABLE_NORMAL_PAGE )
> > > + break;
> > > }
> > >
> > > - unmap_domain_page(map);
> > > + entry = table[offsets[level]];
> > >
> > > - if ( p2m_valid(pte) )
> > > + if ( p2m_valid(entry) )
> > > {
> > > - ASSERT(mask);
> > > - ASSERT(pte.p2m.type != p2m_invalid);
> > > - mfn = _mfn(paddr_to_pfn((pte.bits & PADDR_MASK & mask) |
> > > - (paddr & ~mask)));
> > > - *t = pte.p2m.type;
> > > + *t = entry.p2m.type;
> >
> > Why don't you have a check for ( t ) like you have done in the case of
> > ( a )? It would be more consistent.
>
> This is done at the beginning of the function:
>
> /* Allow t to be NULL */
> t = t ?: &_t;
>
> *t = p2m_invalid;
>
> This was kept from the implementation of __p2m_lookup because some callers
> check the type before checking if the MFN is invalid (e.g get_page_from_gfn).
>
> I didn't follow the same principle for the access because going through the
> radix tree is expensive and most of the hot path does not care about the
> access.
Makes sense.
> > > +
> > > + if ( a )
> > > + *a = p2m_mem_access_radix_get(p2m, gfn);
> > > +
> > > + mfn = _mfn(entry.p2m.base);
> > > + /*
> > > + * The entry may point to a superpage. Find the MFN associated
> > > + * to the GFN.
> > > + */
> > > + mfn = mfn_add(mfn, gfn_x(gfn) & ((1UL << level_orders[level]) -
> > > 1));
> > > }
> > >
> > > +out_unmap:
> > > + unmap_domain_page(table);
> > > +
> > > +out:
> > > + if ( page_order )
> > > + *page_order = level_shifts[level] - PAGE_SHIFT;
> > > +
> > > return mfn;
> > > }
> > >
> > > +/*
> > > + * Lookup the MFN corresponding to a domain's GFN.
> > > + *
> > > + * There are no processor functions to do a stage 2 only lookup therefore
> > > we
> > > + * do a a software walk.
> > > + */
> > > +static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
> > > +{
> > > + return p2m_get_entry(&d->arch.p2m, gfn, t, NULL, NULL);
> > > +}
> > > +
> > > mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
> > > {
> > > mfn_t ret;
> > > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> > > index 05d9f82..1c5bd8b 100644
> > > --- a/xen/include/asm-arm/page.h
> > > +++ b/xen/include/asm-arm/page.h
> > > @@ -457,15 +457,19 @@ static inline int gva_to_ipa(vaddr_t va, paddr_t
> > > *paddr, unsigned int flags)
> > > #define LPAE_ENTRY_MASK (LPAE_ENTRIES - 1)
> > >
> > > #define THIRD_SHIFT (PAGE_SHIFT)
> > > +#define THIRD_ORDER 0
> > > #define THIRD_SIZE ((paddr_t)1 << THIRD_SHIFT)
> > > #define THIRD_MASK (~(THIRD_SIZE - 1))
> > > #define SECOND_SHIFT (THIRD_SHIFT + LPAE_SHIFT)
> > > +#define SECOND_ORDER (THIRD_ORDER + LPAE_SHIFT)
> > > #define SECOND_SIZE ((paddr_t)1 << SECOND_SHIFT)
> > > #define SECOND_MASK (~(SECOND_SIZE - 1))
> > > #define FIRST_SHIFT (SECOND_SHIFT + LPAE_SHIFT)
> > > +#define FIRST_ORDER (SECOND_ORDER + LPAE_SHIFT)
> > > #define FIRST_SIZE ((paddr_t)1 << FIRST_SHIFT)
> > > #define FIRST_MASK (~(FIRST_SIZE - 1))
> > > #define ZEROETH_SHIFT (FIRST_SHIFT + LPAE_SHIFT)
> > > +#define ZEROETH_ORDER (FIRST_ORDER + LPAE_SHIFT)
> > > #define ZEROETH_SIZE ((paddr_t)1 << ZEROETH_SHIFT)
> > > #define ZEROETH_MASK (~(ZEROETH_SIZE - 1))
> >
> > It might be clearer to define them by SHIFT:
> >
> > #define THIRD_ORDER (THIRD_SHIFT - PAGE_SHIFT)
> > #define SECOND_ORDER (SECOND_SHIFT - PAGE_SHIFT)
> > #define FIRST_ORDER (FIRST_SHIFT - PAGE_SHIFT)
> > #define ZEROETH_ORDER (ZEROETH_SHIFT - PAGE_SHIFT)
>
> I was following the way the other constant has been defined. The order of the
> 2nd level can be expressed in term of the order of the 3rd level...
>
> I don't think this is clearer, but I don't mind to use them.
>
> > or avoid them and just use level_shifts which is already defined. I
> > don't think they add much value.
>
> Really? It avoids to spread - {PAGE,LPAE}_SHIFT everywhere in the code so the
> code will be cleaner.
>
> Note that I have noticed that there are still few places where (level_shifts -
> PAGE_SHIFT) is still in use. However, we can replace by level_orders.
The reason why I suggested the alternative implementation is that
"order" is not commonly used when dealing with pagetable levels (while
"mask" and "shift" are). I had a guess about what it meant, but wasn't
sure. To be sure I had to read the implementation. I think this version
is more obvious about what it does. That said, this is just a
suggestion, not a requirement.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |