Re: [Xen-devel] [PATCH 3/3] xen/arm: Don't crash the domain on invalid HVC immediate

2018-01-30 Thread Julien Grall 



On 30/01/18 18:24, Stefano Stabellini wrote:

On Tue, 30 Jan 2018, Julien Grall wrote:

domain_crash_synchronous() should only be used when something went wrong
in Xen. It is better to inject to the guest as it will be in better

   ^ a better



position to provide helpful information (stack trace...).

Signed-off-by: Julien Grall 

---

 We potentially want to return -1 instead. This would make Xen more
 future-proof if we decide to implement the other HVC immediate.
---
  xen/arch/arm/traps.c | 13 -
  1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 843adf4959..18da45dff3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1473,14 +1473,17 @@ static void do_debug_trap(struct cpu_user_regs *regs, 
unsigned int code)
  #endif
  
  static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,

-  unsigned long iss)
+  const union hsr hsr)
  {
  arm_hypercall_fn_t call = NULL;
  
  BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) );
  
-if ( iss != XEN_HYPERCALL_TAG )

-domain_crash_synchronous();
+if ( hsr.iss != XEN_HYPERCALL_TAG )
+{
+gprintk(XENLOG_WARNING, "Invalid HVC imm 0x%x\n", hsr.iss);
+return inject_undef_exception(regs, hsr);


It looks a bit weird, given that inject_undef_exception doesn't return
anything. This is just code style so anyway:


I followed coding style in other part of the emulation (see do_sysreg 
for instance). I am happy to change that though.


Cheers,



Reviewed-by: Stefano Stabellini 



+}
  
  if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )

  {
@@ -2150,7 +2153,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
  if ( hsr.iss == 0 )
  return do_trap_hvc_smccc(regs);
  nr = regs->r12;
-do_trap_hypercall(regs, &nr, hsr.iss);
+do_trap_hypercall(regs, &nr, hsr);
  regs->r12 = (uint32_t)nr;
  break;
  }
@@ -2164,7 +2167,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
  #endif
  if ( hsr.iss == 0 )
  return do_trap_hvc_smccc(regs);
-do_trap_hypercall(regs, ®s->x16, hsr.iss);
+do_trap_hypercall(regs, ®s->x16, hsr);
  break;
  case HSR_EC_SMC64:
  /*
--
2.11.0



--
Julien Grall

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

Re: [Xen-devel] [PATCH 3/3] xen/arm: Don't crash the domain on invalid HVC immediate

2018-01-30 Thread Stefano Stabellini 
On Tue, 30 Jan 2018, Julien Grall wrote:
> domain_crash_synchronous() should only be used when something went wrong
> in Xen. It is better to inject to the guest as it will be in better
  ^ a better


> position to provide helpful information (stack trace...).
> 
> Signed-off-by: Julien Grall 
> 
> ---
> 
> We potentially want to return -1 instead. This would make Xen more
> future-proof if we decide to implement the other HVC immediate.
> ---
>  xen/arch/arm/traps.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 843adf4959..18da45dff3 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1473,14 +1473,17 @@ static void do_debug_trap(struct cpu_user_regs *regs, 
> unsigned int code)
>  #endif
>  
>  static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
> -  unsigned long iss)
> +  const union hsr hsr)
>  {
>  arm_hypercall_fn_t call = NULL;
>  
>  BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) );
>  
> -if ( iss != XEN_HYPERCALL_TAG )
> -domain_crash_synchronous();
> +if ( hsr.iss != XEN_HYPERCALL_TAG )
> +{
> +gprintk(XENLOG_WARNING, "Invalid HVC imm 0x%x\n", hsr.iss);
> +return inject_undef_exception(regs, hsr);

It looks a bit weird, given that inject_undef_exception doesn't return
anything. This is just code style so anyway:

Reviewed-by: Stefano Stabellini 


> +}
>  
>  if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
>  {
> @@ -2150,7 +2153,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>  if ( hsr.iss == 0 )
>  return do_trap_hvc_smccc(regs);
>  nr = regs->r12;
> -do_trap_hypercall(regs, &nr, hsr.iss);
> +do_trap_hypercall(regs, &nr, hsr);
>  regs->r12 = (uint32_t)nr;
>  break;
>  }
> @@ -2164,7 +2167,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>  #endif
>  if ( hsr.iss == 0 )
>  return do_trap_hvc_smccc(regs);
> -do_trap_hypercall(regs, ®s->x16, hsr.iss);
> +do_trap_hypercall(regs, ®s->x16, hsr);
>  break;
>  case HSR_EC_SMC64:
>  /*
> -- 
> 2.11.0
> 

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

[Xen-devel] [PATCH 3/3] xen/arm: Don't crash the domain on invalid HVC immediate

2018-01-30 Thread Julien Grall 
domain_crash_synchronous() should only be used when something went wrong
in Xen. It is better to inject to the guest as it will be in better
position to provide helpful information (stack trace...).

Signed-off-by: Julien Grall 

---

We potentially want to return -1 instead. This would make Xen more
future-proof if we decide to implement the other HVC immediate.
---
 xen/arch/arm/traps.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 843adf4959..18da45dff3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1473,14 +1473,17 @@ static void do_debug_trap(struct cpu_user_regs *regs, 
unsigned int code)
 #endif
 
 static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
-  unsigned long iss)
+  const union hsr hsr)
 {
 arm_hypercall_fn_t call = NULL;
 
 BUILD_BUG_ON(NR_hypercalls < ARRAY_SIZE(arm_hypercall_table) );
 
-if ( iss != XEN_HYPERCALL_TAG )
-domain_crash_synchronous();
+if ( hsr.iss != XEN_HYPERCALL_TAG )
+{
+gprintk(XENLOG_WARNING, "Invalid HVC imm 0x%x\n", hsr.iss);
+return inject_undef_exception(regs, hsr);
+}
 
 if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
 {
@@ -2150,7 +2153,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
 if ( hsr.iss == 0 )
 return do_trap_hvc_smccc(regs);
 nr = regs->r12;
-do_trap_hypercall(regs, &nr, hsr.iss);
+do_trap_hypercall(regs, &nr, hsr);
 regs->r12 = (uint32_t)nr;
 break;
 }
@@ -2164,7 +2167,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
 #endif
 if ( hsr.iss == 0 )
 return do_trap_hvc_smccc(regs);
-do_trap_hypercall(regs, ®s->x16, hsr.iss);
+do_trap_hypercall(regs, ®s->x16, hsr);
 break;
 case HSR_EC_SMC64:
 /*
-- 
2.11.0


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