Re: [Xen-devel] [PATCH 01/18] xen/arm: Move code that initializes VCPU context into a separate function

[Wei Liu <wei.liu2@xxxxxxxxxx>]

First thing first, thanks for posting this series.  I'm not an ARM
maintainer but I do have a few comments.

On Mon, Nov 12, 2018 at 12:30:27PM +0100, Mirela Simonovic wrote:
> From: Saeed Nowshadi <saeed.nowshadi@xxxxxxxxxx>
> 
> The arch_set_info_guest() has code to initialize the context of a VCPU.
> When a VCPU is resumed it needs to go through the same context
> initialization excluding all the validations that this routine does.

OOI why does it not need validation?

> We move the actual VCPU context setting into a function so that it can be
> shared with the resume path.
> 
> Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xxxxxxxxxx>
> Signed-off-by: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> ---
>  xen/arch/arm/domain.c    | 34 +++++++++++++++++++++-------------
>  xen/include/xen/domain.h |  1 +
>  2 files changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 80432872d6..e594b48d81 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -781,6 +781,26 @@ static int is_guest_pv64_psr(uint32_t psr)
>  #endif
>  
>  /*
> + * The actual VCPU initialization after all validations are passed.
> + */
> +void _arch_set_info_guest(struct vcpu *v, struct vcpu_guest_context *ctxt)

A better name is needed, maybe arch_set_info_guest_no_validation?

> +{
> +    vcpu_regs_user_to_hyp(v, &ctxt->user_regs);
> +
> +    v->arch.sctlr = ctxt->sctlr;
> +    v->arch.ttbr0 = ctxt->ttbr0;
> +    v->arch.ttbr1 = ctxt->ttbr1;
> +    v->arch.ttbcr = ctxt->ttbcr;
> +
> +    v->is_initialised = 1;
> +
> +    if ( ctxt->flags & VGCF_online )
> +        clear_bit(_VPF_down, &v->pause_flags);
> +    else
> +        set_bit(_VPF_down, &v->pause_flags);
> +}
> +
> +/*
>   * Initialise VCPU state. The context can be supplied by either the
>   * toolstack (XEN_DOMCTL_setvcpucontext) or the guest
>   * (VCPUOP_initialise) and therefore must be properly validated.
> @@ -818,19 +838,7 @@ int arch_set_info_guest(
>      }
>  #endif
>  
> -    vcpu_regs_user_to_hyp(v, regs);
> -
> -    v->arch.sctlr = ctxt->sctlr;
> -    v->arch.ttbr0 = ctxt->ttbr0;
> -    v->arch.ttbr1 = ctxt->ttbr1;
> -    v->arch.ttbcr = ctxt->ttbcr;
> -
> -    v->is_initialised = 1;
> -
> -    if ( ctxt->flags & VGCF_online )
> -        clear_bit(_VPF_down, &v->pause_flags);
> -    else
> -        set_bit(_VPF_down, &v->pause_flags);
> +    _arch_set_info_guest(v, ctxt);
>  
>      return 0;
>  }
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index 33e41486cb..904624e070 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -73,6 +73,7 @@ int arch_domain_soft_reset(struct domain *d);
>  void arch_p2m_set_access_required(struct domain *d, bool access_required);
>  
>  int arch_set_info_guest(struct vcpu *, vcpu_guest_context_u);
> +void _arch_set_info_guest(struct vcpu *, struct vcpu_guest_context *);

Please put the new function's declaration into asm-arm/domain.h.

Normally we only introduce declaration when the function is needed
externally. Please at least say in the commit message that the
introduction of the declaration is because it will be needed in later
patches.

Wei.

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