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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxx>
> ---
>  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
> 

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