* [PATCH 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards.
2017-04-06 19:47 [PATCH 0/6] Initial Tegra platform support Chris Patterson
@ 2017-04-06 19:47 ` Chris Patterson
2017-04-13 23:09 ` Stefano Stabellini
2017-04-18 7:49 ` Julien Grall
2017-04-06 19:47 ` [PATCH 2/6] xen/arm: domain_build: Inherit GIC's interrupt-parent from host device tree Chris Patterson
` (4 subsequent siblings)
5 siblings, 2 replies; 30+ messages in thread
From: Chris Patterson @ 2017-04-06 19:47 UTC (permalink / raw)
To: xen-devel; +Cc: Chris Patterson, julien.grall, sstabellini, temkink
From: Chris Patterson <pattersonc@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.
This patch adds a quirk for platforms, such as the Tegra, which require
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].
This quirk is selectively enabled for the platform using a new "hw_quirks"
member with a corresponding set of bitmasks. The existing quirk,
dw_usr_bsy was updated to match this approach as well.
[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4539c24fe4f92c09ee668ef959d3e8180df619b9
Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
---
changes from rfc:
- use bitmask for quirks in ns1660, including dw_usr_bsy
---
xen/arch/arm/Rules.mk | 1 +
xen/drivers/char/ns16550.c | 28 ++++++++++++++++++++++++----
xen/include/xen/8250-uart.h | 1 +
3 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
index 569a0ba..43b32d0 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 e4de3b4..1b75e89 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -62,7 +62,7 @@ static struct ns16550 {
struct timer resume_timer;
unsigned int timeout_ms;
bool_t intr_works;
- bool_t dw_usr_bsy;
+ uint8_t hw_quirks;
#ifdef CONFIG_HAS_PCI
/* PCI card parameters. */
bool_t pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
@@ -414,6 +414,10 @@ static const struct ns16550_config __initconst uart_config[] =
};
#endif
+/* Various hardware quirks/features that may be need be enabled per device */
+#define HW_QUIRKS_DW_USR_BSY (1<<0)
+#define HW_QUIRKS_USE_RTOIE (1<<1)
+
static void ns16550_delayed_resume(void *data);
static u8 ns_read_reg(struct ns16550 *uart, unsigned int reg)
@@ -578,7 +582,7 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
/* No interrupts. */
ns_write_reg(uart, UART_IER, 0);
- if ( uart->dw_usr_bsy &&
+ if ( (uart->hw_quirks & HW_QUIRKS_DW_USR_BSY) &&
(ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
{
/* DesignWare 8250 detects if LCR is written while the UART is
@@ -651,12 +655,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->hw_quirks & HW_QUIRKS_USE_RTOIE )
+ ier_value |= UART_IER_RTOIE;
+
+ ns_write_reg(uart, UART_IER, ier_value);
}
if ( uart->irq >= 0 )
@@ -1271,7 +1286,11 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
return -EINVAL;
uart->irq = res;
- uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
+ if ( dt_device_is_compatible(dev, "snps,dw-apb-uart") )
+ uart->hw_quirks |= HW_QUIRKS_DW_USR_BSY;
+
+ if ( dt_device_is_compatible(dev, "nvidia,tegra20-uart") )
+ uart->hw_quirks |= HW_QUIRKS_USE_RTOIE;
uart->vuart.base_addr = uart->io_base;
uart->vuart.size = uart->io_size;
@@ -1292,6 +1311,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.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards.
2017-04-06 19:47 ` [PATCH 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards Chris Patterson
@ 2017-04-13 23:09 ` Stefano Stabellini
2017-04-18 7:49 ` Julien Grall
1 sibling, 0 replies; 30+ messages in thread
From: Stefano Stabellini @ 2017-04-13 23:09 UTC (permalink / raw)
To: Chris Patterson
Cc: Chris Patterson, julien.grall, sstabellini, temkink, xen-devel
On Thu, 6 Apr 2017, Chris Patterson wrote:
> From: Chris Patterson <pattersonc@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.
>
> This patch adds a quirk for platforms, such as the Tegra, which require
> 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].
>
> This quirk is selectively enabled for the platform using a new "hw_quirks"
> member with a corresponding set of bitmasks. The existing quirk,
> dw_usr_bsy was updated to match this approach as well.
>
> [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4539c24fe4f92c09ee668ef959d3e8180df619b9
>
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>
> changes from rfc:
> - use bitmask for quirks in ns1660, including dw_usr_bsy
>
> ---
> xen/arch/arm/Rules.mk | 1 +
> xen/drivers/char/ns16550.c | 28 ++++++++++++++++++++++++----
> xen/include/xen/8250-uart.h | 1 +
> 3 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index 569a0ba..43b32d0 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 e4de3b4..1b75e89 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -62,7 +62,7 @@ static struct ns16550 {
> struct timer resume_timer;
> unsigned int timeout_ms;
> bool_t intr_works;
> - bool_t dw_usr_bsy;
> + uint8_t hw_quirks;
> #ifdef CONFIG_HAS_PCI
> /* PCI card parameters. */
> bool_t pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
> @@ -414,6 +414,10 @@ static const struct ns16550_config __initconst uart_config[] =
> };
> #endif
>
> +/* Various hardware quirks/features that may be need be enabled per device */
> +#define HW_QUIRKS_DW_USR_BSY (1<<0)
> +#define HW_QUIRKS_USE_RTOIE (1<<1)
> +
> static void ns16550_delayed_resume(void *data);
>
> static u8 ns_read_reg(struct ns16550 *uart, unsigned int reg)
> @@ -578,7 +582,7 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
> /* No interrupts. */
> ns_write_reg(uart, UART_IER, 0);
>
> - if ( uart->dw_usr_bsy &&
> + if ( (uart->hw_quirks & HW_QUIRKS_DW_USR_BSY) &&
> (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
> {
> /* DesignWare 8250 detects if LCR is written while the UART is
> @@ -651,12 +655,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->hw_quirks & HW_QUIRKS_USE_RTOIE )
> + ier_value |= UART_IER_RTOIE;
> +
> + ns_write_reg(uart, UART_IER, ier_value);
> }
>
> if ( uart->irq >= 0 )
> @@ -1271,7 +1286,11 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
> return -EINVAL;
> uart->irq = res;
>
> - uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
> + if ( dt_device_is_compatible(dev, "snps,dw-apb-uart") )
> + uart->hw_quirks |= HW_QUIRKS_DW_USR_BSY;
> +
> + if ( dt_device_is_compatible(dev, "nvidia,tegra20-uart") )
> + uart->hw_quirks |= HW_QUIRKS_USE_RTOIE;
>
> uart->vuart.base_addr = uart->io_base;
> uart->vuart.size = uart->io_size;
> @@ -1292,6 +1311,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.1.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards.
2017-04-06 19:47 ` [PATCH 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards Chris Patterson
2017-04-13 23:09 ` Stefano Stabellini
@ 2017-04-18 7:49 ` Julien Grall
2017-04-19 20:37 ` Chris Patterson
1 sibling, 1 reply; 30+ messages in thread
From: Julien Grall @ 2017-04-18 7:49 UTC (permalink / raw)
To: Chris Patterson, xen-devel
Cc: Wei Liu, Chris Patterson, temkink, George Dunlap, Andrew Cooper,
Tim Deegan, sstabellini, Jan Beulich, Ian Jackson, nd
Hello,
Please include all the relevant maintainers for the code you modified.
On 06/04/2017 20:47, Chris Patterson wrote:
> From: Chris Patterson <pattersonc@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.
>
> This patch adds a quirk for platforms, such as the Tegra, which require
> require the NS16550 Rx timeout interrupt to be enabled for receive to
> function properly. The same quirk is applied in the eqvuialent Linux
s/eqvuialent/equivalent/
> driver [1].
>
> This quirk is selectively enabled for the platform using a new "hw_quirks"
> member with a corresponding set of bitmasks. The existing quirk,
> dw_usr_bsy was updated to match this approach as well.
This patch would have benefit to be split in smaller ones as you do
introduce distinct changes in the code.
>
> [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4539c24fe4f92c09ee668ef959d3e8180df619b9
>
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
> ---
>
> changes from rfc:
> - use bitmask for quirks in ns1660, including dw_usr_bsy
>
> ---
> xen/arch/arm/Rules.mk | 1 +
> xen/drivers/char/ns16550.c | 28 ++++++++++++++++++++++++----
> xen/include/xen/8250-uart.h | 1 +
> 3 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index 569a0ba..43b32d0 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
The EARLY_PRINTK_* are sorted alphabetically. So please introduce this
one in the correct place.
Also, I was expecting to see this new earlyprintk documented in
docs/misc/arm/early-printk.txt.
>
> 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 e4de3b4..1b75e89 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -62,7 +62,7 @@ static struct ns16550 {
> struct timer resume_timer;
> unsigned int timeout_ms;
> bool_t intr_works;
> - bool_t dw_usr_bsy;
> + uint8_t hw_quirks;
> #ifdef CONFIG_HAS_PCI
> /* PCI card parameters. */
> bool_t pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
> @@ -414,6 +414,10 @@ static const struct ns16550_config __initconst uart_config[] =
> };
> #endif
>
> +/* Various hardware quirks/features that may be need be enabled per device */
> +#define HW_QUIRKS_DW_USR_BSY (1<<0)
> +#define HW_QUIRKS_USE_RTOIE (1<<1)
> +
> static void ns16550_delayed_resume(void *data);
>
> static u8 ns_read_reg(struct ns16550 *uart, unsigned int reg)
> @@ -578,7 +582,7 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
> /* No interrupts. */
> ns_write_reg(uart, UART_IER, 0);
>
> - if ( uart->dw_usr_bsy &&
> + if ( (uart->hw_quirks & HW_QUIRKS_DW_USR_BSY) &&
> (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
> {
> /* DesignWare 8250 detects if LCR is written while the UART is
> @@ -651,12 +655,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->hw_quirks & HW_QUIRKS_USE_RTOIE )
> + ier_value |= UART_IER_RTOIE;
> +
> + ns_write_reg(uart, UART_IER, ier_value);
> }
>
> if ( uart->irq >= 0 )
> @@ -1271,7 +1286,11 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
> return -EINVAL;
> uart->irq = res;
>
> - uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
> + if ( dt_device_is_compatible(dev, "snps,dw-apb-uart") )
> + uart->hw_quirks |= HW_QUIRKS_DW_USR_BSY;
> +
> + if ( dt_device_is_compatible(dev, "nvidia,tegra20-uart") )
> + uart->hw_quirks |= HW_QUIRKS_USE_RTOIE;
>
> uart->vuart.base_addr = uart->io_base;
> uart->vuart.size = uart->io_size;
> @@ -1292,6 +1311,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 */
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards.
2017-04-18 7:49 ` Julien Grall
@ 2017-04-19 20:37 ` Chris Patterson
0 siblings, 0 replies; 30+ messages in thread
From: Chris Patterson @ 2017-04-19 20:37 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, Chris Patterson, temkink, George Dunlap, Andrew Cooper,
Tim Deegan, xen-devel, Stefano Stabellini, Jan Beulich,
Ian Jackson, nd
Will split patches & fix for v2, thanks!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 2/6] xen/arm: domain_build: Inherit GIC's interrupt-parent from host device tree
2017-04-06 19:47 [PATCH 0/6] Initial Tegra platform support Chris Patterson
2017-04-06 19:47 ` [PATCH 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards Chris Patterson
@ 2017-04-06 19:47 ` Chris Patterson
2017-04-18 8:01 ` Julien Grall
2017-04-06 19:47 ` [PATCH 3/6] xen/arm: Allow platforms to hook IRQ routing Chris Patterson
` (3 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: Chris Patterson @ 2017-04-06 19:47 UTC (permalink / raw)
To: xen-devel; +Cc: Chris Patterson, julien.grall, sstabellini, temkink
From: "Chris Patterson" <pattersonc@ainfosec.com>
Currently, the interrupt parent is left undefined during creation in
make_gic_node(). In cases where a non-GIC interrupt controller is present,
this can lead to incorrect assignment of interrupt parents.
On the Tegra, the gic's interrupt parent is set to itself:
gic: interrupt-controller@0,50041000 {
compatible = "arm,gic-400";
#interrupt-cells = <3>;
interrupt-controller;
reg = <0x0 0x50041000 0x0 0x1000>,
<0x0 0x50042000 0x0 0x2000>,
<0x0 0x50044000 0x0 0x2000>,
<0x0 0x50046000 0x0 0x2000>;
interrupts = <GIC_PPI 9
(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
interrupt-parent = <&gic>;
};
To prevent the hardware domain from assuming the Legacy Interrupt Controller
(lic) as the GIC's interrupt-parent, this change explicitly assigns
the interrupt-parent property from the host device tree.
Authored-by: Kyle Temkin <temkink@ainfosec.com>
Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
changes from rfc:
- commit message documentation improvements
---
xen/arch/arm/domain_build.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index de59e5f..cb66304 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -778,8 +778,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
@@ -809,6 +809,19 @@ static int make_gic_node(const struct domain *d, void *fdt,
return res;
}
+ /*
+ * If available, explicitly inherit interrupt-parent property from host
+ * device tree. This will prevent the risk of incorrect identification
+ * of the parent on platforms with more than one interrupt controller.
+ */
+ 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.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 2/6] xen/arm: domain_build: Inherit GIC's interrupt-parent from host device tree
2017-04-06 19:47 ` [PATCH 2/6] xen/arm: domain_build: Inherit GIC's interrupt-parent from host device tree Chris Patterson
@ 2017-04-18 8:01 ` Julien Grall
2017-04-19 20:09 ` Christopher Patterson
0 siblings, 1 reply; 30+ messages in thread
From: Julien Grall @ 2017-04-18 8:01 UTC (permalink / raw)
To: Chris Patterson, xen-devel; +Cc: Chris Patterson, nd, temkink, sstabellini
Hello,
On 06/04/2017 20:47, Chris Patterson wrote:
> From: "Chris Patterson" <pattersonc@ainfosec.com>
>
> Currently, the interrupt parent is left undefined during creation in
> make_gic_node(). In cases where a non-GIC interrupt controller is present,
> this can lead to incorrect assignment of interrupt parents.
>
> On the Tegra, the gic's interrupt parent is set to itself:
>
> gic: interrupt-controller@0,50041000 {
> compatible = "arm,gic-400";
> #interrupt-cells = <3>;
> interrupt-controller;
> reg = <0x0 0x50041000 0x0 0x1000>,
> <0x0 0x50042000 0x0 0x2000>,
> <0x0 0x50044000 0x0 0x2000>,
> <0x0 0x50046000 0x0 0x2000>;
> interrupts = <GIC_PPI 9
> (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> interrupt-parent = <&gic>;
> };
>
> To prevent the hardware domain from assuming the Legacy Interrupt Controller
> (lic) as the GIC's interrupt-parent, this change explicitly assigns
> the interrupt-parent property from the host device tree.
>
> Authored-by: Kyle Temkin <temkink@ainfosec.com>
We use "From: " for the author and it is different here. So who wrote
this code?
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>
> changes from rfc:
> - commit message documentation improvements
>
> ---
> xen/arch/arm/domain_build.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index de59e5f..cb66304 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -778,8 +778,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
> @@ -809,6 +809,19 @@ static int make_gic_node(const struct domain *d, void *fdt,
> return res;
> }
>
> + /*
> + * If available, explicitly inherit interrupt-parent property from host
> + * device tree. This will prevent the risk of incorrect identification
> + * of the parent on platforms with more than one interrupt controller.
> + */
> + 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 )
> {
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 2/6] xen/arm: domain_build: Inherit GIC's interrupt-parent from host device tree
2017-04-18 8:01 ` Julien Grall
@ 2017-04-19 20:09 ` Christopher Patterson
0 siblings, 0 replies; 30+ messages in thread
From: Christopher Patterson @ 2017-04-19 20:09 UTC (permalink / raw)
To: Julien Grall, Chris Patterson, xen-devel; +Cc: nd, Kyle Temkin, sstabellini
> Authored-by: Kyle Temkin <temkink@ainfosec.com>
We use "From: " for the author and it is different here. So who wrote
this code?
It was a team effort, but my intention was to indicate him as the primary author. I'll set him as From in v2.
Thanks!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 3/6] xen/arm: Allow platforms to hook IRQ routing
2017-04-06 19:47 [PATCH 0/6] Initial Tegra platform support Chris Patterson
2017-04-06 19:47 ` [PATCH 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards Chris Patterson
2017-04-06 19:47 ` [PATCH 2/6] xen/arm: domain_build: Inherit GIC's interrupt-parent from host device tree Chris Patterson
@ 2017-04-06 19:47 ` Chris Patterson
2017-04-13 23:26 ` Stefano Stabellini
2017-04-06 19:47 ` [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic " Chris Patterson
` (2 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: Chris Patterson @ 2017-04-06 19:47 UTC (permalink / raw)
To: xen-devel; +Cc: Chris Patterson, julien.grall, sstabellini, temkink
From: "Chris Patterson" <pattersonc@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. This enables
platform specific logic to routing the IRQ to Xen and Guest.
As dt_irq_translate() is presently hard-coded to just support the
primary interrupt controller, instead rely on the newly added
platform_irq_is_routable() check instead. The default behaviour of this
new function should be consistent with the previous checks for platforms
that do not implement it.
Authored-by: Kyle Temkin <temkink@ainfosec.com>
Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
---
changes since rfc:
- use bool instead of bool_t
- formatting & code style cleanup
- reuse dt_irq_translate() path and drop platform_irq_for_device() approach
- use const qualifier for platform_irq_is_routable rirq argument
---
xen/arch/arm/domain_build.c | 12 ++++++++----
xen/arch/arm/irq.c | 5 +++--
xen/arch/arm/platform.c | 30 ++++++++++++++++++++++++++++++
xen/common/device_tree.c | 8 +++-----
xen/include/asm-arm/platform.h | 12 ++++++++++++
5 files changed, 56 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index cb66304..92536dd 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1120,12 +1120,16 @@ 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;
}
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index f3f20a6..0b4eaa9 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -26,6 +26,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);
@@ -369,7 +370,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
@@ -506,7 +507,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 0af6d57..539ee3b 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -147,6 +147,36 @@ 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 platform_irq_is_routable(const 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);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 7b009ea..061693b 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -22,6 +22,7 @@
#include <xen/string.h>
#include <xen/cpumask.h>
#include <xen/ctype.h>
+#include <asm/platform.h>
#include <asm/setup.h>
#include <xen/err.h>
@@ -1467,11 +1468,8 @@ int dt_irq_translate(const struct dt_raw_irq *raw,
ASSERT(dt_irq_xlate != NULL);
ASSERT(dt_interrupt_controller != NULL);
- /*
- * TODO: Retrieve the right irq_xlate. This is only works for the primary
- * interrupt controller.
- */
- if ( raw->controller != dt_interrupt_controller )
+ /* Only proceed with translation if the irq is routable on the platform. */
+ if ( !platform_irq_is_routable(raw) )
return -EINVAL;
return dt_irq_xlate(raw->specifier, raw->size,
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index 08010ba..119ad2e 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -26,6 +26,12 @@ 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 (*irq_is_routable)(const struct dt_raw_irq * rirq);
+
/*
* Platform quirks
* Defined has a function because a platform can support multiple
@@ -56,6 +62,12 @@ int platform_cpu_up(int cpu);
void platform_reset(void);
void platform_poweroff(void);
bool_t platform_has_quirk(uint32_t quirk);
+
+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 platform_irq_is_routable(const struct dt_raw_irq *rirq);
+
bool_t platform_device_is_blacklisted(const struct dt_device_node *node);
#define PLATFORM_START(_name, _namestr) \
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 3/6] xen/arm: Allow platforms to hook IRQ routing
2017-04-06 19:47 ` [PATCH 3/6] xen/arm: Allow platforms to hook IRQ routing Chris Patterson
@ 2017-04-13 23:26 ` Stefano Stabellini
0 siblings, 0 replies; 30+ messages in thread
From: Stefano Stabellini @ 2017-04-13 23:26 UTC (permalink / raw)
To: Chris Patterson
Cc: Chris Patterson, julien.grall, sstabellini, temkink, xen-devel
On Thu, 6 Apr 2017, Chris Patterson wrote:
> From: "Chris Patterson" <pattersonc@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. This enables
> platform specific logic to routing the IRQ to Xen and Guest.
>
> As dt_irq_translate() is presently hard-coded to just support the
> primary interrupt controller, instead rely on the newly added
> platform_irq_is_routable() check instead. The default behaviour of this
> new function should be consistent with the previous checks for platforms
> that do not implement it.
>
> Authored-by: Kyle Temkin <temkink@ainfosec.com>
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>
> changes since rfc:
> - use bool instead of bool_t
> - formatting & code style cleanup
> - reuse dt_irq_translate() path and drop platform_irq_for_device() approach
> - use const qualifier for platform_irq_is_routable rirq argument
>
> ---
>
> xen/arch/arm/domain_build.c | 12 ++++++++----
> xen/arch/arm/irq.c | 5 +++--
> xen/arch/arm/platform.c | 30 ++++++++++++++++++++++++++++++
> xen/common/device_tree.c | 8 +++-----
> xen/include/asm-arm/platform.h | 12 ++++++++++++
> 5 files changed, 56 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index cb66304..92536dd 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1120,12 +1120,16 @@ 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;
> }
>
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index f3f20a6..0b4eaa9 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -26,6 +26,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);
> @@ -369,7 +370,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
> @@ -506,7 +507,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 0af6d57..539ee3b 100644
> --- a/xen/arch/arm/platform.c
> +++ b/xen/arch/arm/platform.c
> @@ -147,6 +147,36 @@ 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 platform_irq_is_routable(const 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);
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 7b009ea..061693b 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -22,6 +22,7 @@
> #include <xen/string.h>
> #include <xen/cpumask.h>
> #include <xen/ctype.h>
> +#include <asm/platform.h>
> #include <asm/setup.h>
> #include <xen/err.h>
>
> @@ -1467,11 +1468,8 @@ int dt_irq_translate(const struct dt_raw_irq *raw,
> ASSERT(dt_irq_xlate != NULL);
> ASSERT(dt_interrupt_controller != NULL);
>
> - /*
> - * TODO: Retrieve the right irq_xlate. This is only works for the primary
> - * interrupt controller.
> - */
> - if ( raw->controller != dt_interrupt_controller )
> + /* Only proceed with translation if the irq is routable on the platform. */
> + if ( !platform_irq_is_routable(raw) )
> return -EINVAL;
>
> return dt_irq_xlate(raw->specifier, raw->size,
> diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
> index 08010ba..119ad2e 100644
> --- a/xen/include/asm-arm/platform.h
> +++ b/xen/include/asm-arm/platform.h
> @@ -26,6 +26,12 @@ 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 (*irq_is_routable)(const struct dt_raw_irq * rirq);
> +
> /*
> * Platform quirks
> * Defined has a function because a platform can support multiple
> @@ -56,6 +62,12 @@ int platform_cpu_up(int cpu);
> void platform_reset(void);
> void platform_poweroff(void);
> bool_t platform_has_quirk(uint32_t quirk);
> +
> +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 platform_irq_is_routable(const struct dt_raw_irq *rirq);
> +
> bool_t platform_device_is_blacklisted(const struct dt_device_node *node);
>
> #define PLATFORM_START(_name, _namestr) \
> --
> 2.1.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
2017-04-06 19:47 [PATCH 0/6] Initial Tegra platform support Chris Patterson
` (2 preceding siblings ...)
2017-04-06 19:47 ` [PATCH 3/6] xen/arm: Allow platforms to hook IRQ routing Chris Patterson
@ 2017-04-06 19:47 ` Chris Patterson
2017-04-13 23:46 ` Stefano Stabellini
2017-04-18 8:26 ` Julien Grall
2017-04-06 19:47 ` [PATCH 5/6] xen/arm: Add function to query IRQ 'ownership' Chris Patterson
2017-04-06 19:47 ` [PATCH 6/6] xen/arm: platforms/tegra: Ensure the hwdom can only affect its own interrupts Chris Patterson
5 siblings, 2 replies; 30+ messages in thread
From: Chris Patterson @ 2017-04-06 19:47 UTC (permalink / raw)
To: xen-devel; +Cc: Chris Patterson, julien.grall, sstabellini, temkink
From: "Chris Patterson" <pattersonc@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 these 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.
Authored-by: Kyle Temkin <temkink@ainfosec.com>
Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
---
changes since rfc:
- use bool instead of bool_t
- formatting & code style cleanup
- fix dt compat label (nvidia,tegra120 -> nvidia,tegra124) for K1
- separate mediated legacy interrupt controller into its own module
- split tegra_ictlr_set_interrupt_enable() into
tegra_lic_set_interrupt_type_normal() and
tegra_lic_set_interrupt_enable()
- added a couple helper functions to reduce duplicated logic
- added wrapper tegra_lic_readl and writel functions for external use (mlic)
- re-order defines in tegra.h
- cleanup tegra_init() that was previously in patch 6
---
xen/arch/arm/platforms/Makefile | 2 +
xen/arch/arm/platforms/tegra.c | 313 ++++++++++++++++++++++++++++++++++
xen/include/asm-arm/platforms/tegra.h | 54 ++++++
3 files changed, 369 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..d7033d2 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -6,5 +6,7 @@ obj-$(CONFIG_ARM_32) += omap5.o
obj-$(CONFIG_ARM_32) += rcar2.o
obj-$(CONFIG_ARM_64) += seattle.o
obj-$(CONFIG_ARM_32) += sunxi.o
+obj-$(CONFIG_ARM_32) += tegra.o
+obj-$(CONFIG_ARM_64) += tegra.o
obj-$(CONFIG_ARM_64) += xgene-storm.o
obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
diff --git a/xen/arch/arm/platforms/tegra.c b/xen/arch/arm/platforms/tegra.c
new file mode 100644
index 0000000..bdd9966
--- /dev/null
+++ b/xen/arch/arm/platforms/tegra.c
@@ -0,0 +1,312 @@
+/*
+ * 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/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;
+
+/*
+ * List of legacy interrupt controllers that can be used to route
+ * Tegra interrupts.
+ */
+static const char * const tegra_interrupt_compat[] __initconst =
+{
+ "nvidia,tegra124-ictlr", /* Tegra K1 controllers */
+ "nvidia,tegra210-ictlr" /* Tegra X1 controllers */
+};
+
+/*
+ * Returns true iff the given IRQ belongs to a supported tegra interrupt
+ * controller.
+ */
+static bool tegra_irq_belongs_to_ictlr(const 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.
+ */
+static bool tegra_irq_is_routable(const 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;
+}
+
+/*
+ * 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);
+
+ /* Should not get here */
+ iounmap(addr);
+}
+
+/*
+ * Convert irq line to index of servicing legacy interrupt controller.
+ */
+static unsigned int tegra_lic_irq_to_ictlr_index(unsigned int irq)
+{
+ return (irq - NR_LOCAL_IRQS) / TEGRA_IRQS_PER_ICTLR;
+}
+
+/*
+ * Convert irq line to index of irq within servicing interrupt controller.
+ */
+static unsigned int tegra_lic_irq_to_ictlr_irq_index(unsigned int irq)
+{
+ return (irq - NR_LOCAL_IRQS) % TEGRA_IRQS_PER_ICTLR;
+}
+
+/*
+ * Mark interrupt as normal rather than a fast IRQ.
+ */
+static void tegra_lic_set_interrupt_type_normal(unsigned int irq)
+{
+ uint32_t previous_iep_class;
+ unsigned int ictlr_index = tegra_lic_irq_to_ictlr_index(irq);
+ unsigned int ictlr_irq_index = tegra_lic_irq_to_ictlr_irq_index(irq);
+ uint32_t mask = BIT(ictlr_irq_index);
+
+ /* Mark the interrupt as a normal interrupt-- not a fast IRQ. */
+ previous_iep_class = tegra_lic_readl(ictlr_index, TEGRA_ICTLR_CPU_IEP_CLASS);
+ tegra_lic_writel(ictlr_index, TEGRA_ICTLR_CPU_IEP_CLASS, previous_iep_class & ~mask);
+}
+
+/*
+ * Enable/disable interrupt line for specified irq.
+ */
+static void tegra_lic_set_interrupt_enable(unsigned int irq, bool enabled)
+{
+ unsigned int ictlr_index = tegra_lic_irq_to_ictlr_index(irq);
+ unsigned int ictlr_irq_index = tegra_lic_irq_to_ictlr_irq_index(irq);
+ uint32_t mask = BIT(ictlr_irq_index);
+
+ if ( enabled )
+ tegra_lic_writel(ictlr_index, TEGRA_ICTLR_CPU_IER_SET, mask);
+ else
+ tegra_lic_writel(ictlr_index, TEGRA_ICTLR_CPU_IER_CLR, mask);
+}
+
+/*
+ * Routes an IRQ to a guest, applying sane values to the ictlr masks.
+ * Returns 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. Either way, first
+ * set the interrupt type to normal (if previously set to fast IRQ).
+ */
+ tegra_lic_set_interrupt_type_normal(desc->irq);
+ tegra_lic_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.
+ */
+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_lic_set_interrupt_type_normal(desc->irq);
+ tegra_lic_set_interrupt_enable(desc->irq, true);
+}
+
+/*
+ * Read register from specified legacy interrupt interrupt controller.
+ */
+uint32_t tegra_lic_readl(unsigned int ictlr_index, unsigned int register_offset)
+{
+ ASSERT(tegra_ictlr_base);
+ ASSERT(ictlr_index < TEGRA_ICTLR_COUNT);
+ ASSERT(register_offset < TEGRA_ICTLR_SIZE);
+ return readl(tegra_ictlr_base + ictlr_index * TEGRA_ICTLR_SIZE +
+ register_offset);
+}
+
+/*
+ * Write register for specified legacy interrupt interrupt controller.
+ */
+void tegra_lic_writel(unsigned int ictlr_index, unsigned int register_offset, uint32_t value)
+{
+ ASSERT(tegra_ictlr_base);
+ ASSERT(ictlr_index < TEGRA_ICTLR_COUNT);
+ ASSERT(register_offset < TEGRA_ICTLR_SIZE);
+ writel(value, tegra_ictlr_base + ictlr_index * TEGRA_ICTLR_SIZE +
+ register_offset);
+}
+
+/*
+ * 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_lic_init(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");
+
+ /* Initialize each of the legacy interrupt controllers. */
+ for ( i = 0; i < TEGRA_ICTLR_COUNT; i++ ) {
+
+ /* Clear the interrupt enables for every interrupt. */
+ tegra_lic_writel(i, TEGRA_ICTLR_CPU_IER_CLR, ~0);
+
+ /*
+ * Mark all of our interrupts as normal ARM interrupts (as opposed
+ * to Fast Interrupts.)
+ */
+ tegra_lic_writel(i, TEGRA_ICTLR_CPU_IEP_CLASS, 0);
+ }
+
+ return 0;
+}
+
+/**
+ * Startup code for the Tegra.
+ */
+static int tegra_init(void)
+{
+ return tegra_lic_init();
+}
+
+
+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,
+ .route_irq_to_xen = tegra_route_irq_to_xen,
+ .route_irq_to_guest = tegra_route_irq_to_guest,
+PLATFORM_END
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 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..e9cd792
--- /dev/null
+++ b/xen/include/asm-arm/platforms/tegra.h
@@ -0,0 +1,54 @@
+/*
+ * 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_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
+
+#define TEGRA_RESET_BASE 0x7000e400
+#define TEGRA_RESET_SIZE 4
+#define TEGRA_RESET_MASK 0x10
+
+uint32_t tegra_lic_readl(unsigned int ictlr_index, unsigned int register_offset);
+void tegra_lic_writel(unsigned int ictlr_index, unsigned int register_offset, uint32_t value);
+
+#endif /* __ASM_ARM_PLATFORMS_TEGRA_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
2017-04-06 19:47 ` [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic " Chris Patterson
@ 2017-04-13 23:46 ` Stefano Stabellini
2017-04-17 15:03 ` Chris Patterson
2017-04-18 8:26 ` Julien Grall
1 sibling, 1 reply; 30+ messages in thread
From: Stefano Stabellini @ 2017-04-13 23:46 UTC (permalink / raw)
To: Chris Patterson
Cc: Chris Patterson, julien.grall, sstabellini, temkink, xen-devel
On Thu, 6 Apr 2017, Chris Patterson wrote:
> From: "Chris Patterson" <pattersonc@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 these 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.
>
> Authored-by: Kyle Temkin <temkink@ainfosec.com>
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
> ---
>
> changes since rfc:
> - use bool instead of bool_t
> - formatting & code style cleanup
> - fix dt compat label (nvidia,tegra120 -> nvidia,tegra124) for K1
> - separate mediated legacy interrupt controller into its own module
> - split tegra_ictlr_set_interrupt_enable() into
> tegra_lic_set_interrupt_type_normal() and
> tegra_lic_set_interrupt_enable()
> - added a couple helper functions to reduce duplicated logic
> - added wrapper tegra_lic_readl and writel functions for external use (mlic)
> - re-order defines in tegra.h
> - cleanup tegra_init() that was previously in patch 6
>
> ---
>
> xen/arch/arm/platforms/Makefile | 2 +
> xen/arch/arm/platforms/tegra.c | 313 ++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/platforms/tegra.h | 54 ++++++
> 3 files changed, 369 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..d7033d2 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -6,5 +6,7 @@ obj-$(CONFIG_ARM_32) += omap5.o
> obj-$(CONFIG_ARM_32) += rcar2.o
> obj-$(CONFIG_ARM_64) += seattle.o
> obj-$(CONFIG_ARM_32) += sunxi.o
> +obj-$(CONFIG_ARM_32) += tegra.o
> +obj-$(CONFIG_ARM_64) += tegra.o
> obj-$(CONFIG_ARM_64) += xgene-storm.o
> obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
> diff --git a/xen/arch/arm/platforms/tegra.c b/xen/arch/arm/platforms/tegra.c
> new file mode 100644
> index 0000000..bdd9966
> --- /dev/null
> +++ b/xen/arch/arm/platforms/tegra.c
> @@ -0,0 +1,312 @@
> +/*
> + * 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/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;
> +
> +/*
> + * List of legacy interrupt controllers that can be used to route
> + * Tegra interrupts.
> + */
> +static const char * const tegra_interrupt_compat[] __initconst =
> +{
> + "nvidia,tegra124-ictlr", /* Tegra K1 controllers */
> + "nvidia,tegra210-ictlr" /* Tegra X1 controllers */
> +};
> +
> +/*
> + * Returns true iff the given IRQ belongs to a supported tegra interrupt
> + * controller.
> + */
> +static bool tegra_irq_belongs_to_ictlr(const 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.
> + */
> +static bool tegra_irq_is_routable(const 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;
> +}
> +
> +/*
> + * 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);
> +
> + /* Should not get here */
> + iounmap(addr);
> +}
> +
> +/*
> + * Convert irq line to index of servicing legacy interrupt controller.
> + */
> +static unsigned int tegra_lic_irq_to_ictlr_index(unsigned int irq)
> +{
> + return (irq - NR_LOCAL_IRQS) / TEGRA_IRQS_PER_ICTLR;
> +}
> +
> +/*
> + * Convert irq line to index of irq within servicing interrupt controller.
> + */
> +static unsigned int tegra_lic_irq_to_ictlr_irq_index(unsigned int irq)
> +{
> + return (irq - NR_LOCAL_IRQS) % TEGRA_IRQS_PER_ICTLR;
> +}
> +
> +/*
> + * Mark interrupt as normal rather than a fast IRQ.
> + */
> +static void tegra_lic_set_interrupt_type_normal(unsigned int irq)
> +{
> + uint32_t previous_iep_class;
> + unsigned int ictlr_index = tegra_lic_irq_to_ictlr_index(irq);
> + unsigned int ictlr_irq_index = tegra_lic_irq_to_ictlr_irq_index(irq);
> + uint32_t mask = BIT(ictlr_irq_index);
> +
> + /* Mark the interrupt as a normal interrupt-- not a fast IRQ. */
> + previous_iep_class = tegra_lic_readl(ictlr_index, TEGRA_ICTLR_CPU_IEP_CLASS);
> + tegra_lic_writel(ictlr_index, TEGRA_ICTLR_CPU_IEP_CLASS, previous_iep_class & ~mask);
> +}
> +
> +/*
> + * Enable/disable interrupt line for specified irq.
> + */
> +static void tegra_lic_set_interrupt_enable(unsigned int irq, bool enabled)
> +{
> + unsigned int ictlr_index = tegra_lic_irq_to_ictlr_index(irq);
> + unsigned int ictlr_irq_index = tegra_lic_irq_to_ictlr_irq_index(irq);
> + uint32_t mask = BIT(ictlr_irq_index);
> +
> + if ( enabled )
> + tegra_lic_writel(ictlr_index, TEGRA_ICTLR_CPU_IER_SET, mask);
> + else
> + tegra_lic_writel(ictlr_index, TEGRA_ICTLR_CPU_IER_CLR, mask);
> +}
> +
> +/*
> + * Routes an IRQ to a guest, applying sane values to the ictlr masks.
> + * Returns 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;
> + }
Remove the warning.
> + /*
> + * 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. Either way, first
> + * set the interrupt type to normal (if previously set to fast IRQ).
> + */
> + tegra_lic_set_interrupt_type_normal(desc->irq);
> + tegra_lic_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.
> + */
> +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_lic_set_interrupt_type_normal(desc->irq);
> + tegra_lic_set_interrupt_enable(desc->irq, true);
> +}
> +
> +/*
> + * Read register from specified legacy interrupt interrupt controller.
> + */
> +uint32_t tegra_lic_readl(unsigned int ictlr_index, unsigned int register_offset)
> +{
> + ASSERT(tegra_ictlr_base);
> + ASSERT(ictlr_index < TEGRA_ICTLR_COUNT);
> + ASSERT(register_offset < TEGRA_ICTLR_SIZE);
> + return readl(tegra_ictlr_base + ictlr_index * TEGRA_ICTLR_SIZE +
> + register_offset);
> +}
> +
> +/*
> + * Write register for specified legacy interrupt interrupt controller.
> + */
> +void tegra_lic_writel(unsigned int ictlr_index, unsigned int register_offset, uint32_t value)
> +{
> + ASSERT(tegra_ictlr_base);
> + ASSERT(ictlr_index < TEGRA_ICTLR_COUNT);
> + ASSERT(register_offset < TEGRA_ICTLR_SIZE);
> + writel(value, tegra_ictlr_base + ictlr_index * TEGRA_ICTLR_SIZE +
> + register_offset);
> +}
> +
> +/*
> + * 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_lic_init(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");
> +
> + /* Initialize each of the legacy interrupt controllers. */
> + for ( i = 0; i < TEGRA_ICTLR_COUNT; i++ ) {
> +
> + /* Clear the interrupt enables for every interrupt. */
> + tegra_lic_writel(i, TEGRA_ICTLR_CPU_IER_CLR, ~0);
> +
> + /*
> + * Mark all of our interrupts as normal ARM interrupts (as opposed
> + * to Fast Interrupts.)
> + */
> + tegra_lic_writel(i, TEGRA_ICTLR_CPU_IEP_CLASS, 0);
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * Startup code for the Tegra.
> + */
> +static int tegra_init(void)
__init
> +{
> + return tegra_lic_init();
> +}
> +
> +
> +static const char * const tegra_dt_compat[] __initconst =
> +{
> + "nvidia,tegra120", /* Tegra K1 */
This is still tegra120 (not tegra124), is that intended? If so, it is
still missing from arch/arm*/boot/dts. Do you have a pointer?
Also, do we need both tegra_dt_compat and tegra_interrupt_compat? Can we
keep only one?
> + "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,
> + .route_irq_to_xen = tegra_route_irq_to_xen,
> + .route_irq_to_guest = tegra_route_irq_to_guest,
> +PLATFORM_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 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..e9cd792
> --- /dev/null
> +++ b/xen/include/asm-arm/platforms/tegra.h
> @@ -0,0 +1,54 @@
> +/*
> + * 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_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
> +
> +#define TEGRA_RESET_BASE 0x7000e400
> +#define TEGRA_RESET_SIZE 4
> +#define TEGRA_RESET_MASK 0x10
> +
> +uint32_t tegra_lic_readl(unsigned int ictlr_index, unsigned int register_offset);
> +void tegra_lic_writel(unsigned int ictlr_index, unsigned int register_offset, uint32_t value);
> +
> +#endif /* __ASM_ARM_PLATFORMS_TEGRA_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 2.1.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
2017-04-13 23:46 ` Stefano Stabellini
@ 2017-04-17 15:03 ` Chris Patterson
2017-04-18 7:58 ` Julien Grall
0 siblings, 1 reply; 30+ messages in thread
From: Chris Patterson @ 2017-04-17 15:03 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Julien Grall, Chris Patterson, temkink, xen-devel
>> +static const char * const tegra_dt_compat[] __initconst =
>> +{
>> + "nvidia,tegra120", /* Tegra K1 */
>
> This is still tegra120 (not tegra124), is that intended? If so, it is
> still missing from arch/arm*/boot/dts. Do you have a pointer?
It was not intended; thank you for catching it. I must have lost that
fixup somewhere along the way...
> Also, do we need both tegra_dt_compat and tegra_interrupt_compat? Can we
> keep only one?
The purpose of tegra_interrupt_compat is to maintain a tegra-specific
whitelist of interrupt controllers we know how to route. Presumably,
there may be custom boards out there that may have additional
interrupt routing capabilities that this patch set would not support
as-is. I'm not sure of an appropriate way to maintain that logic and
merge them. However, I am certainly open to suggestion, if you have
any ideas.
Thanks for the review!
-Chris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
2017-04-17 15:03 ` Chris Patterson
@ 2017-04-18 7:58 ` Julien Grall
2017-07-06 22:00 ` Chris Patterson
0 siblings, 1 reply; 30+ messages in thread
From: Julien Grall @ 2017-04-18 7:58 UTC (permalink / raw)
To: Chris Patterson, Stefano Stabellini
Cc: Chris Patterson, nd, temkink, xen-devel
Hello Chris,
On 17/04/2017 16:03, Chris Patterson wrote:
>>> +static const char * const tegra_dt_compat[] __initconst =
>>> +{
>>> + "nvidia,tegra120", /* Tegra K1 */
>>
>> This is still tegra120 (not tegra124), is that intended? If so, it is
>> still missing from arch/arm*/boot/dts. Do you have a pointer?
>
> It was not intended; thank you for catching it. I must have lost that
> fixup somewhere along the way...
>
>> Also, do we need both tegra_dt_compat and tegra_interrupt_compat? Can we
>> keep only one?
>
> The purpose of tegra_interrupt_compat is to maintain a tegra-specific
> whitelist of interrupt controllers we know how to route. Presumably,
> there may be custom boards out there that may have additional
> interrupt routing capabilities that this patch set would not support
> as-is. I'm not sure of an appropriate way to maintain that logic and
> merge them. However, I am certainly open to suggestion, if you have
> any ideas.
Those custom boards would likely have a different machine compatible
string because the SOC would be different. So I think the
tegra_interrupt_compat is not necessary.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
2017-04-18 7:58 ` Julien Grall
@ 2017-07-06 22:00 ` Chris Patterson
2017-07-07 16:25 ` Julien Grall
0 siblings, 1 reply; 30+ messages in thread
From: Chris Patterson @ 2017-07-06 22:00 UTC (permalink / raw)
To: Julien Grall; +Cc: Chris Patterson, nd, Stefano Stabellini, temkink, xen-devel
>> The purpose of tegra_interrupt_compat is to maintain a tegra-specific
>> whitelist of interrupt controllers we know how to route. Presumably,
>> there may be custom boards out there that may have additional
>> interrupt routing capabilities that this patch set would not support
>> as-is. I'm not sure of an appropriate way to maintain that logic and
>> merge them. However, I am certainly open to suggestion, if you have
>> any ideas.
>
>
> Those custom boards would likely have a different machine compatible string
> because the SOC would be different. So I think the tegra_interrupt_compat is
> not necessary.
>
Sorry about the delayed response...
This check effectively filters out GPIO controllers. Any GPIO-sourced
interrupts route through the GPIO controller's interrupt, and need not
apply for map_irq_to_domain(). Devices requiring GPIO-based
interrupts would need to be passed through with the associated
controller (or maybe virtualize the GPIO routing?).
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
2017-07-06 22:00 ` Chris Patterson
@ 2017-07-07 16:25 ` Julien Grall
2017-07-07 18:08 ` Chris Patterson
0 siblings, 1 reply; 30+ messages in thread
From: Julien Grall @ 2017-07-07 16:25 UTC (permalink / raw)
To: Chris Patterson
Cc: Chris Patterson, nd, Stefano Stabellini, temkink, xen-devel
Hi Chris,
On 06/07/17 23:00, Chris Patterson wrote:
>>> The purpose of tegra_interrupt_compat is to maintain a tegra-specific
>>> whitelist of interrupt controllers we know how to route. Presumably,
>>> there may be custom boards out there that may have additional
>>> interrupt routing capabilities that this patch set would not support
>>> as-is. I'm not sure of an appropriate way to maintain that logic and
>>> merge them. However, I am certainly open to suggestion, if you have
>>> any ideas.
>>
>>
>> Those custom boards would likely have a different machine compatible string
>> because the SOC would be different. So I think the tegra_interrupt_compat is
>> not necessary.
>>
>
> Sorry about the delayed response...
>
> This check effectively filters out GPIO controllers. Any GPIO-sourced
> interrupts route through the GPIO controller's interrupt, and need not
> apply for map_irq_to_domain(). Devices requiring GPIO-based
> interrupts would need to be passed through with the associated
> controller (or maybe virtualize the GPIO routing?).
I am a bit confused. Could you give a concrete example based on tegra
platform?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
2017-07-07 16:25 ` Julien Grall
@ 2017-07-07 18:08 ` Chris Patterson
2017-07-26 16:49 ` Julien Grall
0 siblings, 1 reply; 30+ messages in thread
From: Chris Patterson @ 2017-07-07 18:08 UTC (permalink / raw)
To: Julien Grall; +Cc: Chris Patterson, nd, Stefano Stabellini, temkink, xen-devel
On Fri, Jul 7, 2017 at 12:25 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Chris,
>
>
> On 06/07/17 23:00, Chris Patterson wrote:
>>>>
>>>> The purpose of tegra_interrupt_compat is to maintain a tegra-specific
>>>> whitelist of interrupt controllers we know how to route. Presumably,
>>>> there may be custom boards out there that may have additional
>>>> interrupt routing capabilities that this patch set would not support
>>>> as-is. I'm not sure of an appropriate way to maintain that logic and
>>>> merge them. However, I am certainly open to suggestion, if you have
>>>> any ideas.
>>>
>>>
>>>
>>> Those custom boards would likely have a different machine compatible
>>> string
>>> because the SOC would be different. So I think the tegra_interrupt_compat
>>> is
>>> not necessary.
>>>
>>
>> Sorry about the delayed response...
>>
>> This check effectively filters out GPIO controllers. Any GPIO-sourced
>> interrupts route through the GPIO controller's interrupt, and need not
>> apply for map_irq_to_domain(). Devices requiring GPIO-based
>> interrupts would need to be passed through with the associated
>> controller (or maybe virtualize the GPIO routing?).
>
>
> I am a bit confused. Could you give a concrete example based on tegra
> platform?
>
I'm thinking of devices where interrupt-parent is &gpio (as opposed to
&lic or &gic). Here is an example on tegra-smaug:
wifi: wifi_bcm4354 {
compatible = "bcm,bcm4354";
interrupt-parent = <&gpio>;
interrupts = <TEGRA_GPIO(H, 2) IRQ_TYPE_LEVEL_HIGH>;
wl_reg_on = <&gpio TEGRA_GPIO(H, 1) GPIO_ACTIVE_HIGH>;
wl_host_wake = <&gpio TEGRA_GPIO(H, 2) IRQ_TYPE_LEVEL_HIGH>;
nvidia,pmc-wakeup = <&pmc
PMC_WAKE_TYPE_EVENT 8 PMC_TRIGGER_TYPE_HIGH>;
};
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
2017-07-07 18:08 ` Chris Patterson
@ 2017-07-26 16:49 ` Julien Grall
0 siblings, 0 replies; 30+ messages in thread
From: Julien Grall @ 2017-07-26 16:49 UTC (permalink / raw)
To: Chris Patterson
Cc: Chris Patterson, nd, Stefano Stabellini, temkink, xen-devel
Hi Chris,
On 07/07/17 19:08, Chris Patterson wrote:
> On Fri, Jul 7, 2017 at 12:25 PM, Julien Grall <julien.grall@arm.com> wrote:
>> Hi Chris,
>>
>>
>> On 06/07/17 23:00, Chris Patterson wrote:
>>>>>
>>>>> The purpose of tegra_interrupt_compat is to maintain a tegra-specific
>>>>> whitelist of interrupt controllers we know how to route. Presumably,
>>>>> there may be custom boards out there that may have additional
>>>>> interrupt routing capabilities that this patch set would not support
>>>>> as-is. I'm not sure of an appropriate way to maintain that logic and
>>>>> merge them. However, I am certainly open to suggestion, if you have
>>>>> any ideas.
>>>>
>>>>
>>>>
>>>> Those custom boards would likely have a different machine compatible
>>>> string
>>>> because the SOC would be different. So I think the tegra_interrupt_compat
>>>> is
>>>> not necessary.
>>>>
>>>
>>> Sorry about the delayed response...
>>>
>>> This check effectively filters out GPIO controllers. Any GPIO-sourced
>>> interrupts route through the GPIO controller's interrupt, and need not
>>> apply for map_irq_to_domain(). Devices requiring GPIO-based
>>> interrupts would need to be passed through with the associated
>>> controller (or maybe virtualize the GPIO routing?).
>>
>>
>> I am a bit confused. Could you give a concrete example based on tegra
>> platform?
>>
>
> I'm thinking of devices where interrupt-parent is &gpio (as opposed to
> &lic or &gic). Here is an example on tegra-smaug:
>
> wifi: wifi_bcm4354 {
> compatible = "bcm,bcm4354";
> interrupt-parent = <&gpio>;
> interrupts = <TEGRA_GPIO(H, 2) IRQ_TYPE_LEVEL_HIGH>;
> wl_reg_on = <&gpio TEGRA_GPIO(H, 1) GPIO_ACTIVE_HIGH>;
> wl_host_wake = <&gpio TEGRA_GPIO(H, 2) IRQ_TYPE_LEVEL_HIGH>;
> nvidia,pmc-wakeup = <&pmc
> PMC_WAKE_TYPE_EVENT 8 PMC_TRIGGER_TYPE_HIGH>;
> };
>
Hmmm, you are right here. What confuse me is the comment on the top of
tegra_irq_is_routable:
/*
* Returns true iff the given IRQ is routable -- that is, if it i descended
* from the platform's primary GIC.
*/
In both GPIO and LIC, the final interrupt will be coming from the GIC.
However, the later seems to have a direct translation with the GIC,
hence why they are routable. Right?
If so I would update the comment to make it clear.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
2017-04-06 19:47 ` [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic " Chris Patterson
2017-04-13 23:46 ` Stefano Stabellini
@ 2017-04-18 8:26 ` Julien Grall
2017-07-06 23:12 ` Chris Patterson
1 sibling, 1 reply; 30+ messages in thread
From: Julien Grall @ 2017-04-18 8:26 UTC (permalink / raw)
To: Chris Patterson, xen-devel; +Cc: Chris Patterson, nd, temkink, sstabellini
Hello,
On 06/04/2017 20:47, Chris Patterson wrote:
> From: "Chris Patterson" <pattersonc@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 these 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.
Can you please provide a link to the public documentation (if there is
any) about the SOC?
>
> Authored-by: Kyle Temkin <temkink@ainfosec.com>
See my question on patch #2.
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
> ---
>
> changes since rfc:
> - use bool instead of bool_t
> - formatting & code style cleanup
> - fix dt compat label (nvidia,tegra120 -> nvidia,tegra124) for K1
> - separate mediated legacy interrupt controller into its own module
> - split tegra_ictlr_set_interrupt_enable() into
> tegra_lic_set_interrupt_type_normal() and
> tegra_lic_set_interrupt_enable()
> - added a couple helper functions to reduce duplicated logic
> - added wrapper tegra_lic_readl and writel functions for external use (mlic)
> - re-order defines in tegra.h
> - cleanup tegra_init() that was previously in patch 6
>
> ---
>
> xen/arch/arm/platforms/Makefile | 2 +
> xen/arch/arm/platforms/tegra.c | 313 ++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/platforms/tegra.h | 54 ++++++
> 3 files changed, 369 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..d7033d2 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -6,5 +6,7 @@ obj-$(CONFIG_ARM_32) += omap5.o
> obj-$(CONFIG_ARM_32) += rcar2.o
> obj-$(CONFIG_ARM_64) += seattle.o
> obj-$(CONFIG_ARM_32) += sunxi.o
> +obj-$(CONFIG_ARM_32) += tegra.o
> +obj-$(CONFIG_ARM_64) += tegra.o
Please use obj-y rather duplicating the code.
> obj-$(CONFIG_ARM_64) += xgene-storm.o
> obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
> diff --git a/xen/arch/arm/platforms/tegra.c b/xen/arch/arm/platforms/tegra.c
> new file mode 100644
> index 0000000..bdd9966
> --- /dev/null
> +++ b/xen/arch/arm/platforms/tegra.c
> @@ -0,0 +1,312 @@
> +/*
> + * NVIDIA Tegra specific settings
> + *
> + * Ian Campbell; Copyright (c) 2014 Citrix Systems
Ian Campbell is not mentioned in the commit message. Is he the original
author of the code? How much did you modify?
> + * 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/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;
> +
> +/*
> + * List of legacy interrupt controllers that can be used to route
> + * Tegra interrupts.
> + */
> +static const char * const tegra_interrupt_compat[] __initconst =
> +{
> + "nvidia,tegra124-ictlr", /* Tegra K1 controllers */
> + "nvidia,tegra210-ictlr" /* Tegra X1 controllers */
> +};
> +
> +/*
> + * Returns true iff the given IRQ belongs to a supported tegra interrupt
> + * controller.
> + */
> +static bool tegra_irq_belongs_to_ictlr(const struct dt_raw_irq * rirq) {
Coding style.
> + int i;
> +
> + for ( i = 0; i < ARRAY_SIZE(tegra_interrupt_compat); i++ ) {
Coding style.
> + 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.
> + */
> +static bool tegra_irq_is_routable(const 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;
> +}
> +
> +/*
> + * 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);
> +
> + /* Should not get here */
> + iounmap(addr);
> +}
> +
> +/*
> + * Convert irq line to index of servicing legacy interrupt controller.
> + */
> +static unsigned int tegra_lic_irq_to_ictlr_index(unsigned int irq)
> +{
> + return (irq - NR_LOCAL_IRQS) / TEGRA_IRQS_PER_ICTLR;
> +}
> +
> +/*
> + * Convert irq line to index of irq within servicing interrupt controller.
> + */
> +static unsigned int tegra_lic_irq_to_ictlr_irq_index(unsigned int irq)
> +{
> + return (irq - NR_LOCAL_IRQS) % TEGRA_IRQS_PER_ICTLR;
> +}
> +
> +/*
> + * Mark interrupt as normal rather than a fast IRQ.
> + */
> +static void tegra_lic_set_interrupt_type_normal(unsigned int irq)
> +{
> + uint32_t previous_iep_class;
> + unsigned int ictlr_index = tegra_lic_irq_to_ictlr_index(irq);
> + unsigned int ictlr_irq_index = tegra_lic_irq_to_ictlr_irq_index(irq);
> + uint32_t mask = BIT(ictlr_irq_index);
> +
> + /* Mark the interrupt as a normal interrupt-- not a fast IRQ. */
> + previous_iep_class = tegra_lic_readl(ictlr_index, TEGRA_ICTLR_CPU_IEP_CLASS);
> + tegra_lic_writel(ictlr_index, TEGRA_ICTLR_CPU_IEP_CLASS, previous_iep_class & ~mask);
> +}
> +
> +/*
> + * Enable/disable interrupt line for specified irq.
> + */
> +static void tegra_lic_set_interrupt_enable(unsigned int irq, bool enabled)
> +{
> + unsigned int ictlr_index = tegra_lic_irq_to_ictlr_index(irq);
> + unsigned int ictlr_irq_index = tegra_lic_irq_to_ictlr_irq_index(irq);
> + uint32_t mask = BIT(ictlr_irq_index);
> +
> + if ( enabled )
> + tegra_lic_writel(ictlr_index, TEGRA_ICTLR_CPU_IER_SET, mask);
> + else
> + tegra_lic_writel(ictlr_index, TEGRA_ICTLR_CPU_IER_CLR, mask);
> +}
> +
> +/*
> + * Routes an IRQ to a guest, applying sane values to the ictlr masks.
> + * Returns 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)
The indentation looks wrong.
> +{
> + /* 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",
s/%d/%u/ for both.
> + 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. Either way, first
> + * set the interrupt type to normal (if previously set to fast IRQ).
So why do you want the hardware domain to interact with the ictlr? Could
not you hide it completely?
> + */
> + tegra_lic_set_interrupt_type_normal(desc->irq);
> + tegra_lic_set_interrupt_enable(desc->irq, !is_hardware_domain(d));
What would happen if you enable the interrupt here for the guest? Should
not you do it when the guest is requesting to enable (see vgic_enable_irqs).
Also, how about EOI an interrupt?
Lastly, newline here please.
> + return rc;
> +}
> +
> +
> +/*
> + * Routes an IRQ to Xen. This method both performs the core IRQ routing, and
> + * sets up any ictlr routing necessary.
> + */
> +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_lic_set_interrupt_type_normal(desc->irq);
> + tegra_lic_set_interrupt_enable(desc->irq, true);
> +}
> +
> +/*
> + * Read register from specified legacy interrupt interrupt controller.
> + */
> +uint32_t tegra_lic_readl(unsigned int ictlr_index, unsigned int register_offset)
> +{
> + ASSERT(tegra_ictlr_base);
> + ASSERT(ictlr_index < TEGRA_ICTLR_COUNT);
> + ASSERT(register_offset < TEGRA_ICTLR_SIZE);
> + return readl(tegra_ictlr_base + ictlr_index * TEGRA_ICTLR_SIZE +
> + register_offset);
> +}
> +
> +/*
> + * Write register for specified legacy interrupt interrupt controller.
> + */
> +void tegra_lic_writel(unsigned int ictlr_index, unsigned int register_offset, uint32_t value)
> +{
> + ASSERT(tegra_ictlr_base);
> + ASSERT(ictlr_index < TEGRA_ICTLR_COUNT);
> + ASSERT(register_offset < TEGRA_ICTLR_SIZE);
> + writel(value, tegra_ictlr_base + ictlr_index * TEGRA_ICTLR_SIZE +
> + register_offset);
> +}
> +
> +/*
> + * 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_lic_init(void)
> +{
> + int i;
> +
> + /* Map in the tegra ictlr. */
> + tegra_ictlr_base = ioremap_nocache(TEGRA_ICTLR_BASE,
> + TEGRA_ICTLR_SIZE * TEGRA_ICTLR_COUNT);
Can we avoid hardcoding MMIO if they can be found through the DT?
> +
> + if ( !tegra_ictlr_base )
> + panic("Failed to map in the Tegra legacy interrupt controller");
> +
> + /* Initialize each of the legacy interrupt controllers. */
> + for ( i = 0; i < TEGRA_ICTLR_COUNT; i++ ) {
Coding style.
> +
> + /* Clear the interrupt enables for every interrupt. */
> + tegra_lic_writel(i, TEGRA_ICTLR_CPU_IER_CLR, ~0);
> +
> + /*
> + * Mark all of our interrupts as normal ARM interrupts (as opposed
> + * to Fast Interrupts.)
> + */
> + tegra_lic_writel(i, TEGRA_ICTLR_CPU_IEP_CLASS, 0);
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * Startup code for the Tegra.
> + */
> +static int tegra_init(void)
> +{
> + return tegra_lic_init();
> +}
> +
> +
No need for 2 newlines. One is enough.
> +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,
> + .route_irq_to_xen = tegra_route_irq_to_xen,
> + .route_irq_to_guest = tegra_route_irq_to_guest,
> +PLATFORM_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 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..e9cd792
> --- /dev/null
> +++ b/xen/include/asm-arm/platforms/tegra.h
> @@ -0,0 +1,54 @@
> +/*
> + * 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_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
> +
> +#define TEGRA_RESET_BASE 0x7000e400
> +#define TEGRA_RESET_SIZE 4
> +#define TEGRA_RESET_MASK 0x10
> +
> +uint32_t tegra_lic_readl(unsigned int ictlr_index, unsigned int register_offset);
> +void tegra_lic_writel(unsigned int ictlr_index, unsigned int register_offset, uint32_t value);
> +
> +#endif /* __ASM_ARM_PLATFORMS_TEGRA_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
2017-04-18 8:26 ` Julien Grall
@ 2017-07-06 23:12 ` Chris Patterson
2017-07-07 16:30 ` Julien Grall
0 siblings, 1 reply; 30+ messages in thread
From: Chris Patterson @ 2017-07-06 23:12 UTC (permalink / raw)
To: Julien Grall; +Cc: Chris Patterson, nd, temkink, Stefano Stabellini, xen-devel
>
> So why do you want the hardware domain to interact with the ictlr? Could not
> you hide it completely?
>
snip
> What would happen if you enable the interrupt here for the guest? Should not
> you do it when the guest is requesting to enable (see vgic_enable_irqs).
>
>
> Also, how about EOI an interrupt?
We could possibly hide the legacy controller, but that has its own
challenges. Notably, the LIC allows configuration for forwarding FIQ
vs IRQ, and setting wake-up sources.
If we accept limitations to those configurations, we could possibly
hide it entirely (or just for non-Dom0 guests with device
passthrough?), and then I think we would need platform hooks for
accomplishing the masking/unmasking/eoi alongside the vgic. If we
don't want to limit the configuration options - we would need to
surface the mediated LIC in the device trees for guests with device
pass-through.
>
Will incorporate rest of review into next version, thanks!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
2017-07-06 23:12 ` Chris Patterson
@ 2017-07-07 16:30 ` Julien Grall
2017-07-07 18:53 ` Chris Patterson
0 siblings, 1 reply; 30+ messages in thread
From: Julien Grall @ 2017-07-07 16:30 UTC (permalink / raw)
To: Chris Patterson
Cc: Chris Patterson, nd, temkink, Stefano Stabellini, xen-devel
Hi Chris,
On 07/07/17 00:12, Chris Patterson wrote:
>>
>> So why do you want the hardware domain to interact with the ictlr? Could not
>> you hide it completely?
>>
>
> snip
>
>> What would happen if you enable the interrupt here for the guest? Should not
>> you do it when the guest is requesting to enable (see vgic_enable_irqs).
>>
>>
>> Also, how about EOI an interrupt?
>
> We could possibly hide the legacy controller, but that has its own
> challenges. Notably, the LIC allows configuration for forwarding FIQ
> vs IRQ, and setting wake-up sources.
FIQ are not supported for domain. So I am not sure why you would want a
guest to configure that.
Furthermore, could you explain what is wake-up sources and why a guest
would need it?
>
> If we accept limitations to those configurations, we could possibly
> hide it entirely (or just for non-Dom0 guests with device
> passthrough?), and then I think we would need platform hooks for
> accomplishing the masking/unmasking/eoi alongside the vgic. If we
> don't want to limit the configuration options - we would need to
> surface the mediated LIC in the device trees for guests with device
> pass-through.
>
>>
>
> Will incorporate rest of review into next version, thanks!
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
2017-07-07 16:30 ` Julien Grall
@ 2017-07-07 18:53 ` Chris Patterson
2017-07-24 19:38 ` Chris Patterson
0 siblings, 1 reply; 30+ messages in thread
From: Chris Patterson @ 2017-07-07 18:53 UTC (permalink / raw)
To: Julien Grall; +Cc: Chris Patterson, nd, temkink, Stefano Stabellini, xen-devel
On Fri, Jul 7, 2017 at 12:30 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Chris,
>
> On 07/07/17 00:12, Chris Patterson wrote:
>>>
>>>
>>> So why do you want the hardware domain to interact with the ictlr? Could
>>> not
>>> you hide it completely?
>>>
>>
>> snip
>>
>>> What would happen if you enable the interrupt here for the guest? Should
>>> not
>>> you do it when the guest is requesting to enable (see vgic_enable_irqs).
>>>
>>>
>>> Also, how about EOI an interrupt?
>>
>>
>> We could possibly hide the legacy controller, but that has its own
>> challenges. Notably, the LIC allows configuration for forwarding FIQ
>> vs IRQ, and setting wake-up sources.
>
>
> FIQ are not supported for domain. So I am not sure why you would want a
> guest to configure that.
>
Fair point, I did not know that and didn't want to assume there was
not a case otherwise...
> Furthermore, could you explain what is wake-up sources and why a guest would
> need it?
>
I would expect any driver using irq_set_irq_wake()? A little more
background from the reference manual
(http://developer.nvidia.com/embedded/dlc/tegra-x1-technical-reference-manual):
"
The Legacy Interrupt Controller (LIC) is primarily used for BPMP
(ARM7). But it is also used for generating interrupts as wake
events for CPUs. This is an important use case when the core is in
retention. All of the device hardware interrupt signals are
sent to the LIC first, which routes them to the ARM7 BPMP-Lite as well
as forwards them to the GIC. The LIC also provides a
software set/clear mechanism for all of the interrupts."
...
3.3.1.5 Interrupt Blocking to Support Retention
The Tegra X1 device implements blocking of interrupts routed to the
GIC, which supports the CPU retention state. The block
implementing this feature is represented in Figure 4 as Blocking.
To support retention, the LIC contains a one shot disable for all the
interrupts. When the system goes into retention, BPMP
software sets this bit to disable the interrupts.
The Flow Controller watches all the interrupts triggered and triggers
the BPMP to bring the core out of retention when any
interrupt bit is asserted. Once the system is out of retention, the
BLOCK _CCPLEX_GIC_INTR bit is cleared, and the interrupt is
serviced by the GIC.
"
Anything using GPIO to wake (e.g. wifi?) is routed through the LIC. I
did not find anything obvious with a quick scan, but perhaps other
devices directly wired to the LIC may require it as well.
Thoughts?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
2017-07-07 18:53 ` Chris Patterson
@ 2017-07-24 19:38 ` Chris Patterson
2017-07-26 16:10 ` Julien Grall
0 siblings, 1 reply; 30+ messages in thread
From: Chris Patterson @ 2017-07-24 19:38 UTC (permalink / raw)
To: Julien Grall; +Cc: Chris Patterson, nd, temkink, Stefano Stabellini, xen-devel
On Fri, Jul 7, 2017 at 2:53 PM, Chris Patterson <cjp256@gmail.com> wrote:
> On Fri, Jul 7, 2017 at 12:30 PM, Julien Grall <julien.grall@arm.com> wrote:
>> Hi Chris,
>>
>> On 07/07/17 00:12, Chris Patterson wrote:
>>>>
>>>>
>>>> So why do you want the hardware domain to interact with the ictlr? Could
>>>> not
>>>> you hide it completely?
>>>>
>>>
>>> snip
>>>
>>>> What would happen if you enable the interrupt here for the guest? Should
>>>> not
>>>> you do it when the guest is requesting to enable (see vgic_enable_irqs).
>>>>
>>>>
>>>> Also, how about EOI an interrupt?
>>>
>>>
>>> We could possibly hide the legacy controller, but that has its own
>>> challenges. Notably, the LIC allows configuration for forwarding FIQ
>>> vs IRQ, and setting wake-up sources.
>>
>>
>> FIQ are not supported for domain. So I am not sure why you would want a
>> guest to configure that.
>>
>
> Fair point, I did not know that and didn't want to assume there was
> not a case otherwise...
>
>> Furthermore, could you explain what is wake-up sources and why a guest would
>> need it?
>>
>
> I would expect any driver using irq_set_irq_wake()? A little more
> background from the reference manual
> (http://developer.nvidia.com/embedded/dlc/tegra-x1-technical-reference-manual):
>
> "
> The Legacy Interrupt Controller (LIC) is primarily used for BPMP
> (ARM7). But it is also used for generating interrupts as wake
> events for CPUs. This is an important use case when the core is in
> retention. All of the device hardware interrupt signals are
> sent to the LIC first, which routes them to the ARM7 BPMP-Lite as well
> as forwards them to the GIC. The LIC also provides a
> software set/clear mechanism for all of the interrupts."
>
> ...
>
> 3.3.1.5 Interrupt Blocking to Support Retention
>
> The Tegra X1 device implements blocking of interrupts routed to the
> GIC, which supports the CPU retention state. The block
> implementing this feature is represented in Figure 4 as Blocking.
>
> To support retention, the LIC contains a one shot disable for all the
> interrupts. When the system goes into retention, BPMP
> software sets this bit to disable the interrupts.
>
> The Flow Controller watches all the interrupts triggered and triggers
> the BPMP to bring the core out of retention when any
> interrupt bit is asserted. Once the system is out of retention, the
> BLOCK _CCPLEX_GIC_INTR bit is cleared, and the interrupt is
> serviced by the GIC.
> "
>
> Anything using GPIO to wake (e.g. wifi?) is routed through the LIC. I
> did not find anything obvious with a quick scan, but perhaps other
> devices directly wired to the LIC may require it as well.
>
> Thoughts?
Hey Julien. Just a quick ping. I was going to submit an updated patch
series, just wanted to see if you had any further thoughts or desired
changes with regards to the above. Thanks!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
2017-07-24 19:38 ` Chris Patterson
@ 2017-07-26 16:10 ` Julien Grall
0 siblings, 0 replies; 30+ messages in thread
From: Julien Grall @ 2017-07-26 16:10 UTC (permalink / raw)
To: Chris Patterson
Cc: Chris Patterson, nd, temkink, Stefano Stabellini, xen-devel
Hi Chris,
Sorry for the late reply.
On 24/07/17 20:38, Chris Patterson wrote:
> On Fri, Jul 7, 2017 at 2:53 PM, Chris Patterson <cjp256@gmail.com> wrote:
>> On Fri, Jul 7, 2017 at 12:30 PM, Julien Grall <julien.grall@arm.com> wrote:
>>> Hi Chris,
>>>
>>> On 07/07/17 00:12, Chris Patterson wrote:
>>>>>
>>>>>
>>>>> So why do you want the hardware domain to interact with the ictlr? Could
>>>>> not
>>>>> you hide it completely?
>>>>>
>>>>
>>>> snip
>>>>
>>>>> What would happen if you enable the interrupt here for the guest? Should
>>>>> not
>>>>> you do it when the guest is requesting to enable (see vgic_enable_irqs).
>>>>>
>>>>>
>>>>> Also, how about EOI an interrupt?
>>>>
>>>>
>>>> We could possibly hide the legacy controller, but that has its own
>>>> challenges. Notably, the LIC allows configuration for forwarding FIQ
>>>> vs IRQ, and setting wake-up sources.
>>>
>>>
>>> FIQ are not supported for domain. So I am not sure why you would want a
>>> guest to configure that.
>>>
>>
>> Fair point, I did not know that and didn't want to assume there was
>> not a case otherwise...
>>
>>> Furthermore, could you explain what is wake-up sources and why a guest would
>>> need it?
>>>
>>
>> I would expect any driver using irq_set_irq_wake()? A little more
>> background from the reference manual
>> (http://developer.nvidia.com/embedded/dlc/tegra-x1-technical-reference-manual):
>>
>> "
>> The Legacy Interrupt Controller (LIC) is primarily used for BPMP
>> (ARM7). But it is also used for generating interrupts as wake
>> events for CPUs. This is an important use case when the core is in
>> retention. All of the device hardware interrupt signals are
>> sent to the LIC first, which routes them to the ARM7 BPMP-Lite as well
>> as forwards them to the GIC. The LIC also provides a
>> software set/clear mechanism for all of the interrupts."
>>
>> ...
>>
>> 3.3.1.5 Interrupt Blocking to Support Retention
>>
>> The Tegra X1 device implements blocking of interrupts routed to the
>> GIC, which supports the CPU retention state. The block
>> implementing this feature is represented in Figure 4 as Blocking.
>>
>> To support retention, the LIC contains a one shot disable for all the
>> interrupts. When the system goes into retention, BPMP
>> software sets this bit to disable the interrupts.
>>
>> The Flow Controller watches all the interrupts triggered and triggers
>> the BPMP to bring the core out of retention when any
>> interrupt bit is asserted. Once the system is out of retention, the
>> BLOCK _CCPLEX_GIC_INTR bit is cleared, and the interrupt is
>> serviced by the GIC.
>> "
>>
>> Anything using GPIO to wake (e.g. wifi?) is routed through the LIC. I
>> did not find anything obvious with a quick scan, but perhaps other
>> devices directly wired to the LIC may require it as well.
>>
>> Thoughts?
I have the feeling it is a lot emulation for supporting only wake-up
sources in the guest (or even Dom0). Have you thought about
para-virtualizing it?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 5/6] xen/arm: Add function to query IRQ 'ownership'
2017-04-06 19:47 [PATCH 0/6] Initial Tegra platform support Chris Patterson
` (3 preceding siblings ...)
2017-04-06 19:47 ` [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic " Chris Patterson
@ 2017-04-06 19:47 ` Chris Patterson
2017-04-18 8:27 ` Julien Grall
2017-04-06 19:47 ` [PATCH 6/6] xen/arm: platforms/tegra: Ensure the hwdom can only affect its own interrupts Chris Patterson
5 siblings, 1 reply; 30+ messages in thread
From: Chris Patterson @ 2017-04-06 19:47 UTC (permalink / raw)
To: xen-devel; +Cc: Chris Patterson, julien.grall, sstabellini, temkink
From: "Chris Patterson" <pattersonc@ainfosec.com>
The addition of new IRQ-related platform hooks now allow platforms to
perform platform-specific interrupt logic, such as allowing virtualization
of platform-specific interrupt controller hardware.
This commit adds the ability for the platform to identify the domain
a given IRQ is routed to, allowing platform logic to 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.
Authored-by: Kyle Temkin <temkink@ainfosec.com>
Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
changes since rfc:
- formatting & code style cleanup
---
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 0b4eaa9..51bce58 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -143,6 +143,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 4849f16..d0fd6db 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -44,6 +44,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.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 5/6] xen/arm: Add function to query IRQ 'ownership'
2017-04-06 19:47 ` [PATCH 5/6] xen/arm: Add function to query IRQ 'ownership' Chris Patterson
@ 2017-04-18 8:27 ` Julien Grall
0 siblings, 0 replies; 30+ messages in thread
From: Julien Grall @ 2017-04-18 8:27 UTC (permalink / raw)
To: Chris Patterson, xen-devel; +Cc: Chris Patterson, nd, temkink, sstabellini
Hello,
On 06/04/2017 20:47, Chris Patterson wrote:
> From: "Chris Patterson" <pattersonc@ainfosec.com>
>
> The addition of new IRQ-related platform hooks now allow platforms to
> perform platform-specific interrupt logic, such as allowing virtualization
> of platform-specific interrupt controller hardware.
>
> This commit adds the ability for the platform to identify the domain
> a given IRQ is routed to, allowing platform logic to 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.
>
> Authored-by: Kyle Temkin <temkink@ainfosec.com>
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>
> changes since rfc:
> - formatting & code style cleanup
>
> ---
>
> 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 0b4eaa9..51bce58 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -143,6 +143,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. */
NIT: s/Otherise/Otherwise/
> + 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 4849f16..d0fd6db 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -44,6 +44,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)))
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 6/6] xen/arm: platforms/tegra: Ensure the hwdom can only affect its own interrupts
2017-04-06 19:47 [PATCH 0/6] Initial Tegra platform support Chris Patterson
` (4 preceding siblings ...)
2017-04-06 19:47 ` [PATCH 5/6] xen/arm: Add function to query IRQ 'ownership' Chris Patterson
@ 2017-04-06 19:47 ` Chris Patterson
2017-04-13 23:54 ` Stefano Stabellini
2017-04-18 8:39 ` Julien Grall
5 siblings, 2 replies; 30+ messages in thread
From: Chris Patterson @ 2017-04-06 19:47 UTC (permalink / raw)
To: xen-devel; +Cc: Chris Patterson, julien.grall, sstabellini, temkink
From: Chris Patterson <pattersonc@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.
Authored-by: Kyle Temkin <temkink@ainfosec.com>
Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
---
changes since rfc:
- documentation, formatting & code style cleanup
- drop tegra_init changes (folded into patch 4)
---
xen/arch/arm/platforms/Makefile | 2 +
xen/arch/arm/platforms/tegra-mlic.c | 261 +++++++++++++++++++++++++++++
xen/arch/arm/platforms/tegra.c | 13 ++
xen/include/asm-arm/platforms/tegra-mlic.h | 34 ++++
4 files changed, 310 insertions(+)
create mode 100644 xen/arch/arm/platforms/tegra-mlic.c
create mode 100644 xen/include/asm-arm/platforms/tegra-mlic.h
diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index d7033d2..5701e62 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -7,6 +7,8 @@ obj-$(CONFIG_ARM_32) += rcar2.o
obj-$(CONFIG_ARM_64) += seattle.o
obj-$(CONFIG_ARM_32) += sunxi.o
obj-$(CONFIG_ARM_32) += tegra.o
+obj-$(CONFIG_ARM_32) += tegra-mlic.o
obj-$(CONFIG_ARM_64) += tegra.o
+obj-$(CONFIG_ARM_64) += tegra-mlic.o
obj-$(CONFIG_ARM_64) += xgene-storm.o
obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
diff --git a/xen/arch/arm/platforms/tegra-mlic.c b/xen/arch/arm/platforms/tegra-mlic.c
new file mode 100644
index 0000000..ad77766
--- /dev/null
+++ b/xen/arch/arm/platforms/tegra-mlic.c
@@ -0,0 +1,260 @@
+/*
+ * xen/arch/arm/tegra_mlic.c
+ *
+ * Mediator for Tegra Legacy Interrupt Controller
+ *
+ * This module allow the hardware domain to have 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.
+ *
+ * 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/lib.h>
+#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/platforms/tegra-mlic.h>
+#include <asm/mmio.h>
+#include <xen/perfc.h>
+
+static int tegra_mlic_mmio_read(struct vcpu *v, mmio_info_t *info,
+ register_t *r, void *priv);
+static int tegra_mlic_mmio_write(struct vcpu *v, mmio_info_t *info,
+ register_t r, void *priv);
+
+static const struct mmio_handler_ops tegra_mlic_mmio_handler = {
+ .read = tegra_mlic_mmio_read,
+ .write = tegra_mlic_mmio_write,
+};
+
+/*
+ * Parses a LIC MMIO read or write, and extracts the information needed to
+ * complete the request.
+ *
+ * info: Information describing the MMIO read/write being performed
+ * ictlr_index: The interrupt controller number in the ictlr (e.g. 0-5)
+ * register_offset: The register offset into the specified interrupt controller
+ * (e.g. TEGRA_ICTLR_CPU_IER_SET)
+ * irq_base: The number of the first IRQ represented by the given ictlr.
+ */
+static void tegra_mlic_parse_mmio_request(mmio_info_t *info,
+ uint32_t *ictlr_index, uint32_t *register_offset, uint32_t *irq_base)
+{
+ /* Determine the offset of the access into the ICTLR region. */
+ uint32_t offset = info->gpa - TEGRA_ICTLR_BASE;
+ uint32_t ictlr = offset / TEGRA_ICTLR_SIZE;
+ uint32_t reg = offset % TEGRA_ICTLR_SIZE;
+
+ if ( ictlr_index )
+ *ictlr_index = ictlr;
+
+ if ( register_offset )
+ *register_offset = reg;
+
+ if ( irq_base )
+ *irq_base = (ictlr * TEGRA_IRQS_PER_ICTLR) + NR_LOCAL_IRQS;
+
+ /* Ensure that we've only been handed a valid offset within our region. */
+ BUG_ON(ictlr >= TEGRA_ICTLR_COUNT);
+ BUG_ON(offset >= (TEGRA_ICTLR_COUNT * TEGRA_ICTLR_SIZE));
+ BUG_ON((ictlr * TEGRA_ICTLR_SIZE + reg) != offset);
+}
+
+/*
+ * Returns true iff the given IRQ is currently routed to the given domain.
+ */
+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_mlic_mmio_read(struct vcpu *v, mmio_info_t *info,
+ register_t *target_register, void *priv)
+{
+ register_t raw_value;
+ unsigned int ictlr_index;
+ unsigned int register_offset;
+ unsigned int irq_base;
+ int i;
+
+ perfc_incr(tegra_mlic_reads);
+
+ tegra_mlic_parse_mmio_request(info, &ictlr_index, ®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 ")\n", 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 "\n", current->domain->domain_id, info->gpa);
+ domain_crash_synchronous();
+ }
+
+
+ /* Perform the core ictlr read. */
+ raw_value = tegra_lic_readl(ictlr_index, 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;
+
+ if ( !irq_owned_by_domain(irq, current->domain) )
+ raw_value &= ~( 1 << i );
+ }
+
+ /* 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_mlic_mmio_write(struct vcpu *v, mmio_info_t *info,
+ register_t new_value, void *priv)
+{
+ register_t write_mask = 0;
+ register_t raw_value;
+ unsigned int ictlr_index;
+ unsigned int register_offset;
+ unsigned int irq_base;
+ int i;
+
+ perfc_incr(tegra_mlic_writes);
+
+ tegra_mlic_parse_mmio_request(info, &ictlr_index, ®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 ")\n", 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 "\n", current->domain->domain_id, info->gpa);
+ domain_crash_synchronous();
+ return 0;
+ }
+
+ /*
+ * 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;
+
+ if ( irq_owned_by_domain(irq, current->domain) )
+ write_mask |= ( 1 << i );
+ }
+
+ /*
+ * 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 = tegra_lic_readl(ictlr_index, register_offset);
+
+ /* Remove bits that the guest is not allowed to write. */
+ raw_value &= ~write_mask;
+ raw_value |= (write_mask & new_value);
+
+ /* Finally perform the write. */
+ tegra_lic_writel(ictlr_index, register_offset, raw_value);
+ return 1;
+}
+
+/*
+ * Set up the hardware domain for the Tegra, giving it mediated access to the
+ * platform's legacy interrupt controller.
+ */
+int domain_tegra_mlic_init(struct domain *d)
+{
+ int rc;
+ unsigned long pfn_start, pfn_end;
+
+ ASSERT( is_hardware_domain(d) );
+
+ 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 mediator. */
+ rc = iomem_deny_access(d, pfn_start, pfn_end);
+
+ if ( rc )
+ panic("Failed to deny access to the Tegra LIC iomem");
+
+ rc = unmap_mmio_regions(d, _gfn(pfn_start),
+ pfn_end - pfn_start + 1,
+ _mfn(pfn_start));
+
+ if ( rc )
+ panic("Failed to deny access to the Tegra LIC");
+
+ register_mmio_handler(d, &tegra_mlic_mmio_handler,
+ TEGRA_ICTLR_BASE,
+ TEGRA_ICTLR_SIZE * TEGRA_ICTLR_COUNT,
+ NULL);
+
+ return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/platforms/tegra.c b/xen/arch/arm/platforms/tegra.c
index bdd9966..ee62708 100644
--- a/xen/arch/arm/platforms/tegra.c
+++ b/xen/arch/arm/platforms/tegra.c
@@ -20,11 +20,14 @@
#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/platforms/tegra-mlic.h>
+#include <asm/mmio.h>
/* Permanent mapping to the Tegra legacy interrupt controller. */
static void __iomem *tegra_ictlr_base;
@@ -211,6 +214,15 @@ static void tegra_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
}
/*
+ * Use platform specific mapping for hook to initialize mediated legacy
+ * interrupt controller for hardware domain.
+ */
+static int tegra_specific_mapping(struct domain *d)
+{
+ return domain_tegra_mlic_init(d);
+}
+
+/*
* Read register from specified legacy interrupt interrupt controller.
*/
uint32_t tegra_lic_readl(unsigned int ictlr_index, unsigned int register_offset)
@@ -300,6 +312,7 @@ PLATFORM_START(tegra, "Tegra")
.irq_is_routable = tegra_irq_is_routable,
.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
/*
diff --git a/xen/include/asm-arm/platforms/tegra-mlic.h b/xen/include/asm-arm/platforms/tegra-mlic.h
new file mode 100644
index 0000000..fee10c4
--- /dev/null
+++ b/xen/include/asm-arm/platforms/tegra-mlic.h
@@ -0,0 +1,34 @@
+/*
+ * xen/arch/arm/vuart.h
+ *
+ * Mediated Tegra Legacy Interrupt Controller
+ *
+ * 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_MLIC_H
+#define __ASM_ARM_PLATFORMS_TEGRA_MLIC_H
+
+int domain_tegra_mlic_init(struct domain *d);
+
+#endif /* __ASM_ARM_PLATFORMS_TEGRA_MLIC_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 6/6] xen/arm: platforms/tegra: Ensure the hwdom can only affect its own interrupts
2017-04-06 19:47 ` [PATCH 6/6] xen/arm: platforms/tegra: Ensure the hwdom can only affect its own interrupts Chris Patterson
@ 2017-04-13 23:54 ` Stefano Stabellini
2017-04-18 8:39 ` Julien Grall
1 sibling, 0 replies; 30+ messages in thread
From: Stefano Stabellini @ 2017-04-13 23:54 UTC (permalink / raw)
To: Chris Patterson
Cc: Chris Patterson, julien.grall, sstabellini, temkink, xen-devel
On Thu, 6 Apr 2017, Chris Patterson wrote:
> From: Chris Patterson <pattersonc@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.
>
> Authored-by: Kyle Temkin <temkink@ainfosec.com>
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
> ---
>
> changes since rfc:
> - documentation, formatting & code style cleanup
> - drop tegra_init changes (folded into patch 4)
>
> ---
> xen/arch/arm/platforms/Makefile | 2 +
> xen/arch/arm/platforms/tegra-mlic.c | 261 +++++++++++++++++++++++++++++
> xen/arch/arm/platforms/tegra.c | 13 ++
> xen/include/asm-arm/platforms/tegra-mlic.h | 34 ++++
> 4 files changed, 310 insertions(+)
> create mode 100644 xen/arch/arm/platforms/tegra-mlic.c
> create mode 100644 xen/include/asm-arm/platforms/tegra-mlic.h
>
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index d7033d2..5701e62 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -7,6 +7,8 @@ obj-$(CONFIG_ARM_32) += rcar2.o
> obj-$(CONFIG_ARM_64) += seattle.o
> obj-$(CONFIG_ARM_32) += sunxi.o
> obj-$(CONFIG_ARM_32) += tegra.o
> +obj-$(CONFIG_ARM_32) += tegra-mlic.o
> obj-$(CONFIG_ARM_64) += tegra.o
> +obj-$(CONFIG_ARM_64) += tegra-mlic.o
> obj-$(CONFIG_ARM_64) += xgene-storm.o
> obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
> diff --git a/xen/arch/arm/platforms/tegra-mlic.c b/xen/arch/arm/platforms/tegra-mlic.c
> new file mode 100644
> index 0000000..ad77766
> --- /dev/null
> +++ b/xen/arch/arm/platforms/tegra-mlic.c
> @@ -0,0 +1,260 @@
> +/*
> + * xen/arch/arm/tegra_mlic.c
> + *
> + * Mediator for Tegra Legacy Interrupt Controller
> + *
> + * This module allow the hardware domain to have 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.
> + *
> + * 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/lib.h>
> +#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/platforms/tegra-mlic.h>
> +#include <asm/mmio.h>
> +#include <xen/perfc.h>
> +
> +static int tegra_mlic_mmio_read(struct vcpu *v, mmio_info_t *info,
> + register_t *r, void *priv);
> +static int tegra_mlic_mmio_write(struct vcpu *v, mmio_info_t *info,
> + register_t r, void *priv);
> +
> +static const struct mmio_handler_ops tegra_mlic_mmio_handler = {
> + .read = tegra_mlic_mmio_read,
> + .write = tegra_mlic_mmio_write,
> +};
> +
> +/*
> + * Parses a LIC MMIO read or write, and extracts the information needed to
> + * complete the request.
> + *
> + * info: Information describing the MMIO read/write being performed
> + * ictlr_index: The interrupt controller number in the ictlr (e.g. 0-5)
> + * register_offset: The register offset into the specified interrupt controller
> + * (e.g. TEGRA_ICTLR_CPU_IER_SET)
> + * irq_base: The number of the first IRQ represented by the given ictlr.
> + */
> +static void tegra_mlic_parse_mmio_request(mmio_info_t *info,
> + uint32_t *ictlr_index, uint32_t *register_offset, uint32_t *irq_base)
style: parameters alignment
> +{
> + /* Determine the offset of the access into the ICTLR region. */
> + uint32_t offset = info->gpa - TEGRA_ICTLR_BASE;
> + uint32_t ictlr = offset / TEGRA_ICTLR_SIZE;
> + uint32_t reg = offset % TEGRA_ICTLR_SIZE;
> +
> + if ( ictlr_index )
> + *ictlr_index = ictlr;
> +
> + if ( register_offset )
> + *register_offset = reg;
> +
> + if ( irq_base )
> + *irq_base = (ictlr * TEGRA_IRQS_PER_ICTLR) + NR_LOCAL_IRQS;
> +
> + /* Ensure that we've only been handed a valid offset within our region. */
> + BUG_ON(ictlr >= TEGRA_ICTLR_COUNT);
> + BUG_ON(offset >= (TEGRA_ICTLR_COUNT * TEGRA_ICTLR_SIZE));
> + BUG_ON((ictlr * TEGRA_ICTLR_SIZE + reg) != offset);
> +}
> +
> +/*
> + * Returns true iff the given IRQ is currently routed to the given domain.
> + */
> +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_mlic_mmio_read(struct vcpu *v, mmio_info_t *info,
> + register_t *target_register, void *priv)
> +{
> + register_t raw_value;
> + unsigned int ictlr_index;
> + unsigned int register_offset;
> + unsigned int irq_base;
> + int i;
> +
> + perfc_incr(tegra_mlic_reads);
> +
> + tegra_mlic_parse_mmio_request(info, &ictlr_index, ®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 ")\n", 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 "\n", current->domain->domain_id, info->gpa);
> + domain_crash_synchronous();
> + }
> +
> +
> + /* Perform the core ictlr read. */
> + raw_value = tegra_lic_readl(ictlr_index, 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 ) {
style: { on newline
> + int irq = irq_base + i;
> +
> + if ( !irq_owned_by_domain(irq, current->domain) )
> + raw_value &= ~( 1 << i );
> + }
> +
> + /* 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_mlic_mmio_write(struct vcpu *v, mmio_info_t *info,
> + register_t new_value, void *priv)
> +{
> + register_t write_mask = 0;
> + register_t raw_value;
> + unsigned int ictlr_index;
> + unsigned int register_offset;
> + unsigned int irq_base;
> + int i;
> +
> + perfc_incr(tegra_mlic_writes);
> +
> + tegra_mlic_parse_mmio_request(info, &ictlr_index, ®ister_offset,
> + &irq_base);
> +
> + /* Sanity check the read. */
> + if ( register_offset & 0x3 ) {
style: { on newline
> + printk(XENLOG_G_ERR "d%u: Tegra LIC: Attempt to write unaligned ictlr addr"
> + "(%" PRIpaddr ")\n", current->domain->domain_id, info->gpa);
> + domain_crash_synchronous();
> + return 0;
> + }
> +
> + if ( info->dabt.size != DABT_WORD ) {
style: { on newline
> + printk(XENLOG_G_ERR "d%u: Tegra LIC: Non-word write to ictlr addr"
> + "%" PRIpaddr "\n", current->domain->domain_id, info->gpa);
> + domain_crash_synchronous();
> + return 0;
> + }
> +
> + /*
> + * 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 ) {
style: { on newline
> + int irq = irq_base + i;
> +
> + if ( irq_owned_by_domain(irq, current->domain) )
> + write_mask |= ( 1 << i );
> + }
> +
> + /*
> + * 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 = tegra_lic_readl(ictlr_index, register_offset);
> +
> + /* Remove bits that the guest is not allowed to write. */
> + raw_value &= ~write_mask;
> + raw_value |= (write_mask & new_value);
> +
> + /* Finally perform the write. */
> + tegra_lic_writel(ictlr_index, register_offset, raw_value);
> + return 1;
> +}
> +
> +/*
> + * Set up the hardware domain for the Tegra, giving it mediated access to the
> + * platform's legacy interrupt controller.
> + */
> +int domain_tegra_mlic_init(struct domain *d)
> +{
> + int rc;
> + unsigned long pfn_start, pfn_end;
> +
> + ASSERT( is_hardware_domain(d) );
> +
> + 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 mediator. */
> + rc = iomem_deny_access(d, pfn_start, pfn_end);
> +
> + if ( rc )
> + panic("Failed to deny access to the Tegra LIC iomem");
> +
> + rc = unmap_mmio_regions(d, _gfn(pfn_start),
> + pfn_end - pfn_start + 1,
> + _mfn(pfn_start));
Please avoid mapping the region at boot time, if possible. Maybe you can
do that using blacklist_dev?
> + if ( rc )
> + panic("Failed to deny access to the Tegra LIC");
> +
> + register_mmio_handler(d, &tegra_mlic_mmio_handler,
> + TEGRA_ICTLR_BASE,
> + TEGRA_ICTLR_SIZE * TEGRA_ICTLR_COUNT,
> + NULL);
> +
> + return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/platforms/tegra.c b/xen/arch/arm/platforms/tegra.c
> index bdd9966..ee62708 100644
> --- a/xen/arch/arm/platforms/tegra.c
> +++ b/xen/arch/arm/platforms/tegra.c
> @@ -20,11 +20,14 @@
> #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/platforms/tegra-mlic.h>
> +#include <asm/mmio.h>
>
> /* Permanent mapping to the Tegra legacy interrupt controller. */
> static void __iomem *tegra_ictlr_base;
> @@ -211,6 +214,15 @@ static void tegra_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
> }
>
> /*
> + * Use platform specific mapping for hook to initialize mediated legacy
> + * interrupt controller for hardware domain.
> + */
> +static int tegra_specific_mapping(struct domain *d)
> +{
> + return domain_tegra_mlic_init(d);
domain_tegra_mlic_init doesn't map anything. In fact, it unmaps a memory
region. Please call domain_tegra_mlic_init from another function, maybe
tegra_init.
> +}
> +
> +/*
> * Read register from specified legacy interrupt interrupt controller.
> */
> uint32_t tegra_lic_readl(unsigned int ictlr_index, unsigned int register_offset)
> @@ -300,6 +312,7 @@ PLATFORM_START(tegra, "Tegra")
> .irq_is_routable = tegra_irq_is_routable,
> .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
>
> /*
> diff --git a/xen/include/asm-arm/platforms/tegra-mlic.h b/xen/include/asm-arm/platforms/tegra-mlic.h
> new file mode 100644
> index 0000000..fee10c4
> --- /dev/null
> +++ b/xen/include/asm-arm/platforms/tegra-mlic.h
> @@ -0,0 +1,34 @@
> +/*
> + * xen/arch/arm/vuart.h
> + *
> + * Mediated Tegra Legacy Interrupt Controller
> + *
> + * 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_MLIC_H
> +#define __ASM_ARM_PLATFORMS_TEGRA_MLIC_H
> +
> +int domain_tegra_mlic_init(struct domain *d);
> +
> +#endif /* __ASM_ARM_PLATFORMS_TEGRA_MLIC_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 2.1.4
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 6/6] xen/arm: platforms/tegra: Ensure the hwdom can only affect its own interrupts
2017-04-06 19:47 ` [PATCH 6/6] xen/arm: platforms/tegra: Ensure the hwdom can only affect its own interrupts Chris Patterson
2017-04-13 23:54 ` Stefano Stabellini
@ 2017-04-18 8:39 ` Julien Grall
2017-07-06 23:13 ` Chris Patterson
1 sibling, 1 reply; 30+ messages in thread
From: Julien Grall @ 2017-04-18 8:39 UTC (permalink / raw)
To: Chris Patterson, xen-devel; +Cc: Chris Patterson, nd, temkink, sstabellini
Hello,
On 06/04/2017 20:47, Chris Patterson wrote:
> From: Chris Patterson <pattersonc@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.
>
> Authored-by: Kyle Temkin <temkink@ainfosec.com>
See my question on patch #2.
> Signed-off-by: Kyle Temkin <temkink@ainfosec.com>
> Signed-off-by: Chris Patterson <pattersonc@ainfosec.com>
> ---
>
> changes since rfc:
> - documentation, formatting & code style cleanup
> - drop tegra_init changes (folded into patch 4)
>
> ---
> xen/arch/arm/platforms/Makefile | 2 +
> xen/arch/arm/platforms/tegra-mlic.c | 261 +++++++++++++++++++++++++++++
> xen/arch/arm/platforms/tegra.c | 13 ++
> xen/include/asm-arm/platforms/tegra-mlic.h | 34 ++++
> 4 files changed, 310 insertions(+)
> create mode 100644 xen/arch/arm/platforms/tegra-mlic.c
> create mode 100644 xen/include/asm-arm/platforms/tegra-mlic.h
>
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index d7033d2..5701e62 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -7,6 +7,8 @@ obj-$(CONFIG_ARM_32) += rcar2.o
> obj-$(CONFIG_ARM_64) += seattle.o
> obj-$(CONFIG_ARM_32) += sunxi.o
> obj-$(CONFIG_ARM_32) += tegra.o
> +obj-$(CONFIG_ARM_32) += tegra-mlic.o
I think it would make sense to introduce a tegra directory where tegra.c
and tegra-mlic would live.
> obj-$(CONFIG_ARM_64) += tegra.o
> +obj-$(CONFIG_ARM_64) += tegra-mlic.o
> obj-$(CONFIG_ARM_64) += xgene-storm.o
> obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 30+ messages in thread