[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 7/8] xen: arm: use superpages in p2m when pages are suitably aligned

Hi Ian,

On 13/06/14 15:08, Ian Campbell wrote:
On Thu, 2014-06-12 at 10:00 +0100, Julien Grall wrote:

On 12/06/14 08:30, Ian Campbell wrote:
On Wed, 2014-06-11 at 23:19 +0100, Julien Grall wrote:
Hi Ian,

While I was looking closer to this patch I found something strange. Why
all the callers of guest_physmap_add_page in the directory common don't
check that the function success to create the mapping?

"directory common"? I don't get your meaning.

Sorry, I meant xen/common/

I don't know the answer then.

CC Jan and Keir. I hope one of them have a clue on this.


+    case INSERT:
+        if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size) &&
+           /* We do not handle replacing an existing table with a superpage */
+             (level == 3 || !p2m_table(orig_pte)) )
+        {
+            /* New mapping is superpage aligned, make it */
+            pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t);
+            if ( level < 3 )

It's funny, sometimes you use level < 3 and some level != 3 (see in


I think this pte.p2m.table set can be handled directly in
mfn_to_p2m_entry. This will avoid duplicating code.

It can't because mfn_to_p2m_entry is used to create both table and
mapping style entries.

There is only on call where we don't override the pte.p2m.table bit (the
one at the end of p2m_create table).

All of those other places *conditionally* set table.

I would move this extra test in the mfn_to_p2m_entry and override only
for this specific case.

mfn_to_p2m_entry doesn't know either the level or whether the intention
of the caller is to create a table or a mapping style entry.

AFAIU, you add/remove every callers of this function in this patch. Extending this function (or adding a new helper) would avoid to copy few time the same check.

-        /* Got the next page */
-        addr += PAGE_SIZE;
+        rc = apply_one_level(d, &third[third_table_offset(addr)],
+                             3, flush_pt, op,
+                             start_gpaddr, end_gpaddr,
+                             &addr, &maddr, &flush,
+                             mattr, t);
+        if ( rc < 0 ) goto out;

Shall we redo the whole range if the mapping has failed here?


Undo sorry.

I'm not sure, you could argue it either way I think. Is Arianna's series
dealing with this in some way?

I think she handled it only for MMIO. I will have to look again into her series.

Anyway, I think changing that behaviour would be a separate patch.

I didn't intend to ask you to undo the mapping in this patch, nor in this series.

I was just wondering what could happen if we fail to map the whole mapping. Hence, some callers of guest_physmap_add_page is not checking the return of this function. It seems that we may end up to partially map the range in guest and crash it.

Anyway, I don't think it's too bad as the guest will likely fail later if we fail to map the region. The only issue is that this won't be trivial to find the source as we don't have any error message.


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.