[PATCH] x86/hvm: Drop more remains of the PVHv1 implementation

[PATCH] x86/hvm: Drop more remains of the PVHv1 implementation

[Andrew Cooper]

These functions don't need is_hvm_{vcpu,domain}() predicates.

hvmop_set_evtchn_upcall_vector() does need the predicate to prevent a PV
caller accessing the hvm union, but swap the copy_from_guest() and
is_hvm_domain() predicate to avoid reading the hypercall parameter if we not
going to use it.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/hvm.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8145385..4fef616 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -506,8 +506,7 @@ void hvm_do_resume(struct vcpu *v)
 {
     check_wakeup_from_wait();
 
-    if ( is_hvm_domain(v->domain) )
-        pt_restore_timer(v);
+    pt_restore_timer(v);
 
     if ( !handle_hvm_io_completion(v) )
         return;
@@ -1544,8 +1543,7 @@ void hvm_vcpu_destroy(struct vcpu *v)
     tasklet_kill(&v->arch.hvm_vcpu.assert_evtchn_irq_tasklet);
     hvm_funcs.vcpu_destroy(v);
 
-    if ( is_hvm_vcpu(v) )
-        vlapic_destroy(v);
+    vlapic_destroy(v);
 
     hvm_vcpu_cacheattr_destroy(v);
 }
@@ -1711,7 +1709,6 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
      * - newer Windows (like Server 2012) for HPET accesses.
      */
     if ( !nestedhvm_vcpu_in_guestmode(curr)
-         && is_hvm_domain(currd)
          && hvm_mmio_internal(gpa) )
     {
         if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec) )
