Re: [PATCH 1/2] x86: drop "index" parameter from get_free_pirq()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 05/09/12 13:24, Jan Beulich wrote:
> It's unused.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -1816,7 +1816,7 @@ static inline bool_t is_free_pirq(const 
>          pirq->arch.hvm.emuirq == IRQ_UNBOUND));
>  }
>  
> -int get_free_pirq(struct domain *d, int type, int index)
> +int get_free_pirq(struct domain *d, int type)
>  {
>      int i;
>  
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -71,7 +71,7 @@ static int physdev_hvm_map_pirq(
>          else
>          {
>              if ( *pirq < 0 )
> -                *pirq = get_free_pirq(d, type, *index);
> +                *pirq = get_free_pirq(d, type);
>              ret = map_domain_emuirq_pirq(d, *pirq, *index);


Relatedly (and I had already noticed this but not got round to making a
patch because of other more urgent bugs)

You still have a chance here of passing an error into
map_domain_emuirq_pirq, in the pirq value.  This is not a security issue
as map_domain_emuirq_pirq does range check pirq, but may turn into a
problem if the implementation of map_domain_emuirq_pirq changes.  I
would say that for correctness sake, physdev_hvm_map_pirq() should range
check get_free_pirq(), even if this will lead to a double range check of
the value.

~Andrew

>          }
>          break;
> @@ -187,7 +187,7 @@ int physdev_map_pirq(domid_t domid, int 
>          }
>          else
>          {
> -            pirq = get_free_pirq(d, type, *index);
> +            pirq = get_free_pirq(d, type);
>              if ( pirq < 0 )
>              {
>                  dprintk(XENLOG_G_ERR, "dom%d: no free pirq\n", d->domain_id);
> @@ -705,7 +705,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
>              break;
>  
>          spin_lock(&d->event_lock);
> -        ret = get_free_pirq(d, out.type, 0);
> +        ret = get_free_pirq(d, out.type);
>          if ( ret >= 0 )
>          {
>              struct pirq *info = pirq_get_info(d, ret);
> --- a/xen/include/asm-x86/irq.h
> +++ b/xen/include/asm-x86/irq.h
> @@ -136,7 +136,7 @@ int pirq_shared(struct domain *d , int i
>  int map_domain_pirq(struct domain *d, int pirq, int irq, int type,
>                             void *data);
>  int unmap_domain_pirq(struct domain *d, int pirq);
> -int get_free_pirq(struct domain *d, int type, int index);
> +int get_free_pirq(struct domain *d, int type);
>  void free_domain_pirqs(struct domain *d);
>  int map_domain_emuirq_pirq(struct domain *d, int pirq, int irq);
>  int unmap_domain_pirq_emuirq(struct domain *d, int pirq);
>
>
>

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com