Xen project Mailing List

On Thu, Sep 18, 2014 at 8:54 PM, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:

On Mon, 2014-09-15 at 16:02 +0200, Tamas K Lengyel wrote:

> - if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) )
> + if ( level < 3 && p2m_access_rwx != a )
> + {
> + /* We create only 4k pages when mem_access is in use. */

I wonder if it might turn out cleaner to integrate this check into
is_mapping_aligned (which really is more of a "can we use a superpage"
function).

i.e.
/* mem access cannot use super pages */
if ( a != p2m_access_rwx && level_size != THIRD_SIZE )
return false;

Ack, that would make sense.

> + }
> + else if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) )
> {
> struct page_info *page;
>
> page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0);
> if ( page )
> {
> + if ( 3 == level )

Please write the conditionals the other way around.

I tend to write equality tests this way where one side of the test is a constant. It has happened in the past that such equality tests were used as a cover in attempting to sneak bugdoors into Linux (see https://freedom-to-tinker.com/blog/felten/the-linux-backdoor-attempt-of-2003 for more info).

> + {
> + rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a);
> + if ( rc < 0 )
> + {
> + free_domheap_page(page);
> + return rc;
> + }

> + }
> + else
> + {
> + a = p2m_access_rwx;
> + }

You have this else clause twice, I think you could pull it up to the
head of the function, or perhaps even into the caller.

The radix tree approach has been retired in v7 as I found the page_info approach you suggested more palatable. Thus this entire block is gone in the latest revision.

> @@ -627,15 +741,11 @@ static int apply_one_level(struct domain *d,
> * and descend.
> */
> *flush = true;
> - rc = p2m_create_table(d, entry,
> - level_shift - PAGE_SHIFT, flush_cache);
> + rc = p2m_shatter_page(d, entry, level, level_shift, flush_cache);
> +

Please keep the error handling if snuggled against the function, (i.e.
drop the additional blank line) here and in at least one other place
which you've changed.

Ack, I think this already was fixed in v7.

> @@ -704,6 +815,49 @@ static int apply_one_level(struct domain *d,
> *addr += PAGE_SIZE;
> return P2M_ONE_PROGRESS_NOP;
> }
> +
> + case MEMACCESS:
> + if ( level < 3 )
> + {
> + if ( !p2m_valid(orig_pte) )
> + {
> + *addr += level_size;
> + return P2M_ONE_PROGRESS_NOP;
> + }
> +
> + /* Shatter large pages as we descend */
> + if ( p2m_mapping(orig_pte) )
> + {
> + rc = p2m_shatter_page(d, entry, level, level_shift, flush_cache);
> +
> + if ( rc < 0 )
> + return rc;
> + } /* else: an existing table mapping -> descend */
> +
> + return P2M_ONE_DESCEND;
> + }
> + else
> + {
> + pte = orig_pte;
> +
> + if ( !p2m_table(pte) )
> + pte.bits = 0;
> +
> + if ( p2m_valid(pte) )
> + {
> + ASSERT(pte.p2m.type != p2m_invalid);
> +
> + rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a);
> + if ( rc < 0 )
> + return rc;
> +
> + p2m_set_permission(&pte, pte.p2m.type, a);

I think this function can always make use of pte.p2m.type itself rather
than receiving it as a parameter. The other caller passes "t" but has
already assigned that to pte.p2m.type as well.

I find the approach as it is right now a bit more straight forward as all the inputs that affect the permissions are explicitly there as inputs. I'll switch it to what you suggest if you feel strongly about it though.

>
> - rc = gva_to_ipa(info.gva, &info.gpa);
> - if ( rc == -EFAULT )
> + switch ( dabt.dfsc )
> + {
> + case DABT_DFSC_PERMISSION_1:
> + case DABT_DFSC_PERMISSION_2:
> + case DABT_DFSC_PERMISSION_3:

Eventually this will need to handle level 0 too. Would it work to mask
out the level bits and check the remainder against the common bit
pattern?

From a performance perspective my understanding is that bitmasks under-perform pure numeric comparisons. I see that with the FSC_FLT defines the masking approach has been taken which I guess I'll switch to as to reduce bloating but it might be worth taking a second look at.

> +/* Data abort data fetch status codes */
> +enum dabt_dfsc {
> + DABT_DFSC_ADDR_SIZE_0 = 0b000000,

Unfortunately I think 0b... is a gcc extension and not standard C
(please correct me if I'm wrong). In which case we should probably avoid
it and use hex instead.

It's also supported by clang but you are right.

Actually, isn't this partially duplicating the existing FSC_* defines?
We should either use those here or move the existing users over to the
new scheme.

Ack. Somehow I was oblivious to the fact we already have defines for these.. I guess I was looking for the values defined in the manual in 0b form. Either way, since we already have defines for these and I only care about the permission defines anyway, I'll drop the new enums and use the approach that's in place already.

Thanks!
Tamas

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.5 v6 13/17] xen/arm: Data abort exception (R/W) mem_events.