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

Re: [Xen-devel] [PATCH v2 19/25] arm/altp2m: Add altp2m_propagate_change.





On 06/08/2016 12:26, Sergej Proskurin wrote:
Hi Julien,

Hello Sergej,

On 08/04/2016 04:50 PM, Julien Grall wrote:
On 01/08/16 18:10, Sergej Proskurin wrote:
+
+        /* Check for a dropped page that may impact this altp2m. */
+        if ( (mfn_eq(smfn, INVALID_MFN) || p2mt == p2m_invalid) &&

Why the check to p2mt against p2m_invalid?


I have encountered p2m entries or rather calls to p2m_apply_change with
valid MFN's, however, invalid types. That is, a page drop would not be
recognized if checked only against an invalid MFN.

Because currently REMOVE operation has the MFN valid to be able to check whether the wanted mapping has been removed.

However, I don't think this is safe to assume that p2m_invalid will always meant "REMOVE". It is actually used in different case.


+             gfn_x(sgfn) >= gfn_x(p2m->lowest_mapped_gfn) &&
+             gfn_x(sgfn) <= gfn_x(p2m->max_mapped_gfn) )
+        {
+            if ( !reset_count++ )
+            {
+                altp2m_reset(p2m);
+                last_reset_idx = i;
+            }
+            else
+            {
+                /* At least 2 altp2m's impacted, so reset
everything. */

So if you remove a 4KB page in more than 2 altp2m, you will flush all
the p2m. This sounds really more time consuming (you have to free all
the intermediate page table) than removing a single 4KB page.

I agree: The solution has been directly adopted from the x86
implementation. It needs further design reconsideration. However, at
this point we would like to finish the overall architecture for ARM
before we go into further reconstructions.


+                for ( i = 0; i < MAX_ALTP2M; i++ )
+                {
+                    if ( i == last_reset_idx ||
+                         d->arch.altp2m_vttbr[i] == INVALID_VTTBR )
+                        continue;
+
+                    p2m = d->arch.altp2m_p2m[i];
+                    altp2m_reset(p2m);
+                }
+                goto out;
+            }
+        }
+        else if ( !mfn_eq(m, INVALID_MFN) )
+            modify_altp2m_range(d, p2m, sgfn, nr, smfn,
+                                mask, p2mt, p2ma);

I am a bit concerned about this function. We decided to limit the size
of the mapping to avoid long running memory operations (see XSA-158).

With this function you multiply up to 10 times the duration of the
operation.


I see your point. However, on an active system, adaptions of the hostp2m
(while altp2m is active) should be very limited. Another solution would
be to simply flush the altp2m views on every hostp2m modification. IMO
this, however, would not really be a huge performance gain due to
de-allocation and freeing of the altp2m tables. Do you have another idea?

I would need to have a think. However, it is wrong to assume that change of hostp2m will be limited when altp2m is active. A guest is free to increase/decrease the memory reservation.

You always need to consider the worth case and not the best case (i.e a guest behaving nicely).

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®.