@@ -3139,7 +3136,7 @@ static enum hvm_copy_result __hvm_copy(
          * - 32-bit WinXP (& older Windows) on AMD CPUs for LAPIC accesses,
          * - newer Windows (like Server 2012) for HPET accesses.
          */
-        if ( v == current && is_hvm_vcpu(v)
+        if ( v == current
              && !nestedhvm_vcpu_in_guestmode(v)
              && hvm_mmio_internal(gpa) )
             return HVMCOPY_bad_gfn_to_mfn;
@@ -3971,12 +3968,12 @@ static int hvmop_set_evtchn_upcall_vector(
     struct domain *d = current->domain;
     struct vcpu *v;
 
-    if ( copy_from_guest(&op, uop, 1) )
-        return -EFAULT;
-
     if ( !is_hvm_domain(d) )
         return -EINVAL;
 
+    if ( copy_from_guest(&op, uop, 1) )
+        return -EFAULT;
+
     if ( op.vector < 0x10 )
         return -EINVAL;
 
-- 
2.1.4


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

Re: [PATCH] x86/hvm: Drop more remains of the PVHv1 implementation

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 19 July 2017 14:28
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Jan Beulich <JBeulich@suse.com>; Wei Liu
> <wei.liu2@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Roger Pau
> Monne <roger.pau@citrix.com>
> Subject: [PATCH] x86/hvm: Drop more remains of the PVHv1 implementation
> 
> These functions don't need is_hvm_{vcpu,domain}() predicates.
> 
> hvmop_set_evtchn_upcall_vector() does need the predicate to prevent a
> PV
> caller accessing the hvm union, but swap the copy_from_guest() and
> is_hvm_domain() predicate to avoid reading the hypercall parameter if we
> not
> going to use it.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 8145385..4fef616 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -506,8 +506,7 @@ void hvm_do_resume(struct vcpu *v)
>  {
>      check_wakeup_from_wait();
> 
> -    if ( is_hvm_domain(v->domain) )
> -        pt_restore_timer(v);
> +    pt_restore_timer(v);
> 
>      if ( !handle_hvm_io_completion(v) )
>          return;
> @@ -1544,8 +1543,7 @@ void hvm_vcpu_destroy(struct vcpu *v)
>      tasklet_kill(&v->arch.hvm_vcpu.assert_evtchn_irq_tasklet);
>      hvm_funcs.vcpu_destroy(v);
> 
> -    if ( is_hvm_vcpu(v) )
> -        vlapic_destroy(v);
> +    vlapic_destroy(v);
> 
>      hvm_vcpu_cacheattr_destroy(v);
>  }
> @@ -1711,7 +1709,6 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
> unsigned long gla,
>       * - newer Windows (like Server 2012) for HPET accesses.
>       */
>      if ( !nestedhvm_vcpu_in_guestmode(curr)
> -         && is_hvm_domain(currd)
>           && hvm_mmio_internal(gpa) )
>      {
>          if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec) )
> @@ -3139,7 +3136,7 @@ static enum hvm_copy_result __hvm_copy(
>           * - 32-bit WinXP (& older Windows) on AMD CPUs for LAPIC accesses,
>           * - newer Windows (like Server 2012) for HPET accesses.
>           */
> -        if ( v == current && is_hvm_vcpu(v)
> +        if ( v == current
>               && !nestedhvm_vcpu_in_guestmode(v)
>               && hvm_mmio_internal(gpa) )
>              return HVMCOPY_bad_gfn_to_mfn;
> @@ -3971,12 +3968,12 @@ static int hvmop_set_evtchn_upcall_vector(
>      struct domain *d = current->domain;
>      struct vcpu *v;
> 
> -    if ( copy_from_guest(&op, uop, 1) )
> -        return -EFAULT;
> -
>      if ( !is_hvm_domain(d) )
>          return -EINVAL;
> 
> +    if ( copy_from_guest(&op, uop, 1) )
> +        return -EFAULT;
> +
>      if ( op.vector < 0x10 )
>          return -EINVAL;
> 
> --
> 2.1.4

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

Re: [PATCH] x86/hvm: Drop more remains of the PVHv1 implementation

[Wei Liu]

On Wed, Jul 19, 2017 at 02:27:31PM +0100, Andrew Cooper wrote:
> These functions don't need is_hvm_{vcpu,domain}() predicates.
> 
> hvmop_set_evtchn_upcall_vector() does need the predicate to prevent a PV
> caller accessing the hvm union, but swap the copy_from_guest() and
> is_hvm_domain() predicate to avoid reading the hypercall parameter if we not
> going to use it.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

Re: [PATCH] x86/hvm: Drop more remains of the PVHv1 implementation

[Roger Pau Monné]

On Wed, Jul 19, 2017 at 02:27:31PM +0100, Andrew Cooper wrote:
> These functions don't need is_hvm_{vcpu,domain}() predicates.
> 
> hvmop_set_evtchn_upcall_vector() does need the predicate to prevent a PV
> caller accessing the hvm union, but swap the copy_from_guest() and
> is_hvm_domain() predicate to avoid reading the hypercall parameter if we not
> going to use it.

IC, certain HVMOPs are available to PV guests (ie: the control domain).

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks. Just one style nit.

> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 8145385..4fef616 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -506,8 +506,7 @@ void hvm_do_resume(struct vcpu *v)
>  {
>      check_wakeup_from_wait();
>  
> -    if ( is_hvm_domain(v->domain) )
> -        pt_restore_timer(v);
> +    pt_restore_timer(v);
>  
>      if ( !handle_hvm_io_completion(v) )
>          return;
> @@ -1544,8 +1543,7 @@ void hvm_vcpu_destroy(struct vcpu *v)
>      tasklet_kill(&v->arch.hvm_vcpu.assert_evtchn_irq_tasklet);
>      hvm_funcs.vcpu_destroy(v);
>  
> -    if ( is_hvm_vcpu(v) )
> -        vlapic_destroy(v);
> +    vlapic_destroy(v);
>  
>      hvm_vcpu_cacheattr_destroy(v);
>  }
> @@ -1711,7 +1709,6 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
>       * - newer Windows (like Server 2012) for HPET accesses.
>       */
>      if ( !nestedhvm_vcpu_in_guestmode(curr)
> -         && is_hvm_domain(currd)
>           && hvm_mmio_internal(gpa) )

Can this be moved to the previous line?

Roger.

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

Re: [PATCH] x86/hvm: Drop more remains of the PVHv1 implementation

[Andrew Cooper]

On 19/07/17 15:12, Roger Pau Monné wrote:
> On Wed, Jul 19, 2017 at 02:27:31PM +0100, Andrew Cooper wrote:
>> These functions don't need is_hvm_{vcpu,domain}() predicates.
>>
>> hvmop_set_evtchn_upcall_vector() does need the predicate to prevent a PV
>> caller accessing the hvm union, but swap the copy_from_guest() and
>> is_hvm_domain() predicate to avoid reading the hypercall parameter if we not
>> going to use it.
> IC, certain HVMOPs are available to PV guests (ie: the control domain).

At the very least, the control domain needs to use HVMOP_getparam for
construction and migration purposes.  As a result, PV guests have always
had blanket reign on HVMOPs.

>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> Thanks. Just one style nit.
>
>> CC: George Dunlap <George.Dunlap@eu.citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Paul Durrant <paul.durrant@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>>  xen/arch/x86/hvm/hvm.c | 15 ++++++---------
>>  1 file changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 8145385..4fef616 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -506,8 +506,7 @@ void hvm_do_resume(struct vcpu *v)
>>  {
>>      check_wakeup_from_wait();
>>  
>> -    if ( is_hvm_domain(v->domain) )
>> -        pt_restore_timer(v);
>> +    pt_restore_timer(v);
>>  
>>      if ( !handle_hvm_io_completion(v) )
>>          return;
>> @@ -1544,8 +1543,7 @@ void hvm_vcpu_destroy(struct vcpu *v)
>>      tasklet_kill(&v->arch.hvm_vcpu.assert_evtchn_irq_tasklet);
>>      hvm_funcs.vcpu_destroy(v);
>>  
>> -    if ( is_hvm_vcpu(v) )
>> -        vlapic_destroy(v);
>> +    vlapic_destroy(v);
>>  
>>      hvm_vcpu_cacheattr_destroy(v);
>>  }
>> @@ -1711,7 +1709,6 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
>>       * - newer Windows (like Server 2012) for HPET accesses.
>>       */
>>      if ( !nestedhvm_vcpu_in_guestmode(curr)
>> -         && is_hvm_domain(currd)
>>           && hvm_mmio_internal(gpa) )
> Can this be moved to the previous line?

Will fix on commit.

~Andrew

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