[PATCH v2 0/8] xen/arm: Add support for S32CC platforms and LINFlexD UART

[PATCH v2 0/8] xen/arm: Add support for S32CC platforms and LINFlexD UART

[Andrei Cherechesu (OSS)]

From: Andrei Cherechesu <andrei.cherechesu@nxp.com>

This patch series adds support for NXP Automotive S32CC platform
family, which includes S32G [0] and S32R [1].

First patch adds the driver for the NXP LINFlexD UART, available
on S32V, S32G and S32R automotive processors. The compatibles in
the driver match the ones in upstream Linux [2]. The 2nd patch
adds early printk support via LINFlexD UART.

The 3rd patch introduces a generic mechanism for forwarding SCMI
over SMC calls to firmware running at EL3, selected via
CONFIG_SCMI_SMC. The 4th patch refactors the SiP SMC handling in
vSMC mechanism, to rely firstly on `platform_smc()` if implemented,
then on the previously introduced generic SCMI handling/forwarding
mechanism if enabled.

The 5th patch adds S32CC platform code stub and configs to enable
the required drivers for S32CC platforms.

The 6th, 7th and 8th patches modify the CHANGELOG.md, SUPPORT.md
and MAINTAINERS files, respectively, to add mentions about LINFlexD
UART support, S32CC platforms support, and the new SCMI-SMC layer
introduced.

[0] https://www.nxp.com/products/processors-and-microcontrollers/s32-automotive-platform/s32g-vehicle-network-processors:S32G-PROCESSORS
[1] https://www.nxp.com/products/processors-and-microcontrollers/s32-automotive-platform/s32r-radar-processing:S32R-FAMILY
[2] https://elixir.bootlin.com/linux/v6.11/source/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml#L27

v1->v2:
  - linflex-uart: 
    - Addressed coding style comments from Julien:
      - spaces vs. tabs
      - indentation
      - license issue
      - comments format
      - code reusage
      - misplaced loop exit
    - Made the pointer to struct linflex_uart const in most functions
    since it's not modified
    - Added support for customizable baud rate through console options,
    parsing what Xen gets in its bootargs. other parameters following
    <baud> are discarded for the moment.
  - SCMI SMC handling:
    - Added a generic `scmi-smc` layer (enabled by CONFIG_SCMI_SMC)
    to enable forwarding SCMI requests over SMC to EL3 FW if issued
    by Dom0. If the SCMI firmware node is not found in Dom0's DT during
    initialization, it fails silently as it's not mandatory.
    - Changed the handling for SiP SMCs in vsmc.c to firstly try
    the `platform_smc()` callback if implemented, then try to
    handle the SiP SMC as SCMI if enabled and SMC ID matches.
  - S32CC Platform support:
    - Added dependency on CONFIG_SCMI_SMC
    - Removed the S32CC platform-specific forwarding to EL3 mechanism.
    - Dropped unused headers
  - CHANGELOG.md:
    - Added mentions about S32CC support and SCMI-SMC layer.
  - SUPPORT.md:
    - Added SCMI over SMC forwarding to EL3 feature for Arm as supported.
  - MAINTAINERS:
    - Added myself as maintainer for S32CC platforms and NXP S32 Linux Team
    as reviewer list.

Andrei Cherechesu (8):
  xen/arm: Add NXP LINFlexD UART Driver
  xen/arm: Add NXP LINFlexD UART early printk support
  xen/arm: Add SCMI over SMC calls handling layer
  xen/arm: vsmc: Enable handling SiP-owned SCMI SMC calls
  xen/arm: platforms: Add NXP S32CC platform code
  CHANGELOG.md: Add NXP S32CC and SCMI-SMC layer support mentions
  SUPPORT.md: Describe SCMI-SMC layer feature
  MAINTAINERS: Add myself as maintainer for NXP S32CC

 CHANGELOG.md                            |   4 +
 MAINTAINERS                             |   8 +
 SUPPORT.md                              |   8 +
 xen/arch/arm/Kconfig                    |  10 +
 xen/arch/arm/Kconfig.debug              |  12 +
 xen/arch/arm/Makefile                   |   1 +
 xen/arch/arm/arm64/debug-linflex.inc    |  58 ++++
 xen/arch/arm/include/asm/linflex-uart.h |  63 ++++
 xen/arch/arm/include/asm/scmi-smc.h     |  52 ++++
 xen/arch/arm/platforms/Kconfig          |  11 +
 xen/arch/arm/platforms/Makefile         |   1 +
 xen/arch/arm/platforms/s32cc.c          |  32 ++
 xen/arch/arm/scmi-smc.c                 | 163 ++++++++++
 xen/arch/arm/vsmc.c                     |  19 +-
 xen/drivers/char/Kconfig                |   8 +
 xen/drivers/char/Makefile               |   1 +
 xen/drivers/char/linflex-uart.c         | 381 ++++++++++++++++++++++++
 17 files changed, 831 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/arm/arm64/debug-linflex.inc
 create mode 100644 xen/arch/arm/include/asm/linflex-uart.h
 create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
 create mode 100644 xen/arch/arm/platforms/s32cc.c
 create mode 100644 xen/arch/arm/scmi-smc.c
 create mode 100644 xen/drivers/char/linflex-uart.c

-- 
2.45.2

[PATCH v2 1/8] xen/arm: Add NXP LINFlexD UART Driver

[Andrei Cherechesu (OSS)]

From: Andrei Cherechesu <andrei.cherechesu@nxp.com>

The LINFlexD UART is an UART controller available on NXP S32
processors family targeting automotive (for example: S32G2, S32G3,
S32R).

S32G3 Reference Manual:
https://www.nxp.com/webapp/Download?colCode=RMS32G3.

Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
Signed-off-by: Peter van der Perk <peter.vander.perk@nxp.com>
---
 xen/arch/arm/include/asm/linflex-uart.h |  63 ++++
 xen/drivers/char/Kconfig                |   8 +
 xen/drivers/char/Makefile               |   1 +
 xen/drivers/char/linflex-uart.c         | 381 ++++++++++++++++++++++++
 4 files changed, 453 insertions(+)
 create mode 100644 xen/arch/arm/include/asm/linflex-uart.h
 create mode 100644 xen/drivers/char/linflex-uart.c

diff --git a/xen/arch/arm/include/asm/linflex-uart.h b/xen/arch/arm/include/asm/linflex-uart.h
new file mode 100644
index 0000000000..95dcbcb476
--- /dev/null
+++ b/xen/arch/arm/include/asm/linflex-uart.h
@@ -0,0 +1,63 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * xen/arch/arm/include/asm/linflex-uart.h
+ *
+ * Common constant definition between early printk and the UART driver
+ * for NXP LINFlexD UART.
+ *
+ * Andrei Cherechesu <andrei.cherechesu@nxp.com>
+ * Copyright 2018, 2021, 2024 NXP
+ */
+
+#ifndef __ASM_ARM_LINFLEX_UART_H
+#define __ASM_ARM_LINFLEX_UART_H
+
+/* 32-bit register offsets */
+#define LINCR1          (0x0)
+#define LINIER          (0x4)
+#define LINSR           (0x8)
+#define UARTCR          (0x10)
+#define UARTSR          (0x14)
+#define LINFBRR         (0x24)
+#define LINIBRR         (0x28)
+#define BDRL            (0x38)
+#define BDRM            (0x3C)
+#define UARTPTO         (0x50)
+
+#define LINCR1_INIT         BIT(0, U)
+#define LINCR1_MME          BIT(4, U)
+#define LINCR1_BF           BIT(7, U)
+
+#define LINSR_LINS          GENMASK(15, 12)
+#define LINSR_LINS_INIT     BIT(12, U)
+
+#define LINIER_DRIE         BIT(2, U)
+#define LINIER_DTIE         BIT(1, U)
+
+#define UARTCR_UART         BIT(0, U)
+#define UARTCR_WL0          BIT(1, U)
+#define UARTCR_PC0          BIT(3, U)
+#define UARTCR_TXEN         BIT(4, U)
+#define UARTCR_RXEN         BIT(5, U)
+#define UARTCR_PC1          BIT(6, U)
+#define UARTCR_TFBM         BIT(8, U)
+#define UARTCR_RFBM         BIT(9, U)
+#define UARTCR_RDFLRFC      GENMASK(12, 10)
+#define UARTCR_TDFLTFC      GENMASK(15, 13)
+#define UARTCR_ROSE         BIT(23, U)
+#define UARTCR_OSR_SHIFT    (24)
+#define UARTCR_OSR          GENMASK(27, UARTCR_OSR_SHIFT)
+
+#define UARTSR_DTFTFF       BIT(1, U)
+#define UARTSR_DRFRFE       BIT(2, U)
+
+#endif /* __ASM_ARM_LINFLEX_UART_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index 3f836ab301..e175d07c02 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -13,6 +13,14 @@ config HAS_CADENCE_UART
 	  This selects the Xilinx Zynq Cadence UART. If you have a Xilinx Zynq
 	  based board, say Y.
 
+config HAS_LINFLEX
+	bool "NXP LINFlexD UART driver"
+	default y
+	depends on ARM_64
+	help
+	  This selects the NXP LINFlexD UART. If you have an NXP S32G or S32R
+	  based board, say Y.
+
 config HAS_IMX_LPUART
 	bool "i.MX LPUART driver"
 	default y
diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
index e7e374775d..d3b987da1d 100644
--- a/xen/drivers/char/Makefile
+++ b/xen/drivers/char/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_HAS_SCIF) += scif-uart.o
 obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
 obj-$(CONFIG_XHCI) += xhci-dbc.o
 obj-$(CONFIG_HAS_IMX_LPUART) += imx-lpuart.o
+obj-$(CONFIG_HAS_LINFLEX) += linflex-uart.o
 obj-$(CONFIG_ARM) += arm-uart.o
 obj-y += serial.o
 obj-$(CONFIG_XEN_GUEST) += xen_pv_console.o
diff --git a/xen/drivers/char/linflex-uart.c b/xen/drivers/char/linflex-uart.c
new file mode 100644
index 0000000000..2fa195cbf6
--- /dev/null
+++ b/xen/drivers/char/linflex-uart.c
@@ -0,0 +1,381 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * xen/drivers/char/linflex-uart.c
+ *
+ * Driver for NXP LINFlexD UART.
+ *
+ * Andrei Cherechesu <andrei.cherechesu@nxp.com>
+ * Copyright 2018, 2021-2022, 2024 NXP
+ */
+
+#include <xen/config.h>
+#include <xen/console.h>
+#include <xen/errno.h>
+#include <xen/init.h>
+#include <xen/irq.h>
+#include <xen/mm.h>
+#include <xen/serial.h>
+#include <asm/device.h>
+#include <asm/io.h>
+#include <asm/linflex-uart.h>
+
+#define LINFLEX_CLK_FREQ        (125000000)
+#define LINFLEX_MAX_BAUDRATE    (2000000)
+#define LINFLEX_BAUDRATE        (115200)
+#define LINFLEX_LDIV_MULTIPLIER (16)
+
+static struct linflex_uart {
+    uint32_t baud, clock_hz;
+    uint32_t irq;
+    char __iomem *regs;
+    struct irqaction irqaction;
+    struct vuart_info vuart;
+} linflex_com;
+
+static uint32_t linflex_uart_readl(const struct linflex_uart *uart,
+                                   uint32_t off)
+{
+    return readl(uart->regs + off);
+}
+
+static void linflex_uart_writel(const struct linflex_uart *uart, uint32_t off,
+                                uint32_t val)
+{
+    writel(val, uart->regs + off);
+}
+
+static void linflex_uart_writeb(const struct linflex_uart *uart, uint32_t off,
+                                uint8_t val)
+{
+    writeb(val, uart->regs + off);
+}
+
+static uint32_t linflex_uart_get_osr(uint32_t uartcr)
+{
+    return (uartcr & UARTCR_OSR) >> UARTCR_OSR_SHIFT;
+}
+
+static bool linflex_uart_tx_fifo_mode(const struct linflex_uart *uart)
+{
+    return !!(linflex_uart_readl(uart, UARTCR) & UARTCR_TFBM);
+}
+
+static bool linflex_uart_rx_fifo_mode(const struct linflex_uart *uart)
+{
+    return !!(linflex_uart_readl(uart, UARTCR) & UARTCR_RFBM);
+}
+
+static uint32_t linflex_uart_ldiv_multiplier(const struct linflex_uart *uart)
+{
+    uint32_t uartcr, mul = LINFLEX_LDIV_MULTIPLIER;
+
+    uartcr = linflex_uart_readl(uart, UARTCR);
+    if ( uartcr & UARTCR_ROSE )
+        mul = linflex_uart_get_osr(uartcr);
+
+    return mul;
+}
+
+static void linflex_uart_flush(struct serial_port *port)
+{
+    const struct linflex_uart *uart = port->uart;
+
+    if ( linflex_uart_tx_fifo_mode(uart) )
+        while ( linflex_uart_readl(uart, UARTCR) & UARTCR_TDFLTFC )
+            cpu_relax();
+
+    if ( linflex_uart_rx_fifo_mode(uart) )
+        while ( linflex_uart_readl(uart, UARTCR) & UARTCR_RDFLRFC )
+            cpu_relax();
+}
+
+static void __init linflex_uart_init_preirq(struct serial_port *port)
+{
+    struct linflex_uart *uart = port->uart;
+    uint32_t ibr, fbr, divisr, dividr, ctrl;
+
+    /* Disable RX/TX before init mode */
+    ctrl = linflex_uart_readl(uart, UARTCR);
+    ctrl &= ~(UARTCR_RXEN | UARTCR_TXEN);
+    linflex_uart_writel(uart, UARTCR, ctrl);
+
+    /*
+     * Smoothen the transition from early_printk by waiting
+     * for all pending characters to transmit
+     */
+    linflex_uart_flush(port);
+
+    /* Init mode */
+    ctrl = LINCR1_INIT;
+    linflex_uart_writel(uart, LINCR1, ctrl);
+
+    /* Waiting for init mode entry */
+    while ( (linflex_uart_readl(uart, LINSR) & LINSR_LINS) != LINSR_LINS_INIT )
+        cpu_relax();
+
+    /* Set Master Mode */
+    ctrl |= LINCR1_MME;
+    linflex_uart_writel(uart, LINCR1, ctrl);
+
+    if ( uart->baud > LINFLEX_MAX_BAUDRATE )
+        uart->baud = LINFLEX_MAX_BAUDRATE;
+
+    /* Provide data bits, parity, stop bit, etc */
+    divisr = uart->clock_hz;
+    dividr = uart->baud * linflex_uart_ldiv_multiplier(uart);
+
+    ibr = divisr / dividr;
+    fbr = ((divisr % dividr) * 16 / dividr) & 0xF;
+
+    linflex_uart_writel(uart, LINIBRR, ibr);
+    linflex_uart_writel(uart, LINFBRR, fbr);
+
+    /* Set preset timeout register value */
+    linflex_uart_writel(uart, UARTPTO, 0xF);
+
+    /* Setting UARTCR[UART] bit is required for writing other bits in UARTCR */
+    linflex_uart_writel(uart, UARTCR, UARTCR_UART);
+
+    /* 8 bit data, no parity, UART mode, Buffer mode */
+    linflex_uart_writel(uart, UARTCR, UARTCR_PC1 | UARTCR_PC0 | UARTCR_WL0 |
+                        UARTCR_UART);
+
+    /* end init mode */
+    ctrl = linflex_uart_readl(uart, LINCR1);
+    ctrl &= ~LINCR1_INIT;
+    linflex_uart_writel(uart, LINCR1, ctrl);
+
+    /* Enable RX/TX after exiting init mode */
+    ctrl = linflex_uart_readl(uart, UARTCR);
+    ctrl |= UARTCR_RXEN | UARTCR_TXEN;
+    linflex_uart_writel(uart, UARTCR, ctrl);
+}
+
+static void linflex_uart_interrupt(int irq, void *data)
+{
+    struct serial_port *port = data;
+    const struct linflex_uart *uart = port->uart;
+    uint32_t sts;
+
+    sts = linflex_uart_readl(uart, UARTSR);
+
+    if ( sts & UARTSR_DRFRFE )
+        serial_rx_interrupt(port);
+
+    if ( sts & UARTSR_DTFTFF )
+        serial_tx_interrupt(port);
+}
+
+static void __init linflex_uart_init_postirq(struct serial_port *port)
+{
+    struct linflex_uart *uart = port->uart;
+    uint32_t temp;
+
+    uart->irqaction.handler = linflex_uart_interrupt;
+    uart->irqaction.name = "linflex_uart";
+    uart->irqaction.dev_id = port;
+
+    if ( setup_irq(uart->irq, 0, &uart->irqaction) != 0 )
+    {
+        printk("linflex-uart: Failed to allocate IRQ %d\n", uart->irq);
+        return;
+    }
+
+    /* Enable interrupts */
+    temp = linflex_uart_readl(uart, LINIER);
+    temp |= (LINIER_DRIE | LINIER_DTIE);
+    linflex_uart_writel(uart, LINIER, temp);
+    printk("linflex-uart: IRQ %d enabled\n", uart->irq);
+}
+
+static int linflex_uart_tx_ready(struct serial_port *port)
+{
+    const struct linflex_uart *uart = port->uart;
+
+    if ( linflex_uart_tx_fifo_mode(uart) )
+        return (linflex_uart_readl(uart, UARTSR) & UARTSR_DTFTFF) == 0 ? 1 : 0;
+
+    /*
+     * Buffer Mode => TX is waited to be ready after sending a char,
+     * so we can assume it is always ready before.
+     */
+    return 1;
+}
+
+static void linflex_uart_putc(struct serial_port *port, char c)
+{
+    const struct linflex_uart *uart = port->uart;
+    uint32_t uartsr;
+
+    if ( c == '\n' )
+        linflex_uart_putc(port, '\r');
+
+    linflex_uart_writeb(uart, BDRL, c);
+
+    /* Buffer Mode */
+    if ( !linflex_uart_tx_fifo_mode(uart) )
+    {
+        while ( (linflex_uart_readl(uart, UARTSR) & UARTSR_DTFTFF) == 0 )
+                cpu_relax();
+
+        uartsr = linflex_uart_readl(uart, UARTSR) | (UARTSR_DTFTFF);
+        linflex_uart_writel(uart, UARTSR, uartsr);
+    }
+}
+
+static int linflex_uart_getc(struct serial_port *port, char *pc)
+{
+    const struct linflex_uart *uart = port->uart;
+    uint32_t ch, uartsr, rx_fifo_mode;
+
+    rx_fifo_mode = linflex_uart_rx_fifo_mode(uart);
+
+    if ( rx_fifo_mode )
+        while ( linflex_uart_readl(uart, UARTSR) & UARTSR_DRFRFE )
+            cpu_relax();
+    else
+        while ( !(linflex_uart_readl(uart, UARTSR) & UARTSR_DRFRFE) )
+            cpu_relax();
+
+    ch = linflex_uart_readl(uart, BDRM);
+    *pc = ch & 0xff;
+
+    if ( !rx_fifo_mode ) {
+        uartsr = linflex_uart_readl(uart, UARTSR) | UARTSR_DRFRFE;
+        linflex_uart_writel(uart, UARTSR, uartsr);
+    }
+
+    return 1;
+}
+
+static int __init linflex_uart_irq(struct serial_port *port)
+{
+    const struct linflex_uart *uart = port->uart;
+
+    return ((uart->irq > 0) ? uart->irq : -1);
+}
+
+static const struct vuart_info *linflex_vuart_info(struct serial_port *port)
+{
+    const struct linflex_uart *uart = port->uart;
+
+    return &uart->vuart;
+}
+
+static void linflex_uart_start_tx(struct serial_port *port)
+{
+    const struct linflex_uart *uart = port->uart;
+    uint32_t temp;
+
+    temp = linflex_uart_readl(uart, LINIER);
+    linflex_uart_writel(uart, LINIER, temp | LINIER_DTIE);
+}
+
+static void linflex_uart_stop_tx(struct serial_port *port)
+{
+    const struct linflex_uart *uart = port->uart;
+    uint32_t temp;
+
+    temp = linflex_uart_readl(uart, LINIER);
+    temp &= ~(LINIER_DTIE);
+    linflex_uart_writel(uart, LINIER, temp);
+}
+
+static struct uart_driver __read_mostly linflex_uart_driver = {
+    .init_preirq = linflex_uart_init_preirq,
+    .init_postirq = linflex_uart_init_postirq,
+    .tx_ready = linflex_uart_tx_ready,
+    .putc = linflex_uart_putc,
+    .flush = linflex_uart_flush,
+    .getc = linflex_uart_getc,
+    .irq = linflex_uart_irq,
+    .start_tx = linflex_uart_start_tx,
+    .stop_tx = linflex_uart_stop_tx,
+    .vuart_info = linflex_vuart_info,
+};
+
+static int __init linflex_uart_init(struct dt_device_node *dev, const void *data)
+{
+    const char *config = data;
+    struct linflex_uart *uart;
+    paddr_t addr, size;
+    uint32_t baud = 0;
+    int res;
+
+    if ( strcmp(config, "") )
+    {
+        baud = simple_strtoul(config, &config, 10);
+        if ( strcmp(config, "") )
+            printk("linflex-uart: Only baud rate is configurable, discarding other options: %s\n",
+                   config);
+    }
+    else
+    {
+        /* Configuration not provided, use a default one */
+        baud = LINFLEX_BAUDRATE;
+        printk("linflex-uart: Baud rate not provided, using %d as default\n",
+               baud);
+    }
+
+    uart = &linflex_com;
+
+    res = dt_device_get_paddr(dev, 0, &addr, &size);
+    if ( res )
+    {
+        printk("linflex-uart: Unable to retrieve the base address of the UART\n");
+        return res;
+    }
+
+    res = platform_get_irq(dev, 0);
+    if ( res < 0 )
+    {
+        printk("linflex-uart: Unable to retrieve the IRQ\n");
+        return -EINVAL;
+    }
+    uart->irq = res;
+
+    uart->regs = ioremap_nocache(addr, size);
+    if ( !uart->regs )
+    {
+        printk("linflex-uart: Unable to map the UART memory\n");
+        return -ENOMEM;
+    }
+
+    uart->baud = baud;
+    uart->clock_hz = LINFLEX_CLK_FREQ;
+
+    uart->vuart.base_addr = addr;
+    uart->vuart.size = size;
+    uart->vuart.data_off = BDRL;
+    uart->vuart.status_off = UARTSR;
+    uart->vuart.status = UARTSR_DTFTFF;
+
+    /* Register with generic serial driver */
+    serial_register_uart(SERHND_DTUART, &linflex_uart_driver, uart);
+
+    dt_device_set_used_by(dev, DOMID_XEN);
+
+    return 0;
+}
+
+static const struct dt_device_match linflex_uart_dt_compat[] __initconst =
+{
+    DT_MATCH_COMPATIBLE("nxp,s32g2-linflexuart"),
+    DT_MATCH_COMPATIBLE("nxp,s32g3-linflexuart"),
+    DT_MATCH_COMPATIBLE("fsl,s32v234-linflexuart"),
+    { /* sentinel */ },
+};
+
+DT_DEVICE_START(linflex_uart, "NXP LINFlexD UART", DEVICE_SERIAL)
+    .dt_match = linflex_uart_dt_compat,
+    .init = linflex_uart_init,
+DT_DEVICE_END
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.45.2

