* Size of irq field @ 2015-04-02 15:05 Iurii Konovalenko 2015-04-02 15:19 ` Ian Campbell 2015-04-02 15:34 ` Julien Grall 0 siblings, 2 replies; 7+ messages in thread From: Iurii Konovalenko @ 2015-04-02 15:05 UTC (permalink / raw) To: Julien Grall; +Cc: Embedded-pv-devel, xen-devel Hi, Julien! During bringing up Xen on Renesas Lager board we faced with problem. A lot of Xen sources relies on statement, that IRQ number is less then 256 and variables, parameters, fields etc. are of type uint8_t. But we can have IRQs, that are greater then 255, for example on RCar H2 SoC. Also, as I saw from one of your latest commits, GICv supports 1020 physical interrupts. As a result, overflow can occur. So it seems logical to increase all irq staff to uint16_t or uint32_t. We have local patches for increasing some structures, functions etc. But before pushing these patches I want to know your opinion, what do you think about this problem? What type should we use uint16_t or uint32_t? How to find all places where uint8_t type is used for IRQ? Best regards. Iurii Konovalenko | Senior Software Engineer GlobalLogic ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Size of irq field 2015-04-02 15:05 Size of irq field Iurii Konovalenko @ 2015-04-02 15:19 ` Ian Campbell 2015-04-02 15:34 ` Julien Grall 1 sibling, 0 replies; 7+ messages in thread From: Ian Campbell @ 2015-04-02 15:19 UTC (permalink / raw) To: Iurii Konovalenko; +Cc: Embedded-pv-devel, Julien Grall, xen-devel On Thu, 2015-04-02 at 18:05 +0300, Iurii Konovalenko wrote: > Hi, Julien! > > During bringing up Xen on Renesas Lager board we faced with problem. > A lot of Xen sources relies on statement, that IRQ number is less then > 256 and variables, parameters, fields etc. are of type uint8_t. Please can you give some example of this? Internally most irq stuff is unsigned int I think and "git grep uint8_t.*irq -- xen" is not showing lots of hits, there are a few but none which seem terribly scary or hard to fix. > But we > can have IRQs, that are greater then 255, for example on RCar H2 SoC. > Also, as I saw from one of your latest commits, GICv supports 1020 > physical interrupts. This has always been true, in fact Julien's commit was reducing the limit (which was too high), not increasing it. > As a result, overflow can occur. So it seems > logical to increase all irq staff to uint16_t or uint32_t. We have > local patches for increasing some structures, functions etc. > But before pushing these patches I want to know your opinion, what do > you think about this problem? What type should we use uint16_t or > uint32_t? How to find all places where uint8_t type is used for IRQ? We should certainly fix anywhere which is not using a large enough data type. Ian. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Size of irq field 2015-04-02 15:05 Size of irq field Iurii Konovalenko 2015-04-02 15:19 ` Ian Campbell @ 2015-04-02 15:34 ` Julien Grall 2015-04-03 12:40 ` Iurii Konovalenko 1 sibling, 1 reply; 7+ messages in thread From: Julien Grall @ 2015-04-02 15:34 UTC (permalink / raw) To: Iurii Konovalenko, Julien Grall Cc: Embedded-pv-devel, Stefano Stabellini, Ian Campbell, xen-devel (CC Stefano and Ian) On 02/04/2015 16:05, Iurii Konovalenko wrote: > Hi, Julien! Hello Iurii, > > During bringing up Xen on Renesas Lager board we faced with problem. > A lot of Xen sources relies on statement, that IRQ number is less then > 256 and variables, parameters, fields etc. are of type uint8_t. But we > can have IRQs, that are greater then 255, for example on RCar H2 SoC. > Also, as I saw from one of your latest commits, GICv supports 1020 > physical interrupts. As a result, overflow can occur. So it seems > logical to increase all irq staff to uint16_t or uint32_t. We have > local patches for increasing some structures, functions etc. > But before pushing these patches I want to know your opinion, what do > you think about this problem? What type should we use uint16_t or > uint32_t? How to find all places where uint8_t type is used for IRQ? I'm a bit surprised. I looked through the hypervisor and libxl and I was able to found only one place (DOMCTL_irq_permission which is not really supported on ARM). We are usually using uint32_t/int for IRQ. Can you give an example of files/structures using uint8_t for IRQ? Ideally the IRQ should use uint32_t. This is allow us to support LPIs (IRQ number start a 8192 up to a very high number). > Best regards. Regards, -- Julien Grall ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Size of irq field 2015-04-02 15:34 ` Julien Grall @ 2015-04-03 12:40 ` Iurii Konovalenko 2015-04-03 13:31 ` Julien Grall 0 siblings, 1 reply; 7+ messages in thread From: Iurii Konovalenko @ 2015-04-03 12:40 UTC (permalink / raw) To: Julien Grall Cc: Embedded-pv-devel, Julien Grall, Ian Campbell, Stefano Stabellini, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 2884 bytes --] Hi, Ian! Hi, Julien! Thank you for your replies. On Thu, Apr 2, 2015 at 6:19 PM, Ian Campbell <ian.campbell@citrix.com> wrote: > Please can you give some example of this? On Thu, Apr 2, 2015 at 6:34 PM, Julien Grall <julien.grall@citrix.com> wrote: > Can you give an example of files/structures using uint8_t for IRQ? As Julien wrote, it is related to irq_permission. I am not sure if it is supported on ARM, but we use it, and this function fails during domain creation, because IRQ number is truncated and permission operation is applied for incorrect IRQ, so domain creation fails. So, do we need to change logic and do not check IRQ permissions? In tools/libxc/include/xenctrl.h: int xc_domain_irq_permission(xc_interface *xch, uint32_t domid, *uint8_t* pirq, uint8_t allow_access); In tools/libxc/xc_domain.c: int xc_domain_irq_permission(xc_interface *xch, uint32_t domid, *uint8_t* pirq, uint8_t allow_access) In xen/include/public/domctl.h: struct xen_domctl_irq_permission { *uint8_t* pirq; uint8_t allow_access; }; Also, as we plan to use passtrough, we there are such places: In tools/libxc/include/xenctrl.h: int xc_domain_bind_pt_irq(xc_interface *xch, uint32_t domid, *uint8_t* machine_irq, uint8_t irq_type, uint8_t bus, uint8_t device, uint8_t intx, uint8_t isa_irq); int xc_domain_unbind_pt_irq(xc_interface *xch, uint32_t domid, *uint8_t* machine_irq, uint8_t irq_type, uint8_t bus, uint8_t device, uint8_t intx, uint8_t isa_irq); int xc_domain_bind_pt_pci_irq(xc_interface *xch, uint32_t domid, *uint8_t* machine_irq, uint8_t bus, uint8_t device, uint8_t intx); int xc_domain_bind_pt_isa_irq(xc_interface *xch, uint32_t domid, *uint8_t* machine_irq); And theirs implementation in tools/libxc/xc_domain.c On Thu, Apr 2, 2015 at 6:19 PM, Ian Campbell <ian.campbell@citrix.com> wrote: > We should certainly fix anywhere which is not using a large enough data > type. On Thu, Apr 2, 2015 at 6:34 PM, Julien Grall <julien.grall@citrix.com> wrote: > Ideally the IRQ should use uint32_t. This is allow us to support LPIs (IRQ > number start a 8192 up to a very high number). I have patch that increase size to uint32_t for cases, I described previously. Can I push it for review? Best regards. Iurii Konovalenko | Senior Software Engineer GlobalLogic [-- Attachment #1.2: Type: text/html, Size: 5056 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Size of irq field 2015-04-03 12:40 ` Iurii Konovalenko @ 2015-04-03 13:31 ` Julien Grall 2015-04-14 10:33 ` Ian Campbell 0 siblings, 1 reply; 7+ messages in thread From: Julien Grall @ 2015-04-03 13:31 UTC (permalink / raw) To: Iurii Konovalenko, Julien Grall Cc: Embedded-pv-devel, Julien Grall, xen-devel, Ian Campbell, Stefano Stabellini On 03/04/2015 14:40, Iurii Konovalenko wrote: > Hi, Ian! Hi, Julien! Hi Iurii, > Thank you for your replies. > > On Thu, Apr 2, 2015 at 6:19 PM, Ian Campbell <ian.campbell@citrix.com > <mailto:ian.campbell@citrix.com>> wrote: > > Please can you give some example of this? > On Thu, Apr 2, 2015 at 6:34 PM, Julien Grall <julien.grall@citrix.com > <mailto:julien.grall@citrix.com>> wrote: > > Can you give an example of files/structures using uint8_t for IRQ? > > As Julien wrote, it is related to irq_permission. I am not sure if it is > supported on ARM, but we use it, and this function fails during domain > creation, because IRQ number is truncated and permission operation is > applied for incorrect IRQ, so domain creation fails. So, do we need to > change logic and do not check IRQ permissions? Where do you need to check the IRQ permission? Currently (with my DT device passthrough series) only DOM0 is able to manage the IRQ (i.e bind/unbind) and we don't let the guest to do it. So I don't think it's necessary to to call xc_domain_irq_permission for guest. The DOMCTL is buggy because it relies on vIRQ == IRQ as many place in Xen. Although it would work now because, we chose to map 1:1 the IRQ in the guest. > Also, as we plan to use passtrough, we there are such places: > In tools/libxc/include/xenctrl.h: > int xc_domain_bind_pt_irq(xc_interface *xch, > uint32_t domid, > *uint8_t* machine_irq, > uint8_t irq_type, > uint8_t bus, > uint8_t device, > uint8_t intx, > uint8_t isa_irq); > > int xc_domain_unbind_pt_irq(xc_interface *xch, > uint32_t domid, > *uint8_t* machine_irq, > uint8_t irq_type, > uint8_t bus, > uint8_t device, > uint8_t intx, > uint8_t isa_irq); > > int xc_domain_bind_pt_pci_irq(xc_interface *xch, > uint32_t domid, > *uint8_t* machine_irq, > uint8_t bus, > uint8_t device, > uint8_t intx); > > int xc_domain_bind_pt_isa_irq(xc_interface *xch, > uint32_t domid, > *uint8_t* machine_irq); > > And theirs implementation in tools/libxc/xc_domain.c Whoops. I didn't spot those one thanks. > > On Thu, Apr 2, 2015 at 6:19 PM, Ian Campbell <ian.campbell@citrix.com > <mailto:ian.campbell@citrix.com>> wrote: > > We should certainly fix anywhere which is not using a large enough data > > type. > On Thu, Apr 2, 2015 at 6:34 PM, Julien Grall <julien.grall@citrix.com > <mailto:julien.grall@citrix.com>> wrote: > > Ideally the IRQ should use uint32_t. This is allow us to support LPIs > (IRQ > > number start a 8192 up to a very high number). > > I have patch that increase size to uint32_t for cases, I described > previously. Can I push it for review? Yes please. Regards, -- Julien Grall ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Size of irq field 2015-04-03 13:31 ` Julien Grall @ 2015-04-14 10:33 ` Ian Campbell 2015-04-14 10:34 ` Julien Grall 0 siblings, 1 reply; 7+ messages in thread From: Ian Campbell @ 2015-04-14 10:33 UTC (permalink / raw) To: Julien Grall Cc: Embedded-pv-devel, Iurii Konovalenko, Julien Grall, xen-devel, Stefano Stabellini On Fri, 2015-04-03 at 15:31 +0200, Julien Grall wrote: > > Also, as we plan to use passtrough, we there are such places: > > > In tools/libxc/include/xenctrl.h: > > int xc_domain_bind_pt_irq(xc_interface *xch, > > uint32_t domid, > > *uint8_t* machine_irq, > > uint8_t irq_type, > > uint8_t bus, > > uint8_t device, > > uint8_t intx, > > uint8_t isa_irq); > > > > int xc_domain_unbind_pt_irq(xc_interface *xch, > > uint32_t domid, > > *uint8_t* machine_irq, > > uint8_t irq_type, > > uint8_t bus, > > uint8_t device, > > uint8_t intx, > > uint8_t isa_irq); > > > > int xc_domain_bind_pt_pci_irq(xc_interface *xch, > > uint32_t domid, > > *uint8_t* machine_irq, > > uint8_t bus, > > uint8_t device, > > uint8_t intx); > > > > int xc_domain_bind_pt_isa_irq(xc_interface *xch, > > uint32_t domid, > > *uint8_t* machine_irq); > > > > And theirs implementation in tools/libxc/xc_domain.c > > Whoops. I didn't spot those one thanks. libxc is not ABI stable, we can make these bigger if necessary, or introduce new interface etc as required depending on how things pan out at the hypercall layer. Ian. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Size of irq field 2015-04-14 10:33 ` Ian Campbell @ 2015-04-14 10:34 ` Julien Grall 0 siblings, 0 replies; 7+ messages in thread From: Julien Grall @ 2015-04-14 10:34 UTC (permalink / raw) To: Ian Campbell Cc: Embedded-pv-devel, Iurii Konovalenko, Julien Grall, xen-devel, Stefano Stabellini On 14/04/15 11:33, Ian Campbell wrote: >> Whoops. I didn't spot those one thanks. > > libxc is not ABI stable, we can make these bigger if necessary, or > introduce new interface etc as required depending on how things pan out > at the hypercall layer. FIY, a patch has been sent on the ML see "[PATCH v3] increase size of irq from uint8_t to uint32_t" in order to fix it. Regards, -- Julien Grall ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-04-14 10:34 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-02 15:05 Size of irq field Iurii Konovalenko 2015-04-02 15:19 ` Ian Campbell 2015-04-02 15:34 ` Julien Grall 2015-04-03 12:40 ` Iurii Konovalenko 2015-04-03 13:31 ` Julien Grall 2015-04-14 10:33 ` Ian Campbell 2015-04-14 10:34 ` Julien Grall
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.