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

Re: [Xen-devel] [PATCH 15/18] arm/altp2m: Add altp2m paging mechanism.





On 06/07/16 09:33, Sergej Proskurin wrote:
On 07/04/2016 10:53 PM, Julien Grall wrote:
Can we introduce a function which return the xma, mfn, order,
attribute at once? It will avoid to browse the p2m 3 times which is
really expensive on ARMv7 because the p2m is not mapped in the virtual
address space of Xen.


I was already thinking of at least merging p2m_get_gfn_level_and_attr
with __p2m_lookup. But it would also make sense to introduce an entirely
new function, which does just that.I believe increasing the overhead of
__p2m_lookup would not be a good solution. Thank you.

How the overhead of __p2m_lookup will be increased? level, mattr and type is already available in p2m_lookup. For the mem access, as you pass a pointer you can retrieve the access only when the pointer is not NULL.


+    spin_unlock(&hp2m->lock);
+
+    mask = level_masks[level];
+
+    rc = apply_p2m_changes(d, *ap2m, INSERT,
+                           pfn_to_paddr(gfn_x(gfn)) & mask,
+                           (pfn_to_paddr(gfn_x(gfn)) +
level_sizes[level]) & mask,
+                           maddr & mask, mattr, 0, p2mt,
+                           memaccess[xma]);

The page associated to the MFN is not locked, so another thread could
decide to remove the page from the domain and then the altp2m would
contain an entry to something that does not belong to the domain
anymore. Note that x86 is doing the same. So I am not sure why it is
considered safe there...


If I understand you correctly, unlocking the hp2m->lock after calling
apply_p2m_changeswould already solve this issue, right? Thanks.

That would solve it. However, you will add a lot of contention on hp2m when each vCPU is using a different empty view.


+    if ( rc )
+    {
+        gdprintk(XENLOG_ERR, "failed to set entry for %lx -> %lx p2m
%lx\n",
+                (unsigned long)pfn_to_paddr(gfn_x(gfn)), (unsigned
long)(maddr), (unsigned long)*ap2m);
+        domain_crash(hp2m->domain);
+    }
+
+    return 1;
+
+out:
+    spin_unlock(&hp2m->lock);
+    return 0;
+}
+
  static void p2m_init_altp2m_helper(struct domain *d, unsigned int i)
  {
      struct p2m_domain *p2m = d->arch.altp2m_p2m[i];

[...]

@@ -2429,6 +2460,8 @@ static void do_trap_data_abort_guest(struct
cpu_user_regs *regs,
                                       const union hsr hsr)
  {
      const struct hsr_dabt dabt = hsr.dabt;
+    struct vcpu *v = current;
+    struct p2m_domain *p2m = NULL;
      int rc;
      mmio_info_t info;

@@ -2449,6 +2482,12 @@ static void do_trap_data_abort_guest(struct
cpu_user_regs *regs,
          info.gpa = get_faulting_ipa();
      else
      {
+        /*
+         * When using altp2m, this flush is required to get rid of
old TLB
+         * entries and use the new, lazily copied, ap2m entries.
+         */
+        flush_tlb_local();

Can you give more details why this flush is required?


Without the flush, the guest crashed to an unresolved data abort. To be
more precise, after the first lazy-copy of a mapping from the hostp2m to
the currently active altp2m view, the system crashed because it was not
able to find the new mapping in its altp2m table. The explicit flush
solved this issue quite nicely.

This explicit flush likely means there is another issue in the code. This flush should not be necessary.

Can you give more details about the exact data abort problem? Is it because the translation VA -> IPA is failing?


As I answer your question, I am starting to think that the crash was a
result of of a lack of a memory barrier because even with the old
(hostp2m's) TLBs present, the translation would not present an issue
(the mapping would be the same as the p2m entry is simply copied from
the hostp2m to the active altp2m view). Also, the guest would reuse the
old TLBs, the system would have used the access rights of the hostp2m,
and hence again be trapped by Xen.

I am sorry but I don't understand what you are trying to say.

I will try to solve this issue by means of a memory barrier, thank you.

Can you details where you think there is a lack of memory barrier?

Regards,

--
Julien Grall

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

 


Rackspace

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