[PATCH] x86: restrict HVMOP_pagetable_dying to current

[PATCH] x86: restrict HVMOP_pagetable_dying to current

[Jan Beulich]

This is not used (and probably was never meant to be) by the tool stack.
Limiting it to the current domain in particular allows to eliminate a
bogus use of vCPU 0 in pagetable_dying().

Remove the now unnecessary domain/vCPU parameters from the wrapper/hook
functions at the same time.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4895,10 +4895,12 @@ long do_hvm_op(unsigned long op, XEN_GUE
             return -ESRCH;
 
         rc = -EINVAL;
-        if ( is_hvm_domain(d) && paging_mode_shadow(d) )
+        if ( unlikely(d != current->domain) )
+            rc = -EOPNOTSUPP;
+        else if ( is_hvm_domain(d) && paging_mode_shadow(d) )
             rc = xsm_hvm_param(XSM_TARGET, d, op);
         if ( !rc )
-            pagetable_dying(d, a.gpa);
+            pagetable_dying(a.gpa);
 
         rcu_unlock_domain(d);
         break;
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -851,15 +851,14 @@ int paging_enable(struct domain *d, u32
 
 /* Called from the guest to indicate that a process is being torn down
  * and therefore its pagetables will soon be discarded */
-void pagetable_dying(struct domain *d, paddr_t gpa)
+void pagetable_dying(paddr_t gpa)
 {
 #ifdef CONFIG_SHADOW_PAGING
-    struct vcpu *v;
+    struct vcpu *curr = current;
 
-    ASSERT(paging_mode_shadow(d));
+    ASSERT(paging_mode_shadow(curr->domain));
 
-    v = d->vcpu[0];
-    v->arch.paging.mode->shadow.pagetable_dying(v, gpa);
+    curr->arch.paging.mode->shadow.pagetable_dying(gpa);
 #else
     BUG();
 #endif
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -4525,8 +4525,9 @@ int sh_remove_l3_shadow(struct domain *d
  * and in the meantime we unhook its top-level user-mode entries. */
 
 #if GUEST_PAGING_LEVELS == 3
-static void sh_pagetable_dying(struct vcpu *v, paddr_t gpa)
+static void sh_pagetable_dying(paddr_t gpa)
 {
+    struct vcpu *v = current;
     struct domain *d = v->domain;
     int i = 0;
     int flush = 0;
@@ -4604,8 +4605,9 @@ out_put_gfn:
     put_gfn(d, l3gfn);
 }
 #else
-static void sh_pagetable_dying(struct vcpu *v, paddr_t gpa)
+static void sh_pagetable_dying(paddr_t gpa)
 {
+    struct vcpu *v = current;
     struct domain *d = v->domain;
     mfn_t smfn, gmfn;
     p2m_type_t p2mt;
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -95,7 +95,7 @@ struct shadow_paging_mode {
     void          (*destroy_monitor_table )(struct vcpu *v, mfn_t mmfn);
     int           (*guess_wrmap           )(struct vcpu *v, 
                                             unsigned long vaddr, mfn_t gmfn);
-    void          (*pagetable_dying       )(struct vcpu *v, paddr_t gpa);
+    void          (*pagetable_dying       )(paddr_t gpa);
     void          (*trace_emul_write_val  )(const void *ptr, unsigned long vaddr,
                                             const void *src, unsigned int bytes);
 #endif
@@ -345,7 +345,7 @@ void paging_write_p2m_entry(struct p2m_d
 
 /* Called from the guest to indicate that the a process is being
  * torn down and its pagetables will soon be discarded */
-void pagetable_dying(struct domain *d, paddr_t gpa);
+void pagetable_dying(paddr_t gpa);
 
 /* Print paging-assistance info to the console */
 void paging_dump_domain_info(struct domain *d);





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86: restrict HVMOP_pagetable_dying to current

[Andrew Cooper]

On 05/10/18 12:29, Jan Beulich wrote:
> This is not used (and probably was never meant to be) by the tool stack.
> Limiting it to the current domain in particular allows to eliminate a
> bogus use of vCPU 0 in pagetable_dying().
>
> Remove the now unnecessary domain/vCPU parameters from the wrapper/hook
> functions at the same time.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4895,10 +4895,12 @@ long do_hvm_op(unsigned long op, XEN_GUE
>              return -ESRCH;
>  
>          rc = -EINVAL;
> -        if ( is_hvm_domain(d) && paging_mode_shadow(d) )
> +        if ( unlikely(d != current->domain) )
> +            rc = -EOPNOTSUPP;
> +        else if ( is_hvm_domain(d) && paging_mode_shadow(d) )
>              rc = xsm_hvm_param(XSM_TARGET, d, op);

As we're switching to current-only, shouldn't this turn to XSM_HOOK ?

Everything else LGTM, with one small suggestion....

>          if ( !rc )
> -            pagetable_dying(d, a.gpa);
> +            pagetable_dying(a.gpa);
>  
>          rcu_unlock_domain(d);
>          break;
> --- a/xen/include/asm-x86/paging.h
> +++ b/xen/include/asm-x86/paging.h
> @@ -345,7 +345,7 @@ void paging_write_p2m_entry(struct p2m_d
>  
>  /* Called from the guest to indicate that the a process is being
>   * torn down and its pagetables will soon be discarded */
> -void pagetable_dying(struct domain *d, paddr_t gpa);
> +void pagetable_dying(paddr_t gpa);

Fix the comment style while in this area?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86: restrict HVMOP_pagetable_dying to current

[Jan Beulich]

>>> On 05.10.18 at 13:58, <andrew.cooper3@citrix.com> wrote:
> On 05/10/18 12:29, Jan Beulich wrote:
>> This is not used (and probably was never meant to be) by the tool stack.
>> Limiting it to the current domain in particular allows to eliminate a
>> bogus use of vCPU 0 in pagetable_dying().
>>
>> Remove the now unnecessary domain/vCPU parameters from the wrapper/hook
>> functions at the same time.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4895,10 +4895,12 @@ long do_hvm_op(unsigned long op, XEN_GUE
>>              return -ESRCH;
>>  
>>          rc = -EINVAL;
>> -        if ( is_hvm_domain(d) && paging_mode_shadow(d) )
>> +        if ( unlikely(d != current->domain) )
>> +            rc = -EOPNOTSUPP;
>> +        else if ( is_hvm_domain(d) && paging_mode_shadow(d) )
>>              rc = xsm_hvm_param(XSM_TARGET, d, op);
> 
> As we're switching to current-only, shouldn't this turn to XSM_HOOK ?

Not sure - I simply didn't want to fiddle with any of the semantics,
and keeping it as it is may be sub-optimal, but is certainly not going
to be wromg.

> Everything else LGTM, with one small suggestion....
> 
>>          if ( !rc )
>> -            pagetable_dying(d, a.gpa);
>> +            pagetable_dying(a.gpa);
>>  
>>          rcu_unlock_domain(d);
>>          break;
>> --- a/xen/include/asm-x86/paging.h
>> +++ b/xen/include/asm-x86/paging.h
>> @@ -345,7 +345,7 @@ void paging_write_p2m_entry(struct p2m_d
>>  
>>  /* Called from the guest to indicate that the a process is being
>>   * torn down and its pagetables will soon be discarded */
>> -void pagetable_dying(struct domain *d, paddr_t gpa);
>> +void pagetable_dying(paddr_t gpa);
> 
> Fix the comment style while in this area?

Well, I can certainly do so - I didn't because I didn't touch the
comment itself.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86: restrict HVMOP_pagetable_dying to current

[Wei Liu]

On Fri, Oct 05, 2018 at 05:29:30AM -0600, Jan Beulich wrote:
> This is not used (and probably was never meant to be) by the tool stack.
> Limiting it to the current domain in particular allows to eliminate a
> bogus use of vCPU 0 in pagetable_dying().
> 
> Remove the now unnecessary domain/vCPU parameters from the wrapper/hook
> functions at the same time.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86: restrict HVMOP_pagetable_dying to current

[George Dunlap]

On 10/05/2018 12:29 PM, Jan Beulich wrote:
> This is not used (and probably was never meant to be) by the tool stack.
> Limiting it to the current domain in particular allows to eliminate a
> bogus use of vCPU 0 in pagetable_dying().
> 
> Remove the now unnecessary domain/vCPU parameters from the wrapper/hook
> functions at the same time.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

MM bits Acked-by: George Dunlap <george.dunlap@citrix.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Ping: [PATCH] x86: restrict HVMOP_pagetable_dying to current

[Jan Beulich]

>>> On 05.10.18 at 14:03, <JBeulich@suse.com> wrote:
>>>> On 05.10.18 at 13:58, <andrew.cooper3@citrix.com> wrote:
>> On 05/10/18 12:29, Jan Beulich wrote:
>>> This is not used (and probably was never meant to be) by the tool stack.
>>> Limiting it to the current domain in particular allows to eliminate a
>>> bogus use of vCPU 0 in pagetable_dying().
>>>
>>> Remove the now unnecessary domain/vCPU parameters from the wrapper/hook
>>> functions at the same time.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -4895,10 +4895,12 @@ long do_hvm_op(unsigned long op, XEN_GUE
>>>              return -ESRCH;
>>>  
>>>          rc = -EINVAL;
>>> -        if ( is_hvm_domain(d) && paging_mode_shadow(d) )
>>> +        if ( unlikely(d != current->domain) )
>>> +            rc = -EOPNOTSUPP;
>>> +        else if ( is_hvm_domain(d) && paging_mode_shadow(d) )
>>>              rc = xsm_hvm_param(XSM_TARGET, d, op);
>> 
>> As we're switching to current-only, shouldn't this turn to XSM_HOOK ?
> 
> Not sure - I simply didn't want to fiddle with any of the semantics,
> and keeping it as it is may be sub-optimal, but is certainly not going
> to be wrong.

Are you fine with the above, or do you demand the change in
order to give your ack?

>> Everything else LGTM, with one small suggestion....
>> 
>>>          if ( !rc )
>>> -            pagetable_dying(d, a.gpa);
>>> +            pagetable_dying(a.gpa);
>>>  
>>>          rcu_unlock_domain(d);
>>>          break;
>>> --- a/xen/include/asm-x86/paging.h
>>> +++ b/xen/include/asm-x86/paging.h
>>> @@ -345,7 +345,7 @@ void paging_write_p2m_entry(struct p2m_d
>>>  
>>>  /* Called from the guest to indicate that the a process is being
>>>   * torn down and its pagetables will soon be discarded */
>>> -void pagetable_dying(struct domain *d, paddr_t gpa);
>>> +void pagetable_dying(paddr_t gpa);
>> 
>> Fix the comment style while in this area?
> 
> Well, I can certainly do so - I didn't because I didn't touch the
> comment itself.

I didn't think it was necessary to re-submit with just this adjustment,
the more that it was a suggestion only anyway. Is there anything
else that needs taking care of before I can get you ack for the
non-mm parts?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: Ping: [PATCH] x86: restrict HVMOP_pagetable_dying to current

[Andrew Cooper]

On 26/10/18 12:12, Jan Beulich wrote:
>>>> On 05.10.18 at 14:03, <JBeulich@suse.com> wrote:
>>>>> On 05.10.18 at 13:58, <andrew.cooper3@citrix.com> wrote:
>>> On 05/10/18 12:29, Jan Beulich wrote:
>>>> This is not used (and probably was never meant to be) by the tool stack.
>>>> Limiting it to the current domain in particular allows to eliminate a
>>>> bogus use of vCPU 0 in pagetable_dying().
>>>>
>>>> Remove the now unnecessary domain/vCPU parameters from the wrapper/hook
>>>> functions at the same time.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -4895,10 +4895,12 @@ long do_hvm_op(unsigned long op, XEN_GUE
>>>>              return -ESRCH;
>>>>  
>>>>          rc = -EINVAL;
>>>> -        if ( is_hvm_domain(d) && paging_mode_shadow(d) )
>>>> +        if ( unlikely(d != current->domain) )
>>>> +            rc = -EOPNOTSUPP;
>>>> +        else if ( is_hvm_domain(d) && paging_mode_shadow(d) )
>>>>              rc = xsm_hvm_param(XSM_TARGET, d, op);
>>> As we're switching to current-only, shouldn't this turn to XSM_HOOK ?
>> Not sure - I simply didn't want to fiddle with any of the semantics,
>> and keeping it as it is may be sub-optimal, but is certainly not going
>> to be wrong.
> Are you fine with the above, or do you demand the change in
> order to give your ack?
>
>>> Everything else LGTM, with one small suggestion....
>>>
>>>>          if ( !rc )
>>>> -            pagetable_dying(d, a.gpa);
>>>> +            pagetable_dying(a.gpa);
>>>>  
>>>>          rcu_unlock_domain(d);
>>>>          break;
>>>> --- a/xen/include/asm-x86/paging.h
>>>> +++ b/xen/include/asm-x86/paging.h
>>>> @@ -345,7 +345,7 @@ void paging_write_p2m_entry(struct p2m_d
>>>>  
>>>>  /* Called from the guest to indicate that the a process is being
>>>>   * torn down and its pagetables will soon be discarded */
>>>> -void pagetable_dying(struct domain *d, paddr_t gpa);
>>>> +void pagetable_dying(paddr_t gpa);
>>> Fix the comment style while in this area?
>> Well, I can certainly do so - I didn't because I didn't touch the
>> comment itself.
> I didn't think it was necessary to re-submit with just this adjustment,
> the more that it was a suggestion only anyway. Is there anything
> else that needs taking care of before I can get you ack for the
> non-mm parts?

Lets not waste time arguing.  Acked-by: Andrew Cooper
<andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel