Re: [PATCH v8a 1/6] xen/arm: move and rename is_vcpu_running function to sched.h

[Julien Grall <julien.grall@linaro.org>]

Hi Vijay,

On 07/03/2014 09:34 AM, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> is_vcpu_running function in vgic driver is generic. So move
> this to sched.h and rename it as is_vcpu_online
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> CC: jbeulich@suse.com
> CC: keir@xen.org
> CC: george.dunlap@citrix.com
> ---
> v8a:is_vcpu_online() is modified to take only vcpu as
>     parameter and validity checks on vcpu is done in
>     caller
> 
> v7a:Changed vcpuid parameter from int to unsigned int
>     Removed check for test_bit. Return !test_bit()
> ---
>  xen/arch/arm/vgic.c     |   22 ++++------------------
>  xen/include/xen/sched.h |    5 +++++
>  2 files changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 96bd7c1..4abc682 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -405,22 +405,6 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>      }
>  }
>  
> -static inline int is_vcpu_running(struct domain *d, int vcpuid)
> -{
> -    struct vcpu *v;
> -
> -    if ( vcpuid >= d->max_vcpus )
> -        return 0;
> -
> -    v = d->vcpu[vcpuid];
> -    if ( v == NULL )
> -        return 0;
> -    if (test_bit(_VPF_down, &v->pause_flags) )
> -        return 0;
> -
> -    return 1;
> -}
> -
>  static int vgic_to_sgi(struct vcpu *v, register_t sgir)
>  {
>      struct domain *d = v->domain;
> @@ -444,7 +428,8 @@ static int vgic_to_sgi(struct vcpu *v, register_t sgir)
>          case GICD_SGI_TARGET_OTHERS:
>              for ( i = 0; i < d->max_vcpus; i++ )
>              {
> -                if ( i != current->vcpu_id && is_vcpu_running(d, i) )
> +                if ( i != current->vcpu_id && d->vcpu != NULL && 

d->vcpu can't be NULL at this point. Otherwise you won't be able to run
VCPU0 of the domain :).

Although, I may have miss something during the previous version, why
don't you check that the VCPU is NULL in is_vcpu_online?

> +                     d->vcpu[i] != NULL && is_vcpu_online(d->vcpu[i]) )
>                      set_bit(i, &vcpu_mask);
>              }
>              break;
> @@ -459,7 +444,8 @@ static int vgic_to_sgi(struct vcpu *v, register_t sgir)
>  
>      for_each_set_bit( vcpuid, &vcpu_mask, d->max_vcpus )
>      {
> -        if ( !is_vcpu_running(d, vcpuid) )
> +        if ( d->vcpu != NULL && d->vcpu[vcpuid] != NULL &&

Same here.

> +             !is_vcpu_online(d->vcpu[vcpuid]) )
>          {
>              gdprintk(XENLOG_WARNING, "vGICD: GICD_SGIR write r=%"PRIregister" vcpu_mask=%lx, wrong CPUTargetList\n",
>                       sgir, vcpu_mask);

Regards,

-- 
Julien Grall