* [PATCH RFC 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards.
2016-09-05 10:13 [PATCH RFC 0/6] ARM: Add support for Tegra SoCs (incl. Jetson TK1, Jetson TX1) Kyle Temkin
@ 2016-09-05 10:13 ` Kyle Temkin
2016-09-13 21:06 ` Konrad Rzeszutek Wilk
2016-09-05 10:13 ` [PATCH RFC 2/6] xen/arm: hwdom GIC: inherit interrupt parent from host device tree Kyle Temkin
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Kyle Temkin @ 2016-09-05 10:13 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, sstabellini, Kyle J. Temkin
From: "Kyle J. Temkin" <temkink@ainfosec.com>
Tegra boards feature a NS16550-compatible serial mapped into the MMIO
space. Add support for its use both as a full-featured serial port and
as an earlyprintk driver.
Adds a new "needs_rtoie" (requires Rx Timeout Interrupt) quirk, as some
platforms-- including Tegra-- require the NS16550 Rx timeout interrupt
to be enabled for receive to function properly. The same quirk is
applied in the eqvuialent Linux driver [1].
[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4539c24fe4f92c09ee668ef959d3e8180df619b9
Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
---
xen/arch/arm/Rules.mk | 1 +
xen/drivers/char/ns16550.c | 16 +++++++++++++++-
xen/include/xen/8250-uart.h | 1 +
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
index 93304be..2a1473a 100644
--- a/xen/arch/arm/Rules.mk
+++ b/xen/arch/arm/Rules.mk
@@ -44,6 +44,7 @@ EARLY_PRINTK_vexpress := pl011,0x1c090000
EARLY_PRINTK_xgene-mcdivitt := 8250,0x1c021000,2
EARLY_PRINTK_xgene-storm := 8250,0x1c020000,2
EARLY_PRINTK_zynqmp := cadence,0xff000000
+EARLY_PRINTK_tegra := 8250,0x70006000,2
ifneq ($(EARLY_PRINTK_$(CONFIG_EARLY_PRINTK)),)
EARLY_PRINTK_CFG := $(subst $(comma), ,$(EARLY_PRINTK_$(CONFIG_EARLY_PRINTK)))
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 1da103a..bffdb35 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -64,6 +64,7 @@ static struct ns16550 {
unsigned int timeout_ms;
bool_t intr_works;
bool_t dw_usr_bsy;
+ bool_t needs_rtoie;
#ifdef CONFIG_HAS_PCI
/* PCI card parameters. */
bool_t pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
@@ -652,12 +653,23 @@ static void ns16550_setup_postirq(struct ns16550 *uart)
{
if ( uart->irq > 0 )
{
+ u8 ier_value = 0;
+
/* Master interrupt enable; also keep DTR/RTS asserted. */
ns_write_reg(uart,
UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS);
/* Enable receive interrupts. */
- ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
+ ier_value = UART_IER_ERDAI;
+
+ /*
+ * If we're on a platform that needs Rx timeouts enabled, also
+ * set Rx TimeOut Interrupt Enable (RTOIE).
+ */
+ if ( uart->needs_rtoie )
+ ier_value |= UART_IER_RTOIE;
+
+ ns_write_reg(uart, UART_IER, ier_value);
}
if ( uart->irq >= 0 )
@@ -1273,6 +1285,7 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
uart->irq = res;
uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
+ uart->needs_rtoie = dt_device_is_compatible(dev, "nvidia,tegra20-uart");
uart->vuart.base_addr = uart->io_base;
uart->vuart.size = uart->io_size;
@@ -1293,6 +1306,7 @@ static const struct dt_device_match ns16550_dt_match[] __initconst =
DT_MATCH_COMPATIBLE("ns16550"),
DT_MATCH_COMPATIBLE("ns16550a"),
DT_MATCH_COMPATIBLE("snps,dw-apb-uart"),
+ DT_MATCH_COMPATIBLE("nvidia,tegra20-uart"),
{ /* sentinel */ },
};
diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
index c6b62c8..2ad0ee6 100644
--- a/xen/include/xen/8250-uart.h
+++ b/xen/include/xen/8250-uart.h
@@ -41,6 +41,7 @@
#define UART_IER_ETHREI 0x02 /* tx reg. empty */
#define UART_IER_ELSI 0x04 /* rx line status */
#define UART_IER_EMSI 0x08 /* MODEM status */
+#define UART_IER_RTOIE 0x10 /* rx timeout */
/* Interrupt Identificatiegister */
#define UART_IIR_NOINT 0x01 /* no interrupt pending */
--
2.9.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH RFC 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards.
2016-09-05 10:13 ` [PATCH RFC 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards Kyle Temkin
@ 2016-09-13 21:06 ` Konrad Rzeszutek Wilk
2016-10-21 21:41 ` Stefano Stabellini
0 siblings, 1 reply; 17+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-13 21:06 UTC (permalink / raw)
To: Kyle Temkin; +Cc: julien.grall, sstabellini, Kyle J. Temkin, xen-devel
On Mon, Sep 05, 2016 at 06:13:56AM -0400, Kyle Temkin wrote:
> From: "Kyle J. Temkin" <temkink@ainfosec.com>
>
> Tegra boards feature a NS16550-compatible serial mapped into the MMIO
> space. Add support for its use both as a full-featured serial port and
> as an earlyprintk driver.
>
> Adds a new "needs_rtoie" (requires Rx Timeout Interrupt) quirk, as some
> platforms-- including Tegra-- require the NS16550 Rx timeout interrupt
> to be enabled for receive to function properly. The same quirk is
> applied in the eqvuialent Linux driver [1].
>
> [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4539c24fe4f92c09ee668ef959d3e8180df619b9
>
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> ---
> xen/arch/arm/Rules.mk | 1 +
> xen/drivers/char/ns16550.c | 16 +++++++++++++++-
> xen/include/xen/8250-uart.h | 1 +
> 3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index 93304be..2a1473a 100644
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -44,6 +44,7 @@ EARLY_PRINTK_vexpress := pl011,0x1c090000
> EARLY_PRINTK_xgene-mcdivitt := 8250,0x1c021000,2
> EARLY_PRINTK_xgene-storm := 8250,0x1c020000,2
> EARLY_PRINTK_zynqmp := cadence,0xff000000
> +EARLY_PRINTK_tegra := 8250,0x70006000,2
>
> ifneq ($(EARLY_PRINTK_$(CONFIG_EARLY_PRINTK)),)
> EARLY_PRINTK_CFG := $(subst $(comma), ,$(EARLY_PRINTK_$(CONFIG_EARLY_PRINTK)))
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 1da103a..bffdb35 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -64,6 +64,7 @@ static struct ns16550 {
> unsigned int timeout_ms;
> bool_t intr_works;
> bool_t dw_usr_bsy;
> + bool_t needs_rtoie;
Those three states all look like tri-states? Or could you potentially
have them independenty enabled? As in dw_usr_bsy and needs_rtoie?
> #ifdef CONFIG_HAS_PCI
> /* PCI card parameters. */
> bool_t pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
> @@ -652,12 +653,23 @@ static void ns16550_setup_postirq(struct ns16550 *uart)
> {
> if ( uart->irq > 0 )
> {
> + u8 ier_value = 0;
> +
> /* Master interrupt enable; also keep DTR/RTS asserted. */
> ns_write_reg(uart,
> UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS);
>
> /* Enable receive interrupts. */
> - ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
> + ier_value = UART_IER_ERDAI;
> +
> + /*
> + * If we're on a platform that needs Rx timeouts enabled, also
> + * set Rx TimeOut Interrupt Enable (RTOIE).
> + */
> + if ( uart->needs_rtoie )
> + ier_value |= UART_IER_RTOIE;
> +
> + ns_write_reg(uart, UART_IER, ier_value);
> }
>
> if ( uart->irq >= 0 )
> @@ -1273,6 +1285,7 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
> uart->irq = res;
>
> uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
> + uart->needs_rtoie = dt_device_is_compatible(dev, "nvidia,tegra20-uart");
>
> uart->vuart.base_addr = uart->io_base;
> uart->vuart.size = uart->io_size;
> @@ -1293,6 +1306,7 @@ static const struct dt_device_match ns16550_dt_match[] __initconst =
> DT_MATCH_COMPATIBLE("ns16550"),
> DT_MATCH_COMPATIBLE("ns16550a"),
> DT_MATCH_COMPATIBLE("snps,dw-apb-uart"),
> + DT_MATCH_COMPATIBLE("nvidia,tegra20-uart"),
> { /* sentinel */ },
> };
>
> diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
> index c6b62c8..2ad0ee6 100644
> --- a/xen/include/xen/8250-uart.h
> +++ b/xen/include/xen/8250-uart.h
> @@ -41,6 +41,7 @@
> #define UART_IER_ETHREI 0x02 /* tx reg. empty */
> #define UART_IER_ELSI 0x04 /* rx line status */
> #define UART_IER_EMSI 0x08 /* MODEM status */
> +#define UART_IER_RTOIE 0x10 /* rx timeout */
>
> /* Interrupt Identificatiegister */
> #define UART_IIR_NOINT 0x01 /* no interrupt pending */
> --
> 2.9.2
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH RFC 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards.
2016-09-13 21:06 ` Konrad Rzeszutek Wilk
@ 2016-10-21 21:41 ` Stefano Stabellini
0 siblings, 0 replies; 17+ messages in thread
From: Stefano Stabellini @ 2016-10-21 21:41 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: julien.grall, xen-devel, Kyle J. Temkin, sstabellini, Kyle Temkin
On Tue, 13 Sep 2016, Konrad Rzeszutek Wilk wrote:
> On Mon, Sep 05, 2016 at 06:13:56AM -0400, Kyle Temkin wrote:
> > From: "Kyle J. Temkin" <temkink@ainfosec.com>
> >
> > Tegra boards feature a NS16550-compatible serial mapped into the MMIO
> > space. Add support for its use both as a full-featured serial port and
> > as an earlyprintk driver.
> >
> > Adds a new "needs_rtoie" (requires Rx Timeout Interrupt) quirk, as some
> > platforms-- including Tegra-- require the NS16550 Rx timeout interrupt
> > to be enabled for receive to function properly. The same quirk is
> > applied in the eqvuialent Linux driver [1].
^ equivalent
> > [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4539c24fe4f92c09ee668ef959d3e8180df619b9
> >
> > Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> > ---
> > xen/arch/arm/Rules.mk | 1 +
> > xen/drivers/char/ns16550.c | 16 +++++++++++++++-
> > xen/include/xen/8250-uart.h | 1 +
> > 3 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> > index 93304be..2a1473a 100644
> > --- a/xen/arch/arm/Rules.mk
> > +++ b/xen/arch/arm/Rules.mk
> > @@ -44,6 +44,7 @@ EARLY_PRINTK_vexpress := pl011,0x1c090000
> > EARLY_PRINTK_xgene-mcdivitt := 8250,0x1c021000,2
> > EARLY_PRINTK_xgene-storm := 8250,0x1c020000,2
> > EARLY_PRINTK_zynqmp := cadence,0xff000000
> > +EARLY_PRINTK_tegra := 8250,0x70006000,2
> >
> > ifneq ($(EARLY_PRINTK_$(CONFIG_EARLY_PRINTK)),)
> > EARLY_PRINTK_CFG := $(subst $(comma), ,$(EARLY_PRINTK_$(CONFIG_EARLY_PRINTK)))
> > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> > index 1da103a..bffdb35 100644
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -64,6 +64,7 @@ static struct ns16550 {
> > unsigned int timeout_ms;
> > bool_t intr_works;
> > bool_t dw_usr_bsy;
> > + bool_t needs_rtoie;
>
> Those three states all look like tri-states? Or could you potentially
> have them independenty enabled? As in dw_usr_bsy and needs_rtoie?
Yes, it would be nice to turn dw_usr_bsy and needs_rtoie, which are
hardware features/workarounds, into a bitmask. Something like:
#define DW_USR_BSY (1<<0)
#define NEEDS_RTOIE (1<<1)
uint8_t hw_feats;
> > #ifdef CONFIG_HAS_PCI
> > /* PCI card parameters. */
> > bool_t pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
> > @@ -652,12 +653,23 @@ static void ns16550_setup_postirq(struct ns16550 *uart)
> > {
> > if ( uart->irq > 0 )
> > {
> > + u8 ier_value = 0;
> > +
> > /* Master interrupt enable; also keep DTR/RTS asserted. */
> > ns_write_reg(uart,
> > UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS);
> >
> > /* Enable receive interrupts. */
> > - ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
> > + ier_value = UART_IER_ERDAI;
> > +
> > + /*
> > + * If we're on a platform that needs Rx timeouts enabled, also
> > + * set Rx TimeOut Interrupt Enable (RTOIE).
> > + */
> > + if ( uart->needs_rtoie )
> > + ier_value |= UART_IER_RTOIE;
> > +
> > + ns_write_reg(uart, UART_IER, ier_value);
> > }
> >
> > if ( uart->irq >= 0 )
> > @@ -1273,6 +1285,7 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
> > uart->irq = res;
> >
> > uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
> > + uart->needs_rtoie = dt_device_is_compatible(dev, "nvidia,tegra20-uart");
> >
> > uart->vuart.base_addr = uart->io_base;
> > uart->vuart.size = uart->io_size;
> > @@ -1293,6 +1306,7 @@ static const struct dt_device_match ns16550_dt_match[] __initconst =
> > DT_MATCH_COMPATIBLE("ns16550"),
> > DT_MATCH_COMPATIBLE("ns16550a"),
> > DT_MATCH_COMPATIBLE("snps,dw-apb-uart"),
> > + DT_MATCH_COMPATIBLE("nvidia,tegra20-uart"),
> > { /* sentinel */ },
> > };
> >
> > diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
> > index c6b62c8..2ad0ee6 100644
> > --- a/xen/include/xen/8250-uart.h
> > +++ b/xen/include/xen/8250-uart.h
> > @@ -41,6 +41,7 @@
> > #define UART_IER_ETHREI 0x02 /* tx reg. empty */
> > #define UART_IER_ELSI 0x04 /* rx line status */
> > #define UART_IER_EMSI 0x08 /* MODEM status */
> > +#define UART_IER_RTOIE 0x10 /* rx timeout */
> >
> > /* Interrupt Identificatiegister */
> > #define UART_IIR_NOINT 0x01 /* no interrupt pending */
> > --
> > 2.9.2
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > https://lists.xen.org/xen-devel
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH RFC 2/6] xen/arm: hwdom GIC: inherit interrupt parent from host device tree
2016-09-05 10:13 [PATCH RFC 0/6] ARM: Add support for Tegra SoCs (incl. Jetson TK1, Jetson TX1) Kyle Temkin
2016-09-05 10:13 ` [PATCH RFC 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards Kyle Temkin
@ 2016-09-05 10:13 ` Kyle Temkin
2016-10-21 22:18 ` Stefano Stabellini
2016-09-05 10:13 ` [PATCH RFC 3/6] xen/arm: Allow platforms to hook IRQ routing Kyle Temkin
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Kyle Temkin @ 2016-09-05 10:13 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, sstabellini, Kyle J. Temkin
From: "Kyle J. Temkin" <temkink@ainfosec.com>
Currently, we don't copy in the interrupt parent from the host device
tree; and instead let Xen automatically figure it out when generating
the device tree for the hardware domain.
In cases where a non-GIC interrupt controller is present, this can lead
to incorrect assignment of interrupt parents, and throughly confuse the
hwdom. Instead of letting this fall to chance, pass through the
phandles.
Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
---
xen/arch/arm/domain_build.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 35ab08d..52c9a01 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -764,8 +764,8 @@ static int make_gic_node(const struct domain *d, void *fdt,
{
const struct dt_device_node *gic = dt_interrupt_controller;
int res = 0;
- const void *addrcells, *sizecells;
- u32 addrcells_len, sizecells_len;
+ const void *addrcells, *sizecells, *iparent;
+ u32 addrcells_len, sizecells_len, iparent_len;
/*
* Xen currently supports only a single GIC. Discard any secondary
@@ -795,6 +795,14 @@ static int make_gic_node(const struct domain *d, void *fdt,
return res;
}
+ iparent = dt_get_property(gic, "interrupt-parent", &iparent_len);
+ if ( iparent )
+ {
+ res = fdt_property(fdt, "interrupt-parent", iparent, iparent_len);
+ if ( res )
+ return res;
+ }
+
addrcells = dt_get_property(gic, "#address-cells", &addrcells_len);
if ( addrcells )
{
--
2.9.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH RFC 2/6] xen/arm: hwdom GIC: inherit interrupt parent from host device tree
2016-09-05 10:13 ` [PATCH RFC 2/6] xen/arm: hwdom GIC: inherit interrupt parent from host device tree Kyle Temkin
@ 2016-10-21 22:18 ` Stefano Stabellini
0 siblings, 0 replies; 17+ messages in thread
From: Stefano Stabellini @ 2016-10-21 22:18 UTC (permalink / raw)
To: Kyle Temkin; +Cc: julien.grall, sstabellini, Kyle J. Temkin, xen-devel
Sorry for the late review, I promise I'll be faster next time :-)
On Mon, 5 Sep 2016, Kyle Temkin wrote:
> From: "Kyle J. Temkin" <temkink@ainfosec.com>
>
> Currently, we don't copy in the interrupt parent from the host device
> tree; and instead let Xen automatically figure it out when generating
> the device tree for the hardware domain.
>
> In cases where a non-GIC interrupt controller is present, this can lead
> to incorrect assignment of interrupt parents, and throughly confuse the
> hwdom. Instead of letting this fall to chance, pass through the
> phandles.
Let me get this straight: the problem is that domain_build.c doesn't get
the interrupt-parent property from the host device tree, instead it just
generates it by blindly pointing interrupt-parent to the gic. This
happens in fdt_property_interrupts for generated nodes, such as the
hypervisor and timer nodes. Other nodes are typically just copied over
as is, so they are not affected.
This is OK, unless one of these nodes actually need a different
interrupt-parent. However these nodes don't really correspond to regular
devices and shouldn't actually need a different interrupt-parent.
Certainly not the hypervisor, timer, cpus and memory nodes.
This only leaves out the GIC node, and in fact this patch is setting
interrupt-parent from the host value just for the GIC node.
Looking through device trees I found arch/arm/boot/dts/tegra30.dtsi,
which shows exactly this issue:
intc: interrupt-controller@50041000 {
compatible = "arm,cortex-a9-gic";
reg = <0x50041000 0x1000
0x50040100 0x0100>;
interrupt-controller;
#interrupt-cells = <3>;
interrupt-parent = <&intc>;
};
Did I get it right?
If so, please expand the commit message, to include this info. Also add
an in code comment to explain why you are setting interrupt-parent for
the gic node.
With these changes:
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> ---
> xen/arch/arm/domain_build.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 35ab08d..52c9a01 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -764,8 +764,8 @@ static int make_gic_node(const struct domain *d, void *fdt,
> {
> const struct dt_device_node *gic = dt_interrupt_controller;
> int res = 0;
> - const void *addrcells, *sizecells;
> - u32 addrcells_len, sizecells_len;
> + const void *addrcells, *sizecells, *iparent;
> + u32 addrcells_len, sizecells_len, iparent_len;
>
> /*
> * Xen currently supports only a single GIC. Discard any secondary
> @@ -795,6 +795,14 @@ static int make_gic_node(const struct domain *d, void *fdt,
> return res;
> }
>
> + iparent = dt_get_property(gic, "interrupt-parent", &iparent_len);
> + if ( iparent )
> + {
> + res = fdt_property(fdt, "interrupt-parent", iparent, iparent_len);
> + if ( res )
> + return res;
> + }
>
> addrcells = dt_get_property(gic, "#address-cells", &addrcells_len);
> if ( addrcells )
> {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH RFC 3/6] xen/arm: Allow platforms to hook IRQ routing.
2016-09-05 10:13 [PATCH RFC 0/6] ARM: Add support for Tegra SoCs (incl. Jetson TK1, Jetson TX1) Kyle Temkin
2016-09-05 10:13 ` [PATCH RFC 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards Kyle Temkin
2016-09-05 10:13 ` [PATCH RFC 2/6] xen/arm: hwdom GIC: inherit interrupt parent from host device tree Kyle Temkin
@ 2016-09-05 10:13 ` Kyle Temkin
2016-10-21 23:28 ` Stefano Stabellini
2016-11-01 14:56 ` Julien Grall
2016-09-05 10:13 ` [PATCH RFC 4/6] xen/arm: platforms: Add Tegra platform to support basic " Kyle Temkin
` (2 subsequent siblings)
5 siblings, 2 replies; 17+ messages in thread
From: Kyle Temkin @ 2016-09-05 10:13 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, sstabellini, Kyle J. Temkin
From: "Kyle J. Temkin" <temkink@ainfosec.com>
Some common platforms (e.g. Tegra) have non-traditional IRQ controllers
that must be programmed in addition to their primary GICs-- and which
can come in unusual topologies. Device trees for targets that feature
these controllers often deviate from the conventions that Xen expects.
This commit provides a foundation for support of these platforms, by:
- Allowing the platform to decide which IRQs can be routed by Xen,
rather than assuming that only GIC-connected IRQs can be routed.
- Allowing the platform to extend/replace existing IRQ routing logic,
rather than asssuming that the GIC will always be programmed to route
IRQs.
- Allows the platform to override IRQ translation, rather than assuming
GIC translation will always be followed. This is useful in cases where
device tree IRQ numbers don't correspond to GIC IRQ numbers.
Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
---
xen/arch/arm/domain_build.c | 14 +++++++++-----
xen/arch/arm/irq.c | 5 +++--
xen/arch/arm/platform.c | 39 ++++++++++++++++++++++++++++++++++++++
xen/arch/arm/time.c | 2 +-
xen/drivers/char/cadence-uart.c | 3 ++-
xen/drivers/char/exynos4210-uart.c | 3 ++-
xen/drivers/char/ns16550.c | 3 ++-
xen/drivers/char/omap-uart.c | 3 ++-
xen/drivers/char/pl011.c | 3 ++-
xen/drivers/char/scif-uart.c | 3 ++-
xen/drivers/passthrough/arm/smmu.c | 4 ++--
xen/include/asm-arm/platform.h | 14 ++++++++++++++
12 files changed, 80 insertions(+), 16 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 52c9a01..402c766 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1094,16 +1094,20 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
/*
* Don't map IRQ that have no physical meaning
- * ie: IRQ whose controller is not the GIC
+ * ie: IRQ that does not wind up being controlled by the GIC
+ * (Note that we can't just check to see if an IRQ is owned by the GIC,
+ * as some platforms have a controller between the device irq and the GIC,
+ * such as the Tegra legacy interrupt controller.)
*/
- if ( rirq.controller != dt_interrupt_controller )
+ if ( !platform_irq_is_routable(&rirq) )
{
- dt_dprintk("irq %u not connected to primary controller. Connected to %s\n",
- i, dt_node_full_name(rirq.controller));
+ dt_dprintk("irq %u not (directly or indirectly) connected to primary"
+ "controller. Connected to %s\n", i,
+ dt_node_full_name(rirq.controller));
continue;
}
- res = platform_get_irq(dev, i);
+ res = platform_irq_for_device(dev, i);
if ( res < 0 )
{
printk(XENLOG_ERR "Unable to get irq %u for %s\n",
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 06d4843..dc42817 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -27,6 +27,7 @@
#include <asm/gic.h>
#include <asm/vgic.h>
+#include <asm/platform.h>
static unsigned int local_irqs_type[NR_LOCAL_IRQS];
static DEFINE_SPINLOCK(local_irqs_type_lock);
@@ -370,7 +371,7 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
/* First time the IRQ is setup */
if ( disabled )
{
- gic_route_irq_to_xen(desc, GIC_PRI_IRQ);
+ platform_route_irq_to_xen(desc, GIC_PRI_IRQ);
/* It's fine to use smp_processor_id() because:
* For PPI: irq_desc is banked
* For SPI: we don't care for now which CPU will receive the
@@ -504,7 +505,7 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
if ( retval )
goto out;
- retval = gic_route_irq_to_guest(d, virq, desc, GIC_PRI_IRQ);
+ retval = platform_route_irq_to_guest(d, virq, desc, GIC_PRI_IRQ);
spin_unlock_irqrestore(&desc->lock, flags);
diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index b0bfaa9..74abdc6 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -137,6 +137,45 @@ bool_t platform_device_is_blacklisted(const struct dt_device_node *node)
return (dt_match_node(blacklist, node) != NULL);
}
+int platform_route_irq_to_guest(struct domain *d, unsigned int virq,
+ struct irq_desc *desc, unsigned int priority)
+{
+ if ( platform && platform->route_irq_to_guest )
+ return platform->route_irq_to_guest(d, virq, desc, priority);
+ else
+ return gic_route_irq_to_guest(d, virq, desc, priority);
+}
+
+void platform_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
+{
+ if ( platform && platform->route_irq_to_xen )
+ platform->route_irq_to_xen(desc, priority);
+ else
+ gic_route_irq_to_xen(desc, priority);
+}
+
+bool_t platform_irq_is_routable(struct dt_raw_irq * rirq)
+{
+ /*
+ * If we have a platform-specific method to determine if an IRQ is routable,
+ * check that; otherwise fall back to checking to see if an IRQ belongs to
+ * the GIC.
+ */
+ if ( platform && platform->irq_is_routable )
+ return platform->irq_is_routable(rirq);
+ else
+ return (rirq->controller == dt_interrupt_controller);
+}
+
+int platform_irq_for_device(const struct dt_device_node *dev, int index)
+{
+ if ( platform && platform->irq_for_device )
+ return platform->irq_for_device(dev, index);
+ else
+ return platform_get_irq(dev, index);
+}
+
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 7dae28b..e485b3b 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -162,7 +162,7 @@ static void __init init_dt_xen_time(void)
/* Retrieve all IRQs for the timer */
for ( i = TIMER_PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++ )
{
- res = platform_get_irq(timer, i);
+ res = platform_irq_for_device(timer, i);
if ( res < 0 )
panic("Timer: Unable to retrieve IRQ %u from the device tree", i);
diff --git a/xen/drivers/char/cadence-uart.c b/xen/drivers/char/cadence-uart.c
index 7f90f8d..be6ed10 100644
--- a/xen/drivers/char/cadence-uart.c
+++ b/xen/drivers/char/cadence-uart.c
@@ -29,6 +29,7 @@
#include <xen/vmap.h>
#include <asm/cadence-uart.h>
#include <asm/io.h>
+#include <asm/platform.h>
static struct cuart {
unsigned int irq;
@@ -174,7 +175,7 @@ static int __init cuart_init(struct dt_device_node *dev, const void *data)
return res;
}
- res = platform_get_irq(dev, 0);
+ res = platform_irq_for_device(dev, 0);
if ( res < 0 )
{
printk("cadence: Unable to retrieve the IRQ\n");
diff --git a/xen/drivers/char/exynos4210-uart.c b/xen/drivers/char/exynos4210-uart.c
index bac1c2b..be90ffa 100644
--- a/xen/drivers/char/exynos4210-uart.c
+++ b/xen/drivers/char/exynos4210-uart.c
@@ -27,6 +27,7 @@
#include <asm/device.h>
#include <asm/exynos4210-uart.h>
#include <asm/io.h>
+#include <asm/platform.h>
static struct exynos4210_uart {
unsigned int baud, clock_hz, data_bits, parity, stop_bits;
@@ -323,7 +324,7 @@ static int __init exynos4210_uart_init(struct dt_device_node *dev,
return res;
}
- res = platform_get_irq(dev, 0);
+ res = platform_irq_for_device(dev, 0);
if ( res < 0 )
{
printk("exynos4210: Unable to retrieve the IRQ\n");
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index bffdb35..d68d670 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -26,6 +26,7 @@
#include <asm/io.h>
#ifdef CONFIG_HAS_DEVICE_TREE
#include <asm/device.h>
+#include <asm/platform.h>
#endif
#ifdef CONFIG_X86
#include <asm/fixmap.h>
@@ -1279,7 +1280,7 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
if ( uart->reg_width != 1 && uart->reg_width != 4 )
return -EINVAL;
- res = platform_get_irq(dev, 0);
+ res = platform_irq_for_device(dev, 0);
if ( ! res )
return -EINVAL;
uart->irq = res;
diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
index e96f6f5..1a7fe03 100644
--- a/xen/drivers/char/omap-uart.c
+++ b/xen/drivers/char/omap-uart.c
@@ -22,6 +22,7 @@
#include <xen/vmap.h>
#include <xen/8250-uart.h>
#include <asm/io.h>
+#include <asm/platform.h>
#define REG_SHIFT 2
@@ -353,7 +354,7 @@ static int __init omap_uart_init(struct dt_device_node *dev,
return res;
}
- res = platform_get_irq(dev, 0);
+ res = platform_irq_for_device(dev, 0);
if ( res < 0 )
{
printk("omap-uart: Unable to retrieve the IRQ\n");
diff --git a/xen/drivers/char/pl011.c b/xen/drivers/char/pl011.c
index 102f40d..b6c0fe1 100644
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -29,6 +29,7 @@
#include <xen/vmap.h>
#include <asm/pl011-uart.h>
#include <asm/io.h>
+#include <asm/platform.h>
static struct pl011 {
unsigned int data_bits, parity, stop_bits;
@@ -274,7 +275,7 @@ static int __init pl011_dt_uart_init(struct dt_device_node *dev,
return res;
}
- res = platform_get_irq(dev, 0);
+ res = platform_irq_for_device(dev, 0);
if ( res < 0 )
{
printk("pl011: Unable to retrieve the IRQ\n");
diff --git a/xen/drivers/char/scif-uart.c b/xen/drivers/char/scif-uart.c
index f9ae257..c3ab2bb 100644
--- a/xen/drivers/char/scif-uart.c
+++ b/xen/drivers/char/scif-uart.c
@@ -29,6 +29,7 @@
#include <asm/device.h>
#include <asm/scif-uart.h>
#include <asm/io.h>
+#include <asm/platform.h>
#define PARITY_NONE 0
#define PARITY_EVEN 1
@@ -255,7 +256,7 @@ static int __init scif_uart_init(struct dt_device_node *dev,
return res;
}
- res = platform_get_irq(dev, 0);
+ res = platform_irq_for_device(dev, 0);
if ( res < 0 )
{
printk("scif-uart: Unable to retrieve the IRQ\n");
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index cf8b8b8..94d035a 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -103,7 +103,7 @@ static struct resource *platform_get_resource(struct platform_device *pdev,
return ((ret) ? NULL : &res);
case IORESOURCE_IRQ:
- ret = platform_get_irq(pdev, num);
+ ret = platform_irq_for_device(pdev, num);
if (ret < 0)
return NULL;
@@ -2349,7 +2349,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
}
for (i = 0; i < num_irqs; ++i) {
- int irq = platform_get_irq(pdev, i);
+ int irq = platform_irq_for_device(pdev, i);
if (irq < 0) {
dev_err(dev, "failed to get irq index %d\n", i);
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index f97315d..4ea278b 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -26,6 +26,13 @@ struct platform_desc {
void (*reset)(void);
/* Platform power-off */
void (*poweroff)(void);
+ /* Platform-specific IRQ routing */
+ int (*route_irq_to_guest)(struct domain *d, unsigned int virq,
+ struct irq_desc *desc, unsigned int priority);
+ void (*route_irq_to_xen)(struct irq_desc *desc, unsigned int priority);
+ bool_t (*irq_is_routable)(struct dt_raw_irq * rirq);
+ int (*irq_for_device)(const struct dt_device_node *dev, int index);
+
/*
* Platform blacklist devices
* List of devices which must not pass-through to a guest
@@ -42,6 +49,13 @@ int platform_cpu_up(int cpu);
#endif
void platform_reset(void);
void platform_poweroff(void);
+
+int platform_route_irq_to_guest(struct domain *d, unsigned int virq,
+ struct irq_desc *desc, unsigned int priority);
+void platform_route_irq_to_xen(struct irq_desc *desc, unsigned int priority);
+bool_t platform_irq_is_routable(struct dt_raw_irq *rirq);
+int platform_irq_for_device(const struct dt_device_node *dev, int index);
+
bool_t platform_device_is_blacklisted(const struct dt_device_node *node);
#define PLATFORM_START(_name, _namestr) \
--
2.9.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH RFC 3/6] xen/arm: Allow platforms to hook IRQ routing.
2016-09-05 10:13 ` [PATCH RFC 3/6] xen/arm: Allow platforms to hook IRQ routing Kyle Temkin
@ 2016-10-21 23:28 ` Stefano Stabellini
2016-11-01 14:56 ` Julien Grall
1 sibling, 0 replies; 17+ messages in thread
From: Stefano Stabellini @ 2016-10-21 23:28 UTC (permalink / raw)
To: Kyle Temkin; +Cc: julien.grall, sstabellini, Kyle J. Temkin, xen-devel
On Mon, 5 Sep 2016, Kyle Temkin wrote:
> From: "Kyle J. Temkin" <temkink@ainfosec.com>
>
> Some common platforms (e.g. Tegra) have non-traditional IRQ controllers
> that must be programmed in addition to their primary GICs-- and which
> can come in unusual topologies. Device trees for targets that feature
> these controllers often deviate from the conventions that Xen expects.
>
> This commit provides a foundation for support of these platforms, by:
> - Allowing the platform to decide which IRQs can be routed by Xen,
> rather than assuming that only GIC-connected IRQs can be routed.
> - Allowing the platform to extend/replace existing IRQ routing logic,
> rather than asssuming that the GIC will always be programmed to route
> IRQs.
> - Allows the platform to override IRQ translation, rather than assuming
> GIC translation will always be followed. This is useful in cases where
> device tree IRQ numbers don't correspond to GIC IRQ numbers.
>
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> ---
> xen/arch/arm/domain_build.c | 14 +++++++++-----
> xen/arch/arm/irq.c | 5 +++--
> xen/arch/arm/platform.c | 39 ++++++++++++++++++++++++++++++++++++++
> xen/arch/arm/time.c | 2 +-
> xen/drivers/char/cadence-uart.c | 3 ++-
> xen/drivers/char/exynos4210-uart.c | 3 ++-
> xen/drivers/char/ns16550.c | 3 ++-
> xen/drivers/char/omap-uart.c | 3 ++-
> xen/drivers/char/pl011.c | 3 ++-
> xen/drivers/char/scif-uart.c | 3 ++-
> xen/drivers/passthrough/arm/smmu.c | 4 ++--
> xen/include/asm-arm/platform.h | 14 ++++++++++++++
> 12 files changed, 80 insertions(+), 16 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 52c9a01..402c766 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1094,16 +1094,20 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
>
> /*
> * Don't map IRQ that have no physical meaning
> - * ie: IRQ whose controller is not the GIC
> + * ie: IRQ that does not wind up being controlled by the GIC
> + * (Note that we can't just check to see if an IRQ is owned by the GIC,
> + * as some platforms have a controller between the device irq and the GIC,
> + * such as the Tegra legacy interrupt controller.)
> */
> - if ( rirq.controller != dt_interrupt_controller )
> + if ( !platform_irq_is_routable(&rirq) )
> {
> - dt_dprintk("irq %u not connected to primary controller. Connected to %s\n",
> - i, dt_node_full_name(rirq.controller));
> + dt_dprintk("irq %u not (directly or indirectly) connected to primary"
> + "controller. Connected to %s\n", i,
> + dt_node_full_name(rirq.controller));
> continue;
> }
>
> - res = platform_get_irq(dev, i);
> + res = platform_irq_for_device(dev, i);
> if ( res < 0 )
> {
> printk(XENLOG_ERR "Unable to get irq %u for %s\n",
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 06d4843..dc42817 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -27,6 +27,7 @@
>
> #include <asm/gic.h>
> #include <asm/vgic.h>
> +#include <asm/platform.h>
>
> static unsigned int local_irqs_type[NR_LOCAL_IRQS];
> static DEFINE_SPINLOCK(local_irqs_type_lock);
> @@ -370,7 +371,7 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
> /* First time the IRQ is setup */
> if ( disabled )
> {
> - gic_route_irq_to_xen(desc, GIC_PRI_IRQ);
> + platform_route_irq_to_xen(desc, GIC_PRI_IRQ);
> /* It's fine to use smp_processor_id() because:
> * For PPI: irq_desc is banked
> * For SPI: we don't care for now which CPU will receive the
> @@ -504,7 +505,7 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
> if ( retval )
> goto out;
>
> - retval = gic_route_irq_to_guest(d, virq, desc, GIC_PRI_IRQ);
> + retval = platform_route_irq_to_guest(d, virq, desc, GIC_PRI_IRQ);
>
> spin_unlock_irqrestore(&desc->lock, flags);
>
> diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
> index b0bfaa9..74abdc6 100644
> --- a/xen/arch/arm/platform.c
> +++ b/xen/arch/arm/platform.c
> @@ -137,6 +137,45 @@ bool_t platform_device_is_blacklisted(const struct dt_device_node *node)
> return (dt_match_node(blacklist, node) != NULL);
> }
>
> +int platform_route_irq_to_guest(struct domain *d, unsigned int virq,
> + struct irq_desc *desc, unsigned int priority)
> +{
> + if ( platform && platform->route_irq_to_guest )
> + return platform->route_irq_to_guest(d, virq, desc, priority);
> + else
> + return gic_route_irq_to_guest(d, virq, desc, priority);
> +}
> +
> +void platform_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
> +{
> + if ( platform && platform->route_irq_to_xen )
> + platform->route_irq_to_xen(desc, priority);
> + else
> + gic_route_irq_to_xen(desc, priority);
> +}
> +
> +bool_t platform_irq_is_routable(struct dt_raw_irq * rirq)
> +{
> + /*
> + * If we have a platform-specific method to determine if an IRQ is routable,
> + * check that; otherwise fall back to checking to see if an IRQ belongs to
> + * the GIC.
> + */
> + if ( platform && platform->irq_is_routable )
> + return platform->irq_is_routable(rirq);
> + else
> + return (rirq->controller == dt_interrupt_controller);
> +}
> +
> +int platform_irq_for_device(const struct dt_device_node *dev, int index)
> +{
> + if ( platform && platform->irq_for_device )
> + return platform->irq_for_device(dev, index);
> + else
> + return platform_get_irq(dev, index);
> +}
If it's not too much trouble I would move these functions to platform.h
as static inlines.
In any case the patch is OK:
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH RFC 3/6] xen/arm: Allow platforms to hook IRQ routing.
2016-09-05 10:13 ` [PATCH RFC 3/6] xen/arm: Allow platforms to hook IRQ routing Kyle Temkin
2016-10-21 23:28 ` Stefano Stabellini
@ 2016-11-01 14:56 ` Julien Grall
1 sibling, 0 replies; 17+ messages in thread
From: Julien Grall @ 2016-11-01 14:56 UTC (permalink / raw)
To: Kyle Temkin, xen-devel; +Cc: sstabellini, Kyle J. Temkin
Hello Kyle,
On 05/09/2016 11:13, Kyle Temkin wrote:
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 52c9a01..402c766 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
[...]
> +
> +bool_t platform_irq_is_routable(struct dt_raw_irq * rirq)
We are trying to move the code base to bool rather than bool_t.
> +{
> + /*
> + * If we have a platform-specific method to determine if an IRQ is routable,
> + * check that; otherwise fall back to checking to see if an IRQ belongs to
> + * the GIC.
> + */
> + if ( platform && platform->irq_is_routable )
> + return platform->irq_is_routable(rirq);
> + else
> + return (rirq->controller == dt_interrupt_controller);
> +}
> +
> +int platform_irq_for_device(const struct dt_device_node *dev, int index)
> +{
> + if ( platform && platform->irq_for_device )
> + return platform->irq_for_device(dev, index);
> + else
> + return platform_get_irq(dev, index);
IHMO the naming of this new function will confuse the user. Without any
documentation it is not clear whether the user should call
platform_irq_for_device or platform_get_irq.
However, I am not sure to understand why you need to override
platform_get_irq as the code is exactly the same and dt_irq_xlate could
be overidden to return the IRQ.
> +}
> +
> +
> /*
> * Local variables:
> * mode: C
[...]
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index cf8b8b8..94d035a 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -103,7 +103,7 @@ static struct resource *platform_get_resource(struct platform_device *pdev,
> return ((ret) ? NULL : &res);
>
> case IORESOURCE_IRQ:
> - ret = platform_get_irq(pdev, num);
> + ret = platform_irq_for_device(pdev, num);
> if (ret < 0)
> return NULL;
>
> @@ -2349,7 +2349,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
> }
>
> for (i = 0; i < num_irqs; ++i) {
> - int irq = platform_get_irq(pdev, i);
> + int irq = platform_irq_for_device(pdev, i);
>
> if (irq < 0) {
> dev_err(dev, "failed to get irq index %d\n", i);
> diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
> index f97315d..4ea278b 100644
> --- a/xen/include/asm-arm/platform.h
> +++ b/xen/include/asm-arm/platform.h
> @@ -26,6 +26,13 @@ struct platform_desc {
> void (*reset)(void);
> /* Platform power-off */
> void (*poweroff)(void);
> + /* Platform-specific IRQ routing */
> + int (*route_irq_to_guest)(struct domain *d, unsigned int virq,
> + struct irq_desc *desc, unsigned int priority);
> + void (*route_irq_to_xen)(struct irq_desc *desc, unsigned int priority);
> + bool_t (*irq_is_routable)(struct dt_raw_irq * rirq);
Please use bool here.
> + int (*irq_for_device)(const struct dt_device_node *dev, int index);
> +
> /*
> * Platform blacklist devices
> * List of devices which must not pass-through to a guest
> @@ -42,6 +49,13 @@ int platform_cpu_up(int cpu);
> #endif
> void platform_reset(void);
> void platform_poweroff(void);
> +
> +int platform_route_irq_to_guest(struct domain *d, unsigned int virq,
> + struct irq_desc *desc, unsigned int priority);
> +void platform_route_irq_to_xen(struct irq_desc *desc, unsigned int priority);
> +bool_t platform_irq_is_routable(struct dt_raw_irq *rirq);
Ditto.
> +int platform_irq_for_device(const struct dt_device_node *dev, int index);
> +
> bool_t platform_device_is_blacklisted(const struct dt_device_node *node);
>
> #define PLATFORM_START(_name, _namestr) \
>
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH RFC 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing.
2016-09-05 10:13 [PATCH RFC 0/6] ARM: Add support for Tegra SoCs (incl. Jetson TK1, Jetson TX1) Kyle Temkin
` (2 preceding siblings ...)
2016-09-05 10:13 ` [PATCH RFC 3/6] xen/arm: Allow platforms to hook IRQ routing Kyle Temkin
@ 2016-09-05 10:13 ` Kyle Temkin
2016-09-16 14:50 ` Konrad Rzeszutek Wilk
2016-09-05 10:14 ` [PATCH RFC 5/6] xen/arm: Add function to query IRQ 'ownership' Kyle Temkin
2016-09-05 10:14 ` [PATCH RFC 6/6] xen/arm: platforms/tegra: ensure the hwdom can only affect its own interrupts Kyle Temkin
5 siblings, 1 reply; 17+ messages in thread
From: Kyle Temkin @ 2016-09-05 10:13 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, sstabellini, Kyle J. Temkin
From: "Kyle J. Temkin" <temkink@ainfosec.com>
Tegra devices have a legacy interrupt controller (lic, or ictlr) that
must be programmed in parallel with their primary GIC. For all intents
and purposes, we treat this devices attached to this controller as
connected to the primary GIC, as it will be handling their interrupts.
This commit adds support for exposing the ictlr to the hardware domain;
but a future commit will extend this to support exposing a virtualized
version of the ictlr to the hardware domain, and to ensure that
interrupts are unmasked properly when routed to a Xen, or to a domain
other than the hardware domain.
Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
---
xen/arch/arm/platforms/Makefile | 2 +
xen/arch/arm/platforms/tegra.c | 339 ++++++++++++++++++++++++++++++++++
xen/include/asm-arm/platforms/tegra.h | 50 +++++
3 files changed, 391 insertions(+)
create mode 100644 xen/arch/arm/platforms/tegra.c
create mode 100644 xen/include/asm-arm/platforms/tegra.h
diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index 49fa683..0c3a7f4 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -4,7 +4,9 @@ obj-$(CONFIG_ARM_32) += exynos5.o
obj-$(CONFIG_ARM_32) += midway.o
obj-$(CONFIG_ARM_32) += omap5.o
obj-$(CONFIG_ARM_32) += rcar2.o
+obj-$(CONFIG_ARM_32) += tegra.o
obj-$(CONFIG_ARM_64) += seattle.o
obj-$(CONFIG_ARM_32) += sunxi.o
obj-$(CONFIG_ARM_64) += xgene-storm.o
obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
+obj-$(CONFIG_ARM_64) += tegra.o
diff --git a/xen/arch/arm/platforms/tegra.c b/xen/arch/arm/platforms/tegra.c
new file mode 100644
index 0000000..e75242c
--- /dev/null
+++ b/xen/arch/arm/platforms/tegra.c
@@ -0,0 +1,339 @@
+/*
+ * NVIDIA Tegra specific settings
+ *
+ * Ian Campbell; Copyright (c) 2014 Citrix Systems
+ * Kyle Temkin; Copyright (c) 2016 Assured Information Security, Inc.
+ * Chris Patterson; Copyright (c) 2016 Assured Information Security, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <xen/config.h>
+#include <xen/lib.h>
+#include <xen/stdbool.h>
+#include <xen/sched.h>
+#include <xen/vmap.h>
+
+#include <asm/io.h>
+#include <asm/gic.h>
+#include <asm/platform.h>
+#include <asm/platforms/tegra.h>
+
+
+/* Permanent mapping to the Tegra legacy interrupt controller. */
+static void __iomem *tegra_ictlr_base = NULL;
+
+/*
+ * List of legacy interrupt controllers that can be used to route
+ * Tegra interrupts.
+ */
+static const char * const tegra_interrupt_compat[] __initconst =
+{
+ "nvidia,tegra120-ictlr", /* Tegra K1 controllers */
+ "nvidia,tegra210-ictlr" /* Tegra X1 controllers */
+};
+
+
+/**
+ * Returns true if the given IRQ belongs to a supported tegra interrupt
+ * controller.
+ *
+ * @param rirq The raw IRQ to be identified.
+ * @return True iff the given IRQ belongs to a Tegra ictlr.
+ */
+static bool_t tegra_irq_belongs_to_ictlr(struct dt_raw_irq * rirq) {
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(tegra_interrupt_compat); i++)
+ {
+ if ( dt_device_is_compatible(rirq->controller, tegra_interrupt_compat[i]) )
+ return true;
+ }
+
+ return false;
+}
+
+
+/**
+ * Returns true iff the given IRQ is routable -- that is, if it is descended
+ * from the platform's primary GIC.
+ *
+ * @param rirq The raw IRQ in question.
+ * @return True iff the given IRQ routes to a platform GIC.
+ */
+static bool_t tegra_irq_is_routable(struct dt_raw_irq * rirq)
+{
+ /* If the IRQ connects directly to our GIC, it's trivially routable. */
+ if ( rirq->controller == dt_interrupt_controller )
+ return true;
+
+ /*
+ * If the IRQ belongs to a legacy interrupt controller, then it's
+ * effectively owned by the GIC, and is routable.
+ */
+ if ( tegra_irq_belongs_to_ictlr(rirq) )
+ return true;
+
+ return false;
+}
+
+/**
+ * Returns the IRQ number for a given device. Tegra IRQs transalate using the
+ * same algorithm as normal GIC IRQs, but aren't parented by the system GIC.
+ *
+ * As a result, translation fails an assertion in the normal translation path.
+ * The normal version is essentially dt_irq_xlate wrapped with an assert, so
+ * we'll just call dt_irq_xlate directly.
+ *
+ * @param device The DT node describing the device.
+ * @param index The index of the interrupt within the device node.
+ * @return The translated number of the IRQ, or a negative error code.
+ */
+static int tegra_irq_for_device(const struct dt_device_node *device, int index)
+{
+ struct dt_raw_irq raw;
+ struct dt_irq dt_irq;
+ int res;
+
+ res = dt_device_get_raw_irq(device, index, &raw);
+ if ( res )
+ return -ENODEV;
+
+ /*
+ * The translation function for the Tegra ictlr happens to match the
+ * translation function for the normal GIC, so we'll use that in either
+ * case.
+ */
+ res = dt_irq_xlate(raw.specifier, raw.size, &dt_irq.irq, &dt_irq.type);
+ if ( res )
+ return -ENODEV;
+
+ if ( irq_set_type(dt_irq.irq, dt_irq.type) )
+ return -ENODEV;
+
+ return dt_irq.irq;
+}
+
+/**
+ * Platform-specific reset code for the Tegra devices.
+ * Should not return.
+ */
+static void tegra_reset(void)
+{
+ void __iomem *addr;
+ u32 val;
+
+ addr = ioremap_nocache(TEGRA_RESET_BASE, TEGRA_RESET_SIZE);
+ if ( !addr )
+ {
+ printk(XENLOG_ERR "Tegra: Unable to map tegra reset address. Reset failed!\n");
+ return;
+ }
+
+ /* Write into the reset device. */
+ val = readl(addr) | TEGRA_RESET_MASK;
+ writel(val, addr);
+
+ iounmap(addr);
+}
+
+/**
+ * Applies an interrupt enable to a given interrupt via the legacy
+ * interrupt controller, and marks that interrupt as a normal interrupt,
+ * rather than a fast IRQ.
+ *
+ * @param irq The hardware IRQ number for the given interrupt.
+ */
+static void tegra_ictlr_set_interrupt_enable(unsigned int irq, bool enabled)
+{
+ uint32_t previous_iep_class;
+
+ /* If we're enabling a given bit, use the SET register; otherwise CLR. */
+ unsigned int register_number =
+ enabled ? TEGRA_ICTLR_CPU_IER_SET : TEGRA_ICTLR_CPU_IER_CLR;
+
+ /*
+ * Determine the IRQ number in the ictlr domain, and figure out the indexA
+ * of the individual controller we're working with. */
+ unsigned int ictlr_irq = irq - NR_LOCAL_IRQS;
+ unsigned int ictlr_number = ictlr_irq / TEGRA_IRQS_PER_ICTLR;
+
+ /* Get a pointer to the target ictlr. */
+ void __iomem * target_ictlr = tegra_ictlr_base + TEGRA_ICTLR_SIZE * ictlr_number;
+
+ /* Determine the mask we'll be working with. */
+ uint32_t mask = BIT(ictlr_irq % TEGRA_IRQS_PER_ICTLR);
+
+ /* Sanity check our memory access. */
+ ASSERT(tegra_ictlr_base);
+ ASSERT(ictlr_number < TEGRA_ICTLR_COUNT);
+ ASSERT(irq >= NR_LOCAL_IRQS);
+
+ /* Enable the given IRQ. */
+ writel(mask, target_ictlr + register_number);
+
+ /* Mark the interrupt as a normal interrupt-- not a fast IRQ. */
+ previous_iep_class = readl(target_ictlr + TEGRA_ICTLR_CPU_IEP_CLASS);
+ writel(previous_iep_class & ~mask, target_ictlr + TEGRA_ICTLR_CPU_IEP_CLASS);
+}
+
+
+/**
+ * Routes an IRQ to a guest, applying sane values to the ictlr masks.
+ *
+ * @param domain The domain to which the IRQ will be routed.
+ * @param virq The virtual IRQ number.
+ * @param desc The IRQ to be routed.
+ * @param priority The IRQ priority.
+ * @return 0 on success, or an error code on failure.
+ */
+static int tegra_route_irq_to_guest(struct domain *d, unsigned int virq,
+ struct irq_desc *desc, unsigned int priority)
+{
+ /* Program the core GIC to deliver the interrupt to the guest. */
+ int rc = gic_route_irq_to_guest(d, virq, desc, priority);
+
+ /* If we couldn't route the IRQ via the GIC, bail out. */
+ if(rc)
+ {
+ printk(XENLOG_ERR "Tegra LIC: Couldn't program GIC to route vIRQ %d (%d).\n",
+ desc->irq, rc);
+ return rc;
+ }
+
+ /*
+ * If this is a local IRQ, it's not masked by the ictlr, so we
+ * don't need to perform any ictlr manipulation.
+ */
+ if( desc->irq < NR_LOCAL_IRQS )
+ return rc;
+
+ /*
+ * If this is the hardware domain, it will have real access to the ictlr,
+ * and will program the ictlr itself, so it should start with the ictlr
+ * disabled. If we're not the hwdom, the domain won't interact with the
+ * ictlr, and the interrupt shouldn't be masked.
+ */
+ tegra_ictlr_set_interrupt_enable(desc->irq, !is_hardware_domain(d));
+ return rc;
+}
+
+
+/**
+ * Routes an IRQ to Xen. This method both performs the core IRQ routing, and
+ * sets up any ictlr routing necessary.
+ *
+ * @param desc The IRQ to be routed.
+ * @param priority The IRQ priority.
+ */
+static void tegra_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
+{
+ unsigned int irq = desc->irq;
+
+ /* Program the core GIC to deliver the interrupt to Xen. */
+ gic_route_irq_to_xen(desc, priority);
+
+ /*
+ * If this is a local IRQ, it's not masked by the ictlr, so we
+ * don't need to perform any ictlr manipulation.
+ */
+ if( irq < NR_LOCAL_IRQS )
+ return;
+
+ /*
+ * Enable the interrupt in the ictlr. Xen only uses the GIC to
+ * perform masking, so we'll enable the interrupt to prevent ictlr
+ * gating of the interrupt.
+ */
+ tegra_ictlr_set_interrupt_enable(irq, true);
+
+}
+
+/**
+ * Initialize the Tegra legacy interrupt controller, placing each interrupt
+ * into a default state. These defaults ensure that stray interrupts don't
+ * affect Xen.
+ */
+static int tegra_initialize_legacy_interrupt_controller(void)
+{
+ int i;
+
+ /* Map in the tegra ictlr. */
+ tegra_ictlr_base = ioremap_nocache(TEGRA_ICTLR_BASE,
+ TEGRA_ICTLR_SIZE * TEGRA_ICTLR_COUNT);
+
+ if ( !tegra_ictlr_base )
+ panic("Failed to map in the Tegra legacy interrupt controller!\n");
+
+ /* Initialize each of the legacy interrupt controllers. */
+ for (i = 0; i < TEGRA_ICTLR_COUNT; i++)
+ {
+ void __iomem *ictlr_n = tegra_ictlr_base + TEGRA_ICTLR_SIZE * i;
+
+ /* Clear the interrupt enables for every interrupt. */
+ writel(~0, ictlr_n + TEGRA_ICTLR_CPU_IER_CLR);
+
+ /*
+ * Mark all of our interrupts as normal ARM interrupts (as opposed
+ * to Fast Interrupts.)
+ */
+ writel(0, ictlr_n + TEGRA_ICTLR_CPU_IEP_CLASS);
+ }
+
+ return 0;
+}
+
+/**
+ * Startup code for the Tegra.
+ */
+static int tegra_init(void)
+{
+ int rc;
+
+ rc = tegra_initialize_legacy_interrupt_controller();
+
+ /*
+ * TODO: Virtualize the MMIO regions for the ictlr, ensuring the hwdom
+ * (and possibly passhtrough domains) can only access ictlr registers for
+ * interrupts they actually own.
+ */
+
+ return rc;
+}
+
+
+static const char * const tegra_dt_compat[] __initconst =
+{
+ "nvidia,tegra120", /* Tegra K1 */
+ "nvidia,tegra210", /* Tegra X1 */
+ NULL
+};
+
+static const struct dt_device_match tegra_blacklist_dev[] __initconst =
+{
+ /*
+ * The UARTs share a page which runs the risk of mapping the Xen console
+ * UART to dom0, so don't map any of them.
+ */
+ DT_MATCH_COMPATIBLE("nvidia,tegra20-uart"),
+ { /* sentinel */ },
+};
+
+PLATFORM_START(tegra, "Tegra")
+ .blacklist_dev = tegra_blacklist_dev,
+ .compatible = tegra_dt_compat,
+ .init = tegra_init,
+ .reset = tegra_reset,
+ .irq_is_routable = tegra_irq_is_routable,
+ .irq_for_device = tegra_irq_for_device,
+ .route_irq_to_xen = tegra_route_irq_to_xen,
+ .route_irq_to_guest = tegra_route_irq_to_guest,
+PLATFORM_END
diff --git a/xen/include/asm-arm/platforms/tegra.h b/xen/include/asm-arm/platforms/tegra.h
new file mode 100644
index 0000000..821add5
--- /dev/null
+++ b/xen/include/asm-arm/platforms/tegra.h
@@ -0,0 +1,50 @@
+/*
+ * NVIDIA Tegra platform definitions
+ *
+ * Kyle Temkin; Copyright (c) 2016 Assured Information Security, Inc.
+ * Chris Patterson; Copyright (c) 2016 Assured Information Security, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+
+#ifndef __ASM_ARM_PLATFORMS_TEGRA_H
+#define __ASM_ARM_PLATFORMS_TEGRA_H
+
+#define TEGRA_ICTLR_BASE 0x60004000
+#define TEGRA_ICTLR_SIZE 0x00000100
+#define TEGRA_ICTLR_COUNT 6
+#define TEGRA_IRQS_PER_ICTLR 32
+
+#define TEGRA_RESET_BASE 0x7000e400
+#define TEGRA_RESET_SIZE 4
+#define TEGRA_RESET_MASK 0x10
+
+#define TEGRA_ICTLR_CPU_IER 0x20
+#define TEGRA_ICTLR_CPU_IER_SET 0x24
+#define TEGRA_ICTLR_CPU_IER_CLR 0x28
+#define TEGRA_ICTLR_CPU_IEP_CLASS 0x2C
+
+#define TEGRA_ICTLR_COP_IER 0x30
+#define TEGRA_ICTLR_COP_IER_SET 0x34
+#define TEGRA_ICTLR_COP_IER_CLR 0x38
+#define TEGRA_ICTLR_COP_IEP_CLASS 0x3c
+
+
+#endif /* __ASM_ARM_PLATFORMS_TEGRA_H */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.9.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH RFC 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing.
2016-09-05 10:13 ` [PATCH RFC 4/6] xen/arm: platforms: Add Tegra platform to support basic " Kyle Temkin
@ 2016-09-16 14:50 ` Konrad Rzeszutek Wilk
2016-10-22 0:06 ` Stefano Stabellini
0 siblings, 1 reply; 17+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-16 14:50 UTC (permalink / raw)
To: Kyle Temkin; +Cc: julien.grall, sstabellini, Kyle J. Temkin, xen-devel
On Mon, Sep 05, 2016 at 06:13:59AM -0400, Kyle Temkin wrote:
> From: "Kyle J. Temkin" <temkink@ainfosec.com>
>
> Tegra devices have a legacy interrupt controller (lic, or ictlr) that
> must be programmed in parallel with their primary GIC. For all intents
> and purposes, we treat this devices attached to this controller as
> connected to the primary GIC, as it will be handling their interrupts.
Is there a link to the specification? Could that be included in the file
or in the commit description?
>
> This commit adds support for exposing the ictlr to the hardware domain;
> but a future commit will extend this to support exposing a virtualized
> version of the ictlr to the hardware domain, and to ensure that
> interrupts are unmasked properly when routed to a Xen, or to a domain
> other than the hardware domain.
>
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> ---
> xen/arch/arm/platforms/Makefile | 2 +
> xen/arch/arm/platforms/tegra.c | 339 ++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/platforms/tegra.h | 50 +++++
> 3 files changed, 391 insertions(+)
> create mode 100644 xen/arch/arm/platforms/tegra.c
> create mode 100644 xen/include/asm-arm/platforms/tegra.h
>
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index 49fa683..0c3a7f4 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -4,7 +4,9 @@ obj-$(CONFIG_ARM_32) += exynos5.o
> obj-$(CONFIG_ARM_32) += midway.o
> obj-$(CONFIG_ARM_32) += omap5.o
> obj-$(CONFIG_ARM_32) += rcar2.o
> +obj-$(CONFIG_ARM_32) += tegra.o
> obj-$(CONFIG_ARM_64) += seattle.o
> obj-$(CONFIG_ARM_32) += sunxi.o
> obj-$(CONFIG_ARM_64) += xgene-storm.o
> obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
> +obj-$(CONFIG_ARM_64) += tegra.o
> diff --git a/xen/arch/arm/platforms/tegra.c b/xen/arch/arm/platforms/tegra.c
> new file mode 100644
> index 0000000..e75242c
> --- /dev/null
> +++ b/xen/arch/arm/platforms/tegra.c
> @@ -0,0 +1,339 @@
> +/*
> + * NVIDIA Tegra specific settings
> + *
> + * Ian Campbell; Copyright (c) 2014 Citrix Systems
> + * Kyle Temkin; Copyright (c) 2016 Assured Information Security, Inc.
> + * Chris Patterson; Copyright (c) 2016 Assured Information Security, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <xen/config.h>
No config.h pls.
> +#include <xen/lib.h>
> +#include <xen/stdbool.h>
> +#include <xen/sched.h>
> +#include <xen/vmap.h>
> +
> +#include <asm/io.h>
> +#include <asm/gic.h>
> +#include <asm/platform.h>
> +#include <asm/platforms/tegra.h>
> +
> +
> +/* Permanent mapping to the Tegra legacy interrupt controller. */
> +static void __iomem *tegra_ictlr_base = NULL;
No need for NULL.
> +
> +/*
> + * List of legacy interrupt controllers that can be used to route
> + * Tegra interrupts.
> + */
> +static const char * const tegra_interrupt_compat[] __initconst =
> +{
> + "nvidia,tegra120-ictlr", /* Tegra K1 controllers */
> + "nvidia,tegra210-ictlr" /* Tegra X1 controllers */
> +};
> +
> +
> +/**
> + * Returns true if the given IRQ belongs to a supported tegra interrupt
> + * controller.
> + *
> + * @param rirq The raw IRQ to be identified.
> + * @return True iff the given IRQ belongs to a Tegra ictlr.
> + */
> +static bool_t tegra_irq_belongs_to_ictlr(struct dt_raw_irq * rirq) {
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(tegra_interrupt_compat); i++)
Something is off with your style ({, and the lack of spaces around ()).
> + {
> + if ( dt_device_is_compatible(rirq->controller, tegra_interrupt_compat[i]) )
> + return true;
> + }
> +
> + return false;
> +}
> +
> +
> +/**
> + * Returns true iff the given IRQ is routable -- that is, if it is descended
> + * from the platform's primary GIC.
> + *
> + * @param rirq The raw IRQ in question.
> + * @return True iff the given IRQ routes to a platform GIC.
> + */
> +static bool_t tegra_irq_is_routable(struct dt_raw_irq * rirq)
And as I found out just recently, use 'bool' instead of bool_t.
> +{
> + /* If the IRQ connects directly to our GIC, it's trivially routable. */
> + if ( rirq->controller == dt_interrupt_controller )
> + return true;
> +
> + /*
> + * If the IRQ belongs to a legacy interrupt controller, then it's
> + * effectively owned by the GIC, and is routable.
> + */
> + if ( tegra_irq_belongs_to_ictlr(rirq) )
> + return true;
> +
> + return false;
> +}
> +
> +/**
> + * Returns the IRQ number for a given device. Tegra IRQs transalate using the
> + * same algorithm as normal GIC IRQs, but aren't parented by the system GIC.
> + *
> + * As a result, translation fails an assertion in the normal translation path.
> + * The normal version is essentially dt_irq_xlate wrapped with an assert, so
> + * we'll just call dt_irq_xlate directly.
> + *
> + * @param device The DT node describing the device.
> + * @param index The index of the interrupt within the device node.
> + * @return The translated number of the IRQ, or a negative error code.
> + */
> +static int tegra_irq_for_device(const struct dt_device_node *device, int index)
> +{
> + struct dt_raw_irq raw;
> + struct dt_irq dt_irq;
> + int res;
> +
> + res = dt_device_get_raw_irq(device, index, &raw);
> + if ( res )
> + return -ENODEV;
> +
> + /*
> + * The translation function for the Tegra ictlr happens to match the
> + * translation function for the normal GIC, so we'll use that in either
> + * case.
> + */
> + res = dt_irq_xlate(raw.specifier, raw.size, &dt_irq.irq, &dt_irq.type);
> + if ( res )
> + return -ENODEV;
> +
> + if ( irq_set_type(dt_irq.irq, dt_irq.type) )
> + return -ENODEV;
> +
> + return dt_irq.irq;
> +}
> +
> +/**
> + * Platform-specific reset code for the Tegra devices.
> + * Should not return.
> + */
> +static void tegra_reset(void)
> +{
> + void __iomem *addr;
> + u32 val;
> +
> + addr = ioremap_nocache(TEGRA_RESET_BASE, TEGRA_RESET_SIZE);
> + if ( !addr )
> + {
> + printk(XENLOG_ERR "Tegra: Unable to map tegra reset address. Reset failed!\n");
> + return;
> + }
> +
> + /* Write into the reset device. */
> + val = readl(addr) | TEGRA_RESET_MASK;
> + writel(val, addr);
No need to re-read the register?
> +
> + iounmap(addr);
> +}
> +
> +/**
> + * Applies an interrupt enable to a given interrupt via the legacy
> + * interrupt controller, and marks that interrupt as a normal interrupt,
> + * rather than a fast IRQ.
> + *
> + * @param irq The hardware IRQ number for the given interrupt.
> + */
> +static void tegra_ictlr_set_interrupt_enable(unsigned int irq, bool enabled)
> +{
> + uint32_t previous_iep_class;
> +
> + /* If we're enabling a given bit, use the SET register; otherwise CLR. */
> + unsigned int register_number =
> + enabled ? TEGRA_ICTLR_CPU_IER_SET : TEGRA_ICTLR_CPU_IER_CLR;
> +
> + /*
> + * Determine the IRQ number in the ictlr domain, and figure out the indexA
> + * of the individual controller we're working with. */
Put the */ on its own line please.
> + unsigned int ictlr_irq = irq - NR_LOCAL_IRQS;
> + unsigned int ictlr_number = ictlr_irq / TEGRA_IRQS_PER_ICTLR;
> +
> + /* Get a pointer to the target ictlr. */
> + void __iomem * target_ictlr = tegra_ictlr_base + TEGRA_ICTLR_SIZE * ictlr_number;
> +
> + /* Determine the mask we'll be working with. */
> + uint32_t mask = BIT(ictlr_irq % TEGRA_IRQS_PER_ICTLR);
> +
> + /* Sanity check our memory access. */
> + ASSERT(tegra_ictlr_base);
> + ASSERT(ictlr_number < TEGRA_ICTLR_COUNT);
> + ASSERT(irq >= NR_LOCAL_IRQS);
> +
> + /* Enable the given IRQ. */
> + writel(mask, target_ictlr + register_number);
> +
> + /* Mark the interrupt as a normal interrupt-- not a fast IRQ. */
> + previous_iep_class = readl(target_ictlr + TEGRA_ICTLR_CPU_IEP_CLASS);
> + writel(previous_iep_class & ~mask, target_ictlr + TEGRA_ICTLR_CPU_IEP_CLASS);
> +}
> +
> +
> +/**
> + * Routes an IRQ to a guest, applying sane values to the ictlr masks.
> + *
> + * @param domain The domain to which the IRQ will be routed.
> + * @param virq The virtual IRQ number.
> + * @param desc The IRQ to be routed.
> + * @param priority The IRQ priority.
> + * @return 0 on success, or an error code on failure.
> + */
> +static int tegra_route_irq_to_guest(struct domain *d, unsigned int virq,
> + struct irq_desc *desc, unsigned int priority)
> +{
> + /* Program the core GIC to deliver the interrupt to the guest. */
> + int rc = gic_route_irq_to_guest(d, virq, desc, priority);
> +
> + /* If we couldn't route the IRQ via the GIC, bail out. */
> + if(rc)
Uh oh.
> + {
> + printk(XENLOG_ERR "Tegra LIC: Couldn't program GIC to route vIRQ %d (%d).\n",
> + desc->irq, rc);
> + return rc;
> + }
> +
> + /*
> + * If this is a local IRQ, it's not masked by the ictlr, so we
> + * don't need to perform any ictlr manipulation.
> + */
> + if( desc->irq < NR_LOCAL_IRQS )
> + return rc;
> +
> + /*
> + * If this is the hardware domain, it will have real access to the ictlr,
> + * and will program the ictlr itself, so it should start with the ictlr
> + * disabled. If we're not the hwdom, the domain won't interact with the
> + * ictlr, and the interrupt shouldn't be masked.
> + */
> + tegra_ictlr_set_interrupt_enable(desc->irq, !is_hardware_domain(d));
> + return rc;
> +}
> +
> +
> +/**
> + * Routes an IRQ to Xen. This method both performs the core IRQ routing, and
> + * sets up any ictlr routing necessary.
> + *
> + * @param desc The IRQ to be routed.
> + * @param priority The IRQ priority.
> + */
> +static void tegra_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
> +{
> + unsigned int irq = desc->irq;
> +
> + /* Program the core GIC to deliver the interrupt to Xen. */
> + gic_route_irq_to_xen(desc, priority);
> +
> + /*
> + * If this is a local IRQ, it's not masked by the ictlr, so we
> + * don't need to perform any ictlr manipulation.
> + */
> + if( irq < NR_LOCAL_IRQS )
> + return;
> +
> + /*
> + * Enable the interrupt in the ictlr. Xen only uses the GIC to
> + * perform masking, so we'll enable the interrupt to prevent ictlr
> + * gating of the interrupt.
> + */
> + tegra_ictlr_set_interrupt_enable(irq, true);
> +
> +}
> +
> +/**
> + * Initialize the Tegra legacy interrupt controller, placing each interrupt
> + * into a default state. These defaults ensure that stray interrupts don't
> + * affect Xen.
> + */
> +static int tegra_initialize_legacy_interrupt_controller(void)
> +{
> + int i;
> +
> + /* Map in the tegra ictlr. */
> + tegra_ictlr_base = ioremap_nocache(TEGRA_ICTLR_BASE,
> + TEGRA_ICTLR_SIZE * TEGRA_ICTLR_COUNT);
Odd, style?
> +
> + if ( !tegra_ictlr_base )
> + panic("Failed to map in the Tegra legacy interrupt controller!\n");
> +
> + /* Initialize each of the legacy interrupt controllers. */
> + for (i = 0; i < TEGRA_ICTLR_COUNT; i++)
Style.
> + {
> + void __iomem *ictlr_n = tegra_ictlr_base + TEGRA_ICTLR_SIZE * i;
> +
> + /* Clear the interrupt enables for every interrupt. */
> + writel(~0, ictlr_n + TEGRA_ICTLR_CPU_IER_CLR);
> +
> + /*
> + * Mark all of our interrupts as normal ARM interrupts (as opposed
> + * to Fast Interrupts.)
> + */
> + writel(0, ictlr_n + TEGRA_ICTLR_CPU_IEP_CLASS);
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * Startup code for the Tegra.
> + */
> +static int tegra_init(void)
__init ?
> +{
> + int rc;
> +
> + rc = tegra_initialize_legacy_interrupt_controller();
How about you jsut make this:
return tegra_initializa...()
> +
> + /*
> + * TODO: Virtualize the MMIO regions for the ictlr, ensuring the hwdom
> + * (and possibly passhtrough domains) can only access ictlr registers for
> + * interrupts they actually own.
> + */
> +
> + return rc;
> +}
> +
> +
> +static const char * const tegra_dt_compat[] __initconst =
> +{
> + "nvidia,tegra120", /* Tegra K1 */
> + "nvidia,tegra210", /* Tegra X1 */
> + NULL
> +};
> +
> +static const struct dt_device_match tegra_blacklist_dev[] __initconst =
> +{
> + /*
> + * The UARTs share a page which runs the risk of mapping the Xen console
> + * UART to dom0, so don't map any of them.
> + */
> + DT_MATCH_COMPATIBLE("nvidia,tegra20-uart"),
> + { /* sentinel */ },
> +};
> +
> +PLATFORM_START(tegra, "Tegra")
> + .blacklist_dev = tegra_blacklist_dev,
> + .compatible = tegra_dt_compat,
> + .init = tegra_init,
> + .reset = tegra_reset,
> + .irq_is_routable = tegra_irq_is_routable,
> + .irq_for_device = tegra_irq_for_device,
> + .route_irq_to_xen = tegra_route_irq_to_xen,
> + .route_irq_to_guest = tegra_route_irq_to_guest,
> +PLATFORM_END
You are missing this:
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> diff --git a/xen/include/asm-arm/platforms/tegra.h b/xen/include/asm-arm/platforms/tegra.h
> new file mode 100644
> index 0000000..821add5
> --- /dev/null
> +++ b/xen/include/asm-arm/platforms/tegra.h
> @@ -0,0 +1,50 @@
> +/*
> + * NVIDIA Tegra platform definitions
> + *
> + * Kyle Temkin; Copyright (c) 2016 Assured Information Security, Inc.
> + * Chris Patterson; Copyright (c) 2016 Assured Information Security, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +
> +#ifndef __ASM_ARM_PLATFORMS_TEGRA_H
Add please two underscores at the far end.
> +#define __ASM_ARM_PLATFORMS_TEGRA_H
> +
> +#define TEGRA_ICTLR_BASE 0x60004000
> +#define TEGRA_ICTLR_SIZE 0x00000100
> +#define TEGRA_ICTLR_COUNT 6
> +#define TEGRA_IRQS_PER_ICTLR 32
> +
> +#define TEGRA_RESET_BASE 0x7000e400
> +#define TEGRA_RESET_SIZE 4
> +#define TEGRA_RESET_MASK 0x10
> +
> +#define TEGRA_ICTLR_CPU_IER 0x20
> +#define TEGRA_ICTLR_CPU_IER_SET 0x24
> +#define TEGRA_ICTLR_CPU_IER_CLR 0x28
> +#define TEGRA_ICTLR_CPU_IEP_CLASS 0x2C
> +
> +#define TEGRA_ICTLR_COP_IER 0x30
> +#define TEGRA_ICTLR_COP_IER_SET 0x34
> +#define TEGRA_ICTLR_COP_IER_CLR 0x38
> +#define TEGRA_ICTLR_COP_IEP_CLASS 0x3c
> +
> +
> +#endif /* __ASM_ARM_PLATFORMS_TEGRA_H */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 2.9.2
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH RFC 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing.
2016-09-16 14:50 ` Konrad Rzeszutek Wilk
@ 2016-10-22 0:06 ` Stefano Stabellini
0 siblings, 0 replies; 17+ messages in thread
From: Stefano Stabellini @ 2016-10-22 0:06 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: julien.grall, xen-devel, Kyle J. Temkin, sstabellini, Kyle Temkin
Thanks for the patch! I would like to see it in Xen 4.9.
On Fri, 16 Sep 2016, Konrad Rzeszutek Wilk wrote:
> On Mon, Sep 05, 2016 at 06:13:59AM -0400, Kyle Temkin wrote:
> > From: "Kyle J. Temkin" <temkink@ainfosec.com>
> >
> > Tegra devices have a legacy interrupt controller (lic, or ictlr) that
> > must be programmed in parallel with their primary GIC. For all intents
> > and purposes, we treat this devices attached to this controller as
^ these
> > connected to the primary GIC, as it will be handling their interrupts.
>
> Is there a link to the specification? Could that be included in the file
> or in the commit description?
>
> >
> > This commit adds support for exposing the ictlr to the hardware domain;
> > but a future commit will extend this to support exposing a virtualized
> > version of the ictlr to the hardware domain, and to ensure that
> > interrupts are unmasked properly when routed to a Xen, or to a domain
> > other than the hardware domain.
> >
> > Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> > ---
> > xen/arch/arm/platforms/Makefile | 2 +
> > xen/arch/arm/platforms/tegra.c | 339 ++++++++++++++++++++++++++++++++++
> > xen/include/asm-arm/platforms/tegra.h | 50 +++++
> > 3 files changed, 391 insertions(+)
> > create mode 100644 xen/arch/arm/platforms/tegra.c
> > create mode 100644 xen/include/asm-arm/platforms/tegra.h
> >
> > diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> > index 49fa683..0c3a7f4 100644
> > --- a/xen/arch/arm/platforms/Makefile
> > +++ b/xen/arch/arm/platforms/Makefile
> > @@ -4,7 +4,9 @@ obj-$(CONFIG_ARM_32) += exynos5.o
> > obj-$(CONFIG_ARM_32) += midway.o
> > obj-$(CONFIG_ARM_32) += omap5.o
> > obj-$(CONFIG_ARM_32) += rcar2.o
> > +obj-$(CONFIG_ARM_32) += tegra.o
> > obj-$(CONFIG_ARM_64) += seattle.o
> > obj-$(CONFIG_ARM_32) += sunxi.o
> > obj-$(CONFIG_ARM_64) += xgene-storm.o
> > obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
> > +obj-$(CONFIG_ARM_64) += tegra.o
Are there both 32bit and 64bit versions of tegra?
> > diff --git a/xen/arch/arm/platforms/tegra.c b/xen/arch/arm/platforms/tegra.c
> > new file mode 100644
> > index 0000000..e75242c
> > --- /dev/null
> > +++ b/xen/arch/arm/platforms/tegra.c
> > @@ -0,0 +1,339 @@
> > +/*
> > + * NVIDIA Tegra specific settings
> > + *
> > + * Ian Campbell; Copyright (c) 2014 Citrix Systems
> > + * Kyle Temkin; Copyright (c) 2016 Assured Information Security, Inc.
> > + * Chris Patterson; Copyright (c) 2016 Assured Information Security, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <xen/config.h>
>
> No config.h pls.
> > +#include <xen/lib.h>
> > +#include <xen/stdbool.h>
> > +#include <xen/sched.h>
> > +#include <xen/vmap.h>
> > +
> > +#include <asm/io.h>
> > +#include <asm/gic.h>
> > +#include <asm/platform.h>
> > +#include <asm/platforms/tegra.h>
> > +
> > +
> > +/* Permanent mapping to the Tegra legacy interrupt controller. */
> > +static void __iomem *tegra_ictlr_base = NULL;
>
> No need for NULL.
> > +
> > +/*
> > + * List of legacy interrupt controllers that can be used to route
> > + * Tegra interrupts.
> > + */
> > +static const char * const tegra_interrupt_compat[] __initconst =
> > +{
> > + "nvidia,tegra120-ictlr", /* Tegra K1 controllers */
> > + "nvidia,tegra210-ictlr" /* Tegra X1 controllers */
> > +};
> > +
> > +
> > +/**
> > + * Returns true if the given IRQ belongs to a supported tegra interrupt
> > + * controller.
> > + *
> > + * @param rirq The raw IRQ to be identified.
> > + * @return True iff the given IRQ belongs to a Tegra ictlr.
> > + */
> > +static bool_t tegra_irq_belongs_to_ictlr(struct dt_raw_irq * rirq) {
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(tegra_interrupt_compat); i++)
>
> Something is off with your style ({, and the lack of spaces around ()).
>
> > + {
> > + if ( dt_device_is_compatible(rirq->controller, tegra_interrupt_compat[i]) )
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +
> > +/**
> > + * Returns true iff the given IRQ is routable -- that is, if it is descended
> > + * from the platform's primary GIC.
> > + *
> > + * @param rirq The raw IRQ in question.
> > + * @return True iff the given IRQ routes to a platform GIC.
> > + */
> > +static bool_t tegra_irq_is_routable(struct dt_raw_irq * rirq)
>
> And as I found out just recently, use 'bool' instead of bool_t.
>
> > +{
> > + /* If the IRQ connects directly to our GIC, it's trivially routable. */
> > + if ( rirq->controller == dt_interrupt_controller )
> > + return true;
> > +
> > + /*
> > + * If the IRQ belongs to a legacy interrupt controller, then it's
> > + * effectively owned by the GIC, and is routable.
> > + */
> > + if ( tegra_irq_belongs_to_ictlr(rirq) )
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +/**
> > + * Returns the IRQ number for a given device. Tegra IRQs transalate using the
> > + * same algorithm as normal GIC IRQs, but aren't parented by the system GIC.
> > + *
> > + * As a result, translation fails an assertion in the normal translation path.
> > + * The normal version is essentially dt_irq_xlate wrapped with an assert, so
> > + * we'll just call dt_irq_xlate directly.
> > + *
> > + * @param device The DT node describing the device.
> > + * @param index The index of the interrupt within the device node.
> > + * @return The translated number of the IRQ, or a negative error code.
> > + */
> > +static int tegra_irq_for_device(const struct dt_device_node *device, int index)
> > +{
> > + struct dt_raw_irq raw;
> > + struct dt_irq dt_irq;
> > + int res;
> > +
> > + res = dt_device_get_raw_irq(device, index, &raw);
> > + if ( res )
> > + return -ENODEV;
> > +
> > + /*
> > + * The translation function for the Tegra ictlr happens to match the
> > + * translation function for the normal GIC, so we'll use that in either
> > + * case.
> > + */
> > + res = dt_irq_xlate(raw.specifier, raw.size, &dt_irq.irq, &dt_irq.type);
> > + if ( res )
> > + return -ENODEV;
> > +
> > + if ( irq_set_type(dt_irq.irq, dt_irq.type) )
> > + return -ENODEV;
> > +
> > + return dt_irq.irq;
> > +}
Instead of duplicating this code, I am tempted to suggest to modify
dt_irq_translate to make it work for tegra too. You just need to change
the raw->controller != dt_interrupt_controller check to also check for
tegra right?
> > +/**
> > + * Platform-specific reset code for the Tegra devices.
> > + * Should not return.
> > + */
> > +static void tegra_reset(void)
> > +{
> > + void __iomem *addr;
> > + u32 val;
> > +
> > + addr = ioremap_nocache(TEGRA_RESET_BASE, TEGRA_RESET_SIZE);
> > + if ( !addr )
> > + {
> > + printk(XENLOG_ERR "Tegra: Unable to map tegra reset address. Reset failed!\n");
> > + return;
> > + }
> > +
> > + /* Write into the reset device. */
> > + val = readl(addr) | TEGRA_RESET_MASK;
> > + writel(val, addr);
>
> No need to re-read the register?
>
> > +
> > + iounmap(addr);
> > +}
> > +
> > +/**
> > + * Applies an interrupt enable to a given interrupt via the legacy
> > + * interrupt controller, and marks that interrupt as a normal interrupt,
> > + * rather than a fast IRQ.
> > + *
> > + * @param irq The hardware IRQ number for the given interrupt.
> > + */
> > +static void tegra_ictlr_set_interrupt_enable(unsigned int irq, bool enabled)
> > +{
> > + uint32_t previous_iep_class;
> > +
> > + /* If we're enabling a given bit, use the SET register; otherwise CLR. */
> > + unsigned int register_number =
> > + enabled ? TEGRA_ICTLR_CPU_IER_SET : TEGRA_ICTLR_CPU_IER_CLR;
> > +
> > + /*
> > + * Determine the IRQ number in the ictlr domain, and figure out the indexA
> > + * of the individual controller we're working with. */
>
> Put the */ on its own line please.
>
> > + unsigned int ictlr_irq = irq - NR_LOCAL_IRQS;
> > + unsigned int ictlr_number = ictlr_irq / TEGRA_IRQS_PER_ICTLR;
> > +
> > + /* Get a pointer to the target ictlr. */
> > + void __iomem * target_ictlr = tegra_ictlr_base + TEGRA_ICTLR_SIZE * ictlr_number;
> > +
> > + /* Determine the mask we'll be working with. */
> > + uint32_t mask = BIT(ictlr_irq % TEGRA_IRQS_PER_ICTLR);
> > +
> > + /* Sanity check our memory access. */
> > + ASSERT(tegra_ictlr_base);
> > + ASSERT(ictlr_number < TEGRA_ICTLR_COUNT);
> > + ASSERT(irq >= NR_LOCAL_IRQS);
> > +
> > + /* Enable the given IRQ. */
> > + writel(mask, target_ictlr + register_number);
> > +
> > + /* Mark the interrupt as a normal interrupt-- not a fast IRQ. */
> > + previous_iep_class = readl(target_ictlr + TEGRA_ICTLR_CPU_IEP_CLASS);
> > + writel(previous_iep_class & ~mask, target_ictlr + TEGRA_ICTLR_CPU_IEP_CLASS);
> > +}
I think it makes sense to split this function into two: one that set the
interrupt as normal, and the other to enable/disable it.
> > +
> > +/**
> > + * Routes an IRQ to a guest, applying sane values to the ictlr masks.
> > + *
> > + * @param domain The domain to which the IRQ will be routed.
> > + * @param virq The virtual IRQ number.
> > + * @param desc The IRQ to be routed.
> > + * @param priority The IRQ priority.
> > + * @return 0 on success, or an error code on failure.
> > + */
> > +static int tegra_route_irq_to_guest(struct domain *d, unsigned int virq,
> > + struct irq_desc *desc, unsigned int priority)
> > +{
> > + /* Program the core GIC to deliver the interrupt to the guest. */
> > + int rc = gic_route_irq_to_guest(d, virq, desc, priority);
> > +
> > + /* If we couldn't route the IRQ via the GIC, bail out. */
> > + if(rc)
>
> Uh oh.
Style. No need to add another warning message here, given that this
error doesn't seem to be tegra specific.
> > + {
> > + printk(XENLOG_ERR "Tegra LIC: Couldn't program GIC to route vIRQ %d (%d).\n",
> > + desc->irq, rc);
> > + return rc;
> > + }
> > +
> > + /*
> > + * If this is a local IRQ, it's not masked by the ictlr, so we
> > + * don't need to perform any ictlr manipulation.
> > + */
> > + if( desc->irq < NR_LOCAL_IRQS )
> > + return rc;
> > +
> > + /*
> > + * If this is the hardware domain, it will have real access to the ictlr,
> > + * and will program the ictlr itself, so it should start with the ictlr
> > + * disabled. If we're not the hwdom, the domain won't interact with the
> > + * ictlr, and the interrupt shouldn't be masked.
> > + */
> > + tegra_ictlr_set_interrupt_enable(desc->irq, !is_hardware_domain(d));
> > + return rc;
> > +}
> > +
> > +
> > +/**
> > + * Routes an IRQ to Xen. This method both performs the core IRQ routing, and
> > + * sets up any ictlr routing necessary.
> > + *
> > + * @param desc The IRQ to be routed.
> > + * @param priority The IRQ priority.
> > + */
> > +static void tegra_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
> > +{
> > + unsigned int irq = desc->irq;
> > +
> > + /* Program the core GIC to deliver the interrupt to Xen. */
> > + gic_route_irq_to_xen(desc, priority);
> > +
> > + /*
> > + * If this is a local IRQ, it's not masked by the ictlr, so we
> > + * don't need to perform any ictlr manipulation.
> > + */
> > + if( irq < NR_LOCAL_IRQS )
> > + return;
> > +
> > + /*
> > + * Enable the interrupt in the ictlr. Xen only uses the GIC to
> > + * perform masking, so we'll enable the interrupt to prevent ictlr
> > + * gating of the interrupt.
> > + */
> > + tegra_ictlr_set_interrupt_enable(irq, true);
> > +
> > +}
> > +
> > +/**
> > + * Initialize the Tegra legacy interrupt controller, placing each interrupt
> > + * into a default state. These defaults ensure that stray interrupts don't
> > + * affect Xen.
> > + */
> > +static int tegra_initialize_legacy_interrupt_controller(void)
> > +{
> > + int i;
> > +
> > + /* Map in the tegra ictlr. */
> > + tegra_ictlr_base = ioremap_nocache(TEGRA_ICTLR_BASE,
> > + TEGRA_ICTLR_SIZE * TEGRA_ICTLR_COUNT);
>
> Odd, style?
>
> > +
> > + if ( !tegra_ictlr_base )
> > + panic("Failed to map in the Tegra legacy interrupt controller!\n");
> > +
> > + /* Initialize each of the legacy interrupt controllers. */
> > + for (i = 0; i < TEGRA_ICTLR_COUNT; i++)
>
> Style.
> > + {
> > + void __iomem *ictlr_n = tegra_ictlr_base + TEGRA_ICTLR_SIZE * i;
> > +
> > + /* Clear the interrupt enables for every interrupt. */
> > + writel(~0, ictlr_n + TEGRA_ICTLR_CPU_IER_CLR);
> > +
> > + /*
> > + * Mark all of our interrupts as normal ARM interrupts (as opposed
> > + * to Fast Interrupts.)
> > + */
> > + writel(0, ictlr_n + TEGRA_ICTLR_CPU_IEP_CLASS);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
style
> > + * Startup code for the Tegra.
> > + */
> > +static int tegra_init(void)
>
> __init ?
> > +{
> > + int rc;
> > +
> > + rc = tegra_initialize_legacy_interrupt_controller();
>
> How about you jsut make this:
>
> return tegra_initializa...()
> > +
> > + /*
> > + * TODO: Virtualize the MMIO regions for the ictlr, ensuring the hwdom
> > + * (and possibly passhtrough domains) can only access ictlr registers for
> > + * interrupts they actually own.
> > + */
> > +
> > + return rc;
> > +}
> > +
> > +
> > +static const char * const tegra_dt_compat[] __initconst =
> > +{
> > + "nvidia,tegra120", /* Tegra K1 */
> > + "nvidia,tegra210", /* Tegra X1 */
I can find "nvidia,tegra210" under arch/arm64/boot/dts/nvidia, but I
cannot find "nvidia,tegra120" anywhere. Do you have a pointer? If it
hasn't been upstreamed in Linux, we need to provide a pointer to the
Linux tree and the device tree for K1.
> > + NULL
> > +};
> > +
> > +static const struct dt_device_match tegra_blacklist_dev[] __initconst =
> > +{
> > + /*
> > + * The UARTs share a page which runs the risk of mapping the Xen console
> > + * UART to dom0, so don't map any of them.
> > + */
> > + DT_MATCH_COMPATIBLE("nvidia,tegra20-uart"),
> > + { /* sentinel */ },
> > +};
> > +
> > +PLATFORM_START(tegra, "Tegra")
> > + .blacklist_dev = tegra_blacklist_dev,
> > + .compatible = tegra_dt_compat,
> > + .init = tegra_init,
> > + .reset = tegra_reset,
> > + .irq_is_routable = tegra_irq_is_routable,
> > + .irq_for_device = tegra_irq_for_device,
> > + .route_irq_to_xen = tegra_route_irq_to_xen,
> > + .route_irq_to_guest = tegra_route_irq_to_guest,
> > +PLATFORM_END
>
> You are missing this:
>
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
>
> > diff --git a/xen/include/asm-arm/platforms/tegra.h b/xen/include/asm-arm/platforms/tegra.h
> > new file mode 100644
> > index 0000000..821add5
> > --- /dev/null
> > +++ b/xen/include/asm-arm/platforms/tegra.h
> > @@ -0,0 +1,50 @@
> > +/*
> > + * NVIDIA Tegra platform definitions
> > + *
> > + * Kyle Temkin; Copyright (c) 2016 Assured Information Security, Inc.
> > + * Chris Patterson; Copyright (c) 2016 Assured Information Security, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +
> > +#ifndef __ASM_ARM_PLATFORMS_TEGRA_H
>
> Add please two underscores at the far end.
>
> > +#define __ASM_ARM_PLATFORMS_TEGRA_H
> > +
> > +#define TEGRA_ICTLR_BASE 0x60004000
> > +#define TEGRA_ICTLR_SIZE 0x00000100
> > +#define TEGRA_ICTLR_COUNT 6
> > +#define TEGRA_IRQS_PER_ICTLR 32
> > +
> > +#define TEGRA_RESET_BASE 0x7000e400
> > +#define TEGRA_RESET_SIZE 4
> > +#define TEGRA_RESET_MASK 0x10
Please keep all the TEGRA_ICTLR* defines together: move TEGRA_RESET*
below.
> > +#define TEGRA_ICTLR_CPU_IER 0x20
> > +#define TEGRA_ICTLR_CPU_IER_SET 0x24
> > +#define TEGRA_ICTLR_CPU_IER_CLR 0x28
> > +#define TEGRA_ICTLR_CPU_IEP_CLASS 0x2C
> > +
> > +#define TEGRA_ICTLR_COP_IER 0x30
> > +#define TEGRA_ICTLR_COP_IER_SET 0x34
> > +#define TEGRA_ICTLR_COP_IER_CLR 0x38
> > +#define TEGRA_ICTLR_COP_IEP_CLASS 0x3c
> > +
> > +
> > +#endif /* __ASM_ARM_PLATFORMS_TEGRA_H */
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH RFC 5/6] xen/arm: Add function to query IRQ 'ownership'.
2016-09-05 10:13 [PATCH RFC 0/6] ARM: Add support for Tegra SoCs (incl. Jetson TK1, Jetson TX1) Kyle Temkin
` (3 preceding siblings ...)
2016-09-05 10:13 ` [PATCH RFC 4/6] xen/arm: platforms: Add Tegra platform to support basic " Kyle Temkin
@ 2016-09-05 10:14 ` Kyle Temkin
2016-09-16 14:53 ` Konrad Rzeszutek Wilk
2016-09-05 10:14 ` [PATCH RFC 6/6] xen/arm: platforms/tegra: ensure the hwdom can only affect its own interrupts Kyle Temkin
5 siblings, 1 reply; 17+ messages in thread
From: Kyle Temkin @ 2016-09-05 10:14 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, sstabellini, Kyle J. Temkin
From: "Kyle J. Temkin" <temkink@ainfosec.com>
The addition of new IRQ-related platform hooks now allow platforms to
perform platform-specific interrupt logic; allowing e.g. virtualization
of platform-specific interrupt controller hardware.
This commit adds the ability to for the platform to identify the domain
a given IRQ routes to, allowing platform logic to e.g. deny access to
registers associated with a given IRQ unless the requesting domain
'owns' the IRQ. This will be used on Tegra platforms, where the hardware
domain needs access to its legacy interrupt controller, but should not
be able to control registers that correspond to other domains' IRQs, or
sections associated with IRQs routed to Xen.
Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
---
xen/arch/arm/irq.c | 10 ++++++++++
xen/include/asm-arm/irq.h | 2 ++
2 files changed, 12 insertions(+)
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index dc42817..c6e1a24 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -144,6 +144,16 @@ static inline struct domain *irq_get_domain(struct irq_desc *desc)
return irq_get_guest_info(desc)->d;
}
+domid_t irq_get_domain_id(struct irq_desc *desc)
+{
+ // If this domain isn't routed to a guest, return DOMID_XEN.
+ if(!test_bit(_IRQ_GUEST, &desc->status))
+ return DOMID_XEN;
+
+ // Otherise, get the guest domain's information.
+ return irq_get_domain(desc)->domain_id;
+}
+
void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
{
if ( desc != NULL )
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 8f7a167..55300a8 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -45,6 +45,8 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
unsigned int irq, const char *devname);
int release_guest_irq(struct domain *d, unsigned int irq);
+domid_t irq_get_domain_id(struct irq_desc *desc);
+
void arch_move_irqs(struct vcpu *v);
#define arch_evtchn_bind_pirq(d, pirq) ((void)((d) + (pirq)))
--
2.9.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH RFC 5/6] xen/arm: Add function to query IRQ 'ownership'.
2016-09-05 10:14 ` [PATCH RFC 5/6] xen/arm: Add function to query IRQ 'ownership' Kyle Temkin
@ 2016-09-16 14:53 ` Konrad Rzeszutek Wilk
2016-10-22 0:10 ` Stefano Stabellini
0 siblings, 1 reply; 17+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-16 14:53 UTC (permalink / raw)
To: Kyle Temkin; +Cc: julien.grall, sstabellini, Kyle J. Temkin, xen-devel
On Mon, Sep 05, 2016 at 06:14:00AM -0400, Kyle Temkin wrote:
> From: "Kyle J. Temkin" <temkink@ainfosec.com>
>
> The addition of new IRQ-related platform hooks now allow platforms to
> perform platform-specific interrupt logic; allowing e.g. virtualization
> of platform-specific interrupt controller hardware.
>
> This commit adds the ability to for the platform to identify the domain
> a given IRQ routes to, allowing platform logic to e.g. deny access to
> registers associated with a given IRQ unless the requesting domain
> 'owns' the IRQ. This will be used on Tegra platforms, where the hardware
> domain needs access to its legacy interrupt controller, but should not
> be able to control registers that correspond to other domains' IRQs, or
> sections associated with IRQs routed to Xen.
>
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> ---
> xen/arch/arm/irq.c | 10 ++++++++++
> xen/include/asm-arm/irq.h | 2 ++
> 2 files changed, 12 insertions(+)
>
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index dc42817..c6e1a24 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -144,6 +144,16 @@ static inline struct domain *irq_get_domain(struct irq_desc *desc)
> return irq_get_guest_info(desc)->d;
> }
>
> +domid_t irq_get_domain_id(struct irq_desc *desc)
> +{
> + // If this domain isn't routed to a guest, return DOMID_XEN.
So that is some odd style
> + if(!test_bit(_IRQ_GUEST, &desc->status))
Ditto here?
I think your v1 should have at least these fixed..
> + return DOMID_XEN;
> +
> + // Otherise, get the guest domain's information.
> + return irq_get_domain(desc)->domain_id;
> +}
> +
> void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
> {
> if ( desc != NULL )
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index 8f7a167..55300a8 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -45,6 +45,8 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
> unsigned int irq, const char *devname);
> int release_guest_irq(struct domain *d, unsigned int irq);
>
> +domid_t irq_get_domain_id(struct irq_desc *desc);
> +
> void arch_move_irqs(struct vcpu *v);
>
> #define arch_evtchn_bind_pirq(d, pirq) ((void)((d) + (pirq)))
> --
> 2.9.2
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH RFC 5/6] xen/arm: Add function to query IRQ 'ownership'.
2016-09-16 14:53 ` Konrad Rzeszutek Wilk
@ 2016-10-22 0:10 ` Stefano Stabellini
0 siblings, 0 replies; 17+ messages in thread
From: Stefano Stabellini @ 2016-10-22 0:10 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: julien.grall, xen-devel, Kyle J. Temkin, sstabellini, Kyle Temkin
On Fri, 16 Sep 2016, Konrad Rzeszutek Wilk wrote:
> On Mon, Sep 05, 2016 at 06:14:00AM -0400, Kyle Temkin wrote:
> > From: "Kyle J. Temkin" <temkink@ainfosec.com>
> >
> > The addition of new IRQ-related platform hooks now allow platforms to
> > perform platform-specific interrupt logic; allowing e.g. virtualization
> > of platform-specific interrupt controller hardware.
> >
> > This commit adds the ability to for the platform to identify the domain
> > a given IRQ routes to, allowing platform logic to e.g. deny access to
> > registers associated with a given IRQ unless the requesting domain
> > 'owns' the IRQ. This will be used on Tegra platforms, where the hardware
> > domain needs access to its legacy interrupt controller, but should not
> > be able to control registers that correspond to other domains' IRQs, or
> > sections associated with IRQs routed to Xen.
> >
> > Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> > ---
> > xen/arch/arm/irq.c | 10 ++++++++++
> > xen/include/asm-arm/irq.h | 2 ++
> > 2 files changed, 12 insertions(+)
> >
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index dc42817..c6e1a24 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -144,6 +144,16 @@ static inline struct domain *irq_get_domain(struct irq_desc *desc)
> > return irq_get_guest_info(desc)->d;
> > }
> >
> > +domid_t irq_get_domain_id(struct irq_desc *desc)
> > +{
> > + // If this domain isn't routed to a guest, return DOMID_XEN.
>
> So that is some odd style
> > + if(!test_bit(_IRQ_GUEST, &desc->status))
>
> Ditto here?
>
> I think your v1 should have at least these fixed..
Didn't we have checkpatch.pl somewhere for Xen?
Anyway, with these two small issues fixed:
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > + return DOMID_XEN;
> > +
> > + // Otherise, get the guest domain's information.
> > + return irq_get_domain(desc)->domain_id;
> > +}
> > +
> > void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
> > {
> > if ( desc != NULL )
> > diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> > index 8f7a167..55300a8 100644
> > --- a/xen/include/asm-arm/irq.h
> > +++ b/xen/include/asm-arm/irq.h
> > @@ -45,6 +45,8 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
> > unsigned int irq, const char *devname);
> > int release_guest_irq(struct domain *d, unsigned int irq);
> >
> > +domid_t irq_get_domain_id(struct irq_desc *desc);
> > +
> > void arch_move_irqs(struct vcpu *v);
> >
> > #define arch_evtchn_bind_pirq(d, pirq) ((void)((d) + (pirq)))
> > --
> > 2.9.2
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > https://lists.xen.org/xen-devel
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH RFC 6/6] xen/arm: platforms/tegra: ensure the hwdom can only affect its own interrupts
2016-09-05 10:13 [PATCH RFC 0/6] ARM: Add support for Tegra SoCs (incl. Jetson TK1, Jetson TX1) Kyle Temkin
` (4 preceding siblings ...)
2016-09-05 10:14 ` [PATCH RFC 5/6] xen/arm: Add function to query IRQ 'ownership' Kyle Temkin
@ 2016-09-05 10:14 ` Kyle Temkin
2016-10-22 0:45 ` Stefano Stabellini
5 siblings, 1 reply; 17+ messages in thread
From: Kyle Temkin @ 2016-09-05 10:14 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, sstabellini, Kyle J. Temkin
From: "Kyle J. Temkin" <temkink@ainfosec.com>
Several Tegra hardware devices-- and the Tegra device tree-- expect
the presence of a Tegra Legacy Interrupt Controller (LIC) in the hardware
domain. Accordingly, we'll need to expose (most of) the LIC's registers
to the hardware domain.
As the Tegra LIC provides the ability to modify interrupt delivery (e.g.
by masking interrupts, forcing asserting/clearing them, or adjusting
their prority), it's important that the hardware domain's access be
mediated. This commit adds read/write handlers that prohibit
modification of register sections corresponding to interrupts not owned
by the hardware domain.
Note that this is written to be domain agnostic; this allows the
potential to e.g. map the ictlr into multiple domains if this is desired
for passthrough in the future.
Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
---
xen/arch/arm/platforms/tegra.c | 227 +++++++++++++++++++++++++++++++++++++++--
1 file changed, 216 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/platforms/tegra.c b/xen/arch/arm/platforms/tegra.c
index e75242c..a119c01 100644
--- a/xen/arch/arm/platforms/tegra.c
+++ b/xen/arch/arm/platforms/tegra.c
@@ -21,11 +21,13 @@
#include <xen/stdbool.h>
#include <xen/sched.h>
#include <xen/vmap.h>
+#include <xen/iocap.h>
#include <asm/io.h>
#include <asm/gic.h>
#include <asm/platform.h>
#include <asm/platforms/tegra.h>
+#include <asm/mmio.h>
/* Permanent mapping to the Tegra legacy interrupt controller. */
@@ -258,6 +260,218 @@ static void tegra_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
}
/**
+ * Parses a LIC MMIO read or write, and extracts the information needed to
+ * complete the request.
+ *
+ * @param info Information describing the MMIO read/write being performed.
+ * @param register_number The register number in the ictlr; e.g.
+ * TEGRA_ICTLR_CPU_IER_SET.
+ * @param register_offset The offset into tegra_icltr_base at which the target
+ * register exists.
+ * @param The number of the first IRQ represented by the given ictlr register.
+ */
+static void tegra_ictlr_parse_mmio_request(mmio_info_t *info,
+ int *register_number, int *register_offset, int *irq_base)
+{
+ /* Determine the offset of the access into the ICTLR region. */
+ uint32_t offset = info->gpa - TEGRA_ICTLR_BASE;
+
+ if(register_number)
+ *register_number = offset % TEGRA_ICTLR_SIZE;
+
+ if(register_offset)
+ *register_offset = offset;
+
+ if(irq_base) {
+ int ictlr_number = offset / TEGRA_ICTLR_SIZE;
+ *irq_base = (ictlr_number * TEGRA_IRQS_PER_ICTLR) + NR_LOCAL_IRQS;
+ }
+}
+
+/**
+ * Returns true iff the given IRQ is currently routed to the given domain.
+ *
+ * @param irq The IRQ number for the IRQ in question.
+ * @param d The domain in question.
+ * @return True iff the given domain is the current IRQ target.
+ */
+static bool irq_owned_by_domain(int irq, struct domain *d)
+{
+ struct irq_desc *desc = irq_to_desc(irq);
+ domid_t domid;
+ unsigned long flags;
+
+ BUG_ON(!desc);
+
+ spin_lock_irqsave(&desc->lock, flags);
+ domid = irq_get_domain_id(desc);
+ spin_unlock_irqrestore(&desc->lock, flags);
+
+ return (d->domain_id == domid);
+}
+
+
+/**
+ * Mediates an MMIO-read to the Tegra legacy interrupt controller.
+ * Ensures that each domain only is passed interrupt state for its
+ * own interupts.
+ */
+static int tegra_ictlr_domain_read(struct vcpu *v, mmio_info_t *info,
+ register_t *target_register, void *priv)
+{
+ register_t raw_value;
+
+ int register_number;
+ int register_offset;
+ int irq_base;
+ int i;
+
+ tegra_ictlr_parse_mmio_request(info, ®ister_number, ®ister_offset,
+ &irq_base);
+
+ /* Sanity check the read. */
+ if ( register_offset & 0x3 ) {
+ printk(XENLOG_G_ERR "d%u: Tegra LIC: Attempt to read unaligned ictlr addr"
+ "(%" PRIpaddr ")!", current->domain->domain_id, info->gpa);
+ domain_crash_synchronous();
+ }
+ if ( info->dabt.size != DABT_WORD ) {
+ printk(XENLOG_G_ERR "d%u: Tegra LIC: Non-word read from ictlr addr"
+ "%" PRIpaddr "!", current->domain->domain_id, info->gpa);
+ domain_crash_synchronous();
+ }
+
+ /* Ensure that we've only been handed an offset within our region. */
+ BUG_ON(register_offset > TEGRA_ICTLR_SIZE * TEGRA_ICTLR_COUNT);
+
+ /* Perform the core ictlr read. */
+ raw_value = readl(tegra_ictlr_base + register_offset);
+
+ /*
+ * We don't want to leak information about interrupts not controlled
+ * by the active domain. Thus, we'll zero out any ictlr slots for
+ * IRQs not owned by the given domain.
+ */
+ for (i = 0; i < TEGRA_IRQS_PER_ICTLR; ++i) {
+ int irq = irq_base + i;
+ int mask = BIT(irq % 32);
+
+ if(!irq_owned_by_domain(irq, current->domain))
+ raw_value &= ~mask;
+ }
+
+ /* Finally, set the target register to our read value */
+ *target_register = raw_value;
+ return 1;
+}
+
+
+/**
+ * Mediates an MMIO-read to the Tegra legacy interrupt controller.
+ * Ensures that each domain only can only control is own interrupts.
+ */
+static int tegra_ictlr_domain_write(struct vcpu *v, mmio_info_t *info,
+ register_t new_value, void *priv)
+{
+ register_t write_mask = 0;
+ register_t raw_value;
+
+ int register_number;
+ int register_offset;
+ int irq_base;
+ int i;
+
+ tegra_ictlr_parse_mmio_request(info, ®ister_number, ®ister_offset,
+ &irq_base);
+
+ /* Sanity check the read. */
+ if ( register_offset & 0x3 ) {
+ printk(XENLOG_G_ERR "d%u: Tegra LIC: Attempt to write unaligned ictlr addr"
+ "(%" PRIpaddr ")!", current->domain->domain_id, info->gpa);
+ domain_crash_synchronous();
+ return 0;
+ }
+ if ( info->dabt.size != DABT_WORD ) {
+ printk(XENLOG_G_ERR "d%u: Tegra LIC: Non-word write to ictlr addr"
+ "%" PRIpaddr "!", current->domain->domain_id, info->gpa);
+ domain_crash_synchronous();
+ return 0;
+ }
+
+ /* Ensure that we've only been handed an offset within our region. */
+ BUG_ON(register_offset > TEGRA_ICTLR_SIZE * TEGRA_ICTLR_COUNT);
+
+ /*
+ * We only want to write to bits that correspond to interrupts that the
+ * current domain controls. Accordingly, we'll create a mask that has a
+ * single bit set for each writable bit.
+ */
+ for (i = 0; i < TEGRA_IRQS_PER_ICTLR; ++i) {
+ int irq = irq_base + i;
+ int bit_mask = BIT(irq % 32);
+
+ if(irq_owned_by_domain(irq, current->domain))
+ write_mask |= bit_mask;
+ }
+
+ /*
+ * Read in the original value. We'll use this to ensure that we maintain
+ * the bit values for any bits not actively controlled by this domain. Note
+ * that we can perform this read without side effects, so this shouldn't
+ * change the actual operation being performed.
+ */
+ raw_value = readl(tegra_ictlr_base + register_offset);
+
+ /* Merge in the bit values the guest is allowed to write. */
+ raw_value &= ~write_mask;
+ raw_value |= (write_mask & new_value);
+
+ /* Finally perform the write. */
+ writel(raw_value, tegra_ictlr_base + register_offset);
+ return 1;
+}
+
+
+/**
+ * MMIO operations for Tegra chips. These allow the hwdom 'direct' access to
+ * the sections of the legacy interrupt controller that correspond to its
+ * devices, but disallow access to the sections controlled by other domains
+ * or by Xen.
+ */
+static struct mmio_handler_ops tegra_mmio_ops_ictlr = {
+ .read = tegra_ictlr_domain_read,
+ .write = tegra_ictlr_domain_write,
+};
+
+
+/**
+ * Set up the hardware domain for the Tegra, giving it mediated access to the
+ * platform's legacy interrupt controller.
+ */
+static int tegra_specific_mapping(struct domain *d)
+{
+ int rc;
+ unsigned long pfn_start, pfn_end;
+
+ pfn_start = paddr_to_pfn(TEGRA_ICTLR_BASE);
+ pfn_end = DIV_ROUND_UP(TEGRA_ICTLR_BASE + (TEGRA_ICTLR_SIZE * TEGRA_ICTLR_COUNT), PAGE_SIZE);
+
+ /* Force all access to the ictlr to go through our mediators. */
+ rc = iomem_deny_access(d, pfn_start, pfn_end);
+ if (rc)
+ panic("Could not deny access to the Tegra LIC iomem!\n");
+ rc = unmap_mmio_regions(d, _gfn(pfn_start), pfn_end - pfn_start + 1,
+ _mfn(pfn_start));
+ if (rc)
+ panic("Could not deny access to the Tegra LIC!\n");
+
+ register_mmio_handler(d, &tegra_mmio_ops_ictlr,
+ TEGRA_ICTLR_BASE, TEGRA_ICTLR_SIZE * TEGRA_ICTLR_COUNT, NULL);
+ return 0;
+}
+
+
+/**
* Initialize the Tegra legacy interrupt controller, placing each interrupt
* into a default state. These defaults ensure that stray interrupts don't
* affect Xen.
@@ -296,17 +510,7 @@ static int tegra_initialize_legacy_interrupt_controller(void)
*/
static int tegra_init(void)
{
- int rc;
-
- rc = tegra_initialize_legacy_interrupt_controller();
-
- /*
- * TODO: Virtualize the MMIO regions for the ictlr, ensuring the hwdom
- * (and possibly passhtrough domains) can only access ictlr registers for
- * interrupts they actually own.
- */
-
- return rc;
+ return tegra_initialize_legacy_interrupt_controller();
}
@@ -336,4 +540,5 @@ PLATFORM_START(tegra, "Tegra")
.irq_for_device = tegra_irq_for_device,
.route_irq_to_xen = tegra_route_irq_to_xen,
.route_irq_to_guest = tegra_route_irq_to_guest,
+ .specific_mapping = tegra_specific_mapping,
PLATFORM_END
--
2.9.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH RFC 6/6] xen/arm: platforms/tegra: ensure the hwdom can only affect its own interrupts
2016-09-05 10:14 ` [PATCH RFC 6/6] xen/arm: platforms/tegra: ensure the hwdom can only affect its own interrupts Kyle Temkin
@ 2016-10-22 0:45 ` Stefano Stabellini
0 siblings, 0 replies; 17+ messages in thread
From: Stefano Stabellini @ 2016-10-22 0:45 UTC (permalink / raw)
To: Kyle Temkin; +Cc: julien.grall, sstabellini, Kyle J. Temkin, xen-devel
On Mon, 5 Sep 2016, Kyle Temkin wrote:
> From: "Kyle J. Temkin" <temkink@ainfosec.com>
>
> Several Tegra hardware devices-- and the Tegra device tree-- expect
> the presence of a Tegra Legacy Interrupt Controller (LIC) in the hardware
> domain. Accordingly, we'll need to expose (most of) the LIC's registers
> to the hardware domain.
>
> As the Tegra LIC provides the ability to modify interrupt delivery (e.g.
> by masking interrupts, forcing asserting/clearing them, or adjusting
> their prority), it's important that the hardware domain's access be
> mediated. This commit adds read/write handlers that prohibit
> modification of register sections corresponding to interrupts not owned
> by the hardware domain.
>
> Note that this is written to be domain agnostic; this allows the
> potential to e.g. map the ictlr into multiple domains if this is desired
> for passthrough in the future.
>
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
Overall is a good patch. A few style issues and a couple of simple
improvements but the core is fine.
> xen/arch/arm/platforms/tegra.c | 227 +++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 216 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/platforms/tegra.c b/xen/arch/arm/platforms/tegra.c
> index e75242c..a119c01 100644
> --- a/xen/arch/arm/platforms/tegra.c
> +++ b/xen/arch/arm/platforms/tegra.c
> @@ -21,11 +21,13 @@
> #include <xen/stdbool.h>
> #include <xen/sched.h>
> #include <xen/vmap.h>
> +#include <xen/iocap.h>
>
> #include <asm/io.h>
> #include <asm/gic.h>
> #include <asm/platform.h>
> #include <asm/platforms/tegra.h>
> +#include <asm/mmio.h>
>
>
> /* Permanent mapping to the Tegra legacy interrupt controller. */
> @@ -258,6 +260,218 @@ static void tegra_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
> }
>
> /**
> + * Parses a LIC MMIO read or write, and extracts the information needed to
> + * complete the request.
> + *
> + * @param info Information describing the MMIO read/write being performed.
> + * @param register_number The register number in the ictlr; e.g.
> + * TEGRA_ICTLR_CPU_IER_SET.
> + * @param register_offset The offset into tegra_icltr_base at which the target
> + * register exists.
> + * @param The number of the first IRQ represented by the given ictlr register.
> + */
> +static void tegra_ictlr_parse_mmio_request(mmio_info_t *info,
> + int *register_number, int *register_offset, int *irq_base)
> +{
It doesn't look like you are using register_number anywhere. I would
just remove it from the parameter list.
> + /* Determine the offset of the access into the ICTLR region. */
> + uint32_t offset = info->gpa - TEGRA_ICTLR_BASE;
> +
> + if(register_number)
> + *register_number = offset % TEGRA_ICTLR_SIZE;
style
> + if(register_offset)
> + *register_offset = offset;
style
> + if(irq_base) {
style
> + int ictlr_number = offset / TEGRA_ICTLR_SIZE;
> + *irq_base = (ictlr_number * TEGRA_IRQS_PER_ICTLR) + NR_LOCAL_IRQS;
> + }
> +}
> +
> +/**
style
> + * Returns true iff the given IRQ is currently routed to the given domain.
> + *
> + * @param irq The IRQ number for the IRQ in question.
> + * @param d The domain in question.
> + * @return True iff the given domain is the current IRQ target.
> + */
> +static bool irq_owned_by_domain(int irq, struct domain *d)
> +{
> + struct irq_desc *desc = irq_to_desc(irq);
> + domid_t domid;
> + unsigned long flags;
> +
> + BUG_ON(!desc);
> +
> + spin_lock_irqsave(&desc->lock, flags);
> + domid = irq_get_domain_id(desc);
> + spin_unlock_irqrestore(&desc->lock, flags);
> +
> + return (d->domain_id == domid);
> +}
> +
> +/**
> + * Mediates an MMIO-read to the Tegra legacy interrupt controller.
> + * Ensures that each domain only is passed interrupt state for its
> + * own interupts.
> + */
> +static int tegra_ictlr_domain_read(struct vcpu *v, mmio_info_t *info,
> + register_t *target_register, void *priv)
> +{
> + register_t raw_value;
> +
> + int register_number;
> + int register_offset;
> + int irq_base;
> + int i;
> +
> + tegra_ictlr_parse_mmio_request(info, ®ister_number, ®ister_offset,
> + &irq_base);
> +
> + /* Sanity check the read. */
> + if ( register_offset & 0x3 ) {
> + printk(XENLOG_G_ERR "d%u: Tegra LIC: Attempt to read unaligned ictlr addr"
> + "(%" PRIpaddr ")!", current->domain->domain_id, info->gpa);
> + domain_crash_synchronous();
> + }
> + if ( info->dabt.size != DABT_WORD ) {
> + printk(XENLOG_G_ERR "d%u: Tegra LIC: Non-word read from ictlr addr"
> + "%" PRIpaddr "!", current->domain->domain_id, info->gpa);
> + domain_crash_synchronous();
> + }
> +
> + /* Ensure that we've only been handed an offset within our region. */
> + BUG_ON(register_offset > TEGRA_ICTLR_SIZE * TEGRA_ICTLR_COUNT);
> +
> + /* Perform the core ictlr read. */
> + raw_value = readl(tegra_ictlr_base + register_offset);
> +
> + /*
> + * We don't want to leak information about interrupts not controlled
> + * by the active domain. Thus, we'll zero out any ictlr slots for
> + * IRQs not owned by the given domain.
> + */
> + for (i = 0; i < TEGRA_IRQS_PER_ICTLR; ++i) {
> + int irq = irq_base + i;
> + int mask = BIT(irq % 32);
Isn't (irq % 32) just "i"? So mask is just:
mask = (1 << i);
?
> + if(!irq_owned_by_domain(irq, current->domain))
style
> + raw_value &= ~mask;
> + }
> +
> + /* Finally, set the target register to our read value */
> + *target_register = raw_value;
> + return 1;
> +}
> +
> +
> +/**
> + * Mediates an MMIO-read to the Tegra legacy interrupt controller.
> + * Ensures that each domain only can only control is own interrupts.
> + */
> +static int tegra_ictlr_domain_write(struct vcpu *v, mmio_info_t *info,
> + register_t new_value, void *priv)
> +{
> + register_t write_mask = 0;
> + register_t raw_value;
> +
> + int register_number;
> + int register_offset;
> + int irq_base;
> + int i;
> +
> + tegra_ictlr_parse_mmio_request(info, ®ister_number, ®ister_offset,
> + &irq_base);
> +
> + /* Sanity check the read. */
> + if ( register_offset & 0x3 ) {
> + printk(XENLOG_G_ERR "d%u: Tegra LIC: Attempt to write unaligned ictlr addr"
> + "(%" PRIpaddr ")!", current->domain->domain_id, info->gpa);
> + domain_crash_synchronous();
> + return 0;
> + }
> + if ( info->dabt.size != DABT_WORD ) {
> + printk(XENLOG_G_ERR "d%u: Tegra LIC: Non-word write to ictlr addr"
> + "%" PRIpaddr "!", current->domain->domain_id, info->gpa);
> + domain_crash_synchronous();
> + return 0;
> + }
> +
> + /* Ensure that we've only been handed an offset within our region. */
> + BUG_ON(register_offset > TEGRA_ICTLR_SIZE * TEGRA_ICTLR_COUNT);
> +
> + /*
> + * We only want to write to bits that correspond to interrupts that the
> + * current domain controls. Accordingly, we'll create a mask that has a
> + * single bit set for each writable bit.
> + */
> + for (i = 0; i < TEGRA_IRQS_PER_ICTLR; ++i) {
> + int irq = irq_base + i;
> + int bit_mask = BIT(irq % 32);
same comment as before:
mask = (1 << i);
> + if(irq_owned_by_domain(irq, current->domain))
style
> + write_mask |= bit_mask;
> + }
> +
> + /*
> + * Read in the original value. We'll use this to ensure that we maintain
> + * the bit values for any bits not actively controlled by this domain. Note
> + * that we can perform this read without side effects, so this shouldn't
> + * change the actual operation being performed.
> + */
> + raw_value = readl(tegra_ictlr_base + register_offset);
> +
> + /* Merge in the bit values the guest is allowed to write. */
Please rephrase to:
Remove bits that the guest is allowed to write.
> + raw_value &= ~write_mask;
> + raw_value |= (write_mask & new_value);
> +
> + /* Finally perform the write. */
> + writel(raw_value, tegra_ictlr_base + register_offset);
> + return 1;
> +}
> +
> +
> +/**
> + * MMIO operations for Tegra chips. These allow the hwdom 'direct' access to
> + * the sections of the legacy interrupt controller that correspond to its
> + * devices, but disallow access to the sections controlled by other domains
> + * or by Xen.
> + */
> +static struct mmio_handler_ops tegra_mmio_ops_ictlr = {
> + .read = tegra_ictlr_domain_read,
> + .write = tegra_ictlr_domain_write,
> +};
> +
> +
> +/**
> + * Set up the hardware domain for the Tegra, giving it mediated access to the
> + * platform's legacy interrupt controller.
> + */
> +static int tegra_specific_mapping(struct domain *d)
> +{
> + int rc;
> + unsigned long pfn_start, pfn_end;
> +
> + pfn_start = paddr_to_pfn(TEGRA_ICTLR_BASE);
> + pfn_end = DIV_ROUND_UP(TEGRA_ICTLR_BASE + (TEGRA_ICTLR_SIZE * TEGRA_ICTLR_COUNT), PAGE_SIZE);
> +
> + /* Force all access to the ictlr to go through our mediators. */
> + rc = iomem_deny_access(d, pfn_start, pfn_end);
> + if (rc)
> + panic("Could not deny access to the Tegra LIC iomem!\n");
> + rc = unmap_mmio_regions(d, _gfn(pfn_start), pfn_end - pfn_start + 1,
> + _mfn(pfn_start));
> + if (rc)
> + panic("Could not deny access to the Tegra LIC!\n");
> +
> + register_mmio_handler(d, &tegra_mmio_ops_ictlr,
> + TEGRA_ICTLR_BASE, TEGRA_ICTLR_SIZE * TEGRA_ICTLR_COUNT, NULL);
It is best to avoid mapping the TEGRA_ICTLR_BASE MMIO region to begin
with, rather than mapping it, then unmapping it. Also .specific_mapping
is actually for... mapping, not for unmapping and emulating :-)
I would consider moving the emulator code out of this file into a proper
separate emulator file, such as xen/arch/arm/vuart.c. Or at least
initialize it from tegra_init.
> + return 0;
> +}
> +
> +
> +/**
> * Initialize the Tegra legacy interrupt controller, placing each interrupt
> * into a default state. These defaults ensure that stray interrupts don't
> * affect Xen.
> @@ -296,17 +510,7 @@ static int tegra_initialize_legacy_interrupt_controller(void)
> */
> static int tegra_init(void)
> {
> - int rc;
> -
> - rc = tegra_initialize_legacy_interrupt_controller();
> -
> - /*
> - * TODO: Virtualize the MMIO regions for the ictlr, ensuring the hwdom
> - * (and possibly passhtrough domains) can only access ictlr registers for
> - * interrupts they actually own.
> - */
> -
> - return rc;
> + return tegra_initialize_legacy_interrupt_controller();
> }
Please remove this change from this patch and fold it into patch #4.
> @@ -336,4 +540,5 @@ PLATFORM_START(tegra, "Tegra")
> .irq_for_device = tegra_irq_for_device,
> .route_irq_to_xen = tegra_route_irq_to_xen,
> .route_irq_to_guest = tegra_route_irq_to_guest,
> + .specific_mapping = tegra_specific_mapping,
> PLATFORM_END
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread