Xen project Mailing List

On Fri, Sep 12, 2014 at 10:35 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:

Hello Tamas,

On 12/09/14 01:46, Tamas K Lengyel wrote:

/* New mapping is superpage aligned, make it */
pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT,
mattr, t, a);
if ( level < 3 )
@@ -663,6 +737,7 @@ static int apply_one_level(struct domain *d,

memset(&pte, 0x00, sizeof(pte));
p2m_write_pte(entry, pte, flush_cache);
+ radix_tree_delete(&p2m->mem___access_settings,
paddr_to_pfn(*addr));

*addr += level_size;
*maddr += level_size;
@@ -707,6 +782,53 @@ 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)++;

Why increment by 1? You the PTE doesn't contain valid mapping you
want to skip the whole level range. ie:

*addr += level_size;

It doesn't make a difference, apply_p2m_changes is called with
start=paddr, end=paddr+1 from a separate loop. So just incrementing it
by one or a whole level achieves the same effect, that is, the
apply_p2m_changes loop breaks.

Actually it makes a lots of difference. If you increment by 1 the address, you will call up to level_size time your code before effectively going to the next level entry.

This function can be called with *multiple page*.

It can be, but it isn't. It makes no difference form my perspective so I'm fine with it either way.

+ return P2M_ONE_PROGRESS_NOP;
+ }
+
+ /* Shatter large pages as we descend */
+ if ( p2m_mapping(orig_pte) )
+ {
+ rc = p2m_create_table(d, entry,
+ level_shift - PAGE_SHIFT,
flush_cache);
+ if ( rc < 0 )
+ return rc;
+
+ p2m->stats.shattered[level]++;
+ p2m->stats.mappings[level]--;
+ p2m->stats.mappings[level+1] += LPAE_ENTRIES;
+ } /* else: an existing table mapping -> descend */
+

This piece of code is exactly the same in INSERT, REMOVE and now
MEMACCESS. I would create an helper to shatter and update the stats.

Ack.

+ 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);
+ p2m_write_pte(entry, pte, flush_cache);
+ }
+
+ (*addr)++;

*addr += PAGE_SIZE;

[..]

+/* Set access type for a region of pfns.
+ * If start_pfn == -1ul, sets the default access type */
+long p2m_set_mem_access(struct domain *d, unsigned long pfn,
uint32_t nr,
+ uint32_t start, uint32_t mask,
xenmem_access_t access)
+{
+ struct p2m_domain *p2m = p2m_get_hostp2m(d);
+ p2m_access_t a;
+ long rc = 0;
+ paddr_t paddr;
+
+ static const p2m_access_t memaccess[] = {
+#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
+ ACCESS(n),
+ ACCESS(r),
+ ACCESS(w),
+ ACCESS(rw),
+ ACCESS(x),
+ ACCESS(rx),
+ ACCESS(wx),
+ ACCESS(rwx),
+#undef ACCESS
+ };
+
+ switch ( access )
+ {
+ case 0 ... ARRAY_SIZE(memaccess) - 1:
+ a = memaccess[access];
+ break;
+ case XENMEM_access_default:
+ a = p2m->default_access;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ /* If request to set default access */
+ if ( pfn == ~0ul )
+ {
+ p2m->default_access = a;
+ return 0;
+ }
+
+ for ( pfn += start; nr > start; ++pfn )
+ {
+ paddr = pfn_to_paddr(pfn);
+ rc = apply_p2m_changes(d, MEMACCESS, paddr, paddr+1, 0,
MATTR_MEM, 0, a);

Hmmm... why didn't you call directly apply_p2m_changes with the
whole range?

Because the hypercall continuation. Setting mem_access permissions needs
to be preemptible and it has its own separate routine to do that here.
See http://xenbits.xen.org/xsa/advisory-89.html for more info.

We do have hypercall continuation in apply_p2m_changes (see for relinquish). Please do the same for MEMACCESS rather than using your own loop.

Hence, with your solution, the p2m lookup is taken/released at each loop. This is inefficient.

It is a bit less efficient for sure. However, the continuation setup for mem_access is in common/mem_access for XENMEM_access_op_set_access. In order to make use of apply_p2m_change to setup continuation I would have to remove that code from common and make it arch specific in both ARM and x86. Is that really a good idea just to have this optimization in ARM?

Regards,

--
Julien Grall

Re: [Xen-devel] [PATCH v5 12/17] xen/arm: Data abort exception (R/W) mem_events.