[PATCH v2 2/8] xen/arm: Add NXP LINFlexD UART early printk support

[Andrei Cherechesu (OSS)]

From: Andrei Cherechesu <andrei.cherechesu@nxp.com>

This adds support for early printk debug via the NXP LINFlexD
UART controller.

Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
Signed-off-by: Peter van der Perk <peter.vander.perk@nxp.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/Kconfig.debug           | 12 ++++++
 xen/arch/arm/arm64/debug-linflex.inc | 58 ++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)
 create mode 100644 xen/arch/arm/arm64/debug-linflex.inc

diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug
index 2fa0acd2a3..7660e599c0 100644
--- a/xen/arch/arm/Kconfig.debug
+++ b/xen/arch/arm/Kconfig.debug
@@ -44,6 +44,14 @@ choice
 			Say Y here if you wish the early printk to direct their
 			output to a i.MX LPUART.
 
+	config EARLY_UART_CHOICE_LINFLEX
+		select EARLY_UART_LINFLEX
+		depends on ARM_64
+		bool "Early printk via NXP LINFlexD UART"
+		help
+			Say Y here if you wish the early printk to direct their
+			output to an NXP LINFlexD UART.
+
 	config EARLY_UART_CHOICE_MESON
 		select EARLY_UART_MESON
 		depends on ARM_64
@@ -89,6 +97,9 @@ config EARLY_UART_EXYNOS4210
 config EARLY_UART_IMX_LPUART
 	select EARLY_PRINTK
 	bool
+config EARLY_UART_LINFLEX
+	select EARLY_PRINTK
+	bool
 config EARLY_UART_MESON
 	select EARLY_PRINTK
 	bool
@@ -167,6 +178,7 @@ config EARLY_PRINTK_INC
 	default "debug-cadence.inc" if EARLY_UART_CADENCE
 	default "debug-exynos4210.inc" if EARLY_UART_EXYNOS4210
 	default "debug-imx-lpuart.inc" if EARLY_UART_IMX_LPUART
+	default "debug-linflex.inc" if EARLY_UART_LINFLEX
 	default "debug-meson.inc" if EARLY_UART_MESON
 	default "debug-mvebu.inc" if EARLY_UART_MVEBU
 	default "debug-pl011.inc" if EARLY_UART_PL011
diff --git a/xen/arch/arm/arm64/debug-linflex.inc b/xen/arch/arm/arm64/debug-linflex.inc
new file mode 100644
index 0000000000..cf4f5ce224
--- /dev/null
+++ b/xen/arch/arm/arm64/debug-linflex.inc
@@ -0,0 +1,58 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * xen/arch/arm/arm64/debug-linflex.inc
+ *
+ * NXP LINFlexD UART specific debug code
+ *
+ * Andrei Cherechesu <andrei.cherechesu@nxp.com>
+ * Copyright 2018, 2021, 2023-2024 NXP
+ */
+
+#include <asm/asm_defns.h>
+#include <asm/linflex-uart.h>
+
+/*
+ * wait LINFlexD UART to be ready to transmit
+ * xb: register which contains the UART base address
+ * c: scratch register number
+ */
+.macro early_uart_ready xb, c
+    ldr   w\c, [\xb, #UARTCR]       /* <= Control Register */
+    and   w\c, w\c, #UARTCR_TFBM    /* Check Buffer/FIFO (0/1) Mode */
+    cbz   w\c, 2f                   /* Buffer Mode => return */
+1:
+    ldrb  w\c, [\xb, #UARTSR]       /* <= Status Register */
+    tst   w\c, #UARTSR_DTFTFF       /* FIFO Mode => Check DTF bit */
+    b.ne  1b
+2:
+.endm
+
+/*
+ * LINFlexD UART transmit character
+ * xb: register which contains the UART base address
+ * wt: register which contains the character to transmit
+ */
+.macro early_uart_transmit xb, wt
+    strb  \wt, [\xb, #BDRL]
+
+    ldr   \wt, [\xb, #UARTCR]       /* <= Control Register */
+    and   \wt, \wt, #UARTCR_TFBM    /* Check Buffer/FIFO (0/1) Mode */
+    cbnz  \wt, 2f                   /* FIFO Mode => goto exit */
+
+3:  /* Buffer Mode */
+    ldrb  \wt, [\xb, #UARTSR]       /* <= Status Register */
+    and   \wt, \wt, #UARTSR_DTFTFF  /* Check Transmission Completed */
+    cbz   \wt, 3b
+
+    ldr   \wt, [\xb, #UARTSR]       /* <= Status Register */
+    orr   \wt, \wt, #UARTSR_DTFTFF  /* Clear DTF bit */
+    str   \wt, [\xb, #UARTSR]
+2:
+.endm
+
+/*
+ * Local variables:
+ * mode: ASM
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.45.2

[PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer

[Andrei Cherechesu (OSS)]

From: Andrei Cherechesu <andrei.cherechesu@nxp.com>

Introduce the SCMI layer to have some basic degree of awareness
about SMC calls that are based on the ARM System Control and
Management Interface (SCMI) specification (DEN0056E).

The SCMI specification includes various protocols for managing
system-level resources, such as: clocks, pins, reset, system power,
power domains, performance domains, etc. The clients are named
"SCMI agents" and the server is named "SCMI platform".

Only support the shared-memory based transport with SMCs as
the doorbell mechanism for notifying the platform. Also, this
implementation only handles the "arm,scmi-smc" compatible,
requiring the following properties:
	- "arm,smc-id" (unique SMC ID)
	- "shmem" (one or more phandles pointing to shmem zones
	for each channel)

The initialization is done as 'presmp_initcall', since we need
SMCs and PSCI should already probe EL3 FW for supporting SMCCC.
If no "arm,scmi-smc" compatible node is found in Dom0's
DT, the initialization fails silently, as it's not mandatory.
Otherwise, we get the 'arm,smc-id' DT property from the node,
to know the SCMI SMC ID we handle. The 'shmem' memory ranges
are not validated, as the SMC calls are only passed through
to EL3 FW if coming from Dom0 and as if Dom0 would be natively
running.

Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
---
 xen/arch/arm/Kconfig                |  10 ++
 xen/arch/arm/Makefile               |   1 +
 xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
 xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
 4 files changed, 226 insertions(+)
 create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
 create mode 100644 xen/arch/arm/scmi-smc.c

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 323c967361..adf53e2de1 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -245,6 +245,16 @@ config PARTIAL_EMULATION
 	  not been emulated to their complete functionality. Enabling this might
 	  result in unwanted/non-spec compliant behavior.
 
+config SCMI_SMC
+	bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
+	default y
+	help
+	  This option enables basic awareness for SCMI calls using SMC as
+	  doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
+	  compatible only). The value of "arm,smc-id" DT property from SCMI
+	  firmware node is used to trap and forward corresponding SCMI SMCs
+	  to firmware running at EL3, if the call comes from Dom0.
+
 endmenu
 
 menu "ARM errata workaround via the alternative framework"
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 7792bff597..b85ad9c13f 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
 obj-y += physdev.o
 obj-y += processor.o
 obj-y += psci.o
+obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
 obj-y += setup.o
 obj-y += shutdown.o
 obj-y += smp.o
diff --git a/xen/arch/arm/include/asm/scmi-smc.h b/xen/arch/arm/include/asm/scmi-smc.h
new file mode 100644
index 0000000000..c6c0079e86
--- /dev/null
+++ b/xen/arch/arm/include/asm/scmi-smc.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * xen/arch/arm/include/asm/scmi-smc.h
+ *
+ * ARM System Control and Management Interface (SCMI) over SMC
+ * Generic handling layer
+ *
+ * Andrei Cherechesu <andrei.cherechesu@nxp.com>
+ * Copyright 2024 NXP
+ */
+
+#ifndef __ASM_SCMI_SMC_H__
+#define __ASM_SCMI_SMC_H__
+
+#include <xen/types.h>
+#include <asm/regs.h>
+
+#ifdef CONFIG_SCMI_SMC
+
+bool scmi_is_enabled(void);
+bool scmi_is_valid_smc_id(uint32_t fid);
+bool scmi_handle_smc(struct cpu_user_regs *regs);
+
+#else
+
+static inline bool scmi_is_enabled(void)
+{
+    return false;
+}
+
+static inline bool scmi_is_valid_smc_id(uint32_t fid)
+{
+    return false;
+}
+
+static inline bool scmi_handle_smc(struct cpu_user_regs *regs)
+{
+    return false;
+}
+
+#endif /* CONFIG_SCMI_SMC */
+
+#endif /* __ASM_SCMI_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/scmi-smc.c b/xen/arch/arm/scmi-smc.c
new file mode 100644
index 0000000000..373ca7ba5f
--- /dev/null
+++ b/xen/arch/arm/scmi-smc.c
@@ -0,0 +1,163 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * xen/arch/arm/scmi-smc.c
+ *
+ * ARM System Control and Management Interface (SCMI) over SMC
+ * Generic handling layer
+ *
+ * Andrei Cherechesu <andrei.cherechesu@nxp.com>
+ * Copyright 2024 NXP
+ */
+
+#include <xen/acpi.h>
+#include <xen/device_tree.h>
+#include <xen/errno.h>
+#include <xen/init.h>
+#include <xen/sched.h>
+#include <xen/types.h>
+
+#include <asm/scmi-smc.h>
+#include <asm/smccc.h>
+
+#define SCMI_SMC_ID_PROP   "arm,smc-id"
+
+static bool scmi_support;
+static uint32_t scmi_smc_id;
+
+/* Check if SCMI layer correctly initialized and can be used. */
+bool scmi_is_enabled(void)
+{
+    return scmi_support;
+}
+
+/*
+ * Check if provided SMC Function Identifier matches the one known by the SCMI
+ * layer, as read from DT prop 'arm,smc-id' during initialiation.
+ */
+bool scmi_is_valid_smc_id(uint32_t fid)
+{
+    return (fid == scmi_smc_id);
+}
+
+/*
+ * Generic handler for SCMI-SMC requests, currently only forwarding the
+ * request to FW running at EL3 if it came from Dom0. Is called from the vSMC
+ * layer for SiP SMCs, since SCMI calls are usually provided this way.
+ * Can also be called from `platform_smc()` plat-specific callback.
+ *
+ * Returns true if SMC was handled (regardless of response), false otherwise.
+ */
+bool scmi_handle_smc(struct cpu_user_regs *regs)
+{
+    struct arm_smccc_res res;
+
+    /* Only the hardware domain should use SCMI calls */
+    if ( !is_hardware_domain(current->domain) )
+    {
+        gprintk(XENLOG_ERR, "SCMI: Unprivileged d%d cannot use SCMI.\n",
+                current->domain->domain_id);
+        return false;
+    }
+
+    /* For the moment, forward the SCMI Request to FW running at EL3 */
+    arm_smccc_1_1_smc(scmi_smc_id,
+                      get_user_reg(regs, 1),
+                      get_user_reg(regs, 2),
+                      get_user_reg(regs, 3),
+                      get_user_reg(regs, 4),
+                      get_user_reg(regs, 5),
+                      get_user_reg(regs, 6),
+                      get_user_reg(regs, 7),
+                      &res);
+
+    set_user_reg(regs, 0, res.a0);
+    set_user_reg(regs, 1, res.a1);
+    set_user_reg(regs, 2, res.a2);
+    set_user_reg(regs, 3, res.a3);
+
+    return true;
+}
+
+static int __init scmi_check_smccc_ver(void)
+{
+    if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
+    {
+        printk(XENLOG_ERR
+               "SCMI: No SMCCC 1.1 support, SCMI calls forwarding disabled\n");
+        return -ENOSYS;
+    }
+
+    return 0;
+}
+
+static int __init scmi_dt_init_smccc(void)
+{
+    static const struct dt_device_match scmi_ids[] __initconst =
+    {
+        /* We only support "arm,scmi-smc" binding for now */
+        DT_MATCH_COMPATIBLE("arm,scmi-smc"),
+        { /* sentinel */ },
+    };
+    const struct dt_device_node *scmi_node;
+    const char *smc_id_prop = SCMI_SMC_ID_PROP;
+    int ret;
+
+    /* If no SCMI firmware node found, fail silently as it's not mandatory */
+    scmi_node = dt_find_matching_node(NULL, scmi_ids);
+    if ( !scmi_node )
+        return -EOPNOTSUPP;
+
+    ret = dt_property_read_u32(scmi_node, smc_id_prop, &scmi_smc_id);
+    if ( !ret )
+    {
+        printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" DT node\n",
+               smc_id_prop, scmi_node->full_name);
+        return -ENOENT;
+    }
+
+    scmi_support = true;
+
+    return 0;
+}
+
+/* Initialize the SCMI layer based on SMCs and Device-tree */
+static int __init scmi_init(void)
+{
+    int ret;
+
+    if ( !acpi_disabled )
+    {
+        printk("SCMI is not supported when using ACPI\n");
+        return -EINVAL;
+    }
+
+    ret = scmi_check_smccc_ver();
+    if ( ret )
+        goto err;
+
+    ret = scmi_dt_init_smccc();
+    if ( ret == -EOPNOTSUPP )
+        return ret;
+    if ( ret )
+        goto err;
+
+    printk(XENLOG_INFO "Using SCMI with SMC ID: 0x%x\n", scmi_smc_id);
+
+    return 0;
+
+err:
+    printk(XENLOG_ERR "SCMI: Initialization failed (ret = %d)\n", ret);
+    return ret;
+}
+
+presmp_initcall(scmi_init);
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.45.2

[PATCH v2 4/8] xen/arm: vsmc: Enable handling SiP-owned SCMI SMC calls

[Andrei Cherechesu (OSS)]

From: Andrei Cherechesu <andrei.cherechesu@nxp.com>

Change the handling of SiP SMC calls to be more generic,
instead of directly relying on the `platform_smc()` callback
implementation.

Try to handle the SiP SMC first through the `platform_smc()`
callback (if implemented). If not handled, check if the
SCMI layer is available and that the SMC is a valid SCMI
message. Handle it then within the SCMI layer which forwards
it to EL3 FW, only if the SMC comes from Dom0.

Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
---
 xen/arch/arm/vsmc.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 7f2f5eb9ce..0de194a132 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -14,6 +14,7 @@
 #include <asm/cpufeature.h>
 #include <asm/monitor.h>
 #include <asm/regs.h>
+#include <asm/scmi-smc.h>
 #include <asm/smccc.h>
 #include <asm/tee/ffa.h>
 #include <asm/tee/tee.h>
@@ -224,6 +225,22 @@ static bool handle_sssc(struct cpu_user_regs *regs)
     }
 }
 
+/* Secure Calls defined by the Silicon Provider (SiP) */
+static bool handle_sip(struct cpu_user_regs *regs)
+{
+    uint32_t fid = (uint32_t)get_user_reg(regs, 0);
+
+    /* Firstly, let each platform define custom handling for these SMCs */
+    if ( platform_smc(regs) )
+        return true;
+
+    /* Otherwise, if valid SCMI SMC, forward the call to EL3 */
+    if ( scmi_is_enabled() && scmi_is_valid_smc_id(fid) )
+        return scmi_handle_smc(regs);
+
+    return false;
+}
+
 /*
  * vsmccc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
  * returns true if that was valid SMCCC call (even if function number
@@ -288,7 +305,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
             handled = handle_sssc(regs);
             break;
         case ARM_SMCCC_OWNER_SIP:
-            handled = platform_smc(regs);
+            handled = handle_sip(regs);
             break;
         case ARM_SMCCC_OWNER_TRUSTED_APP ... ARM_SMCCC_OWNER_TRUSTED_APP_END:
         case ARM_SMCCC_OWNER_TRUSTED_OS ... ARM_SMCCC_OWNER_TRUSTED_OS_END:
-- 
2.45.2

[PATCH v2 5/8] xen/arm: platforms: Add NXP S32CC platform code

[Andrei Cherechesu (OSS)]

From: Andrei Cherechesu <andrei.cherechesu@nxp.com>

Add code for NXP S32CC platforms (S32G2, S32G3, S32R45).

S32CC platforms use the NXP LINFlexD UART driver for
console by default, and rely on Dom0 having access to
SCMI services for system-level resources from firmware
at EL3.

Platform-related quirks will be addressed in following
commits.

Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
---
 xen/arch/arm/platforms/Kconfig  | 11 +++++++++++
 xen/arch/arm/platforms/Makefile |  1 +
 xen/arch/arm/platforms/s32cc.c  | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 44 insertions(+)
 create mode 100644 xen/arch/arm/platforms/s32cc.c

diff --git a/xen/arch/arm/platforms/Kconfig b/xen/arch/arm/platforms/Kconfig
index 76f7e76b1b..9837cba475 100644
--- a/xen/arch/arm/platforms/Kconfig
+++ b/xen/arch/arm/platforms/Kconfig
@@ -37,6 +37,14 @@ config MPSOC
 	help
 	Enable all the required drivers for Xilinx Ultrascale+ MPSoC
 
+config S32CC
+	bool "NXP S32CC platforms support"
+	depends on ARM_64
+	select HAS_LINFLEX
+	select SCMI_SMC
+	help
+	Enable all the required drivers for NXP S32CC platforms
+
 config NO_PLAT
 	bool "No Platforms"
 	help
@@ -56,3 +64,6 @@ config MPSOC_PLATFORM
 	bool
 	default (ALL64_PLAT || MPSOC)
 
+config S32CC_PLATFORM
+	bool
+	default (ALL64_PLAT || S32CC)
diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index bec6e55d1f..2c304b964d 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -10,5 +10,6 @@ obj-$(CONFIG_ALL64_PLAT) += thunderx.o
 obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
 obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
 obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
+obj-$(CONFIG_S32CC_PLATFORM)  += s32cc.o
 obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
 obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o
diff --git a/xen/arch/arm/platforms/s32cc.c b/xen/arch/arm/platforms/s32cc.c
new file mode 100644
index 0000000000..f99a2d56f6
--- /dev/null
+++ b/xen/arch/arm/platforms/s32cc.c
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * xen/arch/arm/platforms/s32cc.c
+ *
+ * NXP S32CC Platform-specific settings
+ *
+ * Andrei Cherechesu <andrei.cherechesu@nxp.com>
+ * Copyright 2021-2024 NXP
+ */
+
+#include <asm/platform.h>
+
+static const char * const s32cc_dt_compat[] __initconst =
+{
+    "nxp,s32g2",
+    "nxp,s32g3",
+    "nxp,s32r45",
+    NULL
+};
+
+PLATFORM_START(s32cc, "NXP S32CC")
+    .compatible = s32cc_dt_compat,
+PLATFORM_END
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.45.2

[PATCH v2 6/8] CHANGELOG.md: Add NXP S32CC and SCMI-SMC layer support mentions

[Andrei Cherechesu (OSS)]

From: Andrei Cherechesu <andrei.cherechesu@nxp.com>

Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
---
 CHANGELOG.md | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 26e7d8dd2a..66ea505843 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
    - Prefer ACPI reboot over UEFI ResetSystem() run time service call.
 
 ### Added
+  - On Arm:
+   - Add support for NXP S32CC platforms and NXP LINFlexD UART driver.
+   - Add basic handling for SCMI requests over SMC using Shared Memory,
+   by allowing forwarding the calls to EL3 FW if coming from Dom0.
 
 ### Removed
  - On x86:
-- 
2.45.2

[PATCH v2 7/8] SUPPORT.md: Describe SCMI-SMC layer feature

[Andrei Cherechesu (OSS)]

From: Andrei Cherechesu <andrei.cherechesu@nxp.com>

Describe the layer which enables SCMI over SMC calls forwarding
to EL3 FW if issued by Dom0. If the SCMI firmware node is not
found in Dom0's DT during initialization, it fails silently
as it's not mandatory.

The SCMI SMCs trapping at EL2 now lets Dom0 perform SCMI ops for
interacting with system-level resources almost as if it would be
running natively.

Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
---
 SUPPORT.md | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/SUPPORT.md b/SUPPORT.md
index 23dd7e6424..d8ffddfc44 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -927,6 +927,14 @@ Add/Remove device tree nodes using a device tree overlay binary (.dtbo).
 
     Status: Tech Preview
 
+### Arm: SCMI over SMC calls forwarding to EL3 Firmware
+
+Enable SCMI calls using SMC as doorbell mechanism and Shared Memory for
+transport ("arm,scmi-smc" compatible only) to reach EL3 Firmware if issued
+by Dom0. Some platforms use SCMI for access to system-level resources.
+
+    Status: Supported
+
 ## Virtual Hardware, QEMU
 
 This section describes supported devices available in HVM mode using a
-- 
2.45.2

[PATCH v2 8/8] MAINTAINERS: Add myself as maintainer for NXP S32CC

[Andrei Cherechesu (OSS)]

From: Andrei Cherechesu <andrei.cherechesu@nxp.com>

Add myself as maintainer for NXP S32CC SoCs Family,
and the S32 Linux Team as relevant reviewers list.

Also add the linflex-uart.c driver to the list of other
UART drivers in the ARM section.

Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index fb0ebf0939..2e273a5c78 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -249,6 +249,7 @@ F:	xen/drivers/char/arm-uart.c
 F:	xen/drivers/char/cadence-uart.c
 F:	xen/drivers/char/exynos4210-uart.c
 F:	xen/drivers/char/imx-lpuart.c
+F:	xen/drivers/char/linflex-uart.c
 F:	xen/drivers/char/meson-uart.c
 F:	xen/drivers/char/mvebu-uart.c
 F:	xen/drivers/char/omap-uart.c
@@ -428,6 +429,13 @@ L:	minios-devel@lists.xenproject.org
 T:	git https://xenbits.xenproject.org/git-http/mini-os.git
 F:	config/MiniOS.mk
 
+NXP S32CC FAMILY SUPPORT
+M:	Andrei Cherechesu <andrei.cherechesu@oss.nxp.com>
+L:	NXP S32 Linux Team <s32@nxp.com>
+F:	xen/arch/arm/include/asm/linflex-uart.h
+F:	xen/arch/arm/platforms/s32cc.c
+F:	xen/drivers/char/linflex-uart.c
+
 OCAML TOOLS
 M:	Christian Lindig <christian.lindig@citrix.com>
 M:	David Scott <dave@recoil.org>
-- 
2.45.2

Re: [PATCH v2 6/8] CHANGELOG.md: Add NXP S32CC and SCMI-SMC layer support mentions

[Jan Beulich]

On 30.09.2024 13:47, Andrei Cherechesu (OSS) wrote:
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -11,6 +11,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>     - Prefer ACPI reboot over UEFI ResetSystem() run time service call.
>  
>  ### Added
> +  - On Arm:

Nits (style): One less indenting blanks here, but ...

> +   - Add support for NXP S32CC platforms and NXP LINFlexD UART driver.
> +   - Add basic handling for SCMI requests over SMC using Shared Memory,
> +   by allowing forwarding the calls to EL3 FW if coming from Dom0.

... two more of them on this last line.

Jan

Re: [PATCH v2 6/8] CHANGELOG.md: Add NXP S32CC and SCMI-SMC layer support mentions

[oleksii.kurochko]

On Mon, 2024-09-30 at 14:53 +0200, Jan Beulich wrote:
> On 30.09.2024 13:47, Andrei Cherechesu (OSS) wrote:
> > --- a/CHANGELOG.md
> > +++ b/CHANGELOG.md
> > @@ -11,6 +11,10 @@ The format is based on [Keep a
> > Changelog](https://keepachangelog.com/en/1.0.0/)
> >     - Prefer ACPI reboot over UEFI ResetSystem() run time service
> > call.
> >  
> >  ### Added
> > +  - On Arm:
> 
> Nits (style): One less indenting blanks here, but ...
> 
> > +   - Add support for NXP S32CC platforms and NXP LINFlexD UART
> > driver.
> > +   - Add basic handling for SCMI requests over SMC using Shared
> > Memory,
> > +   by allowing forwarding the calls to EL3 FW if coming from Dom0.
> 
> ... two more of them on this last line.
With the suggested changes applied: Acked-by: Oleksii Kurochko
oleksii.kurochko@gmail.com

~ Oleksii

Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer

[Stefano Stabellini]

On Mon, 30 Sep 2024, Andrei Cherechesu (OSS) wrote:
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> Introduce the SCMI layer to have some basic degree of awareness
> about SMC calls that are based on the ARM System Control and
> Management Interface (SCMI) specification (DEN0056E).
> 
> The SCMI specification includes various protocols for managing
> system-level resources, such as: clocks, pins, reset, system power,
> power domains, performance domains, etc. The clients are named
> "SCMI agents" and the server is named "SCMI platform".
> 
> Only support the shared-memory based transport with SMCs as
> the doorbell mechanism for notifying the platform. Also, this
> implementation only handles the "arm,scmi-smc" compatible,
> requiring the following properties:
> 	- "arm,smc-id" (unique SMC ID)
> 	- "shmem" (one or more phandles pointing to shmem zones
> 	for each channel)
> 
> The initialization is done as 'presmp_initcall', since we need
> SMCs and PSCI should already probe EL3 FW for supporting SMCCC.
> If no "arm,scmi-smc" compatible node is found in Dom0's
> DT, the initialization fails silently, as it's not mandatory.
> Otherwise, we get the 'arm,smc-id' DT property from the node,
> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
> are not validated, as the SMC calls are only passed through
> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
> running.
> 
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/Kconfig                |  10 ++
>  xen/arch/arm/Makefile               |   1 +
>  xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>  xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
>  4 files changed, 226 insertions(+)
>  create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>  create mode 100644 xen/arch/arm/scmi-smc.c
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 323c967361..adf53e2de1 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>  	  not been emulated to their complete functionality. Enabling this might
>  	  result in unwanted/non-spec compliant behavior.
>  
> +config SCMI_SMC
> +	bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
> +	default y
> +	help
> +	  This option enables basic awareness for SCMI calls using SMC as
> +	  doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
> +	  compatible only). The value of "arm,smc-id" DT property from SCMI
> +	  firmware node is used to trap and forward corresponding SCMI SMCs
> +	  to firmware running at EL3, if the call comes from Dom0.
> +
>  endmenu
>  
>  menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 7792bff597..b85ad9c13f 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
>  obj-y += physdev.o
>  obj-y += processor.o
>  obj-y += psci.o
> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
>  obj-y += setup.o
>  obj-y += shutdown.o
>  obj-y += smp.o
> diff --git a/xen/arch/arm/include/asm/scmi-smc.h b/xen/arch/arm/include/asm/scmi-smc.h
> new file mode 100644
> index 0000000000..c6c0079e86
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/scmi-smc.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/include/asm/scmi-smc.h
> + *
> + * ARM System Control and Management Interface (SCMI) over SMC
> + * Generic handling layer
> + *
> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
> + * Copyright 2024 NXP
> + */
> +
> +#ifndef __ASM_SCMI_SMC_H__
> +#define __ASM_SCMI_SMC_H__
> +
> +#include <xen/types.h>
> +#include <asm/regs.h>
> +
> +#ifdef CONFIG_SCMI_SMC
> +
> +bool scmi_is_enabled(void);
> +bool scmi_is_valid_smc_id(uint32_t fid);
> +bool scmi_handle_smc(struct cpu_user_regs *regs);
> +
> +#else
> +
> +static inline bool scmi_is_enabled(void)
> +{
> +    return false;
> +}
> +
> +static inline bool scmi_is_valid_smc_id(uint32_t fid)
> +{
> +    return false;
> +}
> +
> +static inline bool scmi_handle_smc(struct cpu_user_regs *regs)
> +{
> +    return false;
> +}
> +
> +#endif /* CONFIG_SCMI_SMC */
> +
> +#endif /* __ASM_SCMI_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/scmi-smc.c b/xen/arch/arm/scmi-smc.c
> new file mode 100644
> index 0000000000..373ca7ba5f
> --- /dev/null
> +++ b/xen/arch/arm/scmi-smc.c
> @@ -0,0 +1,163 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/scmi-smc.c
> + *
> + * ARM System Control and Management Interface (SCMI) over SMC
> + * Generic handling layer
> + *
> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
> + * Copyright 2024 NXP
> + */
> +
> +#include <xen/acpi.h>
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/sched.h>
> +#include <xen/types.h>
> +
> +#include <asm/scmi-smc.h>
> +#include <asm/smccc.h>
> +
> +#define SCMI_SMC_ID_PROP   "arm,smc-id"
> +
> +static bool scmi_support;
> +static uint32_t scmi_smc_id;
> +
> +/* Check if SCMI layer correctly initialized and can be used. */
> +bool scmi_is_enabled(void)
> +{
> +    return scmi_support;
> +}
> +
> +/*
> + * Check if provided SMC Function Identifier matches the one known by the SCMI
> + * layer, as read from DT prop 'arm,smc-id' during initialiation.
> + */
> +bool scmi_is_valid_smc_id(uint32_t fid)
> +{
> +    return (fid == scmi_smc_id);
> +}
> +
> +/*
> + * Generic handler for SCMI-SMC requests, currently only forwarding the
> + * request to FW running at EL3 if it came from Dom0. Is called from the vSMC
> + * layer for SiP SMCs, since SCMI calls are usually provided this way.
> + * Can also be called from `platform_smc()` plat-specific callback.
> + *
> + * Returns true if SMC was handled (regardless of response), false otherwise.
> + */
> +bool scmi_handle_smc(struct cpu_user_regs *regs)
> +{
> +    struct arm_smccc_res res;
> +
> +    /* Only the hardware domain should use SCMI calls */
> +    if ( !is_hardware_domain(current->domain) )
> +    {
> +        gprintk(XENLOG_ERR, "SCMI: Unprivileged d%d cannot use SCMI.\n",
> +                current->domain->domain_id);
> +        return false;
> +    }
> +
> +    /* For the moment, forward the SCMI Request to FW running at EL3 */
> +    arm_smccc_1_1_smc(scmi_smc_id,
> +                      get_user_reg(regs, 1),
> +                      get_user_reg(regs, 2),
> +                      get_user_reg(regs, 3),
> +                      get_user_reg(regs, 4),
> +                      get_user_reg(regs, 5),
> +                      get_user_reg(regs, 6),
> +                      get_user_reg(regs, 7),
> +                      &res);
> +
> +    set_user_reg(regs, 0, res.a0);
> +    set_user_reg(regs, 1, res.a1);
> +    set_user_reg(regs, 2, res.a2);
> +    set_user_reg(regs, 3, res.a3);
> +
> +    return true;
> +}
> +
> +static int __init scmi_check_smccc_ver(void)
> +{
> +    if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
> +    {
> +        printk(XENLOG_ERR
> +               "SCMI: No SMCCC 1.1 support, SCMI calls forwarding disabled\n");
> +        return -ENOSYS;
> +    }
> +
> +    return 0;
> +}
> +
> +static int __init scmi_dt_init_smccc(void)
> +{
> +    static const struct dt_device_match scmi_ids[] __initconst =
> +    {
> +        /* We only support "arm,scmi-smc" binding for now */
> +        DT_MATCH_COMPATIBLE("arm,scmi-smc"),
> +        { /* sentinel */ },
> +    };
> +    const struct dt_device_node *scmi_node;
> +    const char *smc_id_prop = SCMI_SMC_ID_PROP;
> +    int ret;
> +
> +    /* If no SCMI firmware node found, fail silently as it's not mandatory */
> +    scmi_node = dt_find_matching_node(NULL, scmi_ids);
> +    if ( !scmi_node )
> +        return -EOPNOTSUPP;
> +
> +    ret = dt_property_read_u32(scmi_node, smc_id_prop, &scmi_smc_id);
> +    if ( !ret )
> +    {
> +        printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" DT node\n",
> +               smc_id_prop, scmi_node->full_name);
> +        return -ENOENT;
> +    }
> +
> +    scmi_support = true;
> +
> +    return 0;
> +}
> +
> +/* Initialize the SCMI layer based on SMCs and Device-tree */
> +static int __init scmi_init(void)
> +{
> +    int ret;
> +
> +    if ( !acpi_disabled )
> +    {
> +        printk("SCMI is not supported when using ACPI\n");
> +        return -EINVAL;
> +    }
> +
> +    ret = scmi_check_smccc_ver();
> +    if ( ret )
> +        goto err;
> +
> +    ret = scmi_dt_init_smccc();
> +    if ( ret == -EOPNOTSUPP )
> +        return ret;
> +    if ( ret )
> +        goto err;
> +
> +    printk(XENLOG_INFO "Using SCMI with SMC ID: 0x%x\n", scmi_smc_id);
> +
> +    return 0;
> +
> +err:
> +    printk(XENLOG_ERR "SCMI: Initialization failed (ret = %d)\n", ret);
> +    return ret;
> +}
> +
> +presmp_initcall(scmi_init);
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> -- 
> 2.45.2
> 

Re: [PATCH v2 4/8] xen/arm: vsmc: Enable handling SiP-owned SCMI SMC calls

[Stefano Stabellini]

On Mon, 30 Sep 2024, Andrei Cherechesu (OSS) wrote:
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> Change the handling of SiP SMC calls to be more generic,
> instead of directly relying on the `platform_smc()` callback
> implementation.
> 
> Try to handle the SiP SMC first through the `platform_smc()`
> callback (if implemented). If not handled, check if the
> SCMI layer is available and that the SMC is a valid SCMI
> message. Handle it then within the SCMI layer which forwards
> it to EL3 FW, only if the SMC comes from Dom0.
> 
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/vsmc.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 7f2f5eb9ce..0de194a132 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -14,6 +14,7 @@
>  #include <asm/cpufeature.h>
>  #include <asm/monitor.h>
>  #include <asm/regs.h>
> +#include <asm/scmi-smc.h>
>  #include <asm/smccc.h>
>  #include <asm/tee/ffa.h>
>  #include <asm/tee/tee.h>
> @@ -224,6 +225,22 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>      }
>  }
>  
> +/* Secure Calls defined by the Silicon Provider (SiP) */
> +static bool handle_sip(struct cpu_user_regs *regs)
> +{
> +    uint32_t fid = (uint32_t)get_user_reg(regs, 0);
> +
> +    /* Firstly, let each platform define custom handling for these SMCs */
> +    if ( platform_smc(regs) )
> +        return true;
> +
> +    /* Otherwise, if valid SCMI SMC, forward the call to EL3 */
> +    if ( scmi_is_enabled() && scmi_is_valid_smc_id(fid) )
> +        return scmi_handle_smc(regs);
> +
> +    return false;
> +}
> +
>  /*
>   * vsmccc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
>   * returns true if that was valid SMCCC call (even if function number
> @@ -288,7 +305,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
>              handled = handle_sssc(regs);
>              break;
>          case ARM_SMCCC_OWNER_SIP:
> -            handled = platform_smc(regs);
> +            handled = handle_sip(regs);
>              break;
>          case ARM_SMCCC_OWNER_TRUSTED_APP ... ARM_SMCCC_OWNER_TRUSTED_APP_END:
>          case ARM_SMCCC_OWNER_TRUSTED_OS ... ARM_SMCCC_OWNER_TRUSTED_OS_END:
> -- 
> 2.45.2
> 

Re: [PATCH v2 5/8] xen/arm: platforms: Add NXP S32CC platform code

[Stefano Stabellini]

On Mon, 30 Sep 2024, Andrei Cherechesu (OSS) wrote:
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> Add code for NXP S32CC platforms (S32G2, S32G3, S32R45).
> 
> S32CC platforms use the NXP LINFlexD UART driver for
> console by default, and rely on Dom0 having access to
> SCMI services for system-level resources from firmware
> at EL3.
> 
> Platform-related quirks will be addressed in following
> commits.
> 
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/platforms/Kconfig  | 11 +++++++++++
>  xen/arch/arm/platforms/Makefile |  1 +
>  xen/arch/arm/platforms/s32cc.c  | 32 ++++++++++++++++++++++++++++++++
>  3 files changed, 44 insertions(+)
>  create mode 100644 xen/arch/arm/platforms/s32cc.c
> 
> diff --git a/xen/arch/arm/platforms/Kconfig b/xen/arch/arm/platforms/Kconfig
> index 76f7e76b1b..9837cba475 100644
> --- a/xen/arch/arm/platforms/Kconfig
> +++ b/xen/arch/arm/platforms/Kconfig
> @@ -37,6 +37,14 @@ config MPSOC
>  	help
>  	Enable all the required drivers for Xilinx Ultrascale+ MPSoC
>  
> +config S32CC
> +	bool "NXP S32CC platforms support"
> +	depends on ARM_64
> +	select HAS_LINFLEX
> +	select SCMI_SMC
> +	help
> +	Enable all the required drivers for NXP S32CC platforms
> +
>  config NO_PLAT
>  	bool "No Platforms"
>  	help
> @@ -56,3 +64,6 @@ config MPSOC_PLATFORM
>  	bool
>  	default (ALL64_PLAT || MPSOC)
>  
> +config S32CC_PLATFORM
> +	bool
> +	default (ALL64_PLAT || S32CC)
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index bec6e55d1f..2c304b964d 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -10,5 +10,6 @@ obj-$(CONFIG_ALL64_PLAT) += thunderx.o
>  obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
>  obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
>  obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
> +obj-$(CONFIG_S32CC_PLATFORM)  += s32cc.o
>  obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
>  obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o
> diff --git a/xen/arch/arm/platforms/s32cc.c b/xen/arch/arm/platforms/s32cc.c
> new file mode 100644
> index 0000000000..f99a2d56f6
> --- /dev/null
> +++ b/xen/arch/arm/platforms/s32cc.c
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * xen/arch/arm/platforms/s32cc.c
> + *
> + * NXP S32CC Platform-specific settings
> + *
> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
> + * Copyright 2021-2024 NXP
> + */
> +
> +#include <asm/platform.h>
> +
> +static const char * const s32cc_dt_compat[] __initconst =
> +{
> +    "nxp,s32g2",
> +    "nxp,s32g3",
> +    "nxp,s32r45",
> +    NULL
> +};
> +
> +PLATFORM_START(s32cc, "NXP S32CC")
> +    .compatible = s32cc_dt_compat,
> +PLATFORM_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> -- 
> 2.45.2
> 

Re: [PATCH v2 6/8] CHANGELOG.md: Add NXP S32CC and SCMI-SMC layer support mentions

[Stefano Stabellini]

On Mon, 30 Sep 2024, Andrei Cherechesu (OSS) wrote:
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> ---
>  CHANGELOG.md | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 26e7d8dd2a..66ea505843 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -11,6 +11,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>     - Prefer ACPI reboot over UEFI ResetSystem() run time service call.
>  
>  ### Added
> +  - On Arm:
> +   - Add support for NXP S32CC platforms and NXP LINFlexD UART driver.
> +   - Add basic handling for SCMI requests over SMC using Shared Memory,
> +   by allowing forwarding the calls to EL3 FW if coming from Dom0.

Misalignment?

Other than that:

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


>  ### Removed
>   - On x86:
> -- 
> 2.45.2
> 
> 

Re: [PATCH v2 8/8] MAINTAINERS: Add myself as maintainer for NXP S32CC

[Stefano Stabellini]

On Mon, 30 Sep 2024, Andrei Cherechesu (OSS) wrote:
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> Add myself as maintainer for NXP S32CC SoCs Family,
> and the S32 Linux Team as relevant reviewers list.
> 
> Also add the linflex-uart.c driver to the list of other
> UART drivers in the ARM section.
> 
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  MAINTAINERS | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fb0ebf0939..2e273a5c78 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -249,6 +249,7 @@ F:	xen/drivers/char/arm-uart.c
>  F:	xen/drivers/char/cadence-uart.c
>  F:	xen/drivers/char/exynos4210-uart.c
>  F:	xen/drivers/char/imx-lpuart.c
> +F:	xen/drivers/char/linflex-uart.c

I would be also happy if you left this out from here and only have it
below under "NXP S32CC FAMILY SUPPORT"



>  F:	xen/drivers/char/meson-uart.c
>  F:	xen/drivers/char/mvebu-uart.c
>  F:	xen/drivers/char/omap-uart.c
> @@ -428,6 +429,13 @@ L:	minios-devel@lists.xenproject.org
>  T:	git https://xenbits.xenproject.org/git-http/mini-os.git
>  F:	config/MiniOS.mk
>  
> +NXP S32CC FAMILY SUPPORT
> +M:	Andrei Cherechesu <andrei.cherechesu@oss.nxp.com>
> +L:	NXP S32 Linux Team <s32@nxp.com>
> +F:	xen/arch/arm/include/asm/linflex-uart.h
> +F:	xen/arch/arm/platforms/s32cc.c
> +F:	xen/drivers/char/linflex-uart.c
> +
>  OCAML TOOLS
>  M:	Christian Lindig <christian.lindig@citrix.com>
>  M:	David Scott <dave@recoil.org>
> -- 
> 2.45.2
> 

Re: [PATCH v2 1/8] xen/arm: Add NXP LINFlexD UART Driver

[Julien Grall]

Hi Andrei,

On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
> +static void __init linflex_uart_init_preirq(struct serial_port *port)
> +{
> +    struct linflex_uart *uart = port->uart;
> +    uint32_t ibr, fbr, divisr, dividr, ctrl;
> +
> +    /* Disable RX/TX before init mode */
> +    ctrl = linflex_uart_readl(uart, UARTCR);
> +    ctrl &= ~(UARTCR_RXEN | UARTCR_TXEN);
> +    linflex_uart_writel(uart, UARTCR, ctrl);
> +
> +    /*
> +     * Smoothen the transition from early_printk by waiting
> +     * for all pending characters to transmit
> +     */

Just to note that early_printk() will still be used by secondary CPUs 
when booting which happens after init_preirq() is called. Will this be a 
problem for you?

I haven't compared the code against the specification. But the logic 
LGTM from a Xen PoV. So:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer

[Julien Grall]

On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> Introduce the SCMI layer to have some basic degree of awareness
> about SMC calls that are based on the ARM System Control and
> Management Interface (SCMI) specification (DEN0056E).
> 
> The SCMI specification includes various protocols for managing
> system-level resources, such as: clocks, pins, reset, system power,
> power domains, performance domains, etc. The clients are named
> "SCMI agents" and the server is named "SCMI platform".
> 
> Only support the shared-memory based transport with SMCs as
> the doorbell mechanism for notifying the platform. Also, this
> implementation only handles the "arm,scmi-smc" compatible,
> requiring the following properties:
> 	- "arm,smc-id" (unique SMC ID)
> 	- "shmem" (one or more phandles pointing to shmem zones
> 	for each channel)
> 
> The initialization is done as 'presmp_initcall', since we need
> SMCs and PSCI should already probe EL3 FW for supporting SMCCC.

presmp_initcall() should only be used when we really need to initialize 
a subsytem very early. But it is not clear why this can't be called in 
initcall. Can you clarify?

> If no "arm,scmi-smc" compatible node is found in Dom0's
> DT

You are checking the host device tree. Dom0's DT will be created by Xen 
based on the host device tree when the domain is ceated.

>, the initialization fails silently, as it's not mandatory.
> Otherwise, we get the 'arm,smc-id' DT property from the node,
> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
> are not validated, as the SMC calls are only passed through
> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
> running.
> 
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> ---
>   xen/arch/arm/Kconfig                |  10 ++
>   xen/arch/arm/Makefile               |   1 +
>   xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>   xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
>   4 files changed, 226 insertions(+)
>   create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>   create mode 100644 xen/arch/arm/scmi-smc.c
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 323c967361..adf53e2de1 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>   	  not been emulated to their complete functionality. Enabling this might
>   	  result in unwanted/non-spec compliant behavior.
>   
> +config SCMI_SMC
> +	bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"

Strictly speaking you are forwarding SMC from the hardware domain. For 
Arm, it is dom0 but it doesn't need to.

> +	default y
 > +	help> +	  This option enables basic awareness for SCMI calls using 
SMC as
> +	  doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
> +	  compatible only). The value of "arm,smc-id" DT property from SCMI
> +	  firmware node is used to trap and forward corresponding SCMI SMCs
> +	  to firmware running at EL3, if the call comes from Dom0.

Same here.

 > +>   endmenu
>   
>   menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 7792bff597..b85ad9c13f 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
>   obj-y += physdev.o
>   obj-y += processor.o
>   obj-y += psci.o
> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
>   obj-y += setup.o
>   obj-y += shutdown.o
>   obj-y += smp.o
> diff --git a/xen/arch/arm/include/asm/scmi-smc.h b/xen/arch/arm/include/asm/scmi-smc.h
> new file mode 100644
> index 0000000000..c6c0079e86
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/scmi-smc.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/include/asm/scmi-smc.h
> + *
> + * ARM System Control and Management Interface (SCMI) over SMC
> + * Generic handling layer
> + *
> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
> + * Copyright 2024 NXP
> + */
> +
> +#ifndef __ASM_SCMI_SMC_H__
> +#define __ASM_SCMI_SMC_H__
> +
> +#include <xen/types.h>
> +#include <asm/regs.h>
> +
> +#ifdef CONFIG_SCMI_SMC
> +
> +bool scmi_is_enabled(void);
> +bool scmi_is_valid_smc_id(uint32_t fid);
> +bool scmi_handle_smc(struct cpu_user_regs *regs);
> +
> +#else
> +
> +static inline bool scmi_is_enabled(void)
> +{
> +    return false;
> +}
> +
> +static inline bool scmi_is_valid_smc_id(uint32_t fid)
> +{
> +    return false;
> +}
> +
> +static inline bool scmi_handle_smc(struct cpu_user_regs *regs)
> +{
> +    return false;
> +}
> +
> +#endif /* CONFIG_SCMI_SMC */
> +
> +#endif /* __ASM_SCMI_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/scmi-smc.c b/xen/arch/arm/scmi-smc.c
> new file mode 100644
> index 0000000000..373ca7ba5f
> --- /dev/null
> +++ b/xen/arch/arm/scmi-smc.c
> @@ -0,0 +1,163 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/scmi-smc.c
> + *
> + * ARM System Control and Management Interface (SCMI) over SMC
> + * Generic handling layer
> + *
> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
> + * Copyright 2024 NXP
> + */
> +
> +#include <xen/acpi.h>
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/sched.h>
> +#include <xen/types.h>
> +
> +#include <asm/scmi-smc.h>
> +#include <asm/smccc.h>
> +
> +#define SCMI_SMC_ID_PROP   "arm,smc-id"
> +
> +static bool scmi_support;
> +static uint32_t scmi_smc_id;

AFAICT, the two variables should not change after boot. So they should 
be marked __ro_after_init.

> +
> +/* Check if SCMI layer correctly initialized and can be used. */
> +bool scmi_is_enabled(void)
> +{
> +    return scmi_support;
> +}
> +
> +/*
> + * Check if provided SMC Function Identifier matches the one known by the SCMI
> + * layer, as read from DT prop 'arm,smc-id' during initialiation.
> + */
> +bool scmi_is_valid_smc_id(uint32_t fid)
> +{
> +    return (fid == scmi_smc_id);
> +}
> +
> +/*
> + * Generic handler for SCMI-SMC requests, currently only forwarding the
> + * request to FW running at EL3 if it came from Dom0. Is called from the vSMC

s/dom0/hardware domain/

> + * layer for SiP SMCs, since SCMI calls are usually provided this way.
> + * Can also be called from `platform_smc()` plat-specific callback.

I am not entirely sure I understand the value of calling the function 
from platform_smc(). I also don't see any use of it in this series, so 
probably best to remove the sentence for now.

 > + *> + * Returns true if SMC was handled (regardless of response), 
false otherwise.
> + */
> +bool scmi_handle_smc(struct cpu_user_regs *regs)
> +{
> +    struct arm_smccc_res res;
> +
> +    /* Only the hardware domain should use SCMI calls */
> +    if ( !is_hardware_domain(current->domain) )
> +    {
> +        gprintk(XENLOG_ERR, "SCMI: Unprivileged d%d cannot use SCMI.\n",

Please use %pd instead d%d. With that you can simply use...

> +                current->domain->domain_id);

... current->domain here.

 > +        return false;> +    }
> +
> +    /* For the moment, forward the SCMI Request to FW running at EL3 */
> +    arm_smccc_1_1_smc(scmi_smc_id,
> +                      get_user_reg(regs, 1),
> +                      get_user_reg(regs, 2),
> +                      get_user_reg(regs, 3),
> +                      get_user_reg(regs, 4),
> +                      get_user_reg(regs, 5),
> +                      get_user_reg(regs, 6),
> +                      get_user_reg(regs, 7),
> +                      &res);
> +
> +    set_user_reg(regs, 0, res.a0);
> +    set_user_reg(regs, 1, res.a1);
> +    set_user_reg(regs, 2, res.a2);
> +    set_user_reg(regs, 3, res.a3);
> +
> +    return true;
> +}
> +
> +static int __init scmi_check_smccc_ver(void)
> +{
> +    if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
> +    {
> +        printk(XENLOG_ERR
 > +               "SCMI: No SMCCC 1.1 support, SCMI calls forwarding 
disabled\n");> +        return -ENOSYS;
> +    }
> +
> +    return 0;
> +}
> +
> +static int __init scmi_dt_init_smccc(void)
> +{
> +    static const struct dt_device_match scmi_ids[] __initconst =
> +    {
> +        /* We only support "arm,scmi-smc" binding for now */
> +        DT_MATCH_COMPATIBLE("arm,scmi-smc"),
> +        { /* sentinel */ },
> +    };
> +    const struct dt_device_node *scmi_node;
> +    const char *smc_id_prop = SCMI_SMC_ID_PROP;

NIT: I would remove smc_id_prop and use SCMI_SMC_ID_PROP directly.

> +    int ret;
> +
> +    /* If no SCMI firmware node found, fail silently as it's not mandatory */
 > +    scmi_node = dt_find_matching_node(NULL, scmi_ids);> +    if ( 
!scmi_node )
> +        return -EOPNOTSUPP;
> +
> +    ret = dt_property_read_u32(scmi_node, smc_id_prop, &scmi_smc_id);
> +    if ( !ret )
> +    {
> +        printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" DT node\n",
> +               smc_id_prop, scmi_node->full_name);
> +        return -ENOENT;
> +    }
> +
> +    scmi_support = true;
> +
> +    return 0;
> +}
> +
> +/* Initialize the SCMI layer based on SMCs and Device-tree */
> +static int __init scmi_init(void)
> +{
> +    int ret;
> +
> +    if ( !acpi_disabled )
> +    {
> +        printk("SCMI is not supported when using ACPI\n");

NIT: Can you use XENLOG_WARN in front?

> +        return -EINVAL;
> +    }
> +
> +    ret = scmi_check_smccc_ver();
> +    if ( ret )
> +        goto err;
> +
> +    ret = scmi_dt_init_smccc();
> +    if ( ret == -EOPNOTSUPP )
> +        return ret;
> +    if ( ret )
> +        goto err;
> +
> +    printk(XENLOG_INFO "Using SCMI with SMC ID: 0x%x\n", scmi_smc_id);
> +
> +    return 0;
> +
> +err:
> +    printk(XENLOG_ERR "SCMI: Initialization failed (ret = %d)\n", ret);

In the commit message, you said the SCMI subsystem was optional. But 
here you use XENLOG_ERR. Shouldn't it be a warn or XENLOG_INFO/XENLOG_WARN?

> +    return ret;
> +}
> +
> +presmp_initcall(scmi_init);

See my question above about using __initcall.

> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Cheers,

-- 
Julien Grall

Re: [PATCH v2 4/8] xen/arm: vsmc: Enable handling SiP-owned SCMI SMC calls

[Julien Grall]

Hi,

On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> Change the handling of SiP SMC calls to be more generic,
> instead of directly relying on the `platform_smc()` callback
> implementation.
> 
> Try to handle the SiP SMC first through the `platform_smc()`
> callback (if implemented). If not handled, check if the
> SCMI layer is available and that the SMC is a valid SCMI
> message. Handle it then within the SCMI layer which forwards
> it to EL3 FW, only if the SMC comes from Dom0.

NIT: I would remove the last sentence as this is implementation details. 
But if you want to keep it, then s/Dom0/Hardware domain/

> 
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> ---
>   xen/arch/arm/vsmc.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 7f2f5eb9ce..0de194a132 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -14,6 +14,7 @@
>   #include <asm/cpufeature.h>
>   #include <asm/monitor.h>
>   #include <asm/regs.h>
> +#include <asm/scmi-smc.h>
>   #include <asm/smccc.h>
>   #include <asm/tee/ffa.h>
>   #include <asm/tee/tee.h>
> @@ -224,6 +225,22 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>       }
>   }
>   
> +/* Secure Calls defined by the Silicon Provider (SiP) */
> +static bool handle_sip(struct cpu_user_regs *regs)
> +{
> +    uint32_t fid = (uint32_t)get_user_reg(regs, 0);
> +
> +    /* Firstly, let each platform define custom handling for these SMCs */
> +    if ( platform_smc(regs) )
> +        return true;
> +
> +    /* Otherwise, if valid SCMI SMC, forward the call to EL3 */

This comment is likely going to stale. This is up to smci_handle_smc() 
to decide what to do. So I would remove this comment.

With that:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer

[Julien Grall]

Hi Andrei,

On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
> +/*
> + * Generic handler for SCMI-SMC requests, currently only forwarding the
> + * request to FW running at EL3 if it came from Dom0. Is called from the vSMC
> + * layer for SiP SMCs, since SCMI calls are usually provided this way.
> + * Can also be called from `platform_smc()` plat-specific callback.
> + *
> + * Returns true if SMC was handled (regardless of response), false otherwise.
> + */
> +bool scmi_handle_smc(struct cpu_user_regs *regs)
> +{
> +    struct arm_smccc_res res;
> +
> +    /* Only the hardware domain should use SCMI calls */
> +    if ( !is_hardware_domain(current->domain) )
> +    {
> +        gprintk(XENLOG_ERR, "SCMI: Unprivileged d%d cannot use SCMI.\n",
> +                current->domain->domain_id);
> +        return false;
> +    }
> +
> +    /* For the moment, forward the SCMI Request to FW running at EL3 */
> +    arm_smccc_1_1_smc(scmi_smc_id,

Actually, shouldn't this be get_user_reg(regs, 0) so we don't rely on 
scmi_handle_smc() to be called only when this is equal to scmi_smc_id?

The other option is to move the check for scmi_smc_id in this function.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 5/8] xen/arm: platforms: Add NXP S32CC platform code

[Julien Grall]

Hi Andrei,

On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> Add code for NXP S32CC platforms (S32G2, S32G3, S32R45).
> 
> S32CC platforms use the NXP LINFlexD UART driver for
> console by default, and rely on Dom0 having access to
> SCMI services for system-level resources from firmware
> at EL3.
> 
> Platform-related quirks will be addressed in following
> commits.

I don't see any specific quirks in the series. Are you intended to send 
follow-up?

[...]

> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index bec6e55d1f..2c304b964d 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -10,5 +10,6 @@ obj-$(CONFIG_ALL64_PLAT) += thunderx.o
>   obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
>   obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
>   obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
> +obj-$(CONFIG_S32CC_PLATFORM)  += s32cc.o
>   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
>   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o
> diff --git a/xen/arch/arm/platforms/s32cc.c b/xen/arch/arm/platforms/s32cc.c
> new file mode 100644
> index 0000000000..f99a2d56f6
> --- /dev/null
> +++ b/xen/arch/arm/platforms/s32cc.c

We only add a new platform if there are platform specific hook to 
implement. AFAICT, you don't implement any, so it should not be necessary.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 7/8] SUPPORT.md: Describe SCMI-SMC layer feature

[Julien Grall]

Hi,

On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> Describe the layer which enables SCMI over SMC calls forwarding
> to EL3 FW if issued by Dom0. 

s/Dom0/Hardware domain/ and everywhere else below.

> If the SCMI firmware node is not
> found in Dom0's DT during initialization, it fails silently
> as it's not mandatory.

You are looking for the node in the host's DT.

> 
> The SCMI SMCs trapping at EL2 now lets Dom0 perform SCMI ops for
> interacting with system-level resources almost as if it would be
> running natively.
> 
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> ---
>   SUPPORT.md | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/SUPPORT.md b/SUPPORT.md
> index 23dd7e6424..d8ffddfc44 100644
> --- a/SUPPORT.md
> +++ b/SUPPORT.md
> @@ -927,6 +927,14 @@ Add/Remove device tree nodes using a device tree overlay binary (.dtbo).
>   
>       Status: Tech Preview
>   
> +### Arm: SCMI over SMC calls forwarding to EL3 Firmware
> +
> +Enable SCMI calls using SMC as doorbell mechanism and Shared Memory for
> +transport ("arm,scmi-smc" compatible only) to reach EL3 Firmware if issued
> +by Dom0. Some platforms use SCMI for access to system-level resources.
> +
> +    Status: Supported

Bertrand, can you confirm this should be ok to support it (includingn 
security support)?

Cheers,

-- 
Julien Grall

Re: [PATCH v2 1/8] xen/arm: Add NXP LINFlexD UART Driver

[Andrei Cherechesu]

Hi Julien,

On 01/10/2024 12:20, Julien Grall wrote:
> Hi Andrei,
>
> On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
>> +static void __init linflex_uart_init_preirq(struct serial_port *port)
>> +{
>> +    struct linflex_uart *uart = port->uart;
>> +    uint32_t ibr, fbr, divisr, dividr, ctrl;
>> +
>> +    /* Disable RX/TX before init mode */
>> +    ctrl = linflex_uart_readl(uart, UARTCR);
>> +    ctrl &= ~(UARTCR_RXEN | UARTCR_TXEN);
>> +    linflex_uart_writel(uart, UARTCR, ctrl);
>> +
>> +    /*
>> +     * Smoothen the transition from early_printk by waiting
>> +     * for all pending characters to transmit
>> +     */
>
> Just to note that early_printk() will still be used by secondary CPUs when booting which happens after init_preirq() is called. Will this be a problem for you?
>

No, there's no problem with that. At the end of init_preirq() the
UART can still be used via early_printk(). I know the secondary
cores have a mechanism to print some messages themselves when
being brought up, otherwise their initialization fails. 

But they're being enabled correctly in our case:
(XEN) Bringing up CPU1
- CPU 0000000000000001 booting -
- Current EL 0000000000000008 -
- Initialize CPU -
- Turning on paging -
- Paging turned on -
- Ready -
(XEN) GICv3: CPU1: Found redistributor in region 0 @00000a004003e000
(XEN) CPU1: Guest atomics will try 13 times before pausing the domain
(XEN) CPU 1 booted.
(XEN) Bringing up CPU2
- CPU 0000000000000002 booting -
- Current EL 0000000000000008 -
- Initialize CPU -
- Turning on paging -
- Paging turned on -
- Ready -
(XEN) GICv3: CPU2: Found redistributor in region 0 @00000a004005e000
(XEN) CPU2: Guest atomics will try 13 times before pausing the domain
(XEN) CPU 2 booted.
(XEN) Bringing up CPU3
...

> I haven't compared the code against the specification. But the logic LGTM from a Xen PoV. So:
>
> Acked-by: Julien Grall <jgrall@amazon.com>
>
> Cheers,
>

Thank you for the review!

Regards,
Andrei Cherechesu

Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer

[Andrei Cherechesu]

Hi Julien,


On 01/10/2024 12:35, Julien Grall wrote:
>
>
> On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>
>> Introduce the SCMI layer to have some basic degree of awareness
>> about SMC calls that are based on the ARM System Control and
>> Management Interface (SCMI) specification (DEN0056E).
>>
>> The SCMI specification includes various protocols for managing
>> system-level resources, such as: clocks, pins, reset, system power,
>> power domains, performance domains, etc. The clients are named
>> "SCMI agents" and the server is named "SCMI platform".
>>
>> Only support the shared-memory based transport with SMCs as
>> the doorbell mechanism for notifying the platform. Also, this
>> implementation only handles the "arm,scmi-smc" compatible,
>> requiring the following properties:
>>     - "arm,smc-id" (unique SMC ID)
>>     - "shmem" (one or more phandles pointing to shmem zones
>>     for each channel)
>>
>> The initialization is done as 'presmp_initcall', since we need
>> SMCs and PSCI should already probe EL3 FW for supporting SMCCC.
>
> presmp_initcall() should only be used when we really need to initialize a subsytem very early. But it is not clear why this can't be called in initcall. Can you clarify?

Right, I will change it to __initcall(), as there's no specific reason
to do it as a presmp_initcall().

The commit message was meant to imply that PSCI initialization is
a prerequisite (because it also initializes SMC capabilities,
which IMO should be decoupled from PSCI).

But of course there shouldn't be any hard deadline to initializing
SCMI, if obviously we're doing it before Dom0 starts booting.

>
>> If no "arm,scmi-smc" compatible node is found in Dom0's
>> DT
>
> You are checking the host device tree. Dom0's DT will be created by Xen based on the host device tree when the domain is ceated.

Indeed, I will change it to Host DT.

My choice of words came from the train of thought that Dom0
is the closest to a native Linux, which would have the SCMI
firmware node as part of its DT. And Dom0 basically gets
most of the original host device tree by default, except
some special nodes. But I will change it.

Thanks for the suggestion.

>
>> , the initialization fails silently, as it's not mandatory.
>> Otherwise, we get the 'arm,smc-id' DT property from the node,
>> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
>> are not validated, as the SMC calls are only passed through
>> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
>> running.
>>
>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>> ---
>>   xen/arch/arm/Kconfig                |  10 ++
>>   xen/arch/arm/Makefile               |   1 +
>>   xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>>   xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
>>   4 files changed, 226 insertions(+)
>>   create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>>   create mode 100644 xen/arch/arm/scmi-smc.c
>>
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index 323c967361..adf53e2de1 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>>         not been emulated to their complete functionality. Enabling this might
>>         result in unwanted/non-spec compliant behavior.
>>   +config SCMI_SMC
>> +    bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
>
> Strictly speaking you are forwarding SMC from the hardware domain. For Arm, it is dom0 but it doesn't need to.

Well, SCMI is Arm-specific and so are SMCs, but I agree to change
to "hardware domain" / "hwdom" in order to keep the language generic.

Will update everywhere.

>
>> +    default y
> > +    help> +      This option enables basic awareness for SCMI calls using SMC as
>> +      doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
>> +      compatible only). The value of "arm,smc-id" DT property from SCMI
>> +      firmware node is used to trap and forward corresponding SCMI SMCs
>> +      to firmware running at EL3, if the call comes from Dom0.
>
> Same here.

Will update.

>
> > +>   endmenu
>>     menu "ARM errata workaround via the alternative framework"
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 7792bff597..b85ad9c13f 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
>>   obj-y += physdev.o
>>   obj-y += processor.o
>>   obj-y += psci.o
>> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
>>   obj-y += setup.o
>>   obj-y += shutdown.o
>>   obj-y += smp.o
>> diff --git a/xen/arch/arm/include/asm/scmi-smc.h b/xen/arch/arm/include/asm/scmi-smc.h
>> new file mode 100644
>> index 0000000000..c6c0079e86
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/scmi-smc.h
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * xen/arch/arm/include/asm/scmi-smc.h
>> + *
>> + * ARM System Control and Management Interface (SCMI) over SMC
>> + * Generic handling layer
>> + *
>> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
>> + * Copyright 2024 NXP
>> + */
>> +
>> +#ifndef __ASM_SCMI_SMC_H__
>> +#define __ASM_SCMI_SMC_H__
>> +
>> +#include <xen/types.h>
>> +#include <asm/regs.h>
>> +
>> +#ifdef CONFIG_SCMI_SMC
>> +
>> +bool scmi_is_enabled(void);
>> +bool scmi_is_valid_smc_id(uint32_t fid);
>> +bool scmi_handle_smc(struct cpu_user_regs *regs);
>> +
>> +#else
>> +
>> +static inline bool scmi_is_enabled(void)
>> +{
>> +    return false;
>> +}
>> +
>> +static inline bool scmi_is_valid_smc_id(uint32_t fid)
>> +{
>> +    return false;
>> +}
>> +
>> +static inline bool scmi_handle_smc(struct cpu_user_regs *regs)
>> +{
>> +    return false;
>> +}
>> +
>> +#endif /* CONFIG_SCMI_SMC */
>> +
>> +#endif /* __ASM_SCMI_H__ */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/scmi-smc.c b/xen/arch/arm/scmi-smc.c
>> new file mode 100644
>> index 0000000000..373ca7ba5f
>> --- /dev/null
>> +++ b/xen/arch/arm/scmi-smc.c
>> @@ -0,0 +1,163 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * xen/arch/arm/scmi-smc.c
>> + *
>> + * ARM System Control and Management Interface (SCMI) over SMC
>> + * Generic handling layer
>> + *
>> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
>> + * Copyright 2024 NXP
>> + */
>> +
>> +#include <xen/acpi.h>
>> +#include <xen/device_tree.h>
>> +#include <xen/errno.h>
>> +#include <xen/init.h>
>> +#include <xen/sched.h>
>> +#include <xen/types.h>
>> +
>> +#include <asm/scmi-smc.h>
>> +#include <asm/smccc.h>
>> +
>> +#define SCMI_SMC_ID_PROP   "arm,smc-id"
>> +
>> +static bool scmi_support;
>> +static uint32_t scmi_smc_id;
>
> AFAICT, the two variables should not change after boot. So they should be marked __ro_after_init.

Right, will update.

>
>
>> +
>> +/* Check if SCMI layer correctly initialized and can be used. */
>> +bool scmi_is_enabled(void)
>> +{
>> +    return scmi_support;
>> +}
>> +
>> +/*
>> + * Check if provided SMC Function Identifier matches the one known by the SCMI
>> + * layer, as read from DT prop 'arm,smc-id' during initialiation.
>> + */
>> +bool scmi_is_valid_smc_id(uint32_t fid)
>> +{
>> +    return (fid == scmi_smc_id);
>> +}
>> +
>> +/*
>> + * Generic handler for SCMI-SMC requests, currently only forwarding the
>> + * request to FW running at EL3 if it came from Dom0. Is called from the vSMC
>
> s/dom0/hardware domain/

Will update.

>
>
>> + * layer for SiP SMCs, since SCMI calls are usually provided this way.
>> + * Can also be called from `platform_smc()` plat-specific callback.
>
> I am not entirely sure I understand the value of calling the function from platform_smc(). I also don't see any use of it in this series, so probably best to remove the sentence for now.

Will remove. It doesn't need to be called from platform_smc(),
since it's a call chain.

>
>
> > + *> + * Returns true if SMC was handled (regardless of response), false otherwise.
>> + */
>> +bool scmi_handle_smc(struct cpu_user_regs *regs)
>> +{
>> +    struct arm_smccc_res res;
>> +
>> +    /* Only the hardware domain should use SCMI calls */
>> +    if ( !is_hardware_domain(current->domain) )
>> +    {
>> +        gprintk(XENLOG_ERR, "SCMI: Unprivileged d%d cannot use SCMI.\n",
>
> Please use %pd instead d%d. With that you can simply use...

Will update.

>
>> +                current->domain->domain_id);
>
> ... current->domain here.

Will update.

>
> > +        return false;> +    }
>> +
>> +    /* For the moment, forward the SCMI Request to FW running at EL3 */
>> +    arm_smccc_1_1_smc(scmi_smc_id,
>> +                      get_user_reg(regs, 1),
>> +                      get_user_reg(regs, 2),
>> +                      get_user_reg(regs, 3),
>> +                      get_user_reg(regs, 4),
>> +                      get_user_reg(regs, 5),
>> +                      get_user_reg(regs, 6),
>> +                      get_user_reg(regs, 7),
>> +                      &res);
>> +
>> +    set_user_reg(regs, 0, res.a0);
>> +    set_user_reg(regs, 1, res.a1);
>> +    set_user_reg(regs, 2, res.a2);
>> +    set_user_reg(regs, 3, res.a3);
>> +
>> +    return true;
>> +}
>> +
>> +static int __init scmi_check_smccc_ver(void)
>> +{
>> +    if ( smccc_ver < ARM_SMCCC_VERSION_1_1 )
>> +    {
>> +        printk(XENLOG_ERR
> > +               "SCMI: No SMCCC 1.1 support, SCMI calls forwarding disabled\n");> +        return -ENOSYS;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int __init scmi_dt_init_smccc(void)
>> +{
>> +    static const struct dt_device_match scmi_ids[] __initconst =
>> +    {
>> +        /* We only support "arm,scmi-smc" binding for now */
>> +        DT_MATCH_COMPATIBLE("arm,scmi-smc"),
>> +        { /* sentinel */ },
>> +    };
>> +    const struct dt_device_node *scmi_node;
>> +    const char *smc_id_prop = SCMI_SMC_ID_PROP;
>
> NIT: I would remove smc_id_prop and use SCMI_SMC_ID_PROP directly.

Sure, will update.

>
>
>> +    int ret;
>> +
>> +    /* If no SCMI firmware node found, fail silently as it's not mandatory */
> > +    scmi_node = dt_find_matching_node(NULL, scmi_ids);> +    if ( !scmi_node )
>> +        return -EOPNOTSUPP;
>> +
>> +    ret = dt_property_read_u32(scmi_node, smc_id_prop, &scmi_smc_id);
>> +    if ( !ret )
>> +    {
>> +        printk(XENLOG_ERR "SCMI: No valid \"%s\" property in \"%s\" DT node\n",
>> +               smc_id_prop, scmi_node->full_name);
>> +        return -ENOENT;
>> +    }
>> +
>> +    scmi_support = true;
>> +
>> +    return 0;
>> +}
>> +
>> +/* Initialize the SCMI layer based on SMCs and Device-tree */
>> +static int __init scmi_init(void)
>> +{
>> +    int ret;
>> +
>> +    if ( !acpi_disabled )
>> +    {
>> +        printk("SCMI is not supported when using ACPI\n");
>
> NIT: Can you use XENLOG_WARN in front?

Will add.

>
>
>> +        return -EINVAL;
>> +    }
>> +
>> +    ret = scmi_check_smccc_ver();
>> +    if ( ret )
>> +        goto err;
>> +
>> +    ret = scmi_dt_init_smccc();
>> +    if ( ret == -EOPNOTSUPP )
>> +        return ret;
>> +    if ( ret )
>> +        goto err;
>> +
>> +    printk(XENLOG_INFO "Using SCMI with SMC ID: 0x%x\n", scmi_smc_id);
>> +
>> +    return 0;
>> +
>> +err:
>> +    printk(XENLOG_ERR "SCMI: Initialization failed (ret = %d)\n", ret);
>
> In the commit message, you said the SCMI subsystem was optional. But here you use XENLOG_ERR. Shouldn't it be a warn or XENLOG_INFO/XENLOG_WARN?

Well, SCMI is optional, in the sense that if we don't find the
SCMI firmware node in the host DT, we exit silently (-EOPNOTSUPP)
and we return right away (no error printed).

But if we do find the SCMI node, it means that we should initialize
the SCMI subsystem, right? And if we're trying to do that and we
find that the DT node is not correctly formatted (i.e. the
'arm,smc-id' property), I think we should print an error.

However, I think we shouldn't print an error if we return
with an error code from scmi_check_smccc_ver(). And the print
inside scmi_check_smccc_ver() should be a XENLOG_WARN.

So, I think we should print a XENLOG_ERR only if we figured
we need to initialize, and we started to do it but it failed.

What do you think?

>
>> +    return ret;
>> +}
>> +
>> +presmp_initcall(scmi_init);
>
> See my question above about using __initcall.

As mentioned, will change to __initcall.

>
>
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>
> Cheers,
>

Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer

[Andrei Cherechesu]

Hi Julien,


On 01/10/2024 12:59, Julien Grall wrote:
> Hi Andrei,
>
> On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
>> +/*
>> + * Generic handler for SCMI-SMC requests, currently only forwarding the
>> + * request to FW running at EL3 if it came from Dom0. Is called from the vSMC
>> + * layer for SiP SMCs, since SCMI calls are usually provided this way.
>> + * Can also be called from `platform_smc()` plat-specific callback.
>> + *
>> + * Returns true if SMC was handled (regardless of response), false otherwise.
>> + */
>> +bool scmi_handle_smc(struct cpu_user_regs *regs)
>> +{
>> +    struct arm_smccc_res res;
>> +
>> +    /* Only the hardware domain should use SCMI calls */
>> +    if ( !is_hardware_domain(current->domain) )
>> +    {
>> +        gprintk(XENLOG_ERR, "SCMI: Unprivileged d%d cannot use SCMI.\n",
>> +                current->domain->domain_id);
>> +        return false;
>> +    }
>> +
>> +    /* For the moment, forward the SCMI Request to FW running at EL3 */
>> +    arm_smccc_1_1_smc(scmi_smc_id,
>
> Actually, shouldn't this be get_user_reg(regs, 0) so we don't rely on scmi_handle_smc() to be called only when this is equal to scmi_smc_id?
>
> The other option is to move the check for scmi_smc_id in this function.

I'll move the check for scmi_smc_id in this function and make it static,
to avoid exposing unnecessary complexity to the users of this layer.

Thanks for the suggestion & review.

>
> Cheers,
>

Regards,
Andrei Cherechesu

Re: [PATCH v2 4/8] xen/arm: vsmc: Enable handling SiP-owned SCMI SMC calls

[Andrei Cherechesu]

Hi Julien,


On 01/10/2024 12:46, Julien Grall wrote:
> Hi,
>
> On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>
>> Change the handling of SiP SMC calls to be more generic,
>> instead of directly relying on the `platform_smc()` callback
>> implementation.
>>
>> Try to handle the SiP SMC first through the `platform_smc()`
>> callback (if implemented). If not handled, check if the
>> SCMI layer is available and that the SMC is a valid SCMI
>> message. Handle it then within the SCMI layer which forwards
>> it to EL3 FW, only if the SMC comes from Dom0.
>
> NIT: I would remove the last sentence as this is implementation details. But if you want to keep it, then s/Dom0/Hardware domain/

Agree, I'll remove it.

>
>
>>
>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>> ---
>>   xen/arch/arm/vsmc.c | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
>> index 7f2f5eb9ce..0de194a132 100644
>> --- a/xen/arch/arm/vsmc.c
>> +++ b/xen/arch/arm/vsmc.c
>> @@ -14,6 +14,7 @@
>>   #include <asm/cpufeature.h>
>>   #include <asm/monitor.h>
>>   #include <asm/regs.h>
>> +#include <asm/scmi-smc.h>
>>   #include <asm/smccc.h>
>>   #include <asm/tee/ffa.h>
>>   #include <asm/tee/tee.h>
>> @@ -224,6 +225,22 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>>       }
>>   }
>>   +/* Secure Calls defined by the Silicon Provider (SiP) */
>> +static bool handle_sip(struct cpu_user_regs *regs)
>> +{
>> +    uint32_t fid = (uint32_t)get_user_reg(regs, 0);
>> +
>> +    /* Firstly, let each platform define custom handling for these SMCs */
>> +    if ( platform_smc(regs) )
>> +        return true;
>> +
>> +    /* Otherwise, if valid SCMI SMC, forward the call to EL3 */
>
> This comment is likely going to stale. This is up to smci_handle_smc() to decide what to do. So I would remove this comment.

I'll remove this, since the behaviour is up to the SCMI SMC layer.

>
>
> With that:
>
> Acked-by: Julien Grall <jgrall@amazon.com>

Thanks for the review.

>
> Cheers,
>

Regards,
Andrei C

Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer

[Julien Grall]

On 03/10/2024 16:27, Andrei Cherechesu wrote:
> Hi Julien,

Hi Andrei,

> On 01/10/2024 12:35, Julien Grall wrote:
>>
>>> , the initialization fails silently, as it's not mandatory.
>>> Otherwise, we get the 'arm,smc-id' DT property from the node,
>>> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
>>> are not validated, as the SMC calls are only passed through
>>> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
>>> running.
>>>
>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>> ---
>>>    xen/arch/arm/Kconfig                |  10 ++
>>>    xen/arch/arm/Makefile               |   1 +
>>>    xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>>>    xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
>>>    4 files changed, 226 insertions(+)
>>>    create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>>>    create mode 100644 xen/arch/arm/scmi-smc.c
>>>
>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>> index 323c967361..adf53e2de1 100644
>>> --- a/xen/arch/arm/Kconfig
>>> +++ b/xen/arch/arm/Kconfig
>>> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>>>          not been emulated to their complete functionality. Enabling this might
>>>          result in unwanted/non-spec compliant behavior.
>>>    +config SCMI_SMC
>>> +    bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
>>
>> Strictly speaking you are forwarding SMC from the hardware domain. For Arm, it is dom0 but it doesn't need to.
> 
> Well, SCMI is Arm-specific and so are SMCs, but I agree to change
> to "hardware domain" / "hwdom" in order to keep the language generic.

To clarify, I meant that it would be possible to have an hardware domain 
on Arm. This is not used/implemented today but most of the code is 
adhering to the language. The only reason where we would use dom0 is 
when we explicitely check for domain_id 0 in the code.

[...]

>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    ret = scmi_check_smccc_ver();
>>> +    if ( ret )
>>> +        goto err;
>>> +
>>> +    ret = scmi_dt_init_smccc();
>>> +    if ( ret == -EOPNOTSUPP )
>>> +        return ret;
>>> +    if ( ret )
>>> +        goto err;
>>> +
>>> +    printk(XENLOG_INFO "Using SCMI with SMC ID: 0x%x\n", scmi_smc_id);
>>> +
>>> +    return 0;
>>> +
>>> +err:
>>> +    printk(XENLOG_ERR "SCMI: Initialization failed (ret = %d)\n", ret);
>>
>> In the commit message, you said the SCMI subsystem was optional. But here you use XENLOG_ERR. Shouldn't it be a warn or XENLOG_INFO/XENLOG_WARN?
> 
> Well, SCMI is optional, in the sense that if we don't find the
> SCMI firmware node in the host DT, we exit silently (-EOPNOTSUPP)
> and we return right away (no error printed).
> 
> But if we do find the SCMI node, it means that we should initialize
> the SCMI subsystem, right? And if we're trying to do that and we
> find that the DT node is not correctly formatted (i.e. the
> 'arm,smc-id' property), I think we should print an error.

Correct me if I am wrong, but from the doc, I understood that the 
property arm,smc-id is only necessary if the transport is SMC/HVC.

So I don't think we should print an error if it is not found as this is 
effectively optional.

I agree...

> 
> However, I think we shouldn't print an error if we return
> with an error code from scmi_check_smccc_ver(). And the print
> inside scmi_check_smccc_ver() should be a XENLOG_WARN.
> 
> So, I think we should print a XENLOG_ERR only if we figured
> we need to initialize, and we started to do it but it failed.

... with the two other points.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer

[Andrei Cherechesu]

Hi Julien,

On 03/10/2024 19:07, Julien Grall wrote:
>
>
> On 03/10/2024 16:27, Andrei Cherechesu wrote:
>> Hi Julien,
>
> Hi Andrei,
>
>> On 01/10/2024 12:35, Julien Grall wrote:
>>>
>>>> , the initialization fails silently, as it's not mandatory.
>>>> Otherwise, we get the 'arm,smc-id' DT property from the node,
>>>> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
>>>> are not validated, as the SMC calls are only passed through
>>>> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
>>>> running.
>>>>
>>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>> ---
>>>>    xen/arch/arm/Kconfig                |  10 ++
>>>>    xen/arch/arm/Makefile               |   1 +
>>>>    xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>>>>    xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
>>>>    4 files changed, 226 insertions(+)
>>>>    create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>>>>    create mode 100644 xen/arch/arm/scmi-smc.c
>>>>
>>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>>> index 323c967361..adf53e2de1 100644
>>>> --- a/xen/arch/arm/Kconfig
>>>> +++ b/xen/arch/arm/Kconfig
>>>> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>>>>          not been emulated to their complete functionality. Enabling this might
>>>>          result in unwanted/non-spec compliant behavior.
>>>>    +config SCMI_SMC
>>>> +    bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
>>>
>>> Strictly speaking you are forwarding SMC from the hardware domain. For Arm, it is dom0 but it doesn't need to.
>>
>> Well, SCMI is Arm-specific and so are SMCs, but I agree to change
>> to "hardware domain" / "hwdom" in order to keep the language generic.
>
> To clarify, I meant that it would be possible to have an hardware domain on Arm. This is not used/implemented today but most of the code is adhering to the language. The only reason where we would use dom0 is when we explicitely check for domain_id 0 in the code.
>
> [...]

Thank you for the clarification.

>
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    ret = scmi_check_smccc_ver();
>>>> +    if ( ret )
>>>> +        goto err;
>>>> +
>>>> +    ret = scmi_dt_init_smccc();
>>>> +    if ( ret == -EOPNOTSUPP )
>>>> +        return ret;
>>>> +    if ( ret )
>>>> +        goto err;
>>>> +
>>>> +    printk(XENLOG_INFO "Using SCMI with SMC ID: 0x%x\n", scmi_smc_id);
>>>> +
>>>> +    return 0;
>>>> +
>>>> +err:
>>>> +    printk(XENLOG_ERR "SCMI: Initialization failed (ret = %d)\n", ret);
>>>
>>> In the commit message, you said the SCMI subsystem was optional. But here you use XENLOG_ERR. Shouldn't it be a warn or XENLOG_INFO/XENLOG_WARN?
>>
>> Well, SCMI is optional, in the sense that if we don't find the
>> SCMI firmware node in the host DT, we exit silently (-EOPNOTSUPP)
>> and we return right away (no error printed).
>>
>> But if we do find the SCMI node, it means that we should initialize
>> the SCMI subsystem, right? And if we're trying to do that and we
>> find that the DT node is not correctly formatted (i.e. the
>> 'arm,smc-id' property), I think we should print an error.
>
> Correct me if I am wrong, but from the doc, I understood that the property arm,smc-id is only necessary if the transport is SMC/HVC.
>
> So I don't think we should print an error if it is not found as this is effectively optional.

I believe your understanding is correct, the 'arm,smc-id' property
is needed if the SCMI firmware node has either "arm,scmi-smc" or
"arm,scmi-smc-param" compatibles [0]. We only support the
"arm,scmi-smc" compatible with our implementation, though, as you
mentioned there should not be any addresses passed in the regs
alongside the SMC call. That would need to happen with the
"arm,scmi-smc-param" compatible.

But, by "if we do find the SCMI firmware node" I effectively meant
"if we find the node in Host DT with 'arm,scmi-smc' compatible",
since we're only implementing this specific variant. And that's
why I implied that a valid 'arm,smc-id' property is equivalent to
having a correctly defined SCMI firmware node. Having found the
"arm,scmi-smc" compatible node (which IMO means we should commit to
initializing the SCMI layer), shouldn't we print if we tried to init
the SCMI layer and failed (due to incorrect formatting of the node)?

I hope I explained it better now. If there's still anything unclear,
let me know.
 
Anyway, I'm fine either way regarding the prints. If we consider SCMI
layer initialization optional we could also not print any errors if
it fails (no matter why). Correct me if I'm wrong, but IIUC that is
what you're suggesting.

[0] https://elixir.bootlin.com/linux/v6.11/source/Documentation/devicetree/bindings/firmware/arm,scmi.yaml#L359

>
> I agree...
>
>>
>> However, I think we shouldn't print an error if we return
>> with an error code from scmi_check_smccc_ver(). And the print
>> inside scmi_check_smccc_ver() should be a XENLOG_WARN.
>>
>> So, I think we should print a XENLOG_ERR only if we figured
>> we need to initialize, and we started to do it but it failed.
>
> ... with the two other points.
>
> Cheers,
>

Regards,
Andrei C

Re: [PATCH v2 5/8] xen/arm: platforms: Add NXP S32CC platform code

[Andrei Cherechesu]

Hi Julien, Stefano,

On 01/10/2024 13:01, Julien Grall wrote:
> Hi Andrei,
>
> On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>
>> Add code for NXP S32CC platforms (S32G2, S32G3, S32R45).
>>
>> S32CC platforms use the NXP LINFlexD UART driver for
>> console by default, and rely on Dom0 having access to
>> SCMI services for system-level resources from firmware
>> at EL3.
>>
>> Platform-related quirks will be addressed in following
>> commits.
>
> I don't see any specific quirks in the series. Are you intended to send follow-up?
>
> [...]

Well, we could use an opinion on some SoC erratum implementation
and if it would be acceptable for you or not, from any of
the ARM maintainers.

The erratum is about some TLB Invalidate (tlbi) instruction
variants (by Virtual Address) which are handled incorrectly
in some cases (for some VAs), and that need to be replaced
with broader versions.

It doesn't apply for all S32CC platforms, but we can use the
Host DT compatible to identify this. My idea is that we define
a platform quirk for this and check for it using
platform_has_quirk().
Then, we either:
    - modify the definition for the affected 'tlbi' variant
    in arm64/flushtlb.h to actually execute the broader one
    if needed
or:
    - define a new wrapper for the tlbi variant we want to
    replace the affected one with
    - check for this quirk before its usage and call the
    more potent version if needed (there's only one
    occurrence of usage for the affected version).

Number 2 seems better to me, it's more customizable and
it's similar to the existing handling for
PLATFORM_QUIRK_GIC_64K_STRIDE for some other Arm platform.

Do you have any suggestion/preference regarding how this
should be implemented?


>
>
>> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
>> index bec6e55d1f..2c304b964d 100644
>> --- a/xen/arch/arm/platforms/Makefile
>> +++ b/xen/arch/arm/platforms/Makefile
>> @@ -10,5 +10,6 @@ obj-$(CONFIG_ALL64_PLAT) += thunderx.o
>>   obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
>>   obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
>>   obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
>> +obj-$(CONFIG_S32CC_PLATFORM)  += s32cc.o
>>   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
>>   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o
>> diff --git a/xen/arch/arm/platforms/s32cc.c b/xen/arch/arm/platforms/s32cc.c
>> new file mode 100644
>> index 0000000000..f99a2d56f6
>> --- /dev/null
>> +++ b/xen/arch/arm/platforms/s32cc.c
>
> We only add a new platform if there are platform specific hook to implement. AFAICT, you don't implement any, so it should not be necessary.

Like I mentioned above, there's some erratum workaround which
could make use of the platform_quirk() callback, that we want
to send in the near future.

>
> Cheers,
>

Thank you for the review and support once again,

Regards,
Andrei C

Re: [PATCH v2 5/8] xen/arm: platforms: Add NXP S32CC platform code

[Julien Grall]

On 04/10/2024 16:37, Andrei Cherechesu wrote:
> Hi Julien, Stefano,

Hi Andrei,

> 
> On 01/10/2024 13:01, Julien Grall wrote:
>> Hi Andrei,
>>
>> On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>
>>> Add code for NXP S32CC platforms (S32G2, S32G3, S32R45).
>>>
>>> S32CC platforms use the NXP LINFlexD UART driver for
>>> console by default, and rely on Dom0 having access to
>>> SCMI services for system-level resources from firmware
>>> at EL3.
>>>
>>> Platform-related quirks will be addressed in following
>>> commits.
>>
>> I don't see any specific quirks in the series. Are you intended to send follow-up?
>>
>> [...]
> 
> Well, we could use an opinion on some SoC erratum implementation
> and if it would be acceptable for you or not, from any of
> the ARM maintainers.
> 
> The erratum is about some TLB Invalidate (tlbi) instruction
> variants (by Virtual Address) which are handled incorrectly
> in some cases (for some VAs), and that need to be replaced
> with broader versions.

Can you provide more details:
   1. Is this a processor issue?
   2. Which VAs are affected?
   3. What is the impact if the erratum is hit?
   3. Do we need to do anything on the behalf of the guests?

> It doesn't apply for all S32CC platforms, but we can use the
> Host DT compatible to identify this. My idea is that we define
> a platform quirk for this and check for it using
> platform_has_quirk().
 > Then, we either:>      - modify the definition for the affected 
'tlbi' variant
>      in arm64/flushtlb.h to actually execute the broader one
>      if needed
> or:
>      - define a new wrapper for the tlbi variant we want to
>      replace the affected one with
>      - check for this quirk before its usage and call the
>      more potent version if needed (there's only one
>      occurrence of usage for the affected version).
> 
> Number 2 seems better to me, it's more customizable and
> it's similar to the existing handling for
> PLATFORM_QUIRK_GIC_64K_STRIDE for some other Arm platform.

I need a bit more details first (see my questions above). But if there 
are not many VAs affected, then we may be able to re-order the Xen 
memory layout to avoid the VAs. So we wouldn't need any specific change 
in the TLB flush operations.

>>> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
>>> index bec6e55d1f..2c304b964d 100644
>>> --- a/xen/arch/arm/platforms/Makefile
>>> +++ b/xen/arch/arm/platforms/Makefile
>>> @@ -10,5 +10,6 @@ obj-$(CONFIG_ALL64_PLAT) += thunderx.o
>>>    obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
>>>    obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
>>>    obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
>>> +obj-$(CONFIG_S32CC_PLATFORM)  += s32cc.o
>>>    obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
>>>    obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o
>>> diff --git a/xen/arch/arm/platforms/s32cc.c b/xen/arch/arm/platforms/s32cc.c
>>> new file mode 100644
>>> index 0000000000..f99a2d56f6
>>> --- /dev/null
>>> +++ b/xen/arch/arm/platforms/s32cc.c
>>
>> We only add a new platform if there are platform specific hook to implement. AFAICT, you don't implement any, so it should not be necessary.
> 
> Like I mentioned above, there's some erratum workaround which
> could make use of the platform_quirk() callback, that we want
> to send in the near future.

I think it would be best to introduce platforms/s32cc.c at that point.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer

[Julien Grall]

Hi,

On 03/10/2024 17:40, Andrei Cherechesu wrote:
> Hi Julien,
> 
> On 03/10/2024 19:07, Julien Grall wrote:
>>
>>
>> On 03/10/2024 16:27, Andrei Cherechesu wrote:
>>> Hi Julien,
>>
>> Hi Andrei,
>>
>>> On 01/10/2024 12:35, Julien Grall wrote:
>>>>
>>>>> , the initialization fails silently, as it's not mandatory.
>>>>> Otherwise, we get the 'arm,smc-id' DT property from the node,
>>>>> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
>>>>> are not validated, as the SMC calls are only passed through
>>>>> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
>>>>> running.
>>>>>
>>>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>> ---
>>>>>     xen/arch/arm/Kconfig                |  10 ++
>>>>>     xen/arch/arm/Makefile               |   1 +
>>>>>     xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>>>>>     xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
>>>>>     4 files changed, 226 insertions(+)
>>>>>     create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>>>>>     create mode 100644 xen/arch/arm/scmi-smc.c
>>>>>
>>>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>>>> index 323c967361..adf53e2de1 100644
>>>>> --- a/xen/arch/arm/Kconfig
>>>>> +++ b/xen/arch/arm/Kconfig
>>>>> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>>>>>           not been emulated to their complete functionality. Enabling this might
>>>>>           result in unwanted/non-spec compliant behavior.
>>>>>     +config SCMI_SMC
>>>>> +    bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
>>>>
>>>> Strictly speaking you are forwarding SMC from the hardware domain. For Arm, it is dom0 but it doesn't need to.
>>>
>>> Well, SCMI is Arm-specific and so are SMCs, but I agree to change
>>> to "hardware domain" / "hwdom" in order to keep the language generic.
>>
>> To clarify, I meant that it would be possible to have an hardware domain on Arm. This is not used/implemented today but most of the code is adhering to the language. The only reason where we would use dom0 is when we explicitely check for domain_id 0 in the code.
>>
>> [...]
> 
> Thank you for the clarification.
> 
>>
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    ret = scmi_check_smccc_ver();
>>>>> +    if ( ret )
>>>>> +        goto err;
>>>>> +
>>>>> +    ret = scmi_dt_init_smccc();
>>>>> +    if ( ret == -EOPNOTSUPP )
>>>>> +        return ret;
>>>>> +    if ( ret )
>>>>> +        goto err;
>>>>> +
>>>>> +    printk(XENLOG_INFO "Using SCMI with SMC ID: 0x%x\n", scmi_smc_id);
>>>>> +
>>>>> +    return 0;
>>>>> +
>>>>> +err:
>>>>> +    printk(XENLOG_ERR "SCMI: Initialization failed (ret = %d)\n", ret);
>>>>
>>>> In the commit message, you said the SCMI subsystem was optional. But here you use XENLOG_ERR. Shouldn't it be a warn or XENLOG_INFO/XENLOG_WARN?
>>>
>>> Well, SCMI is optional, in the sense that if we don't find the
>>> SCMI firmware node in the host DT, we exit silently (-EOPNOTSUPP)
>>> and we return right away (no error printed).
>>>
>>> But if we do find the SCMI node, it means that we should initialize
>>> the SCMI subsystem, right? And if we're trying to do that and we
>>> find that the DT node is not correctly formatted (i.e. the
>>> 'arm,smc-id' property), I think we should print an error.
>>
>> Correct me if I am wrong, but from the doc, I understood that the property arm,smc-id is only necessary if the transport is SMC/HVC.
>>
>> So I don't think we should print an error if it is not found as this is effectively optional.
> 
> I believe your understanding is correct, the 'arm,smc-id' property
> is needed if the SCMI firmware node has either "arm,scmi-smc" or
> "arm,scmi-smc-param" compatibles [0]. We only support the
> "arm,scmi-smc" compatible with our implementation, though, as you
> mentioned there should not be any addresses passed in the regs
> alongside the SMC call. That would need to happen with the
> "arm,scmi-smc-param" compatible.
> 
> But, by "if we do find the SCMI firmware node" I effectively meant
> "if we find the node in Host DT with 'arm,scmi-smc' compatible",
> since we're only implementing this specific variant. And that's
> why I implied that a valid 'arm,smc-id' property is equivalent to
> having a correctly defined SCMI firmware node. Having found the
> "arm,scmi-smc" compatible node (which IMO means we should commit to
> initializing the SCMI layer), shouldn't we print if we tried to init
> the SCMI layer and failed (due to incorrect formatting of the node)?
> 
> I hope I explained it better now.

Yes. Thanks for the clarification!

  If there's still anything unclear,
> let me know.

It is now clear. For I agree this should be an error if we can't find 
the property arm,smc-id.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 5/8] xen/arm: platforms: Add NXP S32CC platform code

[Andrei Cherechesu]

Hi Julien,

On 04/10/2024 19:24, Julien Grall wrote:
>
>
> On 04/10/2024 16:37, Andrei Cherechesu wrote:
>> Hi Julien, Stefano,
>
> Hi Andrei,
>
>>
>> On 01/10/2024 13:01, Julien Grall wrote:
>>> Hi Andrei,
>>>
>>> On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
>>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>
>>>> Add code for NXP S32CC platforms (S32G2, S32G3, S32R45).
>>>>
>>>> S32CC platforms use the NXP LINFlexD UART driver for
>>>> console by default, and rely on Dom0 having access to
>>>> SCMI services for system-level resources from firmware
>>>> at EL3.
>>>>
>>>> Platform-related quirks will be addressed in following
>>>> commits.
>>>
>>> I don't see any specific quirks in the series. Are you intended to send follow-up?
>>>
>>> [...]
>>
>> Well, we could use an opinion on some SoC erratum implementation
>> and if it would be acceptable for you or not, from any of
>> the ARM maintainers.
>>
>> The erratum is about some TLB Invalidate (tlbi) instruction
>> variants (by Virtual Address) which are handled incorrectly
>> in some cases (for some VAs), and that need to be replaced
>> with broader versions.
>
> Can you provide more details:
>   1. Is this a processor issue?
>   2. Which VAs are affected?
>   3. What is the impact if the erratum is hit?
>   3. Do we need to do anything on the behalf of the guests?

Sure:
1. This is an SoC issue, present in a subset of the S32CC processors family.
2. VAs whose bits [48:41] are not all zero.
3. Well, TLB corruption.
4. There are instructions affected at both levels 1 and 2 of translation.
The guest will have to work its own way around it.

In Xen, just `tlbi vae2is` (only used in flush_xen_tlb_range_va()) seems to
be affected, and if we go with the 2nd approach from the ones proposed,
it should not be very ugly to implement. I am of course open to any of your
suggestions.

>
>
>> It doesn't apply for all S32CC platforms, but we can use the
>> Host DT compatible to identify this. My idea is that we define
>> a platform quirk for this and check for it using
>> platform_has_quirk().
> > Then, we either:>      - modify the definition for the affected 'tlbi' variant
>>      in arm64/flushtlb.h to actually execute the broader one
>>      if needed
>> or:
>>      - define a new wrapper for the tlbi variant we want to
>>      replace the affected one with
>>      - check for this quirk before its usage and call the
>>      more potent version if needed (there's only one
>>      occurrence of usage for the affected version).
>>
>> Number 2 seems better to me, it's more customizable and
>> it's similar to the existing handling for
>> PLATFORM_QUIRK_GIC_64K_STRIDE for some other Arm platform.
>
> I need a bit more details first (see my questions above). But if there are not many VAs affected, then we may be able to re-order the Xen memory layout to avoid the VAs. So we wouldn't need any specific change in the TLB flush operations.

I'm not sure what to say here, we would need to have the layout
limited under the mentioned VA range in our case, right? What
about the GVA layout?

>
>>>> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
>>>> index bec6e55d1f..2c304b964d 100644
>>>> --- a/xen/arch/arm/platforms/Makefile
>>>> +++ b/xen/arch/arm/platforms/Makefile
>>>> @@ -10,5 +10,6 @@ obj-$(CONFIG_ALL64_PLAT) += thunderx.o
>>>>    obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
>>>>    obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
>>>>    obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
>>>> +obj-$(CONFIG_S32CC_PLATFORM)  += s32cc.o
>>>>    obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
>>>>    obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp-eemi.o
>>>> diff --git a/xen/arch/arm/platforms/s32cc.c b/xen/arch/arm/platforms/s32cc.c
>>>> new file mode 100644
>>>> index 0000000000..f99a2d56f6
>>>> --- /dev/null
>>>> +++ b/xen/arch/arm/platforms/s32cc.c
>>>
>>> We only add a new platform if there are platform specific hook to implement. AFAICT, you don't implement any, so it should not be necessary.
>>
>> Like I mentioned above, there's some erratum workaround which
>> could make use of the platform_quirk() callback, that we want
>> to send in the near future.
>
> I think it would be best to introduce platforms/s32cc.c at that point.
>
> Cheers,

Thank you for the review and suggestions.

Regards,
Andrei Cherechesu

Re: [PATCH v2 5/8] xen/arm: platforms: Add NXP S32CC platform code

[Julien Grall]

Hi,

On 08/10/2024 20:01, Andrei Cherechesu wrote:
> Hi Julien,
> 
> On 04/10/2024 19:24, Julien Grall wrote:
>>
>>
>> On 04/10/2024 16:37, Andrei Cherechesu wrote:
>>> Hi Julien, Stefano,
>>
>> Hi Andrei,
>>
>>>
>>> On 01/10/2024 13:01, Julien Grall wrote:
>>>> Hi Andrei,
>>>>
>>>> On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
>>>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>
>>>>> Add code for NXP S32CC platforms (S32G2, S32G3, S32R45).
>>>>>
>>>>> S32CC platforms use the NXP LINFlexD UART driver for
>>>>> console by default, and rely on Dom0 having access to
>>>>> SCMI services for system-level resources from firmware
>>>>> at EL3.
>>>>>
>>>>> Platform-related quirks will be addressed in following
>>>>> commits.
>>>>
>>>> I don't see any specific quirks in the series. Are you intended to send follow-up?
>>>>
>>>> [...]
>>>
>>> Well, we could use an opinion on some SoC erratum implementation
>>> and if it would be acceptable for you or not, from any of
>>> the ARM maintainers.
>>>
>>> The erratum is about some TLB Invalidate (tlbi) instruction
>>> variants (by Virtual Address) which are handled incorrectly
>>> in some cases (for some VAs), and that need to be replaced
>>> with broader versions.
>>
>> Can you provide more details:
>>    1. Is this a processor issue?
>>    2. Which VAs are affected?
>>    3. What is the impact if the erratum is hit?
>>    3. Do we need to do anything on the behalf of the guests?
> 
> Sure:
> 1. This is an SoC issue, present in a subset of the S32CC processors family.
> 2. VAs whose bits [48:41] are not all zero.
> 3. Well, TLB corruption.

Can you provide a bit more details? Is it a corruption within the same 
stage/ASID/VMID? IOW if a guest corrupt the TLB, does this mean only the 
stage-1?

Also, if a guest manage to corrupt its TLB, does this mean it could end 
up to access host physical address it should not?

BTW, do you have any more documentation about the erratum?

> 4. There are instructions affected at both levels 1 and 2 of translation.
> The guest will have to work its own way around it.
> 
> In Xen, just `tlbi vae2is` (only used in flush_xen_tlb_range_va()) seems to
> be affected, and if we go with the 2nd approach from the ones proposed,
> it should not be very ugly to implement. I am of course open to any of your
> suggestions.
> 
>>
>>
>>> It doesn't apply for all S32CC platforms, but we can use the
>>> Host DT compatible to identify this. My idea is that we define
>>> a platform quirk for this and check for it using
>>> platform_has_quirk().
>>> Then, we either:>      - modify the definition for the affected 'tlbi' variant
>>>       in arm64/flushtlb.h to actually execute the broader one
>>>       if needed
>>> or:
>>>       - define a new wrapper for the tlbi variant we want to
>>>       replace the affected one with
>>>       - check for this quirk before its usage and call the
>>>       more potent version if needed (there's only one
>>>       occurrence of usage for the affected version).
>>>
>>> Number 2 seems better to me, it's more customizable and
>>> it's similar to the existing handling for
>>> PLATFORM_QUIRK_GIC_64K_STRIDE for some other Arm platform.
>>
>> I need a bit more details first (see my questions above). But if there are not many VAs affected, then we may be able to re-order the Xen memory layout to avoid the VAs. So we wouldn't need any specific change in the TLB flush operations.
> 
> I'm not sure what to say here, we would need to have the layout
> limited under the mentioned VA range in our case, right?

Correct. Is your SoC going to ever have more than a couple of TBs of memory?

> What
> about the GVA layout?

That's control by the guest. We could potentially tell the guest less VA 
is supported, but it is free to ignore.

Can you clarify why we would need to care about the GVA layout?

Cheers,

-- 
Julien Grall

Re: [PATCH v2 5/8] xen/arm: platforms: Add NXP S32CC platform code

[Andrei Cherechesu]

Hi Julien,

Sorry for the slow replies.


On 08/10/2024 23:31, Julien Grall wrote:
> Hi,
>
> On 08/10/2024 20:01, Andrei Cherechesu wrote:
>> Hi Julien,
>>
>> On 04/10/2024 19:24, Julien Grall wrote:
>>>
>>>
>>> On 04/10/2024 16:37, Andrei Cherechesu wrote:
>>>> Hi Julien, Stefano,
>>>
>>> Hi Andrei,
>>>
>>>>
>>>> On 01/10/2024 13:01, Julien Grall wrote:
>>>>> Hi Andrei,
>>>>>
>>>>> On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
>>>>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>>>
>>>>>> Add code for NXP S32CC platforms (S32G2, S32G3, S32R45).
>>>>>>
>>>>>> S32CC platforms use the NXP LINFlexD UART driver for
>>>>>> console by default, and rely on Dom0 having access to
>>>>>> SCMI services for system-level resources from firmware
>>>>>> at EL3.
>>>>>>
>>>>>> Platform-related quirks will be addressed in following
>>>>>> commits.
>>>>>
>>>>> I don't see any specific quirks in the series. Are you intended to send follow-up?
>>>>>
>>>>> [...]
>>>>
>>>> Well, we could use an opinion on some SoC erratum implementation
>>>> and if it would be acceptable for you or not, from any of
>>>> the ARM maintainers.
>>>>
>>>> The erratum is about some TLB Invalidate (tlbi) instruction
>>>> variants (by Virtual Address) which are handled incorrectly
>>>> in some cases (for some VAs), and that need to be replaced
>>>> with broader versions.
>>>
>>> Can you provide more details:
>>>    1. Is this a processor issue?
>>>    2. Which VAs are affected?
>>>    3. What is the impact if the erratum is hit?
>>>    3. Do we need to do anything on the behalf of the guests?
>>
>> Sure:
>> 1. This is an SoC issue, present in a subset of the S32CC processors family.
>> 2. VAs whose bits [48:41] are not all zero.
>> 3. Well, TLB corruption.
>
> Can you provide a bit more details? Is it a corruption within the same stage/ASID/VMID? IOW if a guest corrupt the TLB, does this mean only the stage-1?

Corruption might occur in other TLBs from the coherency domain.

When a TLBI by VA is broadcasted from a cluster, the invalidation is not
handled correctly in the other cluster for the affected VAs (with
bits [48:41] not all 0). So the other cluster cores' TLBs will contain
stale entries, and we need to use a broader TLBI variant instead.

It applies to both stage-1 and stage-2 translations.

I hope it is clearer now, but let me know if I can help you with any other details.

>
> Also, if a guest manage to corrupt its TLB, does this mean it could end up to access host physical address it should not?

Indeed, there could be mappings leading the guest to access physical
addresses not under his ownership.

>
> BTW, do you have any more documentation about the erratum?

The errata document for an affected processor variant is publicly available [0]
on NXP website, however, it also needs registering. The erratum can be found
there as ERR050481.

[0] https://www.nxp.com/webapp/Download?colCode=S32G2_1P77B&location=null

>
>> 4. There are instructions affected at both levels 1 and 2 of translation.
>> The guest will have to work its own way around it.
>>
>> In Xen, just `tlbi vae2is` (only used in flush_xen_tlb_range_va()) seems to
>> be affected, and if we go with the 2nd approach from the ones proposed,
>> it should not be very ugly to implement. I am of course open to any of your
>> suggestions.
>>
>>>
>>>
>>>> It doesn't apply for all S32CC platforms, but we can use the
>>>> Host DT compatible to identify this. My idea is that we define
>>>> a platform quirk for this and check for it using
>>>> platform_has_quirk().
>>>> Then, we either:>      - modify the definition for the affected 'tlbi' variant
>>>>       in arm64/flushtlb.h to actually execute the broader one
>>>>       if needed
>>>> or:
>>>>       - define a new wrapper for the tlbi variant we want to
>>>>       replace the affected one with
>>>>       - check for this quirk before its usage and call the
>>>>       more potent version if needed (there's only one
>>>>       occurrence of usage for the affected version).
>>>>
>>>> Number 2 seems better to me, it's more customizable and
>>>> it's similar to the existing handling for
>>>> PLATFORM_QUIRK_GIC_64K_STRIDE for some other Arm platform.
>>>
>>> I need a bit more details first (see my questions above). But if there are not many VAs affected, then we may be able to re-order the Xen memory layout to avoid the VAs. So we wouldn't need any specific change in the TLB flush operations.
>>
>> I'm not sure what to say here, we would need to have the layout
>> limited under the mentioned VA range in our case, right?
>
> Correct. Is your SoC going to ever have more than a couple of TBs of memory?

No, that shouldn't be the case. Is it possible/preferable to do that for Xen?

>
>
>> What
>> about the GVA layout?
>
> That's control by the guest. We could potentially tell the guest less VA is supported, but it is free to ignore.
>
> Can you clarify why we would need to care about the GVA layout?

Right, the hypervisor should not limit it. But the guest is then
required to handle the workaround for stage-1.

>
>
> Cheers,
>

Regards,
Andrei Cherechesu

Re: [PATCH v2 0/8] xen/arm: Add support for S32CC platforms and LINFlexD UART

[Julien Grall]

Hi,

On 30/09/2024 12:47, Andrei Cherechesu (OSS) wrote:
> Andrei Cherechesu (8):
>    xen/arm: Add NXP LINFlexD UART Driver
>    xen/arm: Add NXP LINFlexD UART early printk support

I have committed the first two patches.

Cheers,


-- 
Julien Grall

Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer

[Grygorii Strashko]

Hi

I'd be apprcieated if could consider my comments below.

On 30.09.24 14:47, Andrei Cherechesu (OSS) wrote:
> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> 
> Introduce the SCMI layer to have some basic degree of awareness
> about SMC calls that are based on the ARM System Control and
> Management Interface (SCMI) specification (DEN0056E).
> 
> The SCMI specification includes various protocols for managing
> system-level resources, such as: clocks, pins, reset, system power,
> power domains, performance domains, etc. The clients are named
> "SCMI agents" and the server is named "SCMI platform".
> 
> Only support the shared-memory based transport with SMCs as
> the doorbell mechanism for notifying the platform. Also, this
> implementation only handles the "arm,scmi-smc" compatible,
> requiring the following properties:
> 	- "arm,smc-id" (unique SMC ID)
> 	- "shmem" (one or more phandles pointing to shmem zones
> 	for each channel)
> 
> The initialization is done as 'presmp_initcall', since we need
> SMCs and PSCI should already probe EL3 FW for supporting SMCCC.
> If no "arm,scmi-smc" compatible node is found in Dom0's
> DT, the initialization fails silently, as it's not mandatory.
> Otherwise, we get the 'arm,smc-id' DT property from the node,
> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
> are not validated, as the SMC calls are only passed through
> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
> running.
> 
> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>   xen/arch/arm/Kconfig                |  10 ++
>   xen/arch/arm/Makefile               |   1 +
>   xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>   xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++

Could it be moved in separate folder - for example "sci" or "firmware"?
There are definitely more SCMI specific code will be added in the future
as this solution is little bit too simplified.

>   4 files changed, 226 insertions(+)
>   create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>   create mode 100644 xen/arch/arm/scmi-smc.c
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 323c967361..adf53e2de1 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>   	  not been emulated to their complete functionality. Enabling this might
>   	  result in unwanted/non-spec compliant behavior.
>   
> +config SCMI_SMC

Could you please rename it to clearly specify that it is only dom0/hwdom
specific? Like SCMI_SMC_DOM0 or SCMI_SMC_HW_DOM.

> +	bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
> +	default y
> +	help
> +	  This option enables basic awareness for SCMI calls using SMC as
> +	  doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
> +	  compatible only). The value of "arm,smc-id" DT property from SCMI
> +	  firmware node is used to trap and forward corresponding SCMI SMCs
> +	  to firmware running at EL3, if the call comes from Dom0.
> +
>   endmenu
>   
>   menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 7792bff597..b85ad9c13f 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
>   obj-y += physdev.o
>   obj-y += processor.o
>   obj-y += psci.o
> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
>   obj-y += setup.o
>   obj-y += shutdown.o
>   obj-y += smp.o
> diff --git a/xen/arch/arm/include/asm/scmi-smc.h b/xen/arch/arm/include/asm/scmi-smc.h
> new file mode 100644
> index 0000000000..c6c0079e86
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/scmi-smc.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/include/asm/scmi-smc.h
> + *
> + * ARM System Control and Management Interface (SCMI) over SMC
> + * Generic handling layer
> + *
> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
> + * Copyright 2024 NXP
> + */
> +
> +#ifndef __ASM_SCMI_SMC_H__
> +#define __ASM_SCMI_SMC_H__
> +
> +#include <xen/types.h>
> +#include <asm/regs.h>
> +
> +#ifdef CONFIG_SCMI_SMC
> +
> +bool scmi_is_enabled(void);
> +bool scmi_is_valid_smc_id(uint32_t fid);
> +bool scmi_handle_smc(struct cpu_user_regs *regs);
> +
> +#else
> +
> +static inline bool scmi_is_enabled(void)
> +{
> +    return false;
> +}
> +
> +static inline bool scmi_is_valid_smc_id(uint32_t fid)
> +{
> +    return false;
> +}
> +
> +static inline bool scmi_handle_smc(struct cpu_user_regs *regs)

I propose to add "struct domain *d" as the first parameter to make it
more abstract from Xen internals.

> +{
> +    return false;
> +}
> +
> +#endif /* CONFIG_SCMI_SMC */
> +
> +#endif /* __ASM_SCMI_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/scmi-smc.c b/xen/arch/arm/scmi-smc.c
> new file mode 100644
> index 0000000000..373ca7ba5f
> --- /dev/null
> +++ b/xen/arch/arm/scmi-smc.c
> @@ -0,0 +1,163 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/scmi-smc.c
> + *
> + * ARM System Control and Management Interface (SCMI) over SMC
> + * Generic handling layer
> + *
> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
> + * Copyright 2024 NXP
> + */
> +
> +#include <xen/acpi.h>
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
> +#include <xen/init.h>
> +#include <xen/sched.h>
> +#include <xen/types.h>
> +
> +#include <asm/scmi-smc.h>
> +#include <asm/smccc.h>
> +
> +#define SCMI_SMC_ID_PROP   "arm,smc-id"
> +
> +static bool scmi_support;
> +static uint32_t scmi_smc_id;
> +
> +/* Check if SCMI layer correctly initialized and can be used. */
> +bool scmi_is_enabled(void)
> +{
> +    return scmi_support;
> +}
> +
> +/*
> + * Check if provided SMC Function Identifier matches the one known by the SCMI
> + * layer, as read from DT prop 'arm,smc-id' during initialiation.
> + */
> +bool scmi_is_valid_smc_id(uint32_t fid)

I do not think this API is required at all as it's driver's internals.

> +{
> +    return (fid == scmi_smc_id);
> +}
> +
> +/*
> + * Generic handler for SCMI-SMC requests, currently only forwarding the
> + * request to FW running at EL3 if it came from Dom0. Is called from the vSMC
> + * layer for SiP SMCs, since SCMI calls are usually provided this way.
> + * Can also be called from `platform_smc()` plat-specific callback.
> + *
> + * Returns true if SMC was handled (regardless of response), false otherwise.
> + */
> +bool scmi_handle_smc(struct cpu_user_regs *regs)

[...]

I'd propose to squash patch "[v2,4/8] xen/arm: vsmc: Enable handling
SiP-owned SCMI SMC calls" to clearly show how API interface is going to
be used.


BR,
-Grygorii

Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer

[Julien Grall]

Hi,

On 01/11/2024 15:22, Grygorii Strashko wrote:
> Hi
> 
> I'd be apprcieated if could consider my comments below.
> 
> On 30.09.24 14:47, Andrei Cherechesu (OSS) wrote:
>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>
>> Introduce the SCMI layer to have some basic degree of awareness
>> about SMC calls that are based on the ARM System Control and
>> Management Interface (SCMI) specification (DEN0056E).
>>
>> The SCMI specification includes various protocols for managing
>> system-level resources, such as: clocks, pins, reset, system power,
>> power domains, performance domains, etc. The clients are named
>> "SCMI agents" and the server is named "SCMI platform".
>>
>> Only support the shared-memory based transport with SMCs as
>> the doorbell mechanism for notifying the platform. Also, this
>> implementation only handles the "arm,scmi-smc" compatible,
>> requiring the following properties:
>>     - "arm,smc-id" (unique SMC ID)
>>     - "shmem" (one or more phandles pointing to shmem zones
>>     for each channel)
>>
>> The initialization is done as 'presmp_initcall', since we need
>> SMCs and PSCI should already probe EL3 FW for supporting SMCCC.
>> If no "arm,scmi-smc" compatible node is found in Dom0's
>> DT, the initialization fails silently, as it's not mandatory.
>> Otherwise, we get the 'arm,smc-id' DT property from the node,
>> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
>> are not validated, as the SMC calls are only passed through
>> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
>> running.
>>
>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>> ---
>>   xen/arch/arm/Kconfig                |  10 ++
>>   xen/arch/arm/Makefile               |   1 +
>>   xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>>   xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
> 
> Could it be moved in separate folder - for example "sci" or "firmware"?
> There are definitely more SCMI specific code will be added in the future
> as this solution is little bit too simplified.
> 
>>   4 files changed, 226 insertions(+)
>>   create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>>   create mode 100644 xen/arch/arm/scmi-smc.c
>>
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index 323c967361..adf53e2de1 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>>         not been emulated to their complete functionality. Enabling 
>> this might
>>         result in unwanted/non-spec compliant behavior.
>> +config SCMI_SMC
> 
> Could you please rename it to clearly specify that it is only dom0/hwdom
> specific? Like SCMI_SMC_DOM0 or SCMI_SMC_HW_DOM.

I expect this series to be just a stop gap until we support SCMI for the 
VMs. Once this is merge, I don't expect we would want to keep a Kconfig 
to allow SCMI just for dom0. Therefore, I am not entirely convinced the 
proposed new name is a good idea.

> 
>> +    bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 
>> firmware"
>> +    default y
>> +    help
>> +      This option enables basic awareness for SCMI calls using SMC as
>> +      doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
>> +      compatible only). The value of "arm,smc-id" DT property from SCMI
>> +      firmware node is used to trap and forward corresponding SCMI SMCs
>> +      to firmware running at EL3, if the call comes from Dom0.
>> +
>>   endmenu
>>   menu "ARM errata workaround via the alternative framework"
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 7792bff597..b85ad9c13f 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
>>   obj-y += physdev.o
>>   obj-y += processor.o
>>   obj-y += psci.o
>> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
>>   obj-y += setup.o
>>   obj-y += shutdown.o
>>   obj-y += smp.o
>> diff --git a/xen/arch/arm/include/asm/scmi-smc.h b/xen/arch/arm/ 
>> include/asm/scmi-smc.h
>> new file mode 100644
>> index 0000000000..c6c0079e86
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/scmi-smc.h
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * xen/arch/arm/include/asm/scmi-smc.h
>> + *
>> + * ARM System Control and Management Interface (SCMI) over SMC
>> + * Generic handling layer
>> + *
>> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
>> + * Copyright 2024 NXP
>> + */
>> +
>> +#ifndef __ASM_SCMI_SMC_H__
>> +#define __ASM_SCMI_SMC_H__
>> +
>> +#include <xen/types.h>
>> +#include <asm/regs.h>
>> +
>> +#ifdef CONFIG_SCMI_SMC
>> +
>> +bool scmi_is_enabled(void);
>> +bool scmi_is_valid_smc_id(uint32_t fid);
>> +bool scmi_handle_smc(struct cpu_user_regs *regs);
>> +
>> +#else
>> +
>> +static inline bool scmi_is_enabled(void)
>> +{
>> +    return false;
>> +}
>> +
>> +static inline bool scmi_is_valid_smc_id(uint32_t fid)
>> +{
>> +    return false;
>> +}
>> +
>> +static inline bool scmi_handle_smc(struct cpu_user_regs *regs)
> 
> I propose to add "struct domain *d" as the first parameter to make it
> more abstract from Xen internals.

I am not sure to understand why we would want the call to be more 
abstract. This function should *only* act on the vCPU currently loaded. 
So it makes sense to use "current->domain".

Cheers,

-- 
Julien Grall

Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer

[Andrei Cherechesu]

Hi Julien, Grygorii,

On 11/11/2024 12:33, Julien Grall wrote:
> Hi,
>
> On 01/11/2024 15:22, Grygorii Strashko wrote:
>> Hi
>>
>> I'd be apprcieated if could consider my comments below.
>>
>> On 30.09.24 14:47, Andrei Cherechesu (OSS) wrote:
>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>
>>> Introduce the SCMI layer to have some basic degree of awareness
>>> about SMC calls that are based on the ARM System Control and
>>> Management Interface (SCMI) specification (DEN0056E).
>>>
>>> The SCMI specification includes various protocols for managing
>>> system-level resources, such as: clocks, pins, reset, system power,
>>> power domains, performance domains, etc. The clients are named
>>> "SCMI agents" and the server is named "SCMI platform".
>>>
>>> Only support the shared-memory based transport with SMCs as
>>> the doorbell mechanism for notifying the platform. Also, this
>>> implementation only handles the "arm,scmi-smc" compatible,
>>> requiring the following properties:
>>>     - "arm,smc-id" (unique SMC ID)
>>>     - "shmem" (one or more phandles pointing to shmem zones
>>>     for each channel)
>>>
>>> The initialization is done as 'presmp_initcall', since we need
>>> SMCs and PSCI should already probe EL3 FW for supporting SMCCC.
>>> If no "arm,scmi-smc" compatible node is found in Dom0's
>>> DT, the initialization fails silently, as it's not mandatory.
>>> Otherwise, we get the 'arm,smc-id' DT property from the node,
>>> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
>>> are not validated, as the SMC calls are only passed through
>>> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
>>> running.
>>>
>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>> ---
>>>   xen/arch/arm/Kconfig                |  10 ++
>>>   xen/arch/arm/Makefile               |   1 +
>>>   xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>>>   xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
>>
>> Could it be moved in separate folder - for example "sci" or "firmware"?
>> There are definitely more SCMI specific code will be added in the future
>> as this solution is little bit too simplified.
>>
>>>   4 files changed, 226 insertions(+)
>>>   create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>>>   create mode 100644 xen/arch/arm/scmi-smc.c
>>>
>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>> index 323c967361..adf53e2de1 100644
>>> --- a/xen/arch/arm/Kconfig
>>> +++ b/xen/arch/arm/Kconfig
>>> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>>>         not been emulated to their complete functionality. Enabling this might
>>>         result in unwanted/non-spec compliant behavior.
>>> +config SCMI_SMC
>>
>> Could you please rename it to clearly specify that it is only dom0/hwdom
>> specific? Like SCMI_SMC_DOM0 or SCMI_SMC_HW_DOM.
>
> I expect this series to be just a stop gap until we support SCMI for the VMs. Once this is merge, I don't expect we would want to keep a Kconfig to allow SCMI just for dom0. Therefore, I am not entirely convinced the proposed new name is a good idea.

AFAIU, Julien, you don't agree with renaming the config, nor with moving the
support to a separate folder since it's something temporary? That's my view
as well.

These changes do not introduce support for a layer of mediators for
interacting with system firmware, but only for one simplified implementation.
So I suppose the patch set that adds that support also creates the folder
(named 'sci' - per Gregorii's proposal - or 'firmware' to align with Linux),
and the required config.

But I'm up for doing whatever you consider more suitable.

>
>
>>
>>> +    bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
>>> +    default y
>>> +    help
>>> +      This option enables basic awareness for SCMI calls using SMC as
>>> +      doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
>>> +      compatible only). The value of "arm,smc-id" DT property from SCMI
>>> +      firmware node is used to trap and forward corresponding SCMI SMCs
>>> +      to firmware running at EL3, if the call comes from Dom0.
>>> +
>>>   endmenu
>>>   menu "ARM errata workaround via the alternative framework"
>>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>>> index 7792bff597..b85ad9c13f 100644
>>> --- a/xen/arch/arm/Makefile
>>> +++ b/xen/arch/arm/Makefile
>>> @@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
>>>   obj-y += physdev.o
>>>   obj-y += processor.o
>>>   obj-y += psci.o
>>> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
>>>   obj-y += setup.o
>>>   obj-y += shutdown.o
>>>   obj-y += smp.o
>>> diff --git a/xen/arch/arm/include/asm/scmi-smc.h b/xen/arch/arm/ include/asm/scmi-smc.h
>>> new file mode 100644
>>> index 0000000000..c6c0079e86
>>> --- /dev/null
>>> +++ b/xen/arch/arm/include/asm/scmi-smc.h
>>> @@ -0,0 +1,52 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * xen/arch/arm/include/asm/scmi-smc.h
>>> + *
>>> + * ARM System Control and Management Interface (SCMI) over SMC
>>> + * Generic handling layer
>>> + *
>>> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>> + * Copyright 2024 NXP
>>> + */
>>> +
>>> +#ifndef __ASM_SCMI_SMC_H__
>>> +#define __ASM_SCMI_SMC_H__
>>> +
>>> +#include <xen/types.h>
>>> +#include <asm/regs.h>
>>> +
>>> +#ifdef CONFIG_SCMI_SMC
>>> +
>>> +bool scmi_is_enabled(void);
>>> +bool scmi_is_valid_smc_id(uint32_t fid);
>>> +bool scmi_handle_smc(struct cpu_user_regs *regs);
>>> +
>>> +#else
>>> +
>>> +static inline bool scmi_is_enabled(void)
>>> +{
>>> +    return false;
>>> +}
>>> +
>>> +static inline bool scmi_is_valid_smc_id(uint32_t fid)
>>> +{
>>> +    return false;
>>> +}
>>> +
>>> +static inline bool scmi_handle_smc(struct cpu_user_regs *regs)
>>
>> I propose to add "struct domain *d" as the first parameter to make it
>> more abstract from Xen internals.
>
> I am not sure to understand why we would want the call to be more abstract. This function should *only* act on the vCPU currently loaded. So it makes sense to use "current->domain".

So this should stay the same, right?

@Grygorii,

Regarding `scmi_is_valid_smc_id()`, I will make it private to the
SCMI-SMC driver.

And regarding squashing [v2,4/8] to [v2,3/8], IMO it is clearer
this way: to have the implementation of the driver in a different
commit than the usage/refactorings needed to accomodate it. And
this makes it easier to revert the behaviour too, eventually. But
I don't have a strong preference on this and I'm open to squash
the commits if you believe it is better that way.

>
> Cheers,
>

Regards,
Andrei Cherechesu

Re: [PATCH v2 3/8] xen/arm: Add SCMI over SMC calls handling layer

[Grygorii Strashko]

On 09.12.24 19:37, Andrei Cherechesu wrote:
> Hi Julien, Grygorii,
> 
> On 11/11/2024 12:33, Julien Grall wrote:
>> Hi,
>>
>> On 01/11/2024 15:22, Grygorii Strashko wrote:
>>> Hi
>>>
>>> I'd be apprcieated if could consider my comments below.
>>>
>>> On 30.09.24 14:47, Andrei Cherechesu (OSS) wrote:
>>>> From: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>>
>>>> Introduce the SCMI layer to have some basic degree of awareness
>>>> about SMC calls that are based on the ARM System Control and
>>>> Management Interface (SCMI) specification (DEN0056E).
>>>>
>>>> The SCMI specification includes various protocols for managing
>>>> system-level resources, such as: clocks, pins, reset, system power,
>>>> power domains, performance domains, etc. The clients are named
>>>> "SCMI agents" and the server is named "SCMI platform".
>>>>
>>>> Only support the shared-memory based transport with SMCs as
>>>> the doorbell mechanism for notifying the platform. Also, this
>>>> implementation only handles the "arm,scmi-smc" compatible,
>>>> requiring the following properties:
>>>>      - "arm,smc-id" (unique SMC ID)
>>>>      - "shmem" (one or more phandles pointing to shmem zones
>>>>      for each channel)
>>>>
>>>> The initialization is done as 'presmp_initcall', since we need
>>>> SMCs and PSCI should already probe EL3 FW for supporting SMCCC.
>>>> If no "arm,scmi-smc" compatible node is found in Dom0's
>>>> DT, the initialization fails silently, as it's not mandatory.
>>>> Otherwise, we get the 'arm,smc-id' DT property from the node,
>>>> to know the SCMI SMC ID we handle. The 'shmem' memory ranges
>>>> are not validated, as the SMC calls are only passed through
>>>> to EL3 FW if coming from Dom0 and as if Dom0 would be natively
>>>> running.
>>>>
>>>> Signed-off-by: Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>> ---
>>>>    xen/arch/arm/Kconfig                |  10 ++
>>>>    xen/arch/arm/Makefile               |   1 +
>>>>    xen/arch/arm/include/asm/scmi-smc.h |  52 +++++++++
>>>>    xen/arch/arm/scmi-smc.c             | 163 ++++++++++++++++++++++++++++
>>>
>>> Could it be moved in separate folder - for example "sci" or "firmware"?
>>> There are definitely more SCMI specific code will be added in the future
>>> as this solution is little bit too simplified.
>>>
>>>>    4 files changed, 226 insertions(+)
>>>>    create mode 100644 xen/arch/arm/include/asm/scmi-smc.h
>>>>    create mode 100644 xen/arch/arm/scmi-smc.c
>>>>
>>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>>> index 323c967361..adf53e2de1 100644
>>>> --- a/xen/arch/arm/Kconfig
>>>> +++ b/xen/arch/arm/Kconfig
>>>> @@ -245,6 +245,16 @@ config PARTIAL_EMULATION
>>>>          not been emulated to their complete functionality. Enabling this might
>>>>          result in unwanted/non-spec compliant behavior.
>>>> +config SCMI_SMC
>>>
>>> Could you please rename it to clearly specify that it is only dom0/hwdom
>>> specific? Like SCMI_SMC_DOM0 or SCMI_SMC_HW_DOM.
>>
>> I expect this series to be just a stop gap until we support SCMI for the VMs. Once this is merge, I don't expect we would want to keep a Kconfig to allow SCMI just for dom0. Therefore, I am not entirely convinced the proposed new name is a good idea.
> 
> AFAIU, Julien, you don't agree with renaming the config, nor with moving the
> support to a separate folder since it's something temporary? That's my view
> as well.
> 
> These changes do not introduce support for a layer of mediators for
> interacting with system firmware, but only for one simplified implementation.
> So I suppose the patch set that adds that support also creates the folder
> (named 'sci' - per Gregorii's proposal - or 'firmware' to align with Linux),
> and the required config.
> 
> But I'm up for doing whatever you consider more suitable.
> 
>>
>>
>>>
>>>> +    bool "Enable forwarding SCMI over SMC calls from Dom0 to EL3 firmware"
>>>> +    default y
>>>> +    help
>>>> +      This option enables basic awareness for SCMI calls using SMC as
>>>> +      doorbell mechanism and Shared Memory for transport ("arm,scmi-smc"
>>>> +      compatible only). The value of "arm,smc-id" DT property from SCMI
>>>> +      firmware node is used to trap and forward corresponding SCMI SMCs
>>>> +      to firmware running at EL3, if the call comes from Dom0.
>>>> +
>>>>    endmenu
>>>>    menu "ARM errata workaround via the alternative framework"
>>>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>>>> index 7792bff597..b85ad9c13f 100644
>>>> --- a/xen/arch/arm/Makefile
>>>> +++ b/xen/arch/arm/Makefile
>>>> @@ -45,6 +45,7 @@ obj-y += platform_hypercall.o
>>>>    obj-y += physdev.o
>>>>    obj-y += processor.o
>>>>    obj-y += psci.o
>>>> +obj-$(CONFIG_SCMI_SMC) += scmi-smc.o
>>>>    obj-y += setup.o
>>>>    obj-y += shutdown.o
>>>>    obj-y += smp.o
>>>> diff --git a/xen/arch/arm/include/asm/scmi-smc.h b/xen/arch/arm/ include/asm/scmi-smc.h
>>>> new file mode 100644
>>>> index 0000000000..c6c0079e86
>>>> --- /dev/null
>>>> +++ b/xen/arch/arm/include/asm/scmi-smc.h
>>>> @@ -0,0 +1,52 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +/*
>>>> + * xen/arch/arm/include/asm/scmi-smc.h
>>>> + *
>>>> + * ARM System Control and Management Interface (SCMI) over SMC
>>>> + * Generic handling layer
>>>> + *
>>>> + * Andrei Cherechesu <andrei.cherechesu@nxp.com>
>>>> + * Copyright 2024 NXP
>>>> + */
>>>> +
>>>> +#ifndef __ASM_SCMI_SMC_H__
>>>> +#define __ASM_SCMI_SMC_H__
>>>> +
>>>> +#include <xen/types.h>
>>>> +#include <asm/regs.h>
>>>> +
>>>> +#ifdef CONFIG_SCMI_SMC
>>>> +
>>>> +bool scmi_is_enabled(void);
>>>> +bool scmi_is_valid_smc_id(uint32_t fid);
>>>> +bool scmi_handle_smc(struct cpu_user_regs *regs);
>>>> +
>>>> +#else
>>>> +
>>>> +static inline bool scmi_is_enabled(void)
>>>> +{
>>>> +    return false;
>>>> +}
>>>> +
>>>> +static inline bool scmi_is_valid_smc_id(uint32_t fid)
>>>> +{
>>>> +    return false;
>>>> +}
>>>> +
>>>> +static inline bool scmi_handle_smc(struct cpu_user_regs *regs)
>>>
>>> I propose to add "struct domain *d" as the first parameter to make it
>>> more abstract from Xen internals.
>>
>> I am not sure to understand why we would want the call to be more abstract. This function should *only* act on the vCPU currently loaded. So it makes sense to use "current->domain".
> 
> So this should stay the same, right?
> 
> @Grygorii,
> 
> Regarding `scmi_is_valid_smc_id()`, I will make it private to the
> SCMI-SMC driver.
> 
> And regarding squashing [v2,4/8] to [v2,3/8], IMO it is clearer
> this way: to have the implementation of the driver in a different
> commit than the usage/refactorings needed to accomodate it. And
> this makes it easier to revert the behaviour too, eventually. But
> I don't have a strong preference on this and I'm open to squash
> the commits if you believe it is better that way.

I'm ok with other comments - means it's up to you and maintainers.

But in my opinion it'll be better to consolidate the scmi code in standalone folder from
the beginning to avoid possible future files moving/renaming. It's optional, new features
and it looks not right to mix it with generic Arm code.

Best regards,
Grygorii