Re: [Xen-devel] [RFC 4/9] arm64: utilize time accounting

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Andrii,

Andrii Anisov writes:

> From: Andrii Anisov <andrii_anisov@xxxxxxxx>
>
> Call time accounting hooks from appropriate transition points
> of the ARM64 code.
I'd prefer more elaborate commit message. For example - what are
appropriate transition points? I mean - how you chose ones?

> Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> ---
>  xen/arch/arm/arm64/entry.S | 39 ++++++++++++++++++++++++++++++++++++---
>  xen/arch/arm/domain.c      |  2 ++
>  2 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 2d9a271..6fb2fa9 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -143,12 +143,21 @@
>
>          .endm
>
> -        .macro  exit, hyp, compat
> +        .macro  exit, hyp, compat, tacc=1
>
>          .if \hyp == 0         /* Guest mode */
>
> +     .if \tacc == 1
There is a hard tab, instead of 8 spaces.

Also, while it is easy to guess what 'hyp' and 'compat' mean, it is hard
to tell what 'tacc' stands for. I think, you need either better
name for this or a comment, which explains all parameters.

> +
> +        mov     x0, #1
> +        bl      tacc_hyp
> +
> +     .endif
The same about hard tabs. Probably, there are more of them in this patch.

> +
>          bl      leave_hypervisor_tail /* Disables interrupts on return */
>
> +     mov     x0, #1
> +     bl      tacc_guest
>          exit_guest \compat
>
>          .endif
> @@ -205,9 +214,15 @@ hyp_sync:
>
>  hyp_irq:
>          entry   hyp=1
> +        mov     x0,#5
Space is missing before #5

> +        bl      tacc_irq_enter
>          msr     daifclr, #4
>          mov     x0, sp
>          bl      do_trap_irq
> +
> +        mov     x0,#5
Space is missing before #5

> +        bl      tacc_irq_exit
> +
>          exit    hyp=1
>
>  guest_sync:
> @@ -291,6 +306,9 @@ guest_sync_slowpath:
>           * to save them.
>           */
>          entry   hyp=0, compat=0, save_x0_x1=0
> +
There are trailing whitespaces. I sure that 'git diff' highlights
such mistakes...

> +        mov     x0,#1
Space is missing before #1

> +        bl      tacc_gsync
>          /*
>           * The vSError will be checked while 
> SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>           * is not set. If a vSError took place, the initial exception will be
> @@ -307,6 +325,10 @@ guest_sync_slowpath:
>
>  guest_irq:
>          entry   hyp=0, compat=0
> +
> +        mov     x0,#6
Space is missing before #6

> +        bl      tacc_irq_enter
> +
>          /*
>           * The vSError will be checked while 
> SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>           * is not set. If a vSError took place, the initial exception will be
> @@ -319,6 +341,8 @@ guest_irq:
>          mov     x0, sp
>          bl      do_trap_irq
>  1:
> +     mov     x0,#6
Space is missing before #6, also looks like there is a hard tab character.

> +        bl      tacc_irq_exit
>          exit    hyp=0, compat=0
>
>  guest_fiq_invalid:
> @@ -334,6 +358,9 @@ guest_error:
>
>  guest_sync_compat:
>          entry   hyp=0, compat=1
> +
> +        mov     x0,#2
Space is missing before #2.

> +        bl      tacc_gsync
>          /*
>           * The vSError will be checked while 
> SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>           * is not set. If a vSError took place, the initial exception will be
> @@ -350,6 +377,10 @@ guest_sync_compat:
>
>  guest_irq_compat:
>          entry   hyp=0, compat=1
> +
> +        mov     x0,#7
Space is missing before #7.

> +        bl      tacc_irq_enter
> +
>          /*
>           * The vSError will be checked while 
> SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>           * is not set. If a vSError took place, the initial exception will be
> @@ -362,6 +393,8 @@ guest_irq_compat:
>          mov     x0, sp
>          bl      do_trap_irq
>  1:
> +        mov     x0,#7
Space is missing before #7...

> +        bl      tacc_irq_exit
>          exit    hyp=0, compat=1
>
>  guest_fiq_invalid_compat:
> @@ -376,9 +409,9 @@ guest_error_compat:
>          exit    hyp=0, compat=1
>
>  ENTRY(return_to_new_vcpu32)
> -        exit    hyp=0, compat=1
> +        exit    hyp=0, compat=1, tacc=0
>  ENTRY(return_to_new_vcpu64)
> -        exit    hyp=0, compat=0
> +        exit    hyp=0, compat=0, tacc=0
>
>  return_from_trap:
>          msr     daifset, #2 /* Mask interrupts */


> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index a9c4113..53ef630 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -51,11 +51,13 @@ static void do_idle(void)
>      process_pending_softirqs();
>
>      local_irq_disable();
> +    tacc_idle(1);
1 and 2 (below) look like some magical values to me.
I believe, you need to define some enumeration.

>      if ( cpu_is_haltable(cpu) )
>      {
>          dsb(sy);
>          wfi();
>      }
> +    tacc_hyp(2);
>      local_irq_enable();
>
>      sched_tick_resume();


--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel