[Xen-devel] [PATCH] IRQ: u16 is too narrow for an event channel number

[Xen-devel] [PATCH] IRQ: u16 is too narrow for an event channel number

[Jan Beulich]

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. */
     struct arch_pirq arch;
 };
 

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

Re: [Xen-devel] [PATCH] IRQ: u16 is too narrow for an event channel number

[Julien Grall]

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

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH] IRQ: u16 is too narrow for an event channel number

[Jan Beulich]

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

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

Re: [Xen-devel] [PATCH] IRQ: u16 is too narrow for an event channel number

[Julien Grall]

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,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH] IRQ: u16 is too narrow for an event channel number

[Andrew Cooper]

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


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