[1/2] x86/mm: Change default value for suppress #VE in set_mem_access()

Commit Message

Adrian Pop June 9, 2017, 4:51 p.m. UTC

From: Vlad Ioan Topan <[e-mail]>

The default value for the "suppress #VE" bit set by set_mem_access()
currently depends on whether the call is made from the same domain (the
bit is set when called from another domain and cleared if called from
the same domain). This patch changes that behavior to inherit the old
suppress #VE bit value if it is already set and to set it to 1
otherwise, which is safer and more reliable.

Signed-off-by: Vlad Ioan Topan <[e-mail]>
Signed-off-by: Adrian Pop <[e-mail]>
---
 xen/arch/x86/mm/mem_access.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Tamas K Lengyel June 15, 2017, 6:49 p.m. UTC | #1

On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <[e-mail]> wrote:
> From: Vlad Ioan Topan <[e-mail]>
>
> The default value for the "suppress #VE" bit set by set_mem_access()
> currently depends on whether the call is made from the same domain (the
> bit is set when called from another domain and cleared if called from
> the same domain). This patch changes that behavior to inherit the old
> suppress #VE bit value if it is already set and to set it to 1
> otherwise, which is safer and more reliable.

Could you elaborate on why do you think it is safer and more reliable
to switch the behavior? I believe the original idea was that the
domain should only be allowed to clear an SVE bit set by an external
tool. With this change it will allow the guest to request VE for any
page the external tool hasn't itself reserved specifically.

Tamas

George Dunlap July 20, 2017, 2:38 p.m. UTC | #2

On Thu, Jun 15, 2017 at 7:49 PM, Tamas K Lengyel <[e-mail]> wrote:
> On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <[e-mail]> wrote:
>> From: Vlad Ioan Topan <[e-mail]>
>>
>> The default value for the "suppress #VE" bit set by set_mem_access()
>> currently depends on whether the call is made from the same domain (the
>> bit is set when called from another domain and cleared if called from
>> the same domain). This patch changes that behavior to inherit the old
>> suppress #VE bit value if it is already set and to set it to 1
>> otherwise, which is safer and more reliable.
>
> Could you elaborate on why do you think it is safer and more reliable
> to switch the behavior? I believe the original idea was that the
> domain should only be allowed to clear an SVE bit set by an external
> tool. With this change it will allow the guest to request VE for any
> page the external tool hasn't itself reserved specifically.

Hmm?  This patch by itself simply prevents the guest from changing the
VE bit at all (either setting or clearing it).

Or did you mean, "This patch series"?

 -George

Tamas K Lengyel July 20, 2017, 4:14 p.m. UTC | #3

On Thu, Jul 20, 2017 at 8:38 AM, George Dunlap
<[e-mail]> wrote:
> On Thu, Jun 15, 2017 at 7:49 PM, Tamas K Lengyel <[e-mail]> wrote:
>> On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <[e-mail]> wrote:
>>> From: Vlad Ioan Topan <[e-mail]>
>>>
>>> The default value for the "suppress #VE" bit set by set_mem_access()
>>> currently depends on whether the call is made from the same domain (the
>>> bit is set when called from another domain and cleared if called from
>>> the same domain). This patch changes that behavior to inherit the old
>>> suppress #VE bit value if it is already set and to set it to 1
>>> otherwise, which is safer and more reliable.
>>
>> Could you elaborate on why do you think it is safer and more reliable
>> to switch the behavior? I believe the original idea was that the
>> domain should only be allowed to clear an SVE bit set by an external
>> tool. With this change it will allow the guest to request VE for any
>> page the external tool hasn't itself reserved specifically.
>
> Hmm?  This patch by itself simply prevents the guest from changing the
> VE bit at all (either setting or clearing it).
>
> Or did you mean, "This patch series"?

No, technically the other patch is fine by itself. It can only be used
to set the SVE bit from a privileged domain, but by itself that is
fine. Only this patch is problematic if we want to allow a setup where
there is only an in-guest tool without a corresponding vm_event
mem_access listener.

Tamas

Patch

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 5adaf6df90..d0b0767855 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -273,8 +273,7 @@  int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
         }
     }
 
-    return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
-                         (current->domain != d));
+    return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
 }
 
 static int set_mem_access(struct domain *d, struct p2m_domain *p2m,