| Message ID | 1404912223-9320-2-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, 2014-07-09 at 14:23 +0100, Julien Grall wrote: > Xen is only able to handle one GIC controller. Some platform may contain > other interrupt controller. > > Make sure to only translate IRQ mapped into the GIC handled by Xen. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > Changes in v3: > - Add an ASSERT to check that dt_interrupt_controller is not > NULL. > > Changes in v2: > - Fix compilation... > --- > xen/common/device_tree.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index 310635e..cc45bd1 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -1442,9 +1442,12 @@ int dt_irq_translate(const struct dt_raw_irq *raw, > struct dt_irq *out_irq) > { > ASSERT(dt_irq_xlate != NULL); > + ASSERT(dt_interrupt_controller != NULL); > > - /* TODO: Retrieve the right irq_xlate. This is only work for the gic */ > + if ( raw->controller != dt_interrupt_controller ) > + return -EINVAL; > > + /* TODO: Retrieve the right irq_xlate. This is only work for the gic */ "This only works for ...". Do you mean it to say "primary gic"? In which case I think it was in the correct location before (i.e. before the check which enforced that). And you can probably say "primary interrupt controller" since this code doesn't care if it is a gic or not so long as it has set the callback correct. Ian. > return dt_irq_xlate(raw->specifier, raw->size, > &out_irq->irq, &out_irq->type); > }
On 16/07/14 14:14, Ian Campbell wrote: > On Wed, 2014-07-09 at 14:23 +0100, Julien Grall wrote: >> Xen is only able to handle one GIC controller. Some platform may contain >> other interrupt controller. >> >> Make sure to only translate IRQ mapped into the GIC handled by Xen. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> >> --- >> Changes in v3: >> - Add an ASSERT to check that dt_interrupt_controller is not >> NULL. >> >> Changes in v2: >> - Fix compilation... >> --- >> xen/common/device_tree.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c >> index 310635e..cc45bd1 100644 >> --- a/xen/common/device_tree.c >> +++ b/xen/common/device_tree.c >> @@ -1442,9 +1442,12 @@ int dt_irq_translate(const struct dt_raw_irq *raw, >> struct dt_irq *out_irq) >> { >> ASSERT(dt_irq_xlate != NULL); >> + ASSERT(dt_interrupt_controller != NULL); >> >> - /* TODO: Retrieve the right irq_xlate. This is only work for the gic */ >> + if ( raw->controller != dt_interrupt_controller ) >> + return -EINVAL; >> >> + /* TODO: Retrieve the right irq_xlate. This is only work for the gic */ > > "This only works for ...". > > Do you mean it to say "primary gic"? In which case I think it was in the > correct location before (i.e. before the check which enforced that). Which location are you talking about? The one in map_device? If so, it doesn't protect anything, but only avoid to assign an IRQ to DOM0 which is not routed to the primary interrupt controller. Checking the controller in the location will prevent platform_get_irq to translate IRQ not handled by the gic controller. Futhermore, it will avoid the new hypercall to list interrupt for a specific device node returning an invalid IRQ number. Regards,
On Wed, 2014-07-16 at 15:28 +0100, Julien Grall wrote: > >> - /* TODO: Retrieve the right irq_xlate. This is only work for the gic */ > >> + if ( raw->controller != dt_interrupt_controller ) > >> + return -EINVAL; > >> > >> + /* TODO: Retrieve the right irq_xlate. This is only work for the gic */ > > > > "This only works for ...". > > > > Do you mean it to say "primary gic"? In which case I think it was in the > > correct location before (i.e. before the check which enforced that). > > Which location are you talking about? The one in map_device? I meant the place from where it is removed by this patch. I was trying to say that the correct form would be (modulo the long line): /* TODO: Retrieve the right irq_xlate. This only works for the primary gic */ if ( raw->controller != dt_interrupt_controller ) return -EINVAL; return dt_...xlat(...) i.e. the comment belongs with the check. Ian.
On 16/07/14 15:34, Ian Campbell wrote: > On Wed, 2014-07-16 at 15:28 +0100, Julien Grall wrote: >>>> - /* TODO: Retrieve the right irq_xlate. This is only work for the gic */ >>>> + if ( raw->controller != dt_interrupt_controller ) >>>> + return -EINVAL; >>>> >>>> + /* TODO: Retrieve the right irq_xlate. This is only work for the gic */ >>> >>> "This only works for ...". >>> >>> Do you mean it to say "primary gic"? In which case I think it was in the >>> correct location before (i.e. before the check which enforced that). >> >> Which location are you talking about? The one in map_device? > > I meant the place from where it is removed by this patch. I was trying > to say that the correct form would be (modulo the long line): > > /* TODO: Retrieve the right irq_xlate. This only works for the primary gic */ > if ( raw->controller != dt_interrupt_controller ) > return -EINVAL; > > return dt_...xlat(...) > > i.e. the comment belongs with the check. Oh ok. I will it before the check. Regards,
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index 310635e..cc45bd1 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -1442,9 +1442,12 @@ int dt_irq_translate(const struct dt_raw_irq *raw, struct dt_irq *out_irq) { ASSERT(dt_irq_xlate != NULL); + ASSERT(dt_interrupt_controller != NULL); - /* TODO: Retrieve the right irq_xlate. This is only work for the gic */ + if ( raw->controller != dt_interrupt_controller ) + return -EINVAL; + /* TODO: Retrieve the right irq_xlate. This is only work for the gic */ return dt_irq_xlate(raw->specifier, raw->size, &out_irq->irq, &out_irq->type); }
Xen is only able to handle one GIC controller. Some platform may contain other interrupt controller. Make sure to only translate IRQ mapped into the GIC handled by Xen. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v3: - Add an ASSERT to check that dt_interrupt_controller is not NULL. Changes in v2: - Fix compilation... --- xen/common/device_tree.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)