[Xen-devel,2/3] xen/arm: do_trap_hypervisor: Separate hypervisor and guest traps

The function do_trap_hypervisor is currently handling both trap coming
from the hypervisor and the guest. This makes difficult to get specific
behavior when a trap is coming from either the guest or the hypervisor.

Split the function into two parts:
    - do_trap_guest_sync to handle guest traps
    - do_trap_hyp_sync to handle hypervisor traps

On AArch32, the Hyp Trap Exception provides the standard mechanism for
trapping Guest OS functions to the hypervisor (see B1.14.1 in ARM DDI
0406C.c). It cannot be generated when generated when the processor is in
Hyp Mode, instead other exception will be used. So it is fine to replace
the call to do_trap_hypervisor by do_trap_guest_sync.

For AArch64, there are two distincts exception depending whether the
exception was taken from the current level (hypervisor) or lower level
(guest).

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/arm32/entry.S |  4 ++--
 xen/arch/arm/arm64/entry.S |  6 +++---
 xen/arch/arm/traps.c       | 17 ++++++++++++++++-
 3 files changed, 21 insertions(+), 6 deletions(-)

On 04/05/17 18:54, Julien Grall wrote:
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 6010c96c54..c8ce62ff96 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2805,7 +2805,7 @@ static void enter_hypervisor_head(struct cpu_user_regs *regs)
>      }
>  }
>  
> -asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
> +asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs)
>  {
>      const union hsr hsr = { .bits = regs->hsr };
>  
> @@ -2925,6 +2925,21 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>          do_trap_data_abort_guest(regs, hsr);
>          break;
>  
> +    default:
> +        printk("Guest Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",

Purely cosmetic, but I'd recommend "Unknown Guest Trap ..." or similar.

Simply "Guest Trap" on its own doesn't covey any meaning to a user
reading a console log as to whether is normal or exceptional to
encounter this line.

~Andrew

Hi Andrew,

On 04/05/17 19:03, Andrew Cooper wrote:
> On 04/05/17 18:54, Julien Grall wrote:
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 6010c96c54..c8ce62ff96 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2805,7 +2805,7 @@ static void enter_hypervisor_head(struct cpu_user_regs *regs)
>>      }
>>  }
>>
>> -asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>> +asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs)
>>  {
>>      const union hsr hsr = { .bits = regs->hsr };
>>
>> @@ -2925,6 +2925,21 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>>          do_trap_data_abort_guest(regs, hsr);
>>          break;
>>
>> +    default:
>> +        printk("Guest Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
>
> Purely cosmetic, but I'd recommend "Unknown Guest Trap ..." or similar.
>
> Simply "Guest Trap" on its own doesn't covey any meaning to a user
> reading a console log as to whether is normal or exceptional to
> encounter this line.

Good point. I will update it.

Cheers,

On Thu, 4 May 2017, Julien Grall wrote:
> The function do_trap_hypervisor is currently handling both trap coming
> from the hypervisor and the guest. This makes difficult to get specific
> behavior when a trap is coming from either the guest or the hypervisor.
> 
> Split the function into two parts:
>     - do_trap_guest_sync to handle guest traps
>     - do_trap_hyp_sync to handle hypervisor traps
> 
> On AArch32, the Hyp Trap Exception provides the standard mechanism for
> trapping Guest OS functions to the hypervisor (see B1.14.1 in ARM DDI
> 0406C.c). It cannot be generated when generated when the processor is in
                                     ^ remove


> Hyp Mode, instead other exception will be used. So it is fine to replace
> the call to do_trap_hypervisor by do_trap_guest_sync.
> 
> For AArch64, there are two distincts exception depending whether the
> exception was taken from the current level (hypervisor) or lower level
> (guest).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/arm32/entry.S |  4 ++--
>  xen/arch/arm/arm64/entry.S |  6 +++---
>  xen/arch/arm/traps.c       | 17 ++++++++++++++++-
>  3 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index 05733089f7..120922e64e 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -152,14 +152,14 @@ GLOBAL(hyp_traps_vector)
>          b trap_hypervisor_call          /* 0x08 - Hypervisor Call */
>          b trap_prefetch_abort           /* 0x0c - Prefetch Abort */
>          b trap_data_abort               /* 0x10 - Data Abort */
> -        b trap_hypervisor               /* 0x14 - Hypervisor */
> +        b trap_guest_sync               /* 0x14 - Hypervisor */
>          b trap_irq                      /* 0x18 - IRQ */
>          b trap_fiq                      /* 0x1c - FIQ */
>  
>  DEFINE_TRAP_ENTRY(undefined_instruction)
>  DEFINE_TRAP_ENTRY(hypervisor_call)
>  DEFINE_TRAP_ENTRY(prefetch_abort)
> -DEFINE_TRAP_ENTRY(hypervisor)
> +DEFINE_TRAP_ENTRY(guest_sync)
>  DEFINE_TRAP_ENTRY_NOIRQ(irq)
>  DEFINE_TRAP_ENTRY_NOIRQ(fiq)
>  DEFINE_TRAP_ENTRY_NOABORT(data_abort)
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 137e67c674..06afc8a4e4 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -189,7 +189,7 @@ hyp_sync:
>          entry   hyp=1
>          msr     daifclr, #6
>          mov     x0, sp
> -        bl      do_trap_hypervisor
> +        bl      do_trap_hyp_sync
>          exit    hyp=1
>  
>  hyp_irq:
> @@ -211,7 +211,7 @@ guest_sync:
>                      SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>          msr     daifclr, #6
>          mov     x0, sp
> -        bl      do_trap_hypervisor
> +        bl      do_trap_guest_sync
>  1:
>          exit    hyp=0, compat=0
>  
> @@ -254,7 +254,7 @@ guest_sync_compat:
>                      SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>          msr     daifclr, #6
>          mov     x0, sp
> -        bl      do_trap_hypervisor
> +        bl      do_trap_guest_sync
>  1:
>          exit    hyp=0, compat=1
>  
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 6010c96c54..c8ce62ff96 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2805,7 +2805,7 @@ static void enter_hypervisor_head(struct cpu_user_regs *regs)
>      }
>  }
>  
> -asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
> +asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs)
>  {
>      const union hsr hsr = { .bits = regs->hsr };
>  
> @@ -2925,6 +2925,21 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>          do_trap_data_abort_guest(regs, hsr);
>          break;
>  
> +    default:
> +        printk("Guest Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
> +               hsr.bits, hsr.ec, hsr.len, hsr.iss);
> +        do_unexpected_trap("Guest", regs);

Given that this is a guest trap, I am wondering if panicing is actually
the correct behavior here.


> +    }
> +}
> +
> +asmlinkage void do_trap_hyp_sync(struct cpu_user_regs *regs)
> +{
> +    const union hsr hsr = { .bits = regs->hsr };
> +
> +    enter_hypervisor_head(regs);
> +
> +    switch ( hsr.ec )
> +    {
>  #ifdef CONFIG_ARM_64
>      case HSR_EC_BRK:
>          do_trap_brk(regs, hsr);
> -- 
> 2.11.0
>