| Message ID | cb84362c-d175-7a71-89cc-17ce0c523b09@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IRQ: u16 is too narrow for an event channel number | expand |
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
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
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,
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; > }; >
--- 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; };
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>