IRQ: u16 is too narrow for an event channel number
diff mbox series

Message ID cb84362c-d175-7a71-89cc-17ce0c523b09@suse.com
State New
Headers show
Series
  • IRQ: u16 is too narrow for an event channel number
Related show

Commit Message

Jan Beulich Jan. 14, 2020, 10:02 a.m. UTC
FIFO event channels allow ports up to 2^17, so we need to use a wider
field in struct pirq. Move "masked" such that it may share the 8-byte
slot with struct arch_pirq on 64-bit arches, rather than leaving a
7-byte hole in all cases.

Take the opportunity and also add a comment regarding "arch" placement
within the structure.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Julien Grall Jan. 14, 2020, 10:16 a.m. UTC | #1
Hi Jan,

On 14/01/2020 10:02, Jan Beulich wrote:
> FIFO event channels allow ports up to 2^17, so we need to use a wider
> field in struct pirq. Move "masked" such that it may share the 8-byte
> slot with struct arch_pirq on 64-bit arches, rather than leaving a
> 7-byte hole in all cases.
> 
> Take the opportunity and also add a comment regarding "arch" placement
> within the structure.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -127,9 +127,10 @@ struct vcpu;
>   
>   struct pirq {
>       int pirq;
> -    u16 evtchn;
> -    bool_t masked;
> +    evtchn_port_t evtchn;
>       struct rcu_head rcu_head;
> +    bool masked;
> +    /* Architectures may require this field to be last. */

I orginally planned to add a comment in struct pirq, but I am not in 
favor to continue to encourage quirkiness in the code. I sent a series 
(see [1]) that drop this requirements from x86 and therefore remove the 
need of this comment.

>       struct arch_pirq arch;
>   };
>   
> 

Cheers,

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2020-01/msg00924.html
Jan Beulich Jan. 14, 2020, 10:23 a.m. UTC | #2
On 14.01.2020 11:16, Julien Grall wrote:
> Hi Jan,
> 
> On 14/01/2020 10:02, Jan Beulich wrote:
>> FIFO event channels allow ports up to 2^17, so we need to use a wider
>> field in struct pirq. Move "masked" such that it may share the 8-byte
>> slot with struct arch_pirq on 64-bit arches, rather than leaving a
>> 7-byte hole in all cases.
>>
>> Take the opportunity and also add a comment regarding "arch" placement
>> within the structure.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/include/xen/irq.h
>> +++ b/xen/include/xen/irq.h
>> @@ -127,9 +127,10 @@ struct vcpu;
>>   
>>   struct pirq {
>>       int pirq;
>> -    u16 evtchn;
>> -    bool_t masked;
>> +    evtchn_port_t evtchn;
>>       struct rcu_head rcu_head;
>> +    bool masked;
>> +    /* Architectures may require this field to be last. */
> 
> I orginally planned to add a comment in struct pirq, but I am not in 
> favor to continue to encourage quirkiness in the code. I sent a series 
> (see [1]) that drop this requirements from x86 and therefore remove the 
> need of this comment.

Well, I've found the issue fixed here while doing a first review pass
over said patch of yours (I'm yet to make up my mind whether I have
reservations, hence I didn't reply yet). The change here will want
backporting, so it'll be easier in any event to rebase your patch over
this one. This rebasing can then include dropping the comment again.

Jan
Julien Grall Jan. 14, 2020, 10:39 a.m. UTC | #3
Hi Jan,

On 14/01/2020 10:23, Jan Beulich wrote:
> On 14.01.2020 11:16, Julien Grall wrote:
>> Hi Jan,
>>
>> On 14/01/2020 10:02, Jan Beulich wrote:
>>> FIFO event channels allow ports up to 2^17, so we need to use a wider
>>> field in struct pirq. Move "masked" such that it may share the 8-byte
>>> slot with struct arch_pirq on 64-bit arches, rather than leaving a
>>> 7-byte hole in all cases.
>>>
>>> Take the opportunity and also add a comment regarding "arch" placement
>>> within the structure.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/include/xen/irq.h
>>> +++ b/xen/include/xen/irq.h
>>> @@ -127,9 +127,10 @@ struct vcpu;
>>>    
>>>    struct pirq {
>>>        int pirq;
>>> -    u16 evtchn;
>>> -    bool_t masked;
>>> +    evtchn_port_t evtchn;
>>>        struct rcu_head rcu_head;
>>> +    bool masked;
>>> +    /* Architectures may require this field to be last. */
>>
>> I orginally planned to add a comment in struct pirq, but I am not in
>> favor to continue to encourage quirkiness in the code. I sent a series
>> (see [1]) that drop this requirements from x86 and therefore remove the
>> need of this comment.
> 
> Well, I've found the issue fixed here while doing a first review pass
> over said patch of yours (I'm yet to make up my mind whether I have
> reservations, hence I didn't reply yet). The change here will want
> backporting, so it'll be easier in any event to rebase your patch over
> this one. This rebasing can then include dropping the comment again.

Fair enough.

Cheers,
Andrew Cooper Jan. 14, 2020, 10:41 a.m. UTC | #4
On 14/01/2020 10:02, Jan Beulich wrote:
> FIFO event channels allow ports up to 2^17, so we need to use a wider
> field in struct pirq. Move "masked" such that it may share the 8-byte
> slot with struct arch_pirq on 64-bit arches, rather than leaving a
> 7-byte hole in all cases.
>
> Take the opportunity and also add a comment regarding "arch" placement
> within the structure.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Oops.  In fairness, it was entirely reasonable of the FIFO work to
presume that all code already used evtchn_port_t for evtchn ports.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -127,9 +127,10 @@ struct vcpu;
>  
>  struct pirq {
>      int pirq;
> -    u16 evtchn;
> -    bool_t masked;
> +    evtchn_port_t evtchn;
>      struct rcu_head rcu_head;
> +    bool masked;
> +    /* Architectures may require this field to be last. */
>      struct arch_pirq arch;
>  };
>

Patch
diff mbox series

--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -127,9 +127,10 @@  struct vcpu;
 
 struct pirq {
     int pirq;
-    u16 evtchn;
-    bool_t masked;
+    evtchn_port_t evtchn;
     struct rcu_head rcu_head;
+    bool masked;
+    /* Architectures may require this field to be last. */
     struct arch_pirq arch;
 };