Xen devel - Sep 2012 - [PATCH 1/2] x86: drop "index" parameter from get_free_pirq()

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);
         }
         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);





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

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

>>> On 05.09.12 at 14:36, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 05/09/12 13:24, Jan Beulich wrote:
>> @@ -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.
Yes, that would be more clean.

Jan