On 4/3/19 5:58 PM, Jan Beulich wrote:
On 03.04.19 at 16:29, <aisa...@bitdefender.com> wrote:
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -3011,8 +3011,16 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn,
bool suppress_ve,
mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
if ( !mfn_valid(mfn) )
{
- rc = -ESRCH;
- goto out;
+ unsigned int page_order;
+
+ mfn = __get_gfn_type_access(host_p2m, gfn_x(gfn), &t, &a,
+ P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
I'm not entirely certain about P2M_ALLOC, but I'm pretty sure that
at least P2M_UNSHARE is too heavy: Why would you want to force
un-sharing of a page when all you want to alter is #VE behavior?
That logic was taken from p2m_set_altp2m_mem_access(), we thought the
two cases are very similar.
269 mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
270
271 /* Check host p2m if no valid entry in alternate */
272 if ( !mfn_valid(mfn) )
273 {
274
275 mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
276 P2M_ALLOC | P2M_UNSHARE,
&page_order, 0);
277
278 rc = -ESRCH;
279 if ( !mfn_valid(mfn) || t != p2m_ram_rw )
280 return rc;
281
282 /* If this is a superpage, copy that first */
283 if ( page_order != PAGE_ORDER_4K )
284 {
285 unsigned long mask = ~((1UL << page_order) - 1);
286 gfn_t gfn2 = _gfn(gfn_l & mask);
287 mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
288
289 rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t,
old_a, 1);
290 if ( rc )
291 return rc;
292 }
293 }
294
295 /*
296 * Inherit the old suppress #VE bit value if it is already set,
or set it
297 * to 1 otherwise
298 */
299 return ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a, -1);
300 }
I wonder if we should put the whole logic in the "if ( !mfn_valid(mfn)
)" body in its own function and reuse that for both functions - although
it doesn't look like the extra superpage logic matters for setting the
suppress #VE bit alone (since even the code above only sets it with
PAGE_ORDER_4K).
Additionally, when you add such a lookup as error handling attempt,
I think it is important to leave a code comment. But I wonder
whether this shouldn't be done before the call to ->get_entry(), or
whether in fact there's a bug here in how get_entry() behaves in
this case.
Changes to the hostp2m (also known as altp2m view 0) propagate to all
existing altp2ms, but they do so in a lazy manner, and also that won't
happen for altp2ms created after a while. So altp2ms will not
necessarily know about a page that the hostp2m knows about, which should
not stop us from setting mem access restrictions or the value of the SVE
bit.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
