Size of irq field

Size of irq field

[Iurii Konovalenko]

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

Re: Size of irq field

[Ian Campbell]

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.

Re: Size of irq field

[Julien Grall]

(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

Re: Size of irq field

[Iurii Konovalenko]

[-- 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

Re: Size of irq field

[Julien Grall]

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

Re: Size of irq field

[Ian Campbell]

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.

Re: Size of irq field

[Julien Grall]

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