* [PATCH v3 0/6] zynqmp: Add forwarding of platform specific firmware calls
@ 2018-08-11 0:01 Stefano Stabellini
2018-08-11 0:01 ` [PATCH v3 1/6] xen/arm: introduce platform_smc Stefano Stabellini
` (5 more replies)
0 siblings, 6 replies; 29+ messages in thread
From: Stefano Stabellini @ 2018-08-11 0:01 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, julien.grall, sstabellini
Hi all,
This patch is an update over this old patch series:
https://marc.info/?l=xen-devel&m=148579695530482
I inherited the seriest from Edgar, I made significant changes over the
old version, including addressing (almost) all comments. The main
changes are:
- split the largest patch into multiple smaller patches
- remove all #defines and enum entries which aren't required
- always specify .size in pm_mmio_access
- always specify either .hwdom_access or .node in pm_mmio_access
- remove NODE_UNKNOWN
- simplify domain_has_mmio_access, assuming that addresses only match one entry
- use arm_smccc_1_1_smc and set/get_user_reg
Cheers,
Stefano
The following changes since commit 6aaa9fb308171ec58ddf4cf058ad5341f81a65cf:
hvm/altp2m: Clarify the proper way to extend the altp2m interface (2018-07-31 16:14:01 +0100)
are available in the git repository at:
http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git zynqmp-v3
for you to fetch changes up to a94ee86ff3e37e332a8f22a756f2978e10b5e2a8:
xen/arm: zynqmp: Remove blacklist of ZynqMP's PM node (2018-08-10 14:48:02 -0700)
----------------------------------------------------------------
Edgar E. Iglesias (6):
xen/arm: introduce platform_smc
xen/arm: zynqmp: Forward plaform specific firmware calls
xen/arm: zynqmp: introduce zynqmp specific defines
xen/arm: zynqmp: eemi access control
xen/arm: zynqmp: implement zynqmp_eemi
xen/arm: zynqmp: Remove blacklist of ZynqMP's PM node
xen/arch/arm/platform.c | 8 +
xen/arch/arm/platforms/Makefile | 1 +
xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 962 +++++++++++++++++++++
xen/arch/arm/platforms/xilinx-zynqmp.c | 18 +-
xen/arch/arm/vsmc.c | 4 +
xen/include/asm-arm/platform.h | 3 +
xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 386 +++++++++
7 files changed, 1376 insertions(+), 6 deletions(-)
create mode 100644 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
create mode 100644 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 1/6] xen/arm: introduce platform_smc
2018-08-11 0:01 [PATCH v3 0/6] zynqmp: Add forwarding of platform specific firmware calls Stefano Stabellini
@ 2018-08-11 0:01 ` Stefano Stabellini
2018-08-23 15:37 ` Julien Grall
2018-08-11 0:01 ` [PATCH v3 2/6] xen/arm: zynqmp: Forward plaform specific firmware calls Stefano Stabellini
` (4 subsequent siblings)
5 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2018-08-11 0:01 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, Stefano Stabellini, julien.grall, sstabellini
From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Introduce platform_smc as a way to handle firmware calls that Xen does
not know about in a platform specific way. This is particularly useful
for implementing the SiP (SoC implementation specific) service calls.
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
xen/arch/arm/platform.c | 8 ++++++++
xen/arch/arm/vsmc.c | 4 ++++
xen/include/asm-arm/platform.h | 3 +++
3 files changed, 15 insertions(+)
diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index 3f2989e..9e19023 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -127,6 +127,14 @@ void platform_poweroff(void)
platform->poweroff();
}
+bool platform_smc(struct cpu_user_regs *regs)
+{
+ if ( platform && platform->smc )
+ return platform->smc(regs);
+
+ return false;
+}
+
bool platform_has_quirk(uint32_t quirk)
{
uint32_t quirks = 0;
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index c4ccae6..c72b9a0 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -25,6 +25,7 @@
#include <asm/smccc.h>
#include <asm/traps.h>
#include <asm/vpsci.h>
+#include <asm/platform.h>
/* Number of functions currently supported by Hypervisor Service. */
#define XEN_SMCCC_FUNCTION_COUNT 3
@@ -272,6 +273,9 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
case ARM_SMCCC_OWNER_STANDARD:
handled = handle_sssc(regs);
break;
+ case ARM_SMCCC_OWNER_SIP:
+ handled = platform_smc(regs);
+ break;
}
}
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index 2591d7b..dc55b6d 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -26,6 +26,8 @@ struct platform_desc {
void (*reset)(void);
/* Platform power-off */
void (*poweroff)(void);
+ /* Platform specific SMC handler */
+ bool (*smc)(struct cpu_user_regs *regs);
/*
* Platform quirks
* Defined has a function because a platform can support multiple
@@ -55,6 +57,7 @@ int platform_cpu_up(int cpu);
#endif
void platform_reset(void);
void platform_poweroff(void);
+bool platform_smc(struct cpu_user_regs *regs);
bool platform_has_quirk(uint32_t quirk);
bool platform_device_is_blacklisted(const struct dt_device_node *node);
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v3 2/6] xen/arm: zynqmp: Forward plaform specific firmware calls
2018-08-11 0:01 [PATCH v3 0/6] zynqmp: Add forwarding of platform specific firmware calls Stefano Stabellini
2018-08-11 0:01 ` [PATCH v3 1/6] xen/arm: introduce platform_smc Stefano Stabellini
@ 2018-08-11 0:01 ` Stefano Stabellini
2018-08-23 15:41 ` Julien Grall
2018-08-11 0:01 ` [PATCH v3 3/6] xen/arm: zynqmp: introduce zynqmp specific defines Stefano Stabellini
` (3 subsequent siblings)
5 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2018-08-11 0:01 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, Stefano Stabellini, julien.grall, sstabellini
From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Introduce zynqmp_eemi: a function resposible for implementing access
controls over the firmware calls. Only calls that are allowed are
forwarded to the firmware.
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
xen/arch/arm/platforms/Makefile | 1 +
xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 38 ++++++++++++++++++++++
xen/arch/arm/platforms/xilinx-zynqmp.c | 14 ++++++++
xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 3 ++
4 files changed, 56 insertions(+)
create mode 100644 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
create mode 100644 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index 80e555c..703f915 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -9,3 +9,4 @@ obj-y += sunxi.o
obj-$(CONFIG_ARM_64) += thunderx.o
obj-$(CONFIG_ARM_64) += xgene-storm.o
obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
+obj-$(CONFIG_ARM_64) += xilinx-zynqmp-eemi.o
diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
new file mode 100644
index 0000000..c3a19e9
--- /dev/null
+++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
@@ -0,0 +1,38 @@
+/*
+ * xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
+ *
+ * Xilinx ZynqMP EEMI API
+ *
+ * Copyright (c) 2018 Xilinx Inc.
+ * Written by Edgar E. Iglesias <edgar.iglesias@xilinx.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <xen/iocap.h>
+#include <xen/sched.h>
+#include <xen/types.h>
+#include <asm/smccc.h>
+#include <asm/regs.h>
+#include <asm/platforms/xilinx-zynqmp-eemi.h>
+
+bool zynqmp_eemi(struct cpu_user_regs *regs)
+{
+ return false;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/platforms/xilinx-zynqmp.c b/xen/arch/arm/platforms/xilinx-zynqmp.c
index d8ceded..c318cf5 100644
--- a/xen/arch/arm/platforms/xilinx-zynqmp.c
+++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
@@ -18,6 +18,8 @@
*/
#include <asm/platform.h>
+#include <asm/platforms/xilinx-zynqmp-eemi.h>
+#include <asm/smccc.h>
static const char * const zynqmp_dt_compat[] __initconst =
{
@@ -32,8 +34,20 @@ static const struct dt_device_match zynqmp_blacklist_dev[] __initconst =
{ /* sentinel */ },
};
+static bool zynqmp_smc(struct cpu_user_regs *regs)
+{
+ if ( !is_64bit_domain(current->domain) )
+ return false;
+ /* Only support platforms with SMCCC >= 1.1 */
+ if ( smccc_ver < SMCCC_VERSION(1, 1) )
+ return false;
+
+ return zynqmp_eemi(regs);
+}
+
PLATFORM_START(xilinx_zynqmp, "Xilinx ZynqMP")
.compatible = zynqmp_dt_compat,
+ .smc = zynqmp_smc,
.blacklist_dev = zynqmp_blacklist_dev,
PLATFORM_END
diff --git a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
new file mode 100644
index 0000000..6630dc8
--- /dev/null
+++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
@@ -0,0 +1,3 @@
+#include <asm/processor.h>
+
+extern bool zynqmp_eemi(struct cpu_user_regs *regs);
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v3 3/6] xen/arm: zynqmp: introduce zynqmp specific defines
2018-08-11 0:01 [PATCH v3 0/6] zynqmp: Add forwarding of platform specific firmware calls Stefano Stabellini
2018-08-11 0:01 ` [PATCH v3 1/6] xen/arm: introduce platform_smc Stefano Stabellini
2018-08-11 0:01 ` [PATCH v3 2/6] xen/arm: zynqmp: Forward plaform specific firmware calls Stefano Stabellini
@ 2018-08-11 0:01 ` Stefano Stabellini
2018-08-11 0:01 ` [PATCH v3 4/6] xen/arm: zynqmp: eemi access control Stefano Stabellini
` (2 subsequent siblings)
5 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2018-08-11 0:01 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, Stefano Stabellini, julien.grall, sstabellini
From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Introduce zynqmp specific defines for the firmware calls.
See EEMI:
https://www.xilinx.com/support/documentation/user_guides/ug1200-eemi-api.pdf
The error codes are described, under XIlPM Error Codes:
https://www.xilinx.com/support/documentation/user_guides/ug1137-zynq-ultrascale-mpsoc-swdev.pdf
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 383 +++++++++++++++++++++
1 file changed, 383 insertions(+)
diff --git a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
index 6630dc8..70fad7a 100644
--- a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
+++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
@@ -1,3 +1,386 @@
#include <asm/processor.h>
+#define MM_CRL_APB 0xff5e0000
+#define MM_RPU 0xff9a0000
+#define MM_RTC 0xffa60000
+#define MM_ADMA_CH0 0xffa80000
+
+#define MM_USB3_0_XHCI 0xfe200000
+#define MM_USB3_1_XHCI 0xfe300000
+
+#define MM_SATA_AHCI_HBA 0xfd0c0000
+#define MM_AXIPCIE_MAIN 0xfd0e0000
+#define MM_CRF_APB 0xfd1a0000
+#define MM_PCIE_ATTRIB 0xfd480000
+#define MM_DP 0xfd4a0000
+#define MM_GPU 0xfd4b0000
+#define MM_GDMA_CH0 0xfd500000
+
+#define MM_UART0 0xff000000
+#define MM_UART1 0xff010000
+#define MM_I2C0 0xff020000
+#define MM_I2C1 0xff030000
+#define MM_SPI0 0xff040000
+#define MM_SPI1 0xff050000
+#define MM_CAN0 0xff060000
+#define MM_CAN1 0xff070000
+#define MM_GPIO 0xff0a0000
+#define MM_GEM0 0xff0b0000
+#define MM_GEM1 0xff0c0000
+#define MM_GEM2 0xff0d0000
+#define MM_GEM3 0xff0e0000
+#define MM_QSPI 0xff0f0000
+#define MM_NAND 0xff100000
+#define MM_TTC0 0xff110000
+#define MM_TTC1 0xff120000
+#define MM_TTC2 0xff130000
+#define MM_TTC3 0xff140000
+#define MM_SWDT 0xff150000
+#define MM_SD0 0xff160000
+#define MM_SD1 0xff170000
+#define MM_IOU_SLCR 0xff180000
+
+#define MM_PMU_GLOBAL 0xffd80000
+
+/* Selected set of register definitions: */
+#define R_CRF_APLL_CTRL 0x20
+#define R_CRF_ACPU_CTRL 0x60
+#define R_CRF_DP_VIDEO_REF_CTRL 0x70
+#define R_CRF_DP_AUDIO_REF_CTRL 0x74
+#define R_CRF_DP_STC_REF_CTRL 0x7c
+#define R_CRF_GPU_REF_CTRL 0x84
+#define R_CRF_SATA_REF_CTRL 0xa0
+#define R_CRF_PCIE_REF_CTRL 0xb4
+#define R_CRF_GDMA_REF_CTRL 0xb8
+#define R_CRF_DPDMA_REF_CTRL 0xbc
+
+#define R_CRL_IOPLL_CTRL 0x20
+#define R_CRL_RPLL_TO_FPD_CTRL 0x48
+#define R_CRL_USB3_DUAL_REF_CTRL 0x4c
+#define R_CRL_GEM0_REF_CTRL 0x50
+#define R_CRL_GEM1_REF_CTRL 0x54
+#define R_CRL_GEM2_REF_CTRL 0x58
+#define R_CRL_GEM3_REF_CTRL 0x5c
+#define R_CRL_USB0_BUS_REF_CTRL 0x60
+#define R_CRL_USB1_BUS_REF_CTRL 0x64
+#define R_CRL_QSPI_REF_CTRL 0x68
+#define R_CRL_SDIO0_REF_CTRL 0x6c
+#define R_CRL_SDIO1_REF_CTRL 0x70
+#define R_CRL_UART0_REF_CTRL 0x74
+#define R_CRL_UART1_REF_CTRL 0x78
+#define R_CRL_SPI0_REF_CTRL 0x7c
+#define R_CRL_SPI1_REF_CTRL 0x80
+#define R_CRL_CAN0_REF_CTRL 0x84
+#define R_CRL_CAN1_REF_CTRL 0x88
+#define R_CRL_CPU_R5_CTRL 0x90
+#define R_CRL_IOU_SWITCH_CTRL 0x9c
+#define R_CRL_CSU_PLL_CTRL 0xa0
+#define R_CRL_PCAP_CTRL 0xa4
+#define R_CRL_LPD_SWITCH_CTRL 0xa8
+#define R_CRL_LPD_LSBUS_CTRL 0xac
+#define R_CRL_DBG_LPD_CTRL 0xb0
+#define R_CRL_NAND_REF_CTRL 0xb4
+#define R_CRL_ADMA_REF_CTRL 0xb8
+#define R_CRL_PL0_REF_CTRL 0xc0
+#define R_CRL_PL1_REF_CTRL 0xc4
+#define R_CRL_PL2_REF_CTRL 0xc8
+#define R_CRL_PL3_REF_CTRL 0xcc
+#define R_CRL_PL0_THR_CTRL 0xd0
+#define R_CRL_PL0_THR_CNT 0xd4
+#define R_CRL_PL1_THR_CTRL 0xd8
+#define R_CRL_PL1_THR_CNT 0xdc
+#define R_CRL_PL2_THR_CTRL 0xe0
+#define R_CRL_PL2_THR_CNT 0xe4
+#define R_CRL_PL3_THR_CTRL 0xe8
+#define R_CRL_PL3_THR_CNT 0xfc
+#define R_CRL_GEM_TSU_REF_CTRL 0x100
+#define R_CRL_DLL_REF_CTRL 0x104
+#define R_CRL_AMS_REF_CTRL 0x108
+#define R_CRL_I2C0_REF_CTRL 0x120
+#define R_CRL_I2C1_REF_CTRL 0x124
+#define R_CRL_TIMESTAMP_REF_CTRL 0x128
+
+#define R_PMU_GLOBAL_GLOBAL_GEN_STORAGE0 0x30
+#define R_PMU_GLOBAL_PERS_GLOB_GEN_STORAGE7 0x6c
+
+#define R_PMU_GLOBAL_PWR_STATE 0x100
+
+#define R_IOU_SLCR_MIO_PIN_0 0x0
+#define R_IOU_SLCR_MIO_MST_TRI2 0x20c
+#define R_IOU_SLCR_WDT_CLK_SEL 0x300
+#define R_IOU_SLCR_CAN_MIO_CTRL 0x304
+#define R_IOU_SLCR_GEM_CLK_CTRL 0x308
+#define R_IOU_SLCR_SDIO_CLK_CTRL 0x30c
+#define R_IOU_SLCR_CTRL_REG_SD 0x310
+#define R_IOU_SLCR_SD_ITAPDLY 0x314
+#define R_IOU_SLCR_SD_CDN_CTRL 0x35c
+#define R_IOU_SLCR_GEM_CTRL 0x360
+#define R_IOU_SLCR_IOU_TTC_APB_CLK 0x380
+#define R_IOU_SLCR_IOU_TAPDLY_BYPASS 0x390
+#define R_IOU_SLCR_IOU_COHERENT_CTRL 0x400
+#define R_IOU_SLCR_VIDEO_PSS_CLK_SEL 0x404
+#define R_IOU_SLCR_IOU_RAM_GEM0 0x500
+#define R_IOU_SLCR_IOU_RAM_GEM1 0x504
+#define R_IOU_SLCR_IOU_RAM_GEM2 0x508
+#define R_IOU_SLCR_IOU_RAM_GEM3 0x50c
+#define R_IOU_SLCR_IOU_RAM_SD0 0x510
+#define R_IOU_SLCR_IOU_RAM_SD1 0x514
+#define R_IOU_SLCR_IOU_RAM_CAN0 0x518
+#define R_IOU_SLCR_IOU_RAM_CAN1 0x51c
+#define R_IOU_SLCR_IOU_RAM_LQSPI 0x520
+#define R_IOU_SLCR_IOU_RAM_NAND 0x524
+
+/* Service calls. */
+#define PM_GET_TRUSTZONE_VERSION 0xa03
+
+/* SMC function IDs for SiP Service queries */
+#define ZYNQMP_SIP_SVC_CALL_COUNT 0xff00
+#define ZYNQMP_SIP_SVC_UID 0xff01
+#define ZYNQMP_SIP_SVC_VERSION 0xff03
+
+enum pm_api_id {
+ /* Miscellaneous API functions: */
+ PM_GET_API_VERSION = 1, /* Do not change or move */
+ PM_SET_CONFIGURATION,
+ PM_GET_NODE_STATUS,
+ PM_GET_OP_CHARACTERISTIC,
+ PM_REGISTER_NOTIFIER,
+ /* API for suspending of PUs: */
+ PM_REQ_SUSPEND,
+ PM_SELF_SUSPEND,
+ PM_FORCE_POWERDOWN,
+ PM_ABORT_SUSPEND,
+ PM_REQ_WAKEUP,
+ PM_SET_WAKEUP_SOURCE,
+ PM_SYSTEM_SHUTDOWN,
+ /* API for managing PM slaves: */
+ PM_REQ_NODE,
+ PM_RELEASE_NODE,
+ PM_SET_REQUIREMENT,
+ PM_SET_MAX_LATENCY,
+ /* Direct control API functions: */
+ PM_RESET_ASSERT,
+ PM_RESET_GET_STATUS,
+ PM_MMIO_WRITE,
+ PM_MMIO_READ,
+ PM_INIT,
+ PM_FPGA_LOAD,
+ PM_FPGA_GET_STATUS,
+ PM_GET_CHIPID,
+ /* ID 25 is been used by U-boot to process secure boot images */
+ /* Secure library generic API functions */
+ PM_SECURE_SHA = 26,
+ PM_SECURE_RSA,
+ /* Pin control API functions */
+ PM_PINCTRL_REQUEST,
+ PM_PINCTRL_RELEASE,
+ PM_PINCTRL_GET_FUNCTION,
+ PM_PINCTRL_SET_FUNCTION,
+ PM_PINCTRL_CONFIG_PARAM_GET,
+ PM_PINCTRL_CONFIG_PARAM_SET,
+ /* PM IOCTL API */
+ PM_IOCTL,
+ /* API to query information from firmware */
+ PM_QUERY_DATA,
+ /* Clock control API functions */
+ PM_CLOCK_ENABLE,
+ PM_CLOCK_DISABLE,
+ PM_CLOCK_GETSTATE,
+ PM_CLOCK_SETDIVIDER,
+ PM_CLOCK_GETDIVIDER,
+ PM_CLOCK_SETRATE,
+ PM_CLOCK_GETRATE,
+ PM_CLOCK_SETPARENT,
+ PM_CLOCK_GETPARENT,
+ PM_API_MAX
+};
+
+enum pm_node_id {
+ NODE_RPU = 6,
+ NODE_RPU_0,
+ NODE_RPU_1,
+ NODE_GPU_PP_0 = 20,
+ NODE_GPU_PP_1,
+ NODE_USB_0,
+ NODE_USB_1,
+ NODE_TTC_0,
+ NODE_TTC_1,
+ NODE_TTC_2,
+ NODE_TTC_3,
+ NODE_SATA,
+ NODE_ETH_0,
+ NODE_ETH_1,
+ NODE_ETH_2,
+ NODE_ETH_3,
+ NODE_UART_0,
+ NODE_UART_1,
+ NODE_SPI_0,
+ NODE_SPI_1,
+ NODE_I2C_0,
+ NODE_I2C_1,
+ NODE_SD_0,
+ NODE_SD_1,
+ NODE_DP,
+ NODE_GDMA,
+ NODE_ADMA,
+ NODE_NAND,
+ NODE_QSPI,
+ NODE_GPIO,
+ NODE_CAN_0,
+ NODE_CAN_1,
+ NODE_AFI,
+ NODE_APLL,
+ NODE_VPLL,
+ NODE_DPLL,
+ NODE_RPLL,
+ NODE_IOPLL,
+ NODE_DDR,
+ NODE_IPI_APU,
+ NODE_IPI_RPU_0,
+ NODE_GPU,
+ NODE_PCIE,
+ NODE_PCAP,
+ NODE_RTC,
+};
+
+/**
+ * @XST_PM_SUCCESS: Success
+ * @XST_PM_INTERNAL: Unexpected error
+ * @XST_PM_CONFLICT: Conflicting requirements
+ * @XST_PM_NO_ACCESS: Access rights violation
+ * @XST_PM_INVALID_NODE: Does not apply to node passed as argument
+ * @XST_PM_DOUBLE_REQ: Duplicate request
+ * @XST_PM_ABORT_SUSPEND: Target has aborted suspend
+ */
+enum pm_ret_status {
+ XST_PM_SUCCESS = 0,
+ XST_PM_INTERNAL = 2000,
+ XST_PM_CONFLICT,
+ XST_PM_NO_ACCESS,
+ XST_PM_INVALID_NODE,
+ XST_PM_DOUBLE_REQ,
+ XST_PM_ABORT_SUSPEND,
+};
+
+enum pm_reset {
+ XILPM_RESET_START = 999,
+ XILPM_RESET_PCIE_CFG,
+ XILPM_RESET_PCIE_BRIDGE,
+ XILPM_RESET_PCIE_CTRL,
+ XILPM_RESET_DP,
+ XILPM_RESET_SWDT_CRF,
+ XILPM_RESET_AFI_FM5,
+ XILPM_RESET_AFI_FM4,
+ XILPM_RESET_AFI_FM3,
+ XILPM_RESET_AFI_FM2,
+ XILPM_RESET_AFI_FM1,
+ XILPM_RESET_AFI_FM0,
+ XILPM_RESET_GDMA,
+ XILPM_RESET_GPU_PP1,
+ XILPM_RESET_GPU_PP0,
+ XILPM_RESET_GPU,
+ XILPM_RESET_GT,
+ XILPM_RESET_SATA,
+ XILPM_RESET_ACPU3_PWRON,
+ XILPM_RESET_ACPU2_PWRON,
+ XILPM_RESET_ACPU1_PWRON,
+ XILPM_RESET_ACPU0_PWRON,
+ XILPM_RESET_APU_L2,
+ XILPM_RESET_ACPU3,
+ XILPM_RESET_ACPU2,
+ XILPM_RESET_ACPU1,
+ XILPM_RESET_ACPU0,
+ XILPM_RESET_DDR,
+ XILPM_RESET_APM_FPD,
+ XILPM_RESET_SOFT,
+ XILPM_RESET_GEM0,
+ XILPM_RESET_GEM1,
+ XILPM_RESET_GEM2,
+ XILPM_RESET_GEM3,
+ XILPM_RESET_QSPI,
+ XILPM_RESET_UART0,
+ XILPM_RESET_UART1,
+ XILPM_RESET_SPI0,
+ XILPM_RESET_SPI1,
+ XILPM_RESET_SDIO0,
+ XILPM_RESET_SDIO1,
+ XILPM_RESET_CAN0,
+ XILPM_RESET_CAN1,
+ XILPM_RESET_I2C0,
+ XILPM_RESET_I2C1,
+ XILPM_RESET_TTC0,
+ XILPM_RESET_TTC1,
+ XILPM_RESET_TTC2,
+ XILPM_RESET_TTC3,
+ XILPM_RESET_SWDT_CRL,
+ XILPM_RESET_NAND,
+ XILPM_RESET_ADMA,
+ XILPM_RESET_GPIO,
+ XILPM_RESET_IOU_CC,
+ XILPM_RESET_TIMESTAMP,
+ XILPM_RESET_RPU_R50,
+ XILPM_RESET_RPU_R51,
+ XILPM_RESET_RPU_AMBA,
+ XILPM_RESET_OCM,
+ XILPM_RESET_RPU_PGE,
+ XILPM_RESET_USB0_CORERESET,
+ XILPM_RESET_USB1_CORERESET,
+ XILPM_RESET_USB0_HIBERRESET,
+ XILPM_RESET_USB1_HIBERRESET,
+ XILPM_RESET_USB0_APB,
+ XILPM_RESET_USB1_APB,
+ XILPM_RESET_IPI,
+ XILPM_RESET_APM_LPD,
+ XILPM_RESET_RTC,
+ XILPM_RESET_SYSMON,
+ XILPM_RESET_AFI_FM6,
+ XILPM_RESET_LPD_SWDT,
+ XILPM_RESET_FPD,
+ XILPM_RESET_RPU_DBG1,
+ XILPM_RESET_RPU_DBG0,
+ XILPM_RESET_DBG_LPD,
+ XILPM_RESET_DBG_FPD,
+ XILPM_RESET_APLL,
+ XILPM_RESET_DPLL,
+ XILPM_RESET_VPLL,
+ XILPM_RESET_IOPLL,
+ XILPM_RESET_RPLL,
+ XILPM_RESET_GPO3_PL_0,
+ XILPM_RESET_GPO3_PL_1,
+ XILPM_RESET_GPO3_PL_2,
+ XILPM_RESET_GPO3_PL_3,
+ XILPM_RESET_GPO3_PL_4,
+ XILPM_RESET_GPO3_PL_5,
+ XILPM_RESET_GPO3_PL_6,
+ XILPM_RESET_GPO3_PL_7,
+ XILPM_RESET_GPO3_PL_8,
+ XILPM_RESET_GPO3_PL_9,
+ XILPM_RESET_GPO3_PL_10,
+ XILPM_RESET_GPO3_PL_11,
+ XILPM_RESET_GPO3_PL_12,
+ XILPM_RESET_GPO3_PL_13,
+ XILPM_RESET_GPO3_PL_14,
+ XILPM_RESET_GPO3_PL_15,
+ XILPM_RESET_GPO3_PL_16,
+ XILPM_RESET_GPO3_PL_17,
+ XILPM_RESET_GPO3_PL_18,
+ XILPM_RESET_GPO3_PL_19,
+ XILPM_RESET_GPO3_PL_20,
+ XILPM_RESET_GPO3_PL_21,
+ XILPM_RESET_GPO3_PL_22,
+ XILPM_RESET_GPO3_PL_23,
+ XILPM_RESET_GPO3_PL_24,
+ XILPM_RESET_GPO3_PL_25,
+ XILPM_RESET_GPO3_PL_26,
+ XILPM_RESET_GPO3_PL_27,
+ XILPM_RESET_GPO3_PL_28,
+ XILPM_RESET_GPO3_PL_29,
+ XILPM_RESET_GPO3_PL_30,
+ XILPM_RESET_GPO3_PL_31,
+ XILPM_RESET_RPU_LS,
+ XILPM_RESET_PS_ONLY,
+ XILPM_RESET_PL,
+ XILPM_RESET_END
+};
+
extern bool zynqmp_eemi(struct cpu_user_regs *regs);
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v3 4/6] xen/arm: zynqmp: eemi access control
2018-08-11 0:01 [PATCH v3 0/6] zynqmp: Add forwarding of platform specific firmware calls Stefano Stabellini
` (2 preceding siblings ...)
2018-08-11 0:01 ` [PATCH v3 3/6] xen/arm: zynqmp: introduce zynqmp specific defines Stefano Stabellini
@ 2018-08-11 0:01 ` Stefano Stabellini
2018-08-28 16:05 ` Julien Grall
2018-08-11 0:01 ` [PATCH v3 5/6] xen/arm: zynqmp: implement zynqmp_eemi Stefano Stabellini
2018-08-11 0:01 ` [PATCH v3 6/6] xen/arm: zynqmp: Remove blacklist of ZynqMP's PM node Stefano Stabellini
5 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2018-08-11 0:01 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, Stefano Stabellini, julien.grall, sstabellini
From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Introduce data structs to implement basic access controls.
Introduce the following three functions:
domain_has_node_access: check access to the node
domain_has_reset_access: check access to the reset line
domain_has_mmio_access: check access to the register
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 783 ++++++++++++++++++++++++++++
1 file changed, 783 insertions(+)
diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
index c3a19e9..62cc15c 100644
--- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
+++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
@@ -16,6 +16,74 @@
* GNU General Public License for more details.
*/
+/*
+ * EEMI Power Management API access
+ *
+ * Refs:
+ * https://www.xilinx.com/support/documentation/user_guides/ug1200-eemi-api.pdf
+ *
+ * Background:
+ * The ZynqMP has a subsystem named the PMU with a CPU and special devices
+ * dedicated to running Power Management Firmware. Other masters in the
+ * system need to send requests to the PMU in order to for example:
+ * * Manage power state
+ * * Configure clocks
+ * * Program bitstreams for the programmable logic
+ * * etc
+ *
+ * Although the details of the setup are configurable, in the common case
+ * the PMU lives in the Secure world. NS World cannot directly communicate
+ * with it and must use proxy services from ARM Trusted Firmware to reach
+ * the PMU.
+ *
+ * Power Management on the ZynqMP is implemented in a layered manner.
+ * The PMU knows about various masters and will enforce access controls
+ * based on a pre-configured partitioning. This configuration dictates
+ * which devices are owned by the various masters and the PMU FW makes sure
+ * that a given master cannot turn off a device that it does not own or that
+ * is in use by other masters.
+ *
+ * The PMU is not aware of multiple execution states in masters.
+ * For example, it treats the ARMv8 cores as single units and does not
+ * distinguish between Secure vs NS OS's nor does it know about Hypervisors
+ * and multiple guests. It is up to software on the ARMv8 cores to present
+ * a unified view of its power requirements.
+ *
+ * To implement this unified view, ARM Trusted Firmware at EL3 provides
+ * access to the PM API via SMC calls. ARM Trusted Firmware is responsible
+ * for mediating between the Secure and the NS world, rejecting SMC calls
+ * that request changes that are not allowed.
+ *
+ * Xen running above ATF owns the NS world and is responsible for presenting
+ * unified PM requests taking all guests and the hypervisor into account.
+ *
+ * Implementation:
+ * The PM API contains different classes of calls.
+ * Certain calls are harmless to expose to any guest.
+ * These include calls to get the PM API Version, or to read out the version
+ * of the chip we're running on.
+ *
+ * In order to correctly virtualize these calls, we need to know if
+ * guests issuing these calls have ownership of the given device.
+ * The approach taken here is to map PM API Nodes identifying
+ * a device into base addresses for registers that belong to that
+ * same device.
+ *
+ * If the guest has access to devices registers, we give the guest
+ * access to PM API calls that affect that device. This is implemented
+ * by pm_node_access and domain_has_node_access().
+ *
+ * MMIO access:
+ * These calls allow guests to access certain memory ranges. These ranges
+ * are typically protected for secure-world access only and also from
+ * certain masters only, so guests cannot access them directly.
+ * Registers within the memory regions affect certain nodes. In this case,
+ * our input is an address and we map that address into a node. If the
+ * guest has ownership of that node, the access is allowed.
+ * Some registers contain bitfields and a single register may contain
+ * bits that affect multiple nodes.
+ */
+
#include <xen/iocap.h>
#include <xen/sched.h>
#include <xen/types.h>
@@ -23,6 +91,721 @@
#include <asm/regs.h>
#include <asm/platforms/xilinx-zynqmp-eemi.h>
+struct pm_access
+{
+ paddr_t addr;
+ bool hwdom_access; /* HW domain gets access regardless. */
+};
+
+/*
+ * This table maps a node into a memory address.
+ * If a guest has access to the address, it has enough control
+ * over the node to grant it access to EEMI calls for that node.
+ */
+static const struct pm_access pm_node_access[] = {
+ /* MM_RPU grants access to all RPU Nodes. */
+ [NODE_RPU] = { MM_RPU },
+ [NODE_RPU_0] = { MM_RPU },
+ [NODE_RPU_1] = { MM_RPU },
+ [NODE_IPI_RPU_0] = { MM_RPU },
+
+ /* GPU nodes. */
+ [NODE_GPU] = { MM_GPU },
+ [NODE_GPU_PP_0] = { MM_GPU },
+ [NODE_GPU_PP_1] = { MM_GPU },
+
+ [NODE_USB_0] = { MM_USB3_0_XHCI },
+ [NODE_USB_1] = { MM_USB3_1_XHCI },
+ [NODE_TTC_0] = { MM_TTC0 },
+ [NODE_TTC_1] = { MM_TTC1 },
+ [NODE_TTC_2] = { MM_TTC2 },
+ [NODE_TTC_3] = { MM_TTC3 },
+ [NODE_SATA] = { MM_SATA_AHCI_HBA },
+ [NODE_ETH_0] = { MM_GEM0 },
+ [NODE_ETH_1] = { MM_GEM1 },
+ [NODE_ETH_2] = { MM_GEM2 },
+ [NODE_ETH_3] = { MM_GEM3 },
+ [NODE_UART_0] = { MM_UART0 },
+ [NODE_UART_1] = { MM_UART1 },
+ [NODE_SPI_0] = { MM_SPI0 },
+ [NODE_SPI_1] = { MM_SPI1 },
+ [NODE_I2C_0] = { MM_I2C0 },
+ [NODE_I2C_1] = { MM_I2C1 },
+ [NODE_SD_0] = { MM_SD0 },
+ [NODE_SD_1] = { MM_SD1 },
+ [NODE_DP] = { MM_DP },
+
+ /* Guest with GDMA Channel 0 gets PM access. Other guests don't. */
+ [NODE_GDMA] = { MM_GDMA_CH0 },
+ /* Guest with ADMA Channel 0 gets PM access. Other guests don't. */
+ [NODE_ADMA] = { MM_ADMA_CH0 },
+
+ [NODE_NAND] = { MM_NAND },
+ [NODE_QSPI] = { MM_QSPI },
+ [NODE_GPIO] = { MM_GPIO },
+ [NODE_CAN_0] = { MM_CAN0 },
+ [NODE_CAN_1] = { MM_CAN1 },
+
+ /* Only for the hardware domain. */
+ [NODE_AFI] = { .hwdom_access = true },
+ [NODE_APLL] = { .hwdom_access = true },
+ [NODE_VPLL] = { .hwdom_access = true },
+ [NODE_DPLL] = { .hwdom_access = true },
+ [NODE_RPLL] = { .hwdom_access = true },
+ [NODE_IOPLL] = { .hwdom_access = true },
+ [NODE_DDR] = { .hwdom_access = true },
+ [NODE_IPI_APU] = { .hwdom_access = true },
+ [NODE_PCAP] = { .hwdom_access = true },
+
+ [NODE_PCIE] = { MM_PCIE_ATTRIB },
+ [NODE_RTC] = { MM_RTC },
+};
+
+/*
+ * This table maps reset line IDs into a memory address.
+ * If a guest has access to the address, it has enough control
+ * over the affected node to grant it access to EEMI calls for
+ * resetting that node.
+ */
+#define XILPM_RESET_IDX(n) (n - XILPM_RESET_PCIE_CFG)
+static const struct pm_access pm_reset_access[] = {
+ [XILPM_RESET_IDX(XILPM_RESET_PCIE_CFG)] = { MM_AXIPCIE_MAIN },
+ [XILPM_RESET_IDX(XILPM_RESET_PCIE_BRIDGE)] = { MM_PCIE_ATTRIB },
+ [XILPM_RESET_IDX(XILPM_RESET_PCIE_CTRL)] = { MM_PCIE_ATTRIB },
+
+ [XILPM_RESET_IDX(XILPM_RESET_DP)] = { MM_DP },
+ [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRF)] = { MM_SWDT },
+ [XILPM_RESET_IDX(XILPM_RESET_AFI_FM5)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_AFI_FM4)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_AFI_FM3)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_AFI_FM2)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_AFI_FM1)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_AFI_FM0)] = { .hwdom_access = true },
+
+ /* Channel 0 grants PM access. */
+ [XILPM_RESET_IDX(XILPM_RESET_GDMA)] = { MM_GDMA_CH0 },
+ [XILPM_RESET_IDX(XILPM_RESET_GPU_PP1)] = { MM_GPU },
+ [XILPM_RESET_IDX(XILPM_RESET_GPU_PP0)] = { MM_GPU },
+ [XILPM_RESET_IDX(XILPM_RESET_GT)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_SATA)] = { MM_SATA_AHCI_HBA },
+
+ [XILPM_RESET_IDX(XILPM_RESET_APM_FPD)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_SOFT)] = { .hwdom_access = true },
+
+ [XILPM_RESET_IDX(XILPM_RESET_GEM0)] = { MM_GEM0 },
+ [XILPM_RESET_IDX(XILPM_RESET_GEM1)] = { MM_GEM1 },
+ [XILPM_RESET_IDX(XILPM_RESET_GEM2)] = { MM_GEM2 },
+ [XILPM_RESET_IDX(XILPM_RESET_GEM3)] = { MM_GEM3 },
+
+ [XILPM_RESET_IDX(XILPM_RESET_QSPI)] = { MM_QSPI },
+ [XILPM_RESET_IDX(XILPM_RESET_UART0)] = { MM_UART0 },
+ [XILPM_RESET_IDX(XILPM_RESET_UART1)] = { MM_UART1 },
+ [XILPM_RESET_IDX(XILPM_RESET_SPI0)] = { MM_SPI0 },
+ [XILPM_RESET_IDX(XILPM_RESET_SPI1)] = { MM_SPI1 },
+ [XILPM_RESET_IDX(XILPM_RESET_SDIO0)] = { MM_SD0 },
+ [XILPM_RESET_IDX(XILPM_RESET_SDIO1)] = { MM_SD1 },
+ [XILPM_RESET_IDX(XILPM_RESET_CAN0)] = { MM_CAN0 },
+ [XILPM_RESET_IDX(XILPM_RESET_CAN1)] = { MM_CAN1 },
+ [XILPM_RESET_IDX(XILPM_RESET_I2C0)] = { MM_I2C0 },
+ [XILPM_RESET_IDX(XILPM_RESET_I2C1)] = { MM_I2C1 },
+ [XILPM_RESET_IDX(XILPM_RESET_TTC0)] = { MM_TTC0 },
+ [XILPM_RESET_IDX(XILPM_RESET_TTC1)] = { MM_TTC1 },
+ [XILPM_RESET_IDX(XILPM_RESET_TTC2)] = { MM_TTC2 },
+ [XILPM_RESET_IDX(XILPM_RESET_TTC3)] = { MM_TTC3 },
+ [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRL)] = { MM_SWDT },
+ [XILPM_RESET_IDX(XILPM_RESET_NAND)] = { MM_NAND },
+ [XILPM_RESET_IDX(XILPM_RESET_ADMA)] = { MM_ADMA_CH0 },
+ [XILPM_RESET_IDX(XILPM_RESET_GPIO)] = { MM_GPIO },
+ [XILPM_RESET_IDX(XILPM_RESET_IOU_CC)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_TIMESTAMP)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_RPU_R50)] = { MM_RPU },
+ [XILPM_RESET_IDX(XILPM_RESET_RPU_R51)] = { MM_RPU },
+ [XILPM_RESET_IDX(XILPM_RESET_RPU_AMBA)] = { MM_RPU },
+ [XILPM_RESET_IDX(XILPM_RESET_OCM)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_RPU_PGE)] = { MM_RPU },
+
+ [XILPM_RESET_IDX(XILPM_RESET_USB0_CORERESET)] = { MM_USB3_0_XHCI },
+ [XILPM_RESET_IDX(XILPM_RESET_USB0_HIBERRESET)] = { MM_USB3_0_XHCI },
+ [XILPM_RESET_IDX(XILPM_RESET_USB0_APB)] = { MM_USB3_0_XHCI },
+
+ [XILPM_RESET_IDX(XILPM_RESET_USB1_CORERESET)] = { MM_USB3_1_XHCI },
+ [XILPM_RESET_IDX(XILPM_RESET_USB1_HIBERRESET)] = { MM_USB3_1_XHCI },
+ [XILPM_RESET_IDX(XILPM_RESET_USB1_APB)] = { MM_USB3_1_XHCI },
+
+ [XILPM_RESET_IDX(XILPM_RESET_IPI)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_APM_LPD)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_RTC)] = { MM_RTC },
+ [XILPM_RESET_IDX(XILPM_RESET_SYSMON)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_AFI_FM6)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_LPD_SWDT)] = { MM_SWDT },
+ [XILPM_RESET_IDX(XILPM_RESET_FPD)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG1)] = { MM_RPU },
+ [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG0)] = { MM_RPU },
+ [XILPM_RESET_IDX(XILPM_RESET_DBG_LPD)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_DBG_FPD)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_APLL)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_DPLL)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_VPLL)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_IOPLL)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_RPLL)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_0)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_1)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_2)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_3)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_4)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_5)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_6)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_7)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_8)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_9)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_10)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_11)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_12)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_13)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_14)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_15)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_16)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_17)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_18)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_19)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_20)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_21)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_22)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_23)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_24)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_25)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_26)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_27)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_28)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_29)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_30)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_31)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_RPU_LS)] = { MM_RPU },
+ [XILPM_RESET_IDX(XILPM_RESET_PS_ONLY)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_PL)] = { .hwdom_access = true },
+};
+
+/*
+ * This table maps reset line IDs into a memory address.
+ * If a guest has access to the address, it has enough control
+ * over the affected node to grant it access to EEMI calls for
+ * resetting that node.
+ */
+static const struct {
+ paddr_t start;
+ paddr_t size;
+ uint32_t mask; /* Zero means no mask, i.e all bits. */
+ enum pm_node_id node;
+ bool hwdom_access;
+ bool readonly;
+} pm_mmio_access[] = {
+ {
+ .start = MM_CRF_APB + R_CRF_APLL_CTRL,
+ .size = R_CRF_ACPU_CTRL,
+ .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_IOPLL_CTRL,
+ .size = R_CRL_RPLL_TO_FPD_CTRL,
+ .hwdom_access = true
+ },
+ {
+ .start = MM_CRF_APB + R_CRF_DP_VIDEO_REF_CTRL,
+ .size = 4, .node = NODE_DP
+ },
+ {
+ .start = MM_CRF_APB + R_CRF_DP_AUDIO_REF_CTRL,
+ .size = 4, .node = NODE_DP
+ },
+ {
+ .start = MM_CRF_APB + R_CRF_DP_STC_REF_CTRL,
+ .size = 4, .node = NODE_DP
+ },
+ {
+ .start = MM_CRF_APB + R_CRF_GPU_REF_CTRL,
+ .size = 4, .node = NODE_GPU
+ },
+ {
+ .start = MM_CRF_APB + R_CRF_SATA_REF_CTRL,
+ .size = 4, .node = NODE_SATA
+ },
+ {
+ .start = MM_CRF_APB + R_CRF_PCIE_REF_CTRL,
+ .size = 4, .node = NODE_PCIE
+ },
+ {
+ .start = MM_CRF_APB + R_CRF_GDMA_REF_CTRL,
+ .size = 4, .node = NODE_GDMA
+ },
+ {
+ .start = MM_CRF_APB + R_CRF_DPDMA_REF_CTRL,
+ .size = 4, .node = NODE_DP
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_USB3_DUAL_REF_CTRL,
+ .size = 4, .node = NODE_USB_0
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_USB0_BUS_REF_CTRL,
+ .size = 4, .node = NODE_USB_0
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_USB1_BUS_REF_CTRL,
+ .size = 4, .node = NODE_USB_1
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_USB1_BUS_REF_CTRL,
+ .size = 4, .node = NODE_USB_1
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_GEM0_REF_CTRL,
+ .size = 4, .node = NODE_ETH_0
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_GEM1_REF_CTRL,
+ .size = 4, .node = NODE_ETH_1
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_GEM2_REF_CTRL,
+ .size = 4, .node = NODE_ETH_2
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_GEM3_REF_CTRL,
+ .size = 4, .node = NODE_ETH_3
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_QSPI_REF_CTRL,
+ .size = 4, .node = NODE_QSPI
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_SDIO0_REF_CTRL,
+ .size = 4, .node = NODE_SD_0
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_SDIO1_REF_CTRL,
+ .size = 4, .node = NODE_SD_1
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_UART0_REF_CTRL,
+ .size = 4, .node = NODE_UART_0
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_UART1_REF_CTRL,
+ .size = 4, .node = NODE_UART_1
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_SPI0_REF_CTRL,
+ .size = 4, .node = NODE_SPI_0
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_SPI1_REF_CTRL,
+ .size = 4, .node = NODE_SPI_1
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_CAN0_REF_CTRL,
+ .size = 4, .node = NODE_CAN_0
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_CAN1_REF_CTRL,
+ .size = 4, .node = NODE_CAN_1
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_CPU_R5_CTRL,
+ .size = 4, .node = NODE_RPU
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_IOU_SWITCH_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_CSU_PLL_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_PCAP_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_LPD_SWITCH_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_LPD_LSBUS_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_DBG_LPD_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_NAND_REF_CTRL,
+ .size = 4, .node = NODE_NAND
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_ADMA_REF_CTRL,
+ .size = 4, .node = NODE_ADMA
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_PL0_REF_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_PL1_REF_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_PL2_REF_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_PL3_REF_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_PL0_THR_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_PL1_THR_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_PL2_THR_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_PL3_THR_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_PL0_THR_CNT,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_PL1_THR_CNT,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_PL2_THR_CNT,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_PL3_THR_CNT,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_GEM_TSU_REF_CTRL,
+ .size = 4, .node = NODE_ETH_0
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_DLL_REF_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_AMS_REF_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_I2C0_REF_CTRL,
+ .size = 4, .node = NODE_I2C_0
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_I2C1_REF_CTRL,
+ .size = 4, .node = NODE_I2C_1
+ },
+ {
+ .start = MM_CRL_APB + R_CRL_TIMESTAMP_REF_CTRL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_MIO_PIN_0,
+ .size = R_IOU_SLCR_MIO_MST_TRI2,
+ .hwdom_access = true
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_WDT_CLK_SEL,
+ .hwdom_access = true
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_CAN_MIO_CTRL,
+ .size = 4, .mask = 0x1ff, .node = NODE_CAN_0
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_CAN_MIO_CTRL,
+ .size = 4, .mask = 0x1ff << 15, .node = NODE_CAN_1
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_GEM_CLK_CTRL,
+ .size = 4, .mask = 0xf, .node = NODE_ETH_0
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_GEM_CLK_CTRL,
+ .size = 4, .mask = 0xf << 5, .node = NODE_ETH_1
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_GEM_CLK_CTRL,
+ .size = 4, .mask = 0xf << 10, .node = NODE_ETH_2
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_GEM_CLK_CTRL,
+ .size = 4, .mask = 0xf << 15, .node = NODE_ETH_3
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_GEM_CLK_CTRL,
+ .size = 4, .mask = 0x7 << 20, .hwdom_access = true
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_SDIO_CLK_CTRL,
+ .size = 4, .mask = 0x7, .node = NODE_SD_0
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_SDIO_CLK_CTRL,
+ .size = 4, .mask = 0x7 << 17, .node = NODE_SD_1
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_CTRL_REG_SD,
+ .size = 4, .mask = 0x1, .node = NODE_SD_0
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_CTRL_REG_SD,
+ .size = 4, .mask = 0x1 << 15, .node = NODE_SD_1
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_SD_ITAPDLY,
+ .size = R_IOU_SLCR_SD_CDN_CTRL,
+ .mask = 0x3ff << 0, .node = NODE_SD_0
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_SD_ITAPDLY,
+ .size = R_IOU_SLCR_SD_CDN_CTRL,
+ .mask = 0x3ff << 16, .node = NODE_SD_1
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_GEM_CTRL,
+ .size = 4, .mask = 0x3 << 0, .node = NODE_ETH_0
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_GEM_CTRL,
+ .size = 4, .mask = 0x3 << 2, .node = NODE_ETH_1
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_GEM_CTRL,
+ .size = 4, .mask = 0x3 << 4, .node = NODE_ETH_2
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_GEM_CTRL,
+ .size = 4, .mask = 0x3 << 6, .node = NODE_ETH_3
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_TTC_APB_CLK,
+ .size = 4, .mask = 0x3 << 0, .node = NODE_TTC_0
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_TTC_APB_CLK,
+ .size = 4, .mask = 0x3 << 2, .node = NODE_TTC_1
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_TTC_APB_CLK,
+ .size = 4, .mask = 0x3 << 4, .node = NODE_TTC_2
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_TTC_APB_CLK,
+ .size = 4, .mask = 0x3 << 6, .node = NODE_TTC_3
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_TAPDLY_BYPASS,
+ .size = 4, .mask = 0x3, .node = NODE_NAND
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_TAPDLY_BYPASS,
+ .size = 4, .mask = 0x1 << 2, .node = NODE_QSPI
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_COHERENT_CTRL,
+ .size = 4, .mask = 0xf << 0, .node = NODE_ETH_0
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_COHERENT_CTRL,
+ .size = 4, .mask = 0xf << 4, .node = NODE_ETH_1
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_COHERENT_CTRL,
+ .size = 4, .mask = 0xf << 8, .node = NODE_ETH_2
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_COHERENT_CTRL,
+ .size = 4, .mask = 0xf << 12, .node = NODE_ETH_3
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_COHERENT_CTRL,
+ .size = 4, .mask = 0xf << 16, .node = NODE_SD_0
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_COHERENT_CTRL,
+ .size = 4, .mask = 0xf << 20, .node = NODE_SD_1
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_COHERENT_CTRL,
+ .size = 4, .mask = 0xf << 24, .node = NODE_NAND
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_COHERENT_CTRL,
+ .size = 4, .mask = 0xf << 28, .node = NODE_QSPI
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_VIDEO_PSS_CLK_SEL,
+ .size = 4, .hwdom_access = true
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_RAM_GEM0,
+ .size = 4, .node = NODE_ETH_0
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_RAM_GEM1,
+ .size = 4, .node = NODE_ETH_1
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_RAM_GEM2,
+ .size = 4, .node = NODE_ETH_2
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_RAM_GEM3,
+ .size = 4, .node = NODE_ETH_3
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_RAM_SD0,
+ .size = 4, .node = NODE_SD_0
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_RAM_SD1,
+ .size = 4, .node = NODE_SD_1
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_RAM_CAN0,
+ .size = 4, .node = NODE_CAN_0
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_RAM_CAN1,
+ .size = 4, .node = NODE_CAN_1
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_RAM_LQSPI,
+ .size = 4, .node = NODE_QSPI
+ },
+ {
+ .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_RAM_NAND,
+ .size = 4, .node = NODE_NAND
+ },
+ {
+ .start = MM_PMU_GLOBAL + R_PMU_GLOBAL_PWR_STATE,
+ .size = 4, .readonly = true, .hwdom_access = true
+ },
+ {
+ .start = MM_PMU_GLOBAL + R_PMU_GLOBAL_GLOBAL_GEN_STORAGE0,
+ .size = R_PMU_GLOBAL_PERS_GLOB_GEN_STORAGE7,
+ .readonly = true, .hwdom_access = true
+ },
+ {
+ /* Universal read-only access to CRF. Linux CCF needs this. */
+ .start = MM_CRF_APB, .size = 0x104, .readonly = true,
+ .hwdom_access = true
+ },
+ {
+ /* Universal read-only access to CRL. Linux CCF needs this. */
+ .start = MM_CRL_APB, .size = 0x284, .readonly = true,
+ .hwdom_access = true
+ }
+};
+
+static bool pm_check_access(const struct pm_access *acl, struct domain *d,
+ uint32_t idx)
+{
+ unsigned long mfn;
+
+ if ( acl[idx].hwdom_access && is_hardware_domain(d) )
+ return true;
+
+ if ( !acl[idx].addr )
+ return false;
+
+ mfn = PFN_DOWN(acl[idx].addr);
+ return iomem_access_permitted(d, mfn, mfn);
+}
+
+/* Check if a domain has access to a node. */
+static bool domain_has_node_access(struct domain *d, uint32_t nodeid)
+{
+ if ( nodeid > ARRAY_SIZE(pm_node_access) )
+ return false;
+
+ return pm_check_access(pm_node_access, d, nodeid);
+}
+
+/* Check if a domain has access to a reset line. */
+static bool domain_has_reset_access(struct domain *d, uint32_t rst)
+{
+ if ( rst < XILPM_RESET_PCIE_CFG )
+ return false;
+
+ rst -= XILPM_RESET_PCIE_CFG;
+
+ if ( rst > ARRAY_SIZE(pm_reset_access) )
+ return false;
+
+ return pm_check_access(pm_reset_access, d, rst);
+}
+
+/*
+ * Check if a given domain has access to perform an indirect
+ * MMIO access.
+ *
+ * If the provided mask is invalid, it will be fixed up.
+ */
+static bool domain_has_mmio_access(struct domain *d,
+ bool write, paddr_t addr,
+ uint32_t *mask)
+{
+ unsigned int i;
+ bool ret = false;
+ uint32_t prot_mask = 0;
+
+ /*
+ * The hardware domain gets read access to everything.
+ * Lower layers will do further filtering.
+ */
+ if ( !write && is_hardware_domain(d) )
+ return true;
+
+ /* Scan the ACL. */
+ for ( i = 0; i < ARRAY_SIZE(pm_mmio_access); i++ )
+ {
+ if ( addr < pm_mmio_access[i].start )
+ return false;
+ if ( addr > pm_mmio_access[i].start + pm_mmio_access[i].size )
+ continue;
+
+ if ( write && pm_mmio_access[i].readonly )
+ return false;
+ if ( pm_mmio_access[i].hwdom_access && !is_hardware_domain(d) )
+ return false;
+ if ( !domain_has_node_access(d, pm_mmio_access[i].node) )
+ return false;
+
+ /* We've got access to this reg (or parts of it). */
+ ret = true;
+
+ /* Permit write access to selected bits. */
+ prot_mask |= pm_mmio_access[i].mask ?: 0xFFFFFFFF;
+ break;
+ }
+
+ /* Masking only applies to writes. */
+ if ( write )
+ *mask &= prot_mask;
+
+ return ret;
+}
+
bool zynqmp_eemi(struct cpu_user_regs *regs)
{
return false;
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v3 5/6] xen/arm: zynqmp: implement zynqmp_eemi
2018-08-11 0:01 [PATCH v3 0/6] zynqmp: Add forwarding of platform specific firmware calls Stefano Stabellini
` (3 preceding siblings ...)
2018-08-11 0:01 ` [PATCH v3 4/6] xen/arm: zynqmp: eemi access control Stefano Stabellini
@ 2018-08-11 0:01 ` Stefano Stabellini
2018-08-28 16:29 ` Julien Grall
2018-08-11 0:01 ` [PATCH v3 6/6] xen/arm: zynqmp: Remove blacklist of ZynqMP's PM node Stefano Stabellini
5 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2018-08-11 0:01 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, Stefano Stabellini, julien.grall, sstabellini
From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
zynqmp_eemi uses the defined functions and structs to decide whether to
make a call to the firmware, or to simply return a predefined value.
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 143 +++++++++++++++++++++++++++-
1 file changed, 142 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
index 62cc15c..1dbdf04 100644
--- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
+++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
@@ -808,7 +808,148 @@ static bool domain_has_mmio_access(struct domain *d,
bool zynqmp_eemi(struct cpu_user_regs *regs)
{
- return false;
+ struct arm_smccc_res res;
+ bool is_mmio_write = false;
+ uint32_t fid = get_user_reg(regs, 0);
+ uint32_t nodeid = get_user_reg(regs, 1);
+ unsigned int pm_fn = fid & 0xFFFF;
+ enum pm_ret_status ret;
+
+ switch ( pm_fn )
+ {
+ /*
+ * We can't allow CPUs to suspend without Xen knowing about it.
+ * We accept but ignore the request and wait for the guest to issue
+ * a WFI which Xen will trap and act accordingly upon.
+ */
+ case PM_SELF_SUSPEND:
+ ret = XST_PM_SUCCESS;
+ goto done;
+
+ case PM_GET_NODE_STATUS:
+ /* API for PUs. */
+ case PM_REQ_SUSPEND:
+ case PM_FORCE_POWERDOWN:
+ case PM_ABORT_SUSPEND:
+ case PM_REQ_WAKEUP:
+ case PM_SET_WAKEUP_SOURCE:
+ /* API for slaves. */
+ case PM_REQ_NODE:
+ case PM_RELEASE_NODE:
+ case PM_SET_REQUIREMENT:
+ case PM_SET_MAX_LATENCY:
+ if ( !domain_has_node_access(current->domain, nodeid) )
+ {
+ gprintk(XENLOG_WARNING,
+ "zynqmp-pm: fn=%u No access to node %u\n", pm_fn, nodeid);
+ ret = XST_PM_NO_ACCESS;
+ goto done;
+ }
+ goto forward_to_fw;
+
+ case PM_RESET_ASSERT:
+ case PM_RESET_GET_STATUS:
+ if ( !domain_has_reset_access(current->domain, nodeid) )
+ {
+ gprintk(XENLOG_WARNING,
+ "zynqmp-pm: fn=%u No access to reset %u\n", pm_fn, nodeid);
+ ret = XST_PM_NO_ACCESS;
+ goto done;
+ }
+ goto forward_to_fw;
+
+ /* These calls are safe and always allowed. */
+ case ZYNQMP_SIP_SVC_CALL_COUNT:
+ case ZYNQMP_SIP_SVC_UID:
+ case ZYNQMP_SIP_SVC_VERSION:
+ case PM_GET_TRUSTZONE_VERSION:
+ case PM_GET_API_VERSION:
+ case PM_GET_CHIPID:
+ goto forward_to_fw;
+
+ /* Mediated MMIO access. */
+ case PM_MMIO_WRITE:
+ is_mmio_write = true;
+ /* Fallthrough. */
+ case PM_MMIO_READ:
+ {
+ uint32_t mask = get_user_reg(regs, 1) >> 32;
+ if ( !domain_has_mmio_access(current->domain,
+ is_mmio_write,
+ nodeid, &mask) )
+ {
+ gprintk(XENLOG_WARNING,
+ "eemi: fn=%u No access to MMIO %s %u\n",
+ pm_fn, is_mmio_write ? "write" : "read", nodeid);
+ ret = XST_PM_NO_ACCESS;
+ goto done;
+ }
+ set_user_reg(regs, 1, ((uint64_t)mask << 32) | nodeid);
+ goto forward_to_fw;
+ }
+
+ /* Exclusive to the hardware domain. */
+ case PM_INIT:
+ case PM_SET_CONFIGURATION:
+ case PM_FPGA_LOAD:
+ case PM_FPGA_GET_STATUS:
+ case PM_SECURE_SHA:
+ case PM_SECURE_RSA:
+ case PM_PINCTRL_SET_FUNCTION:
+ case PM_PINCTRL_REQUEST:
+ case PM_PINCTRL_RELEASE:
+ case PM_PINCTRL_GET_FUNCTION:
+ case PM_PINCTRL_CONFIG_PARAM_GET:
+ case PM_PINCTRL_CONFIG_PARAM_SET:
+ case PM_IOCTL:
+ case PM_QUERY_DATA:
+ case PM_CLOCK_ENABLE:
+ case PM_CLOCK_DISABLE:
+ case PM_CLOCK_GETSTATE:
+ case PM_CLOCK_GETDIVIDER:
+ case PM_CLOCK_SETDIVIDER:
+ case PM_CLOCK_SETRATE:
+ case PM_CLOCK_GETRATE:
+ case PM_CLOCK_SETPARENT:
+ case PM_CLOCK_GETPARENT:
+ if ( !is_hardware_domain(current->domain) )
+ {
+ gprintk(XENLOG_WARNING, "eemi: fn=%u No access", pm_fn);
+ ret = XST_PM_NO_ACCESS;
+ goto done;
+ }
+ goto forward_to_fw;
+
+ /* These calls are never allowed. */
+ case PM_SYSTEM_SHUTDOWN:
+ ret = XST_PM_NO_ACCESS;
+ goto done;
+
+ default:
+ gprintk(XENLOG_WARNING, "zynqmp-pm: Unhandled PM Call: %u\n", fid);
+ return false;
+ }
+
+forward_to_fw:
+ arm_smccc_1_1_smc(get_user_reg(regs, 0),
+ 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;
+
+done:
+ set_user_reg(regs, 0, ret);
+ return true;
}
/*
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v3 6/6] xen/arm: zynqmp: Remove blacklist of ZynqMP's PM node
2018-08-11 0:01 [PATCH v3 0/6] zynqmp: Add forwarding of platform specific firmware calls Stefano Stabellini
` (4 preceding siblings ...)
2018-08-11 0:01 ` [PATCH v3 5/6] xen/arm: zynqmp: implement zynqmp_eemi Stefano Stabellini
@ 2018-08-11 0:01 ` Stefano Stabellini
5 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2018-08-11 0:01 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, Stefano Stabellini, julien.grall, sstabellini
From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Stop blacklisting ZynqMP's power management node. It is now possible
since we allow the hardware domain to issue HVC/SMC calls to firmware.
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
xen/arch/arm/platforms/xilinx-zynqmp.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/xen/arch/arm/platforms/xilinx-zynqmp.c b/xen/arch/arm/platforms/xilinx-zynqmp.c
index c318cf5..845e143 100644
--- a/xen/arch/arm/platforms/xilinx-zynqmp.c
+++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
@@ -27,13 +27,6 @@ static const char * const zynqmp_dt_compat[] __initconst =
NULL
};
-static const struct dt_device_match zynqmp_blacklist_dev[] __initconst =
-{
- /* Power management is not yet supported. */
- DT_MATCH_COMPATIBLE("xlnx,zynqmp-pm"),
- { /* sentinel */ },
-};
-
static bool zynqmp_smc(struct cpu_user_regs *regs)
{
if ( !is_64bit_domain(current->domain) )
@@ -48,7 +41,6 @@ static bool zynqmp_smc(struct cpu_user_regs *regs)
PLATFORM_START(xilinx_zynqmp, "Xilinx ZynqMP")
.compatible = zynqmp_dt_compat,
.smc = zynqmp_smc,
- .blacklist_dev = zynqmp_blacklist_dev,
PLATFORM_END
/*
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v3 1/6] xen/arm: introduce platform_smc
2018-08-11 0:01 ` [PATCH v3 1/6] xen/arm: introduce platform_smc Stefano Stabellini
@ 2018-08-23 15:37 ` Julien Grall
2018-08-23 23:40 ` Stefano Stabellini
0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-08-23 15:37 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: edgar.iglesias, Stefano Stabellini
Hi Stefano,
On 08/11/2018 01:01 AM, Stefano Stabellini wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
I am not sure why there is 2 From here? This seems to apply whole the
whole series.
>
> Introduce platform_smc as a way to handle firmware calls that Xen does
> not know about in a platform specific way. This is particularly useful
> for implementing the SiP (SoC implementation specific) service calls.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> xen/arch/arm/platform.c | 8 ++++++++
> xen/arch/arm/vsmc.c | 4 ++++
> xen/include/asm-arm/platform.h | 3 +++
> 3 files changed, 15 insertions(+)
>
> diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
> index 3f2989e..9e19023 100644
> --- a/xen/arch/arm/platform.c
> +++ b/xen/arch/arm/platform.c
> @@ -127,6 +127,14 @@ void platform_poweroff(void)
> platform->poweroff();
> }
>
> +bool platform_smc(struct cpu_user_regs *regs)
> +{
> + if ( platform && platform->smc )
You might want to add a likely here because most likely a guest is using
SIP when it is present on the platform.
The rest looks good to me.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/6] xen/arm: zynqmp: Forward plaform specific firmware calls
2018-08-11 0:01 ` [PATCH v3 2/6] xen/arm: zynqmp: Forward plaform specific firmware calls Stefano Stabellini
@ 2018-08-23 15:41 ` Julien Grall
2018-08-23 23:56 ` Stefano Stabellini
0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-08-23 15:41 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: edgar.iglesias, Stefano Stabellini
Hi Stefano,
2018 01:01 AM, Stefano Stabellini wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>
> Introduce zynqmp_eemi: a function resposible for implementing access
s/resposible/responsible/
> controls over the firmware calls. Only calls that are allowed are
> forwarded to the firmware.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> xen/arch/arm/platforms/Makefile | 1 +
> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 38 ++++++++++++++++++++++
> xen/arch/arm/platforms/xilinx-zynqmp.c | 14 ++++++++
> xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 3 ++
> 4 files changed, 56 insertions(+)
> create mode 100644 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> create mode 100644 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
>
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index 80e555c..703f915 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -9,3 +9,4 @@ obj-y += sunxi.o
> obj-$(CONFIG_ARM_64) += thunderx.o
> obj-$(CONFIG_ARM_64) += xgene-storm.o
> obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
> +obj-$(CONFIG_ARM_64) += xilinx-zynqmp-eemi.o
> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> new file mode 100644
> index 0000000..c3a19e9
> --- /dev/null
> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> @@ -0,0 +1,38 @@
> +/*
> + * xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> + *
> + * Xilinx ZynqMP EEMI API
> + *
> + * Copyright (c) 2018 Xilinx Inc.
> + * Written by Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <xen/iocap.h>
> +#include <xen/sched.h>
> +#include <xen/types.h>
> +#include <asm/smccc.h>
> +#include <asm/regs.h>
> +#include <asm/platforms/xilinx-zynqmp-eemi.h>
Can you please introduce headers when they are actually used?
> +
> +bool zynqmp_eemi(struct cpu_user_regs *regs)
> +{
> + return false;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp.c b/xen/arch/arm/platforms/xilinx-zynqmp.c
> index d8ceded..c318cf5 100644
> --- a/xen/arch/arm/platforms/xilinx-zynqmp.c
> +++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
> @@ -18,6 +18,8 @@
> */
>
> #include <asm/platform.h>
> +#include <asm/platforms/xilinx-zynqmp-eemi.h>
> +#include <asm/smccc.h>
>
> static const char * const zynqmp_dt_compat[] __initconst =
> {
> @@ -32,8 +34,20 @@ static const struct dt_device_match zynqmp_blacklist_dev[] __initconst =
> { /* sentinel */ },
> };
>
> +static bool zynqmp_smc(struct cpu_user_regs *regs)
> +{
> + if ( !is_64bit_domain(current->domain) )
> + return false;
> + /* Only support platforms with SMCCC >= 1.1 */
> + if ( smccc_ver < SMCCC_VERSION(1, 1) )
> + return false;
Do you really need to call this at every SMC call?
> +
> + return zynqmp_eemi(regs);
> +}
> +
> PLATFORM_START(xilinx_zynqmp, "Xilinx ZynqMP")
> .compatible = zynqmp_dt_compat,
> + .smc = zynqmp_smc,
> .blacklist_dev = zynqmp_blacklist_dev,
> PLATFORM_END
>
> diff --git a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> new file mode 100644
> index 0000000..6630dc8
> --- /dev/null
> +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> @@ -0,0 +1,3 @@
Missing copyright headers and guard.
> +#include <asm/processor.h>
> +
> +extern bool zynqmp_eemi(struct cpu_user_regs *regs);
>
Missing emacs magics.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 1/6] xen/arm: introduce platform_smc
2018-08-23 15:37 ` Julien Grall
@ 2018-08-23 23:40 ` Stefano Stabellini
0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2018-08-23 23:40 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, Stefano Stabellini, xen-devel
On Thu, 23 Aug 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 08/11/2018 01:01 AM, Stefano Stabellini wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>
> I am not sure why there is 2 From here? This seems to apply whole the whole
> series.
I think it must be because git format-patch/git send-email add one.
I'll fix it next time.
> > Introduce platform_smc as a way to handle firmware calls that Xen does
> > not know about in a platform specific way. This is particularly useful
> > for implementing the SiP (SoC implementation specific) service calls.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> > xen/arch/arm/platform.c | 8 ++++++++
> > xen/arch/arm/vsmc.c | 4 ++++
> > xen/include/asm-arm/platform.h | 3 +++
> > 3 files changed, 15 insertions(+)
> >
> > diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
> > index 3f2989e..9e19023 100644
> > --- a/xen/arch/arm/platform.c
> > +++ b/xen/arch/arm/platform.c
> > @@ -127,6 +127,14 @@ void platform_poweroff(void)
> > platform->poweroff();
> > }
> > +bool platform_smc(struct cpu_user_regs *regs)
> > +{
> > + if ( platform && platform->smc )
>
> You might want to add a likely here because most likely a guest is using SIP
> when it is present on the platform.
OK
> The rest looks good to me.
Thanks for the review
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/6] xen/arm: zynqmp: Forward plaform specific firmware calls
2018-08-23 15:41 ` Julien Grall
@ 2018-08-23 23:56 ` Stefano Stabellini
2018-08-24 9:01 ` Julien Grall
0 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2018-08-23 23:56 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, Stefano Stabellini, xen-devel
On Thu, 23 Aug 2018, Julien Grall wrote:
> Hi Stefano,
>
> 2018 01:01 AM, Stefano Stabellini wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> >
> > Introduce zynqmp_eemi: a function resposible for implementing access
>
> s/resposible/responsible/
I'll fix
> > controls over the firmware calls. Only calls that are allowed are
> > forwarded to the firmware.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> > xen/arch/arm/platforms/Makefile | 1 +
> > xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 38
> > ++++++++++++++++++++++
> > xen/arch/arm/platforms/xilinx-zynqmp.c | 14 ++++++++
> > xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 3 ++
> > 4 files changed, 56 insertions(+)
> > create mode 100644 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > create mode 100644 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> >
> > diff --git a/xen/arch/arm/platforms/Makefile
> > b/xen/arch/arm/platforms/Makefile
> > index 80e555c..703f915 100644
> > --- a/xen/arch/arm/platforms/Makefile
> > +++ b/xen/arch/arm/platforms/Makefile
> > @@ -9,3 +9,4 @@ obj-y += sunxi.o
> > obj-$(CONFIG_ARM_64) += thunderx.o
> > obj-$(CONFIG_ARM_64) += xgene-storm.o
> > obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
> > +obj-$(CONFIG_ARM_64) += xilinx-zynqmp-eemi.o
> > diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > new file mode 100644
> > index 0000000..c3a19e9
> > --- /dev/null
> > +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > @@ -0,0 +1,38 @@
> > +/*
> > + * xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > + *
> > + * Xilinx ZynqMP EEMI API
> > + *
> > + * Copyright (c) 2018 Xilinx Inc.
> > + * Written by Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms and conditions of the GNU General Public
> > + * License, version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <xen/iocap.h>
> > +#include <xen/sched.h>
> > +#include <xen/types.h>
> > +#include <asm/smccc.h>
> > +#include <asm/regs.h>
> > +#include <asm/platforms/xilinx-zynqmp-eemi.h>
>
> Can you please introduce headers when they are actually used?
I'll do
> > +
> > +bool zynqmp_eemi(struct cpu_user_regs *regs)
> > +{
> > + return false;
> > +}
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/xen/arch/arm/platforms/xilinx-zynqmp.c
> > b/xen/arch/arm/platforms/xilinx-zynqmp.c
> > index d8ceded..c318cf5 100644
> > --- a/xen/arch/arm/platforms/xilinx-zynqmp.c
> > +++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
> > @@ -18,6 +18,8 @@
> > */
> > #include <asm/platform.h>
> > +#include <asm/platforms/xilinx-zynqmp-eemi.h>
> > +#include <asm/smccc.h>
> > static const char * const zynqmp_dt_compat[] __initconst =
> > {
> > @@ -32,8 +34,20 @@ static const struct dt_device_match
> > zynqmp_blacklist_dev[] __initconst =
> > { /* sentinel */ },
> > };
> > +static bool zynqmp_smc(struct cpu_user_regs *regs)
> > +{
> > + if ( !is_64bit_domain(current->domain) )
> > + return false;
> > + /* Only support platforms with SMCCC >= 1.1 */
> > + if ( smccc_ver < SMCCC_VERSION(1, 1) )
> > + return false;
>
> Do you really need to call this at every SMC call?
Do you mean that we could skip the checks? We could do the checks at
init time, but then we would have to memorize the result for each
domain. Not sure it is worth it.
> > +
> > + return zynqmp_eemi(regs);
> > +}
> > +
> > PLATFORM_START(xilinx_zynqmp, "Xilinx ZynqMP")
> > .compatible = zynqmp_dt_compat,
> > + .smc = zynqmp_smc,
> > .blacklist_dev = zynqmp_blacklist_dev,
> > PLATFORM_END
> > diff --git a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > new file mode 100644
> > index 0000000..6630dc8
> > --- /dev/null
> > +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > @@ -0,0 +1,3 @@
>
> Missing copyright headers and guard.
The guard is missing, I'll add it. Regarding the copyright, this is an
header file, none of the others in the same directory have the copyright
header -- are you sure we need it?
> > +#include <asm/processor.h>
> > +
> > +extern bool zynqmp_eemi(struct cpu_user_regs *regs);
> >
>
> Missing emacs magics.
Yeah.. Good point. One of these days I'll send a xen.git wide series to
remove it for everywhere :-D
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/6] xen/arm: zynqmp: Forward plaform specific firmware calls
2018-08-23 23:56 ` Stefano Stabellini
@ 2018-08-24 9:01 ` Julien Grall
2018-10-10 21:50 ` Stefano Stabellini
0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-08-24 9:01 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: edgar.iglesias, Stefano Stabellini, xen-devel
Hi,
On 24/08/18 00:56, Stefano Stabellini wrote:
> On Thu, 23 Aug 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> 2018 01:01 AM, Stefano Stabellini wrote:
>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>
>>> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>
>>> Introduce zynqmp_eemi: a function resposible for implementing access
>>
>> s/resposible/responsible/
>
> I'll fix
>
>
>>> controls over the firmware calls. Only calls that are allowed are
>>> forwarded to the firmware.
>>>
>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>> ---
>>> xen/arch/arm/platforms/Makefile | 1 +
>>> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 38
>>> ++++++++++++++++++++++
>>> xen/arch/arm/platforms/xilinx-zynqmp.c | 14 ++++++++
>>> xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 3 ++
>>> 4 files changed, 56 insertions(+)
>>> create mode 100644 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> create mode 100644 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
>>>
>>> diff --git a/xen/arch/arm/platforms/Makefile
>>> b/xen/arch/arm/platforms/Makefile
>>> index 80e555c..703f915 100644
>>> --- a/xen/arch/arm/platforms/Makefile
>>> +++ b/xen/arch/arm/platforms/Makefile
>>> @@ -9,3 +9,4 @@ obj-y += sunxi.o
>>> obj-$(CONFIG_ARM_64) += thunderx.o
>>> obj-$(CONFIG_ARM_64) += xgene-storm.o
>>> obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
>>> +obj-$(CONFIG_ARM_64) += xilinx-zynqmp-eemi.o
>>> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> new file mode 100644
>>> index 0000000..c3a19e9
>>> --- /dev/null
>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> @@ -0,0 +1,38 @@
>>> +/*
>>> + * xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> + *
>>> + * Xilinx ZynqMP EEMI API
>>> + *
>>> + * Copyright (c) 2018 Xilinx Inc.
>>> + * Written by Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms and conditions of the GNU General Public
>>> + * License, version 2, as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <xen/iocap.h>
>>> +#include <xen/sched.h>
>>> +#include <xen/types.h>
>>> +#include <asm/smccc.h>
>>> +#include <asm/regs.h>
>>> +#include <asm/platforms/xilinx-zynqmp-eemi.h>
>>
>> Can you please introduce headers when they are actually used?
>
> I'll do
>
>
>>> +
>>> +bool zynqmp_eemi(struct cpu_user_regs *regs)
>>> +{
>>> + return false;
>>> +}
>>> +
>>> +/*
>>> + * Local variables:
>>> + * mode: C
>>> + * c-file-style: "BSD"
>>> + * c-basic-offset: 4
>>> + * indent-tabs-mode: nil
>>> + * End:
>>> + */
>>> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp.c
>>> b/xen/arch/arm/platforms/xilinx-zynqmp.c
>>> index d8ceded..c318cf5 100644
>>> --- a/xen/arch/arm/platforms/xilinx-zynqmp.c
>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
>>> @@ -18,6 +18,8 @@
>>> */
>>> #include <asm/platform.h>
>>> +#include <asm/platforms/xilinx-zynqmp-eemi.h>
>>> +#include <asm/smccc.h>
>>> static const char * const zynqmp_dt_compat[] __initconst =
>>> {
>>> @@ -32,8 +34,20 @@ static const struct dt_device_match
>>> zynqmp_blacklist_dev[] __initconst =
>>> { /* sentinel */ },
>>> };
>>> +static bool zynqmp_smc(struct cpu_user_regs *regs)
>>> +{
>>> + if ( !is_64bit_domain(current->domain) )
>>> + return false;
>>> + /* Only support platforms with SMCCC >= 1.1 */
>>> + if ( smccc_ver < SMCCC_VERSION(1, 1) )
>>> + return false;
>>
>> Do you really need to call this at every SMC call?
>
> Do you mean that we could skip the checks? We could do the checks at
> init time, but then we would have to memorize the result for each
> domain. Not sure it is worth it.
I was mostly speaking about smccc_ver. It feels strange to always check
the SMCCC version when calling SMC as the value will never change.
I can see two solutions here:
1) Provide a different .smc callback depending on the SMCCC version
2) Provide wrappers to either call SMCCv1.1 or SMCCCv1.0
>>> +
>>> + return zynqmp_eemi(regs);
>>> +}
>>> +
>>> PLATFORM_START(xilinx_zynqmp, "Xilinx ZynqMP")
>>> .compatible = zynqmp_dt_compat,
>>> + .smc = zynqmp_smc,
>>> .blacklist_dev = zynqmp_blacklist_dev,
>>> PLATFORM_END
>>> diff --git a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
>>> b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
>>> new file mode 100644
>>> index 0000000..6630dc8
>>> --- /dev/null
>>> +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
>>> @@ -0,0 +1,3 @@
>>
>> Missing copyright headers and guard.
>
> The guard is missing, I'll add it. Regarding the copyright, this is an
> header file, none of the others in the same directory have the copyright
> header -- are you sure we need it?
Technically any file require the copyright. This was not followed in the
the header side until recently.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 4/6] xen/arm: zynqmp: eemi access control
2018-08-11 0:01 ` [PATCH v3 4/6] xen/arm: zynqmp: eemi access control Stefano Stabellini
@ 2018-08-28 16:05 ` Julien Grall
2018-10-10 22:35 ` Stefano Stabellini
0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-08-28 16:05 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: edgar.iglesias, Stefano Stabellini
Hi Stefano,
On 11/08/18 01:01, Stefano Stabellini wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>
> Introduce data structs to implement basic access controls.
> Introduce the following three functions:
>
> domain_has_node_access: check access to the node
> domain_has_reset_access: check access to the reset line
> domain_has_mmio_access: check access to the register
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 783 ++++++++++++++++++++++++++++
> 1 file changed, 783 insertions(+)
>
> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> index c3a19e9..62cc15c 100644
> --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> @@ -16,6 +16,74 @@
> * GNU General Public License for more details.
> */
>
> +/*
> + * EEMI Power Management API access
> + *
> + * Refs:
> + * https://www.xilinx.com/support/documentation/user_guides/ug1200-eemi-api.pdf
> + *
> + * Background:
> + * The ZynqMP has a subsystem named the PMU with a CPU and special devices
> + * dedicated to running Power Management Firmware. Other masters in the
> + * system need to send requests to the PMU in order to for example:
> + * * Manage power state
> + * * Configure clocks
> + * * Program bitstreams for the programmable logic
> + * * etc
> + *
> + * Although the details of the setup are configurable, in the common case
> + * the PMU lives in the Secure world. NS World cannot directly communicate
> + * with it and must use proxy services from ARM Trusted Firmware to reach
> + * the PMU.
> + *
> + * Power Management on the ZynqMP is implemented in a layered manner.
> + * The PMU knows about various masters and will enforce access controls
> + * based on a pre-configured partitioning. This configuration dictates
> + * which devices are owned by the various masters and the PMU FW makes sure
> + * that a given master cannot turn off a device that it does not own or that
> + * is in use by other masters.
> + *
> + * The PMU is not aware of multiple execution states in masters.
> + * For example, it treats the ARMv8 cores as single units and does not
> + * distinguish between Secure vs NS OS's nor does it know about Hypervisors
> + * and multiple guests. It is up to software on the ARMv8 cores to present
> + * a unified view of its power requirements.
> + *
> + * To implement this unified view, ARM Trusted Firmware at EL3 provides
> + * access to the PM API via SMC calls. ARM Trusted Firmware is responsible
> + * for mediating between the Secure and the NS world, rejecting SMC calls
> + * that request changes that are not allowed.
> + *
> + * Xen running above ATF owns the NS world and is responsible for presenting
> + * unified PM requests taking all guests and the hypervisor into account.
> + *
> + * Implementation:
> + * The PM API contains different classes of calls.
> + * Certain calls are harmless to expose to any guest.
> + * These include calls to get the PM API Version, or to read out the version
> + * of the chip we're running on.
> + *
> + * In order to correctly virtualize these calls, we need to know if
> + * guests issuing these calls have ownership of the given device.
> + * The approach taken here is to map PM API Nodes identifying
> + * a device into base addresses for registers that belong to that
> + * same device.
> + *
> + * If the guest has access to devices registers, we give the guest
> + * access to PM API calls that affect that device. This is implemented
> + * by pm_node_access and domain_has_node_access().
> + *
> + * MMIO access:
> + * These calls allow guests to access certain memory ranges. These ranges
> + * are typically protected for secure-world access only and also from
> + * certain masters only, so guests cannot access them directly.
> + * Registers within the memory regions affect certain nodes. In this case,
> + * our input is an address and we map that address into a node. If the
> + * guest has ownership of that node, the access is allowed.
> + * Some registers contain bitfields and a single register may contain
> + * bits that affect multiple nodes.
> + */
> +
> #include <xen/iocap.h>
> #include <xen/sched.h>
> #include <xen/types.h>
> @@ -23,6 +91,721 @@
> #include <asm/regs.h>
> #include <asm/platforms/xilinx-zynqmp-eemi.h>
>
> +struct pm_access
> +{
> + paddr_t addr;
It seems that the address will always page-aligned. So could we store a
frame using mfn_t?
> + bool hwdom_access; /* HW domain gets access regardless. */
> +};
> +
> +/*
> + * This table maps a node into a memory address.
> + * If a guest has access to the address, it has enough control
> + * over the node to grant it access to EEMI calls for that node.
> + */
> +static const struct pm_access pm_node_access[] = {
[...]
> +
> +/*
> + * This table maps reset line IDs into a memory address.
> + * If a guest has access to the address, it has enough control
> + * over the affected node to grant it access to EEMI calls for
> + * resetting that node.
> + */
> +#define XILPM_RESET_IDX(n) (n - XILPM_RESET_PCIE_CFG)
> +static const struct pm_access pm_reset_access[] = {
[...]
> +
> +/*
> + * This table maps reset line IDs into a memory address.
> + * If a guest has access to the address, it has enough control
> + * over the affected node to grant it access to EEMI calls for
> + * resetting that node.
> + */
> +static const struct {
> + paddr_t start;
> + paddr_t size;
> + uint32_t mask; /* Zero means no mask, i.e all bits. */
> + enum pm_node_id node;
> + bool hwdom_access;
> + bool readonly;
> +} pm_mmio_access[] = {
Those 3 arrays contains a lot of hardcoded value. Can't any of this be
detected from the device-tree?
I would be interested to know how this is going to work with upstream
Linux. Do you hardcode all the values there as well?
> +static bool pm_check_access(const struct pm_access *acl, struct domain *d,
> + uint32_t idx)
> +{
> + unsigned long mfn;
mfn_t please. The code is not that nice but at least it add more safety
in the code. Hopefully iommu_access_permitted will be converted to
typesafe MFN soon.
> +
> + if ( acl[idx].hwdom_access && is_hardware_domain(d) )
> + return true;
> +
> + if ( !acl[idx].addr )
> + return false;
> +
> + mfn = PFN_DOWN(acl[idx].addr);
maddr_to_mfn(...);
> + return iomem_access_permitted(d, mfn, mfn);
Is the address something that a guest would be allowed to read/write
directly? If not, then iomem_access_permitted(...) should not be used.
> +}
> +
> +/* Check if a domain has access to a node. */
> +static bool domain_has_node_access(struct domain *d, uint32_t nodeid)
> +{
> + if ( nodeid > ARRAY_SIZE(pm_node_access) )
> + return false;
> +
> + return pm_check_access(pm_node_access, d, nodeid);
> +}
> +
> +/* Check if a domain has access to a reset line. */
> +static bool domain_has_reset_access(struct domain *d, uint32_t rst)
> +{
> + if ( rst < XILPM_RESET_PCIE_CFG )
> + return false;
> +
> + rst -= XILPM_RESET_PCIE_CFG;
> +
> + if ( rst > ARRAY_SIZE(pm_reset_access) )
> + return false;
> +
> + return pm_check_access(pm_reset_access, d, rst);
> +}
> +
> +/*
> + * Check if a given domain has access to perform an indirect
> + * MMIO access.
This sentence seems to confirm a domain cannot do direct MMIO access to
that region. So, iomem_access_permitted is definitely not an option here.
> + *
> + * If the provided mask is invalid, it will be fixed up.
> + */
> +static bool domain_has_mmio_access(struct domain *d,
> + bool write, paddr_t addr,
> + uint32_t *mask)
I don't see this function used below, this would lead to a compilation
error. Can you make the series is bisectable?
> +{
> + unsigned int i;
> + bool ret = false;
> + uint32_t prot_mask = 0;
> +
> + /*
> + * The hardware domain gets read access to everything.
> + * Lower layers will do further filtering.
> + */
> + if ( !write && is_hardware_domain(d) )
> + return true;
> +
> + /* Scan the ACL. */
> + for ( i = 0; i < ARRAY_SIZE(pm_mmio_access); i++ )
> + {
> + if ( addr < pm_mmio_access[i].start )
> + return false;
> + if ( addr > pm_mmio_access[i].start + pm_mmio_access[i].size )
I would add an ASSERT(pm_mmio_access[i].start >= pm_mmio_access[i].size)
to catch wrapping.
> + continue;
> +
> + if ( write && pm_mmio_access[i].readonly )
> + return false;
> + if ( pm_mmio_access[i].hwdom_access && !is_hardware_domain(d) )
> + return false;
> + if ( !domain_has_node_access(d, pm_mmio_access[i].node) )
> + return false;
> +
> + /* We've got access to this reg (or parts of it). */
> + ret = true;
> +
> + /* Permit write access to selected bits. */
> + prot_mask |= pm_mmio_access[i].mask ?: 0xFFFFFFFF;
Can you use GENMASK here?
NIT: newline
> + break;
> + }
> +
> + /* Masking only applies to writes. */
> + if ( write )
> + *mask &= prot_mask;
So for reading there are no masking? What should be the value it?
> +
> + return ret;
> +}
> +
> bool zynqmp_eemi(struct cpu_user_regs *regs)
> {
> return false;
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 5/6] xen/arm: zynqmp: implement zynqmp_eemi
2018-08-11 0:01 ` [PATCH v3 5/6] xen/arm: zynqmp: implement zynqmp_eemi Stefano Stabellini
@ 2018-08-28 16:29 ` Julien Grall
2018-10-10 22:49 ` Stefano Stabellini
0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-08-28 16:29 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: edgar.iglesias, Stefano Stabellini
Hi Stefano,
On 11/08/18 01:01, Stefano Stabellini wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>
> zynqmp_eemi uses the defined functions and structs to decide whether to
> make a call to the firmware, or to simply return a predefined value.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 143 +++++++++++++++++++++++++++-
> 1 file changed, 142 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> index 62cc15c..1dbdf04 100644
> --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> @@ -808,7 +808,148 @@ static bool domain_has_mmio_access(struct domain *d,
>
> bool zynqmp_eemi(struct cpu_user_regs *regs)
> {
> - return false;
> + struct arm_smccc_res res;
> + bool is_mmio_write = false;
> + uint32_t fid = get_user_reg(regs, 0);
> + uint32_t nodeid = get_user_reg(regs, 1);
> + unsigned int pm_fn = fid & 0xFFFF;
Here you will receive all the function ID for the SIP subsystem. Can you
confirm the EEMI can be accessed from both SMC32 and SMC64 convention?
> + enum pm_ret_status ret;
> +
> + switch ( pm_fn )
> + {
> + /*
> + * We can't allow CPUs to suspend without Xen knowing about it.
> + * We accept but ignore the request and wait for the guest to issue
> + * a WFI which Xen will trap and act accordingly upon.
Why WFI specifically? The guest should suspend using PSCI normally.
> + */
> + case PM_SELF_SUSPEND:
> + ret = XST_PM_SUCCESS;
> + goto done;
> +
> + case PM_GET_NODE_STATUS:
> + /* API for PUs. */
> + case PM_REQ_SUSPEND:
> + case PM_FORCE_POWERDOWN:
> + case PM_ABORT_SUSPEND:
> + case PM_REQ_WAKEUP:
> + case PM_SET_WAKEUP_SOURCE:
> + /* API for slaves. */
> + case PM_REQ_NODE:
> + case PM_RELEASE_NODE:
> + case PM_SET_REQUIREMENT:
> + case PM_SET_MAX_LATENCY:
> + if ( !domain_has_node_access(current->domain, nodeid) )
> + {
> + gprintk(XENLOG_WARNING,
> + "zynqmp-pm: fn=%u No access to node %u\n", pm_fn, nodeid);
> + ret = XST_PM_NO_ACCESS;
> + goto done;
> + }
> + goto forward_to_fw;
> +
> + case PM_RESET_ASSERT:
> + case PM_RESET_GET_STATUS:
> + if ( !domain_has_reset_access(current->domain, nodeid) )
> + {
> + gprintk(XENLOG_WARNING,
> + "zynqmp-pm: fn=%u No access to reset %u\n", pm_fn, nodeid);
> + ret = XST_PM_NO_ACCESS;
> + goto done;
> + }
> + goto forward_to_fw;
> +
> + /* These calls are safe and always allowed. */
> + case ZYNQMP_SIP_SVC_CALL_COUNT:
> + case ZYNQMP_SIP_SVC_UID:
> + case ZYNQMP_SIP_SVC_VERSION:
> + case PM_GET_TRUSTZONE_VERSION:
> + case PM_GET_API_VERSION:
> + case PM_GET_CHIPID:
> + goto forward_to_fw;
> +
> + /* Mediated MMIO access. */
> + case PM_MMIO_WRITE:
> + is_mmio_write = true;
> + /* Fallthrough. */
> + case PM_MMIO_READ:
> + {
> + uint32_t mask = get_user_reg(regs, 1) >> 32;
NIT: newline.
> + if ( !domain_has_mmio_access(current->domain,
> + is_mmio_write,
> + nodeid, &mask) )
> + {
> + gprintk(XENLOG_WARNING,
> + "eemi: fn=%u No access to MMIO %s %u\n",
> + pm_fn, is_mmio_write ? "write" : "read", nodeid);
> + ret = XST_PM_NO_ACCESS;
> + goto done;
> + }
> + set_user_reg(regs, 1, ((uint64_t)mask << 32) | nodeid);
Can you define 32 in a macro?
> + goto forward_to_fw;
> + }
> +
> + /* Exclusive to the hardware domain. */
> + case PM_INIT:
> + case PM_SET_CONFIGURATION:
> + case PM_FPGA_LOAD:
> + case PM_FPGA_GET_STATUS:
> + case PM_SECURE_SHA:
> + case PM_SECURE_RSA:
> + case PM_PINCTRL_SET_FUNCTION:
> + case PM_PINCTRL_REQUEST:
> + case PM_PINCTRL_RELEASE:
> + case PM_PINCTRL_GET_FUNCTION:
> + case PM_PINCTRL_CONFIG_PARAM_GET:
> + case PM_PINCTRL_CONFIG_PARAM_SET:
> + case PM_IOCTL:
> + case PM_QUERY_DATA:
> + case PM_CLOCK_ENABLE:
> + case PM_CLOCK_DISABLE:
> + case PM_CLOCK_GETSTATE:
> + case PM_CLOCK_GETDIVIDER:
> + case PM_CLOCK_SETDIVIDER:
> + case PM_CLOCK_SETRATE:
> + case PM_CLOCK_GETRATE:
> + case PM_CLOCK_SETPARENT:
> + case PM_CLOCK_GETPARENT:
> + if ( !is_hardware_domain(current->domain) )
> + {
> + gprintk(XENLOG_WARNING, "eemi: fn=%u No access", pm_fn);
> + ret = XST_PM_NO_ACCESS;
> + goto done;
> + }
> + goto forward_to_fw;
> +
> + /* These calls are never allowed. */
> + case PM_SYSTEM_SHUTDOWN:
> + ret = XST_PM_NO_ACCESS;
> + goto done;
> +
> + default:
> + gprintk(XENLOG_WARNING, "zynqmp-pm: Unhandled PM Call: %u\n", fid);
> + return false;
The functions COUNT, UUID, REVISION mandated by the SMCCC are missing here.
> + }
> +
> +forward_to_fw:
> + arm_smccc_1_1_smc(get_user_reg(regs, 0),
> + 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;
> +
> +done:
> + set_user_reg(regs, 0, ret);
> + return true;
> }
>
> /*
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/6] xen/arm: zynqmp: Forward plaform specific firmware calls
2018-08-24 9:01 ` Julien Grall
@ 2018-10-10 21:50 ` Stefano Stabellini
0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2018-10-10 21:50 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, Stefano Stabellini, xen-devel
On Fri, 24 Aug 2018, Julien Grall wrote:
> > > > +
> > > > +bool zynqmp_eemi(struct cpu_user_regs *regs)
> > > > +{
> > > > + return false;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Local variables:
> > > > + * mode: C
> > > > + * c-file-style: "BSD"
> > > > + * c-basic-offset: 4
> > > > + * indent-tabs-mode: nil
> > > > + * End:
> > > > + */
> > > > diff --git a/xen/arch/arm/platforms/xilinx-zynqmp.c
> > > > b/xen/arch/arm/platforms/xilinx-zynqmp.c
> > > > index d8ceded..c318cf5 100644
> > > > --- a/xen/arch/arm/platforms/xilinx-zynqmp.c
> > > > +++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
> > > > @@ -18,6 +18,8 @@
> > > > */
> > > > #include <asm/platform.h>
> > > > +#include <asm/platforms/xilinx-zynqmp-eemi.h>
> > > > +#include <asm/smccc.h>
> > > > static const char * const zynqmp_dt_compat[] __initconst =
> > > > {
> > > > @@ -32,8 +34,20 @@ static const struct dt_device_match
> > > > zynqmp_blacklist_dev[] __initconst =
> > > > { /* sentinel */ },
> > > > };
> > > > +static bool zynqmp_smc(struct cpu_user_regs *regs)
> > > > +{
> > > > + if ( !is_64bit_domain(current->domain) )
> > > > + return false;
> > > > + /* Only support platforms with SMCCC >= 1.1 */
> > > > + if ( smccc_ver < SMCCC_VERSION(1, 1) )
> > > > + return false;
> > >
> > > Do you really need to call this at every SMC call?
> >
> > Do you mean that we could skip the checks? We could do the checks at
> > init time, but then we would have to memorize the result for each
> > domain. Not sure it is worth it.
>
> I was mostly speaking about smccc_ver. It feels strange to always check the
> SMCCC version when calling SMC as the value will never change.
>
> I can see two solutions here:
> 1) Provide a different .smc callback depending on the SMCCC version
> 2) Provide wrappers to either call SMCCv1.1 or SMCCCv1.0
I think we can assume SMCCCv1.1, we don't need to support SMCCCv1.0 with
EEMI. I'll remove the check.
> > > > +
> > > > + return zynqmp_eemi(regs);
> > > > +}
> > > > +
> > > > PLATFORM_START(xilinx_zynqmp, "Xilinx ZynqMP")
> > > > .compatible = zynqmp_dt_compat,
> > > > + .smc = zynqmp_smc,
> > > > .blacklist_dev = zynqmp_blacklist_dev,
> > > > PLATFORM_END
> > > > diff --git a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > > > b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > > > new file mode 100644
> > > > index 0000000..6630dc8
> > > > --- /dev/null
> > > > +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > > > @@ -0,0 +1,3 @@
> > >
> > > Missing copyright headers and guard.
> >
> > The guard is missing, I'll add it. Regarding the copyright, this is an
> > header file, none of the others in the same directory have the copyright
> > header -- are you sure we need it?
>
> Technically any file require the copyright. This was not followed in the the
> header side until recently.
I'll add the copyright notice
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 4/6] xen/arm: zynqmp: eemi access control
2018-08-28 16:05 ` Julien Grall
@ 2018-10-10 22:35 ` Stefano Stabellini
2018-10-15 7:25 ` Julien Grall
0 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2018-10-10 22:35 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, Stefano Stabellini, xen-devel
On Tue, 28 Aug 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 11/08/18 01:01, Stefano Stabellini wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> >
> > Introduce data structs to implement basic access controls.
> > Introduce the following three functions:
> >
> > domain_has_node_access: check access to the node
> > domain_has_reset_access: check access to the reset line
> > domain_has_mmio_access: check access to the register
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> > xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 783
> > ++++++++++++++++++++++++++++
> > 1 file changed, 783 insertions(+)
> >
> > diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > index c3a19e9..62cc15c 100644
> > --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > @@ -16,6 +16,74 @@
> > * GNU General Public License for more details.
> > */
> > +/*
> > + * EEMI Power Management API access
> > + *
> > + * Refs:
> > + *
> > https://www.xilinx.com/support/documentation/user_guides/ug1200-eemi-api.pdf
> > + *
> > + * Background:
> > + * The ZynqMP has a subsystem named the PMU with a CPU and special devices
> > + * dedicated to running Power Management Firmware. Other masters in the
> > + * system need to send requests to the PMU in order to for example:
> > + * * Manage power state
> > + * * Configure clocks
> > + * * Program bitstreams for the programmable logic
> > + * * etc
> > + *
> > + * Although the details of the setup are configurable, in the common case
> > + * the PMU lives in the Secure world. NS World cannot directly communicate
> > + * with it and must use proxy services from ARM Trusted Firmware to reach
> > + * the PMU.
> > + *
> > + * Power Management on the ZynqMP is implemented in a layered manner.
> > + * The PMU knows about various masters and will enforce access controls
> > + * based on a pre-configured partitioning. This configuration dictates
> > + * which devices are owned by the various masters and the PMU FW makes sure
> > + * that a given master cannot turn off a device that it does not own or
> > that
> > + * is in use by other masters.
> > + *
> > + * The PMU is not aware of multiple execution states in masters.
> > + * For example, it treats the ARMv8 cores as single units and does not
> > + * distinguish between Secure vs NS OS's nor does it know about Hypervisors
> > + * and multiple guests. It is up to software on the ARMv8 cores to present
> > + * a unified view of its power requirements.
> > + *
> > + * To implement this unified view, ARM Trusted Firmware at EL3 provides
> > + * access to the PM API via SMC calls. ARM Trusted Firmware is responsible
> > + * for mediating between the Secure and the NS world, rejecting SMC calls
> > + * that request changes that are not allowed.
> > + *
> > + * Xen running above ATF owns the NS world and is responsible for
> > presenting
> > + * unified PM requests taking all guests and the hypervisor into account.
> > + *
> > + * Implementation:
> > + * The PM API contains different classes of calls.
> > + * Certain calls are harmless to expose to any guest.
> > + * These include calls to get the PM API Version, or to read out the
> > version
> > + * of the chip we're running on.
> > + *
> > + * In order to correctly virtualize these calls, we need to know if
> > + * guests issuing these calls have ownership of the given device.
> > + * The approach taken here is to map PM API Nodes identifying
> > + * a device into base addresses for registers that belong to that
> > + * same device.
> > + *
> > + * If the guest has access to devices registers, we give the guest
> > + * access to PM API calls that affect that device. This is implemented
> > + * by pm_node_access and domain_has_node_access().
> > + *
> > + * MMIO access:
> > + * These calls allow guests to access certain memory ranges. These ranges
> > + * are typically protected for secure-world access only and also from
> > + * certain masters only, so guests cannot access them directly.
> > + * Registers within the memory regions affect certain nodes. In this case,
> > + * our input is an address and we map that address into a node. If the
> > + * guest has ownership of that node, the access is allowed.
> > + * Some registers contain bitfields and a single register may contain
> > + * bits that affect multiple nodes.
> > + */
> > +
> > #include <xen/iocap.h>
> > #include <xen/sched.h>
> > #include <xen/types.h>
> > @@ -23,6 +91,721 @@
> > #include <asm/regs.h>
> > #include <asm/platforms/xilinx-zynqmp-eemi.h>
> > +struct pm_access
> > +{
> > + paddr_t addr;
>
> It seems that the address will always page-aligned. So could we store a frame
> using mfn_t?
Yes we could store just the frame. I started to make the change
suggested, and all the required corresponding changes to the
initializations below, for instance:
[NODE_RPU] = { MM_RPU },
needs to become:
[NODE_RPU] = { _mfn(MM_RPU) },
but when I tried to do that, I get a compilation error:
xilinx-zynqmp-eemi.c:106:20: error: initializer element is not constant
[NODE_RPU] = { _mfn(MM_RPU) },
Unfortunately it is not possible to use mfn_t in static initializations,
because it is a static inline. I could do:
[NODE_RPU] = { { MM_RPU } },
which would work for DEBUG builds but wouldn't work for non-DEBUG
builds.
I'll keep paddr_t for the next version of the series.
> > + bool hwdom_access; /* HW domain gets access regardless. */
> > +};
> > +
> > +/*
> > + * This table maps a node into a memory address.
> > + * If a guest has access to the address, it has enough control
> > + * over the node to grant it access to EEMI calls for that node.
> > + */
> > +static const struct pm_access pm_node_access[] = {
>
> [...]
>
> > +
> > +/*
> > + * This table maps reset line IDs into a memory address.
> > + * If a guest has access to the address, it has enough control
> > + * over the affected node to grant it access to EEMI calls for
> > + * resetting that node.
> > + */
> > +#define XILPM_RESET_IDX(n) (n - XILPM_RESET_PCIE_CFG)
> > +static const struct pm_access pm_reset_access[] = {
>
> [...]
>
> > +
> > +/*
> > + * This table maps reset line IDs into a memory address.
> > + * If a guest has access to the address, it has enough control
> > + * over the affected node to grant it access to EEMI calls for
> > + * resetting that node.
> > + */
> > +static const struct {
> > + paddr_t start;
> > + paddr_t size;
> > + uint32_t mask; /* Zero means no mask, i.e all bits. */
> > + enum pm_node_id node;
> > + bool hwdom_access;
> > + bool readonly;
> > +} pm_mmio_access[] = {
>
> Those 3 arrays contains a lot of hardcoded value. Can't any of this be
> detected from the device-tree?
No, the information is not available on device tree unfortunately.
> I would be interested to know how this is going to work with upstream Linux.
> Do you hardcode all the values there as well?
Yes: the IDs are specified on an header file, see
include/linux/firmware/xlnx-zynqmp.h on the zynq/firmware branch of the
arm-soc tree. In addition to the IDs, we also have the memory addresses
in Xen to do the permission checks.
> > +static bool pm_check_access(const struct pm_access *acl, struct domain *d,
> > + uint32_t idx)
> > +{
> > + unsigned long mfn;
>
> mfn_t please. The code is not that nice but at least it add more safety in the
> code. Hopefully iommu_access_permitted will be converted to typesafe MFN soon.
Yes, I can make this change without issues.
> > +
> > + if ( acl[idx].hwdom_access && is_hardware_domain(d) )
> > + return true;
> > +
> > + if ( !acl[idx].addr )
> > + return false;
> > +
> > + mfn = PFN_DOWN(acl[idx].addr);
>
> maddr_to_mfn(...);
OK
> > + return iomem_access_permitted(d, mfn, mfn);
>
> Is the address something that a guest would be allowed to read/write directly?
> If not, then iomem_access_permitted(...) should not be used.
Yes, the address would be something remapped to the guest using iomem.
> > +}
> > +
> > +/* Check if a domain has access to a node. */
> > +static bool domain_has_node_access(struct domain *d, uint32_t nodeid)
> > +{
> > + if ( nodeid > ARRAY_SIZE(pm_node_access) )
> > + return false;
> > +
> > + return pm_check_access(pm_node_access, d, nodeid);
> > +}
> > +
> > +/* Check if a domain has access to a reset line. */
> > +static bool domain_has_reset_access(struct domain *d, uint32_t rst)
> > +{
> > + if ( rst < XILPM_RESET_PCIE_CFG )
> > + return false;
> > +
> > + rst -= XILPM_RESET_PCIE_CFG;
> > +
> > + if ( rst > ARRAY_SIZE(pm_reset_access) )
> > + return false;
> > +
> > + return pm_check_access(pm_reset_access, d, rst);
> > +}
> > +
> > +/*
> > + * Check if a given domain has access to perform an indirect
> > + * MMIO access.
>
> This sentence seems to confirm a domain cannot do direct MMIO access to that
> region. So, iomem_access_permitted is definitely not an option here.
>
> > + *
> > + * If the provided mask is invalid, it will be fixed up.
> > + */
> > +static bool domain_has_mmio_access(struct domain *d,
> > + bool write, paddr_t addr,
> > + uint32_t *mask)
>
> I don't see this function used below, this would lead to a compilation error.
> Can you make the series is bisectable?
I'll will, sorry about that.
> > +{
> > + unsigned int i;
> > + bool ret = false;
> > + uint32_t prot_mask = 0;
> > +
> > + /*
> > + * The hardware domain gets read access to everything.
> > + * Lower layers will do further filtering.
> > + */
> > + if ( !write && is_hardware_domain(d) )
> > + return true;
> > +
> > + /* Scan the ACL. */
> > + for ( i = 0; i < ARRAY_SIZE(pm_mmio_access); i++ )
> > + {
> > + if ( addr < pm_mmio_access[i].start )
> > + return false;
> > + if ( addr > pm_mmio_access[i].start + pm_mmio_access[i].size )
>
> I would add an ASSERT(pm_mmio_access[i].start >= pm_mmio_access[i].size) to
> catch wrapping.
I take that you meant the following:
ASSERT(pm_mmio_access[i].start + pm_mmio_access[i].size >= pm_mmio_access[i].start)
> > + continue;
> > +
> > + if ( write && pm_mmio_access[i].readonly )
> > + return false;
> > + if ( pm_mmio_access[i].hwdom_access && !is_hardware_domain(d) )
> > + return false;
> > + if ( !domain_has_node_access(d, pm_mmio_access[i].node) )
> > + return false;
> > +
> > + /* We've got access to this reg (or parts of it). */
> > + ret = true;
> > +
> > + /* Permit write access to selected bits. */
> > + prot_mask |= pm_mmio_access[i].mask ?: 0xFFFFFFFF;
> Can you use GENMASK here?
Sure
> NIT: newline
OK
> > + break;
> > + }
> > +
> > + /* Masking only applies to writes. */
> > + if ( write )
> > + *mask &= prot_mask;
>
> So for reading there are no masking? What should be the value it?
Yes, this is my understanding. The value is safe to read, but not all
bits are writeable.
> > +
> > + return ret;
> > +}
> > +
> > bool zynqmp_eemi(struct cpu_user_regs *regs)
> > {
> > return false;
> >
>
> Cheers,
>
> --
> Julien Grall
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 5/6] xen/arm: zynqmp: implement zynqmp_eemi
2018-08-28 16:29 ` Julien Grall
@ 2018-10-10 22:49 ` Stefano Stabellini
2018-10-15 7:32 ` Julien Grall
0 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2018-10-10 22:49 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, Stefano Stabellini, xen-devel
On Tue, 28 Aug 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 11/08/18 01:01, Stefano Stabellini wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> >
> > zynqmp_eemi uses the defined functions and structs to decide whether to
> > make a call to the firmware, or to simply return a predefined value.
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> > xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 143
> > +++++++++++++++++++++++++++-
> > 1 file changed, 142 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > index 62cc15c..1dbdf04 100644
> > --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > @@ -808,7 +808,148 @@ static bool domain_has_mmio_access(struct domain *d,
> > bool zynqmp_eemi(struct cpu_user_regs *regs)
> > {
> > - return false;
> > + struct arm_smccc_res res;
> > + bool is_mmio_write = false;
> > + uint32_t fid = get_user_reg(regs, 0);
> > + uint32_t nodeid = get_user_reg(regs, 1);
> > + unsigned int pm_fn = fid & 0xFFFF;
>
> Here you will receive all the function ID for the SIP subsystem. Can you
> confirm the EEMI can be accessed from both SMC32 and SMC64 convention?
Yep
> > + enum pm_ret_status ret;
> > +
> > + switch ( pm_fn )
> > + {
> > + /*
> > + * We can't allow CPUs to suspend without Xen knowing about it.
> > + * We accept but ignore the request and wait for the guest to issue
> > + * a WFI which Xen will trap and act accordingly upon.
>
> Why WFI specifically? The guest should suspend using PSCI normally.
Just a limitation of the comment. I'll a mention PSCI.
> > + */
> > + case PM_SELF_SUSPEND:
> > + ret = XST_PM_SUCCESS;
> > + goto done;
> > +
> > + case PM_GET_NODE_STATUS:
> > + /* API for PUs. */
> > + case PM_REQ_SUSPEND:
> > + case PM_FORCE_POWERDOWN:
> > + case PM_ABORT_SUSPEND:
> > + case PM_REQ_WAKEUP:
> > + case PM_SET_WAKEUP_SOURCE:
> > + /* API for slaves. */
> > + case PM_REQ_NODE:
> > + case PM_RELEASE_NODE:
> > + case PM_SET_REQUIREMENT:
> > + case PM_SET_MAX_LATENCY:
> > + if ( !domain_has_node_access(current->domain, nodeid) )
> > + {
> > + gprintk(XENLOG_WARNING,
> > + "zynqmp-pm: fn=%u No access to node %u\n", pm_fn,
> > nodeid);
> > + ret = XST_PM_NO_ACCESS;
> > + goto done;
> > + }
> > + goto forward_to_fw;
> > +
> > + case PM_RESET_ASSERT:
> > + case PM_RESET_GET_STATUS:
> > + if ( !domain_has_reset_access(current->domain, nodeid) )
> > + {
> > + gprintk(XENLOG_WARNING,
> > + "zynqmp-pm: fn=%u No access to reset %u\n", pm_fn,
> > nodeid);
> > + ret = XST_PM_NO_ACCESS;
> > + goto done;
> > + }
> > + goto forward_to_fw;
> > +
> > + /* These calls are safe and always allowed. */
> > + case ZYNQMP_SIP_SVC_CALL_COUNT:
> > + case ZYNQMP_SIP_SVC_UID:
> > + case ZYNQMP_SIP_SVC_VERSION:
> > + case PM_GET_TRUSTZONE_VERSION:
> > + case PM_GET_API_VERSION:
> > + case PM_GET_CHIPID:
> > + goto forward_to_fw;
> > +
> > + /* Mediated MMIO access. */
> > + case PM_MMIO_WRITE:
> > + is_mmio_write = true;
> > + /* Fallthrough. */
> > + case PM_MMIO_READ:
> > + {
> > + uint32_t mask = get_user_reg(regs, 1) >> 32;
>
> NIT: newline.
OK
>
> > + if ( !domain_has_mmio_access(current->domain,
> > + is_mmio_write,
> > + nodeid, &mask) )
> > + {
> > + gprintk(XENLOG_WARNING,
> > + "eemi: fn=%u No access to MMIO %s %u\n",
> > + pm_fn, is_mmio_write ? "write" : "read", nodeid);
> > + ret = XST_PM_NO_ACCESS;
> > + goto done;
> > + }
> > + set_user_reg(regs, 1, ((uint64_t)mask << 32) | nodeid);
>
> Can you define 32 in a macro?
Yes, I'll do
> > + goto forward_to_fw;
> > + }
> > +
> > + /* Exclusive to the hardware domain. */
> > + case PM_INIT:
> > + case PM_SET_CONFIGURATION:
> > + case PM_FPGA_LOAD:
> > + case PM_FPGA_GET_STATUS:
> > + case PM_SECURE_SHA:
> > + case PM_SECURE_RSA:
> > + case PM_PINCTRL_SET_FUNCTION:
> > + case PM_PINCTRL_REQUEST:
> > + case PM_PINCTRL_RELEASE:
> > + case PM_PINCTRL_GET_FUNCTION:
> > + case PM_PINCTRL_CONFIG_PARAM_GET:
> > + case PM_PINCTRL_CONFIG_PARAM_SET:
> > + case PM_IOCTL:
> > + case PM_QUERY_DATA:
> > + case PM_CLOCK_ENABLE:
> > + case PM_CLOCK_DISABLE:
> > + case PM_CLOCK_GETSTATE:
> > + case PM_CLOCK_GETDIVIDER:
> > + case PM_CLOCK_SETDIVIDER:
> > + case PM_CLOCK_SETRATE:
> > + case PM_CLOCK_GETRATE:
> > + case PM_CLOCK_SETPARENT:
> > + case PM_CLOCK_GETPARENT:
> > + if ( !is_hardware_domain(current->domain) )
> > + {
> > + gprintk(XENLOG_WARNING, "eemi: fn=%u No access", pm_fn);
> > + ret = XST_PM_NO_ACCESS;
> > + goto done;
> > + }
> > + goto forward_to_fw;
> > +
> > + /* These calls are never allowed. */
> > + case PM_SYSTEM_SHUTDOWN:
> > + ret = XST_PM_NO_ACCESS;
> > + goto done;
> > +
> > + default:
> > + gprintk(XENLOG_WARNING, "zynqmp-pm: Unhandled PM Call: %u\n", fid);
> > + return false;
>
> The functions COUNT, UUID, REVISION mandated by the SMCCC are missing here.
They are just above. They are defined as ZYNQMP_SIP_SVC_CALL_COUNT,
ZYNQMP_SIP_SVC_UID and ZYNQMP_SIP_SVC_VERSION. They are set to always
forward to firmware.
> > + }
> > +
> > +forward_to_fw:
> > + arm_smccc_1_1_smc(get_user_reg(regs, 0),
> > + 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;
> > +
> > +done:
> > + set_user_reg(regs, 0, ret);
> > + return true;
> > }
> > /*
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 4/6] xen/arm: zynqmp: eemi access control
2018-10-10 22:35 ` Stefano Stabellini
@ 2018-10-15 7:25 ` Julien Grall
2018-10-15 13:00 ` Julien Grall
0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-10-15 7:25 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: edgar.iglesias, Stefano Stabellini, xen-devel
Hi,
On 10/10/2018 11:35 PM, Stefano Stabellini wrote:
> On Tue, 28 Aug 2018, Julien Grall wrote:
>> On 11/08/18 01:01, Stefano Stabellini wrote:
>>> #include <xen/iocap.h>
>>> #include <xen/sched.h>
>>> #include <xen/types.h>
>>> @@ -23,6 +91,721 @@
>>> #include <asm/regs.h>
>>> #include <asm/platforms/xilinx-zynqmp-eemi.h>
>>> +struct pm_access
>>> +{
>>> + paddr_t addr;
>>
>> It seems that the address will always page-aligned. So could we store a frame
>> using mfn_t?
>
> Yes we could store just the frame. I started to make the change
> suggested, and all the required corresponding changes to the
> initializations below, for instance:
>
> [NODE_RPU] = { MM_RPU },
>
> needs to become:
>
> [NODE_RPU] = { _mfn(MM_RPU) },
>
> but when I tried to do that, I get a compilation error:
>
> xilinx-zynqmp-eemi.c:106:20: error: initializer element is not constant
> [NODE_RPU] = { _mfn(MM_RPU) },
>
> Unfortunately it is not possible to use mfn_t in static initializations,
> because it is a static inline. I could do:
>
This is a bug in GCC older than 5.0.
> [NODE_RPU] = { { MM_RPU } },
>
> which would work for DEBUG builds but wouldn't work for non-DEBUG
> builds.
>
> I'll keep paddr_t for the next version of the series.
What is the issue with that on non-debug build? We are using this
construction in another place without any compiler issue.
>
>
>>> + bool hwdom_access; /* HW domain gets access regardless. */
>>> +};
>>> +
>>> +/*
>>> + * This table maps a node into a memory address.
>>> + * If a guest has access to the address, it has enough control
>>> + * over the node to grant it access to EEMI calls for that node.
>>> + */
>>> +static const struct pm_access pm_node_access[] = {
>>
>> [...]
>>
>>> +
>>> +/*
>>> + * This table maps reset line IDs into a memory address.
>>> + * If a guest has access to the address, it has enough control
>>> + * over the affected node to grant it access to EEMI calls for
>>> + * resetting that node.
>>> + */
>>> +#define XILPM_RESET_IDX(n) (n - XILPM_RESET_PCIE_CFG)
>>> +static const struct pm_access pm_reset_access[] = {
>>
>> [...]
>>
>>> +
>>> +/*
>>> + * This table maps reset line IDs into a memory address.
>>> + * If a guest has access to the address, it has enough control
>>> + * over the affected node to grant it access to EEMI calls for
>>> + * resetting that node.
>>> + */
>>> +static const struct {
>>> + paddr_t start;
>>> + paddr_t size;
>>> + uint32_t mask; /* Zero means no mask, i.e all bits. */
>>> + enum pm_node_id node;
>>> + bool hwdom_access;
>>> + bool readonly;
>>> +} pm_mmio_access[] = {
>>
>> Those 3 arrays contains a lot of hardcoded value. Can't any of this be
>> detected from the device-tree?
>
> No, the information is not available on device tree unfortunately. >
>
>> I would be interested to know how this is going to work with upstream Linux.
>> Do you hardcode all the values there as well?
>
> Yes: the IDs are specified on an header file, see
> include/linux/firmware/xlnx-zynqmp.h on the zynq/firmware branch of the
> arm-soc tree. In addition to the IDs, we also have the memory addresses
> in Xen to do the permission checks.
I am afraid this is not linux upstream. Can you point to the code in
Linus's git or explain the state of the review?
>
>>> +static bool pm_check_access(const struct pm_access *acl, struct domain *d,
>>> + uint32_t idx)
>>> +{
>>> + unsigned long mfn;
>>
>> mfn_t please. The code is not that nice but at least it add more safety in the
>> code. Hopefully iommu_access_permitted will be converted to typesafe MFN soon.
>
> Yes, I can make this change without issues.
>
>
>>> +
>>> + if ( acl[idx].hwdom_access && is_hardware_domain(d) )
>>> + return true;
>>> +
>>> + if ( !acl[idx].addr )
>>> + return false;
>>> +
>>> + mfn = PFN_DOWN(acl[idx].addr);
>>
>> maddr_to_mfn(...);
>
> OK
>
>
>>> + return iomem_access_permitted(d, mfn, mfn);
>>
>> Is the address something that a guest would be allowed to read/write directly?
>> If not, then iomem_access_permitted(...) should not be used.
>
> Yes, the address would be something remapped to the guest using iomem.
In the documentation at the beginning of the file you say that memory
ranges are typically secure memory. So how a guest can access them directly?
I probably need a bit more background here... What is the address
correspond to at the end?
>
>>> +{
>>> + unsigned int i;
>>> + bool ret = false;
>>> + uint32_t prot_mask = 0;
>>> +
>>> + /*
>>> + * The hardware domain gets read access to everything.
>>> + * Lower layers will do further filtering.
>>> + */
>>> + if ( !write && is_hardware_domain(d) )
>>> + return true;
>>> +
>>> + /* Scan the ACL. */
>>> + for ( i = 0; i < ARRAY_SIZE(pm_mmio_access); i++ )
>>> + {
>>> + if ( addr < pm_mmio_access[i].start )
>>> + return false;
>>> + if ( addr > pm_mmio_access[i].start + pm_mmio_access[i].size )
>>
>> I would add an ASSERT(pm_mmio_access[i].start >= pm_mmio_access[i].size) to
>> catch wrapping.
>
> I take that you meant the following:
>
> ASSERT(pm_mmio_access[i].start + pm_mmio_access[i].size >= pm_mmio_access[i].start)
Yes.
>
>
>>> + continue;
>>> +
>>> + if ( write && pm_mmio_access[i].readonly )
>>> + return false;
>>> + if ( pm_mmio_access[i].hwdom_access && !is_hardware_domain(d) )
>>> + return false;
>>> + if ( !domain_has_node_access(d, pm_mmio_access[i].node) )
>>> + return false;
>>> +
>>> + /* We've got access to this reg (or parts of it). */
>>> + ret = true;
>>> +
>>> + /* Permit write access to selected bits. */
>>> + prot_mask |= pm_mmio_access[i].mask ?: 0xFFFFFFFF;
>> Can you use GENMASK here?
>
> Sure
>
>> NIT: newline
>
> OK
>
>>> + break;
>>> + }
>>> +
>>> + /* Masking only applies to writes. */
>>> + if ( write )
>>> + *mask &= prot_mask;
>>
>> So for reading there are no masking? What should be the value it?
>
> Yes, this is my understanding. The value is safe to read, but not all
> bits are writeable.
It would be good to write that assumption somewhere.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 5/6] xen/arm: zynqmp: implement zynqmp_eemi
2018-10-10 22:49 ` Stefano Stabellini
@ 2018-10-15 7:32 ` Julien Grall
2018-10-15 13:01 ` Julien Grall
0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-10-15 7:32 UTC (permalink / raw)
To: Stefano Stabellini
Cc: edgar.iglesias, Stefano Stabellini, Stefano Stabellini, xen-devel
Hi,
On 10/10/2018 11:49 PM, Stefano Stabellini wrote:
> On Tue, 28 Aug 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 11/08/18 01:01, Stefano Stabellini wrote:
>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>
>>> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>
>>> zynqmp_eemi uses the defined functions and structs to decide whether to
>>> make a call to the firmware, or to simply return a predefined value.
>>>
>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>> ---
>>> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 143
>>> +++++++++++++++++++++++++++-
>>> 1 file changed, 142 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> index 62cc15c..1dbdf04 100644
>>> --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> @@ -808,7 +808,148 @@ static bool domain_has_mmio_access(struct domain *d,
>>> bool zynqmp_eemi(struct cpu_user_regs *regs)
>>> {
>>> - return false;
>>> + struct arm_smccc_res res;
>>> + bool is_mmio_write = false;
>>> + uint32_t fid = get_user_reg(regs, 0);
>>> + uint32_t nodeid = get_user_reg(regs, 1);
>>> + unsigned int pm_fn = fid & 0xFFFF;
>>
>> Here you will receive all the function ID for the SIP subsystem. Can you
>> confirm the EEMI can be accessed from both SMC32 and SMC64 convention?
>
> Yep
Can you point me to the documentation? For instance, CALL_COUNT, UID and
VERSION are only accessible via SMC32.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 4/6] xen/arm: zynqmp: eemi access control
2018-10-15 7:25 ` Julien Grall
@ 2018-10-15 13:00 ` Julien Grall
2018-10-16 2:39 ` Stefano Stabellini
0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-10-15 13:00 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: edgar.iglesias, Stefano Stabellini, nd, xen-devel
(Resending the e-mail using a different smtp)
On 15/10/2018 08:25, Julien Grall wrote:
> Hi,
>
> On 10/10/2018 11:35 PM, Stefano Stabellini wrote:
>> On Tue, 28 Aug 2018, Julien Grall wrote:
>>> On 11/08/18 01:01, Stefano Stabellini wrote:
>>>> #include <xen/iocap.h>
>>>> #include <xen/sched.h>
>>>> #include <xen/types.h>
>>>> @@ -23,6 +91,721 @@
>>>> #include <asm/regs.h>
>>>> #include <asm/platforms/xilinx-zynqmp-eemi.h>
>>>> +struct pm_access
>>>> +{
>>>> + paddr_t addr;
>>>
>>> It seems that the address will always page-aligned. So could we store
>>> a frame
>>> using mfn_t?
>>
>> Yes we could store just the frame. I started to make the change
>> suggested, and all the required corresponding changes to the
>> initializations below, for instance:
>>
>> [NODE_RPU] = { MM_RPU },
>>
>> needs to become:
>>
>> [NODE_RPU] = { _mfn(MM_RPU) },
>>
>> but when I tried to do that, I get a compilation error:
>>
>> xilinx-zynqmp-eemi.c:106:20: error: initializer element is not
>> constant
>> [NODE_RPU] = { _mfn(MM_RPU) },
>>
>> Unfortunately it is not possible to use mfn_t in static initializations,
>> because it is a static inline. I could do:
>>
>
> This is a bug in GCC older than 5.0.
>
>> [NODE_RPU] = { { MM_RPU } },
>>
>> which would work for DEBUG builds but wouldn't work for non-DEBUG
>> builds.
>>
>> I'll keep paddr_t for the next version of the series.
>
> What is the issue with that on non-debug build? We are using this
> construction in another place without any compiler issue.
>
>>
>>
>>>> + bool hwdom_access; /* HW domain gets access regardless. */
>>>> +};
>>>> +
>>>> +/*
>>>> + * This table maps a node into a memory address.
>>>> + * If a guest has access to the address, it has enough control
>>>> + * over the node to grant it access to EEMI calls for that node.
>>>> + */
>>>> +static const struct pm_access pm_node_access[] = {
>>>
>>> [...]
>>>
>>>> +
>>>> +/*
>>>> + * This table maps reset line IDs into a memory address.
>>>> + * If a guest has access to the address, it has enough control
>>>> + * over the affected node to grant it access to EEMI calls for
>>>> + * resetting that node.
>>>> + */
>>>> +#define XILPM_RESET_IDX(n) (n - XILPM_RESET_PCIE_CFG)
>>>> +static const struct pm_access pm_reset_access[] = {
>>>
>>> [...]
>>>
>>>> +
>>>> +/*
>>>> + * This table maps reset line IDs into a memory address.
>>>> + * If a guest has access to the address, it has enough control
>>>> + * over the affected node to grant it access to EEMI calls for
>>>> + * resetting that node.
>>>> + */
>>>> +static const struct {
>>>> + paddr_t start;
>>>> + paddr_t size;
>>>> + uint32_t mask; /* Zero means no mask, i.e all bits. */
>>>> + enum pm_node_id node;
>>>> + bool hwdom_access;
>>>> + bool readonly;
>>>> +} pm_mmio_access[] = {
>>>
>>> Those 3 arrays contains a lot of hardcoded value. Can't any of this be
>>> detected from the device-tree?
>>
>> No, the information is not available on device tree unfortunately. >
>>
>>> I would be interested to know how this is going to work with upstream
>>> Linux.
>>> Do you hardcode all the values there as well?
>>
>> Yes: the IDs are specified on an header file, see
>> include/linux/firmware/xlnx-zynqmp.h on the zynq/firmware branch of the
>> arm-soc tree. In addition to the IDs, we also have the memory addresses
>> in Xen to do the permission checks.
>
> I am afraid this is not linux upstream. Can you point to the code in
> Linus's git or explain the state of the review?
>
>>
>>>> +static bool pm_check_access(const struct pm_access *acl, struct
>>>> domain *d,
>>>> + uint32_t idx)
>>>> +{
>>>> + unsigned long mfn;
>>>
>>> mfn_t please. The code is not that nice but at least it add more
>>> safety in the
>>> code. Hopefully iommu_access_permitted will be converted to typesafe
>>> MFN soon.
>>
>> Yes, I can make this change without issues.
>>
>>
>>>> +
>>>> + if ( acl[idx].hwdom_access && is_hardware_domain(d) )
>>>> + return true;
>>>> +
>>>> + if ( !acl[idx].addr )
>>>> + return false;
>>>> +
>>>> + mfn = PFN_DOWN(acl[idx].addr);
>>>
>>> maddr_to_mfn(...);
>>
>> OK
>>
>>
>>>> + return iomem_access_permitted(d, mfn, mfn);
>>>
>>> Is the address something that a guest would be allowed to read/write
>>> directly?
>>> If not, then iomem_access_permitted(...) should not be used.
>>
>> Yes, the address would be something remapped to the guest using iomem.
>
> In the documentation at the beginning of the file you say that memory
> ranges are typically secure memory. So how a guest can access them
> directly?
>
> I probably need a bit more background here... What is the address
> correspond to at the end?
>
>>
>>>> +{
>>>> + unsigned int i;
>>>> + bool ret = false;
>>>> + uint32_t prot_mask = 0;
>>>> +
>>>> + /*
>>>> + * The hardware domain gets read access to everything.
>>>> + * Lower layers will do further filtering.
>>>> + */
>>>> + if ( !write && is_hardware_domain(d) )
>>>> + return true;
>>>> +
>>>> + /* Scan the ACL. */
>>>> + for ( i = 0; i < ARRAY_SIZE(pm_mmio_access); i++ )
>>>> + {
>>>> + if ( addr < pm_mmio_access[i].start )
>>>> + return false;
>>>> + if ( addr > pm_mmio_access[i].start + pm_mmio_access[i].size )
>>>
>>> I would add an ASSERT(pm_mmio_access[i].start >=
>>> pm_mmio_access[i].size) to
>>> catch wrapping.
>>
>> I take that you meant the following:
>>
>> ASSERT(pm_mmio_access[i].start + pm_mmio_access[i].size >=
>> pm_mmio_access[i].start)
>
> Yes.
>
>>
>>
>>>> + continue;
>>>> +
>>>> + if ( write && pm_mmio_access[i].readonly )
>>>> + return false;
>>>> + if ( pm_mmio_access[i].hwdom_access &&
>>>> !is_hardware_domain(d) )
>>>> + return false;
>>>> + if ( !domain_has_node_access(d, pm_mmio_access[i].node) )
>>>> + return false;
>>>> +
>>>> + /* We've got access to this reg (or parts of it). */
>>>> + ret = true;
>>>> +
>>>> + /* Permit write access to selected bits. */
>>>> + prot_mask |= pm_mmio_access[i].mask ?: 0xFFFFFFFF;
>>> Can you use GENMASK here?
>>
>> Sure
>>
>>> NIT: newline
>>
>> OK
>>
>>>> + break;
>>>> + }
>>>> +
>>>> + /* Masking only applies to writes. */
>>>> + if ( write )
>>>> + *mask &= prot_mask;
>>>
>>> So for reading there are no masking? What should be the value it?
>>
>> Yes, this is my understanding. The value is safe to read, but not all
>> bits are writeable.
>
> It would be good to write that assumption somewhere.
>
> Cheers,
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 5/6] xen/arm: zynqmp: implement zynqmp_eemi
2018-10-15 7:32 ` Julien Grall
@ 2018-10-15 13:01 ` Julien Grall
2018-10-16 6:48 ` Stefano Stabellini
0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-10-15 13:01 UTC (permalink / raw)
To: Stefano Stabellini
Cc: edgar.iglesias, Stefano Stabellini, nd, Stefano Stabellini, xen-devel
(Resending the e-mail using a different smtp)
On 15/10/2018 08:32, Julien Grall wrote:
> Hi,
>
> On 10/10/2018 11:49 PM, Stefano Stabellini wrote:
>> On Tue, 28 Aug 2018, Julien Grall wrote:
>>> Hi Stefano,
>>>
>>> On 11/08/18 01:01, Stefano Stabellini wrote:
>>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>>
>>>> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>>
>>>> zynqmp_eemi uses the defined functions and structs to decide whether to
>>>> make a call to the firmware, or to simply return a predefined value.
>>>>
>>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>>> ---
>>>> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 143
>>>> +++++++++++++++++++++++++++-
>>>> 1 file changed, 142 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>> b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>> index 62cc15c..1dbdf04 100644
>>>> --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>> @@ -808,7 +808,148 @@ static bool domain_has_mmio_access(struct
>>>> domain *d,
>>>> bool zynqmp_eemi(struct cpu_user_regs *regs)
>>>> {
>>>> - return false;
>>>> + struct arm_smccc_res res;
>>>> + bool is_mmio_write = false;
>>>> + uint32_t fid = get_user_reg(regs, 0);
>>>> + uint32_t nodeid = get_user_reg(regs, 1);
>>>> + unsigned int pm_fn = fid & 0xFFFF;
>>>
>>> Here you will receive all the function ID for the SIP subsystem. Can you
>>> confirm the EEMI can be accessed from both SMC32 and SMC64 convention?
>>
>> Yep
>
> Can you point me to the documentation? For instance, CALL_COUNT, UID and
> VERSION are only accessible via SMC32.
>
> Cheers,
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 4/6] xen/arm: zynqmp: eemi access control
2018-10-15 13:00 ` Julien Grall
@ 2018-10-16 2:39 ` Stefano Stabellini
2018-10-16 13:23 ` Julien Grall
2018-10-16 13:29 ` Julien Grall
0 siblings, 2 replies; 29+ messages in thread
From: Stefano Stabellini @ 2018-10-16 2:39 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, nd, Stefano Stabellini, xen-devel
[-- Attachment #1: Type: TEXT/PLAIN, Size: 9412 bytes --]
On 15/10/2018 08:25, Julien Grall wrote:
> Hi,
>
> On 10/10/2018 11:35 PM, Stefano Stabellini wrote:
> > On Tue, 28 Aug 2018, Julien Grall wrote:
> > > On 11/08/18 01:01, Stefano Stabellini wrote:
> > > > #include <xen/iocap.h>
> > > > #include <xen/sched.h>
> > > > #include <xen/types.h>
> > > > @@ -23,6 +91,721 @@
> > > > #include <asm/regs.h>
> > > > #include <asm/platforms/xilinx-zynqmp-eemi.h>
> > > > +struct pm_access
> > > > +{
> > > > + paddr_t addr;
> > >
> > > It seems that the address will always page-aligned. So could we store a
> > > frame
> > > using mfn_t?
> >
> > Yes we could store just the frame. I started to make the change
> > suggested, and all the required corresponding changes to the
> > initializations below, for instance:
> >
> > [NODE_RPU] = { MM_RPU },
> >
> > needs to become:
> >
> > [NODE_RPU] = { _mfn(MM_RPU) },
> >
> > but when I tried to do that, I get a compilation error:
> >
> > xilinx-zynqmp-eemi.c:106:20: error: initializer element is not constant
> > [NODE_RPU] = { _mfn(MM_RPU) },
> >
> > Unfortunately it is not possible to use mfn_t in static initializations,
> > because it is a static inline. I could do:
> >
>
> This is a bug in GCC older than 5.0.
>
> > [NODE_RPU] = { { MM_RPU } },
> >
> > which would work for DEBUG builds but wouldn't work for non-DEBUG
> > builds.
> >
> > I'll keep paddr_t for the next version of the series.
>
> What is the issue with that on non-debug build? We are using this
> construction in another place without any compiler issue.
I modified the code as suggested again and tried with newer GCCs (both
6.3.1 and 7.3.1) but I still get the same error:
xilinx-zynqmp-eemi.c:105:20: error: initializer element is not constant
[NODE_RPU] = { _mfn(MM_RPU) },
I pushed a temporary branch here (only with patches up until this one,
removing the #if 0, so if everything goes well you should still get the
function unused warnings) just in case you want to give it a try:
http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git eemi-upstreaming-wip
> >
> >
> > > > + bool hwdom_access; /* HW domain gets access regardless. */
> > > > +};
> > > > +
> > > > +/*
> > > > + * This table maps a node into a memory address.
> > > > + * If a guest has access to the address, it has enough control
> > > > + * over the node to grant it access to EEMI calls for that node.
> > > > + */
> > > > +static const struct pm_access pm_node_access[] = {
> > >
> > > [...]
> > >
> > > > +
> > > > +/*
> > > > + * This table maps reset line IDs into a memory address.
> > > > + * If a guest has access to the address, it has enough control
> > > > + * over the affected node to grant it access to EEMI calls for
> > > > + * resetting that node.
> > > > + */
> > > > +#define XILPM_RESET_IDX(n) (n - XILPM_RESET_PCIE_CFG)
> > > > +static const struct pm_access pm_reset_access[] = {
> > >
> > > [...]
> > >
> > > > +
> > > > +/*
> > > > + * This table maps reset line IDs into a memory address.
> > > > + * If a guest has access to the address, it has enough control
> > > > + * over the affected node to grant it access to EEMI calls for
> > > > + * resetting that node.
> > > > + */
> > > > +static const struct {
> > > > + paddr_t start;
> > > > + paddr_t size;
> > > > + uint32_t mask; /* Zero means no mask, i.e all bits. */
> > > > + enum pm_node_id node;
> > > > + bool hwdom_access;
> > > > + bool readonly;
> > > > +} pm_mmio_access[] = {
> > >
> > > Those 3 arrays contains a lot of hardcoded value. Can't any of this be
> > > detected from the device-tree?
> >
> > No, the information is not available on device tree unfortunately. >
> >
> > > I would be interested to know how this is going to work with upstream
> > > Linux.
> > > Do you hardcode all the values there as well?
> >
> > Yes: the IDs are specified on an header file, see
> > include/linux/firmware/xlnx-zynqmp.h on the zynq/firmware branch of the
> > arm-soc tree. In addition to the IDs, we also have the memory addresses
> > in Xen to do the permission checks.
>
> I am afraid this is not linux upstream. Can you point to the code in Linus's
> git or explain the state of the review?
It hasn't been pulled into Linux yet, I was told it has already been
reviewed and is queued in arm-soc for upstreaming at the next merge
window, which should be imminent.
> > > > +static bool pm_check_access(const struct pm_access *acl, struct
> > > > domain *d,
> > > > + uint32_t idx)
> > > > +{
> > > > + unsigned long mfn;
> > >
> > > mfn_t please. The code is not that nice but at least it add more safety
> > > in the
> > > code. Hopefully iommu_access_permitted will be converted to typesafe MFN
> > > soon.
> >
> > Yes, I can make this change without issues.
> >
> >
> > > > +
> > > > + if ( acl[idx].hwdom_access && is_hardware_domain(d) )
> > > > + return true;
> > > > +
> > > > + if ( !acl[idx].addr )
> > > > + return false;
> > > > +
> > > > + mfn = PFN_DOWN(acl[idx].addr);
> > >
> > > maddr_to_mfn(...);
> >
> > OK
> >
> >
> > > > + return iomem_access_permitted(d, mfn, mfn);
> > >
> > > Is the address something that a guest would be allowed to read/write
> > > directly?
> > > If not, then iomem_access_permitted(...) should not be used.
> >
> > Yes, the address would be something remapped to the guest using iomem.
>
> In the documentation at the beginning of the file you say that memory ranges
> are typically secure memory. So how a guest can access them directly?
>
> I probably need a bit more background here... What is the address correspond
> to at the end?
The address corresponds to the MMIO region of the device. For instance,
MM_GEM0 is 0xff0b0000, which is the address of the network card. It is
accessible. The same goes for MM_CAN0, MM_TTC0, MM_SD0, and all the
others -- they are all accessible. These are the addresses used in this
check that should be remapped into the guest.
However, things are different for the pm_mmio_access array used in
domain_has_mmio_access to check for access rights. (That is also where
the mask is applied to writes and not to reads, see below.) In that
case, the addresses correspond to single registers inside the
zynqmp-psgtr region ("ZynqMP High Speed Processing System Gigabit
Transceiver") and pin controller region. As you can see, in
domain_has_mmio_access the access checks are still done on the
corresponding node address, which is the one that gets remapped. For
instance in the case of the network card, the remapped address is
0xff0b0000, checks are done on it, but the zynqmp-psgtr and pin
contoller region registers are:
{
.start = MM_CRL_APB + R_CRL_GEM0_REF_CTRL,
.size = 4, .node = NODE_ETH_0
}
[...]
{
.start = MM_IOU_SLCR + R_IOU_SLCR_IOU_RAM_GEM0,
.size = 4, .node = NODE_ETH_0
}
The comment comes from Edgar, but I think that he meant that VMs won't
be able to write to these regions directly. I'll improve the comment.
> > > > +{
> > > > + unsigned int i;
> > > > + bool ret = false;
> > > > + uint32_t prot_mask = 0;
> > > > +
> > > > + /*
> > > > + * The hardware domain gets read access to everything.
> > > > + * Lower layers will do further filtering.
> > > > + */
> > > > + if ( !write && is_hardware_domain(d) )
> > > > + return true;
> > > > +
> > > > + /* Scan the ACL. */
> > > > + for ( i = 0; i < ARRAY_SIZE(pm_mmio_access); i++ )
> > > > + {
> > > > + if ( addr < pm_mmio_access[i].start )
> > > > + return false;
> > > > + if ( addr > pm_mmio_access[i].start + pm_mmio_access[i].size
> > > > )
> > >
> > > I would add an ASSERT(pm_mmio_access[i].start >= pm_mmio_access[i].size)
> > > to
> > > catch wrapping.
> >
> > I take that you meant the following:
> >
> > ASSERT(pm_mmio_access[i].start + pm_mmio_access[i].size >=
> > pm_mmio_access[i].start)
>
> Yes.
>
> >
> >
> > > > + continue;
> > > > +
> > > > + if ( write && pm_mmio_access[i].readonly )
> > > > + return false;
> > > > + if ( pm_mmio_access[i].hwdom_access && !is_hardware_domain(d)
> > > > )
> > > > + return false;
> > > > + if ( !domain_has_node_access(d, pm_mmio_access[i].node) )
> > > > + return false;
> > > > +
> > > > + /* We've got access to this reg (or parts of it). */
> > > > + ret = true;
> > > > +
> > > > + /* Permit write access to selected bits. */
> > > > + prot_mask |= pm_mmio_access[i].mask ?: 0xFFFFFFFF;
> > > Can you use GENMASK here?
> >
> > Sure
> >
> > > NIT: newline
> >
> > OK
> >
> > > > + break;
> > > > + }
> > > > +
> > > > + /* Masking only applies to writes. */
> > > > + if ( write )
> > > > + *mask &= prot_mask;
> > >
> > > So for reading there are no masking? What should be the value it?
> >
> > Yes, this is my understanding. The value is safe to read, but not all
> > bits are writeable.
>
> It would be good to write that assumption somewhere.
OK
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 5/6] xen/arm: zynqmp: implement zynqmp_eemi
2018-10-15 13:01 ` Julien Grall
@ 2018-10-16 6:48 ` Stefano Stabellini
2018-10-16 13:44 ` Julien Grall
0 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2018-10-16 6:48 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, Stefano Stabellini,
xen-devel, Stefano Stabellini, nd
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2178 bytes --]
On Mon, 15 Oct 2018, Julien Grall wrote:
> (Resending the e-mail using a different smtp)
>
> On 15/10/2018 08:32, Julien Grall wrote:
> > Hi,
> >
> > On 10/10/2018 11:49 PM, Stefano Stabellini wrote:
> > > On Tue, 28 Aug 2018, Julien Grall wrote:
> > > > Hi Stefano,
> > > >
> > > > On 11/08/18 01:01, Stefano Stabellini wrote:
> > > > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > > > >
> > > > > From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > > > >
> > > > > zynqmp_eemi uses the defined functions and structs to decide whether
> > > > > to
> > > > > make a call to the firmware, or to simply return a predefined value.
> > > > >
> > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > > > > ---
> > > > > xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 143
> > > > > +++++++++++++++++++++++++++-
> > > > > 1 file changed, 142 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > > > > b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > > > > index 62cc15c..1dbdf04 100644
> > > > > --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > > > > +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > > > > @@ -808,7 +808,148 @@ static bool domain_has_mmio_access(struct domain
> > > > > *d,
> > > > > bool zynqmp_eemi(struct cpu_user_regs *regs)
> > > > > {
> > > > > - return false;
> > > > > + struct arm_smccc_res res;
> > > > > + bool is_mmio_write = false;
> > > > > + uint32_t fid = get_user_reg(regs, 0);
> > > > > + uint32_t nodeid = get_user_reg(regs, 1);
> > > > > + unsigned int pm_fn = fid & 0xFFFF;
> > > >
> > > > Here you will receive all the function ID for the SIP subsystem. Can you
> > > > confirm the EEMI can be accessed from both SMC32 and SMC64 convention?
> > >
> > > Yep
> >
> > Can you point me to the documentation? For instance, CALL_COUNT, UID and
> > VERSION are only accessible via SMC32.
https://www.xilinx.com/support/documentation/user_guides/ug1200-eemi-api.pdf
I am checking if there is a newer version of it.
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 4/6] xen/arm: zynqmp: eemi access control
2018-10-16 2:39 ` Stefano Stabellini
@ 2018-10-16 13:23 ` Julien Grall
2018-10-17 13:58 ` Stefano Stabellini
2018-10-16 13:29 ` Julien Grall
1 sibling, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-10-16 13:23 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: edgar.iglesias, Stefano Stabellini, nd, xen-devel
Hi Stefano,
On 10/16/2018 03:39 AM, Stefano Stabellini wrote:
> On 15/10/2018 08:25, Julien Grall wrote:
>> Hi,
>>
>> On 10/10/2018 11:35 PM, Stefano Stabellini wrote:
>>> On Tue, 28 Aug 2018, Julien Grall wrote:
>>>> On 11/08/18 01:01, Stefano Stabellini wrote:
>>>>> #include <xen/iocap.h>
>>>>> #include <xen/sched.h>
>>>>> #include <xen/types.h>
>>>>> @@ -23,6 +91,721 @@
>>>>> #include <asm/regs.h>
>>>>> #include <asm/platforms/xilinx-zynqmp-eemi.h>
>>>>> +struct pm_access
>>>>> +{
>>>>> + paddr_t addr;
>>>>
>>>> It seems that the address will always page-aligned. So could we store a
>>>> frame
>>>> using mfn_t?
>>>
>>> Yes we could store just the frame. I started to make the change
>>> suggested, and all the required corresponding changes to the
>>> initializations below, for instance:
>>>
>>> [NODE_RPU] = { MM_RPU },
>>>
>>> needs to become:
>>>
>>> [NODE_RPU] = { _mfn(MM_RPU) },
>>>
>>> but when I tried to do that, I get a compilation error:
>>>
>>> xilinx-zynqmp-eemi.c:106:20: error: initializer element is not constant
>>> [NODE_RPU] = { _mfn(MM_RPU) },
>>>
>>> Unfortunately it is not possible to use mfn_t in static initializations,
>>> because it is a static inline. I could do:
>>>
>>
>> This is a bug in GCC older than 5.0.
>>
>>> [NODE_RPU] = { { MM_RPU } },
>>>
>>> which would work for DEBUG builds but wouldn't work for non-DEBUG
>>> builds.
>>>
>>> I'll keep paddr_t for the next version of the series.
>>
>> What is the issue with that on non-debug build? We are using this
>> construction in another place without any compiler issue.
>
> I modified the code as suggested again and tried with newer GCCs (both
> 6.3.1 and 7.3.1) but I still get the same error:
>
> xilinx-zynqmp-eemi.c:105:20: error: initializer element is not constant
> [NODE_RPU] = { _mfn(MM_RPU) },
I actually misremembered the problem. _mfn is using a static inline
helper which is not considered as a constant.
The correct solution would be (mfn_t){ MM_RPU } but GCC 5.0 (and older)
does not parse correctly the type cast. So we workaround by dropping the
cast for the initializer.
On your previous e-mail you said that { { MM_RPU } } would not work for
debug. One solution would be to introduce _mfn_initializer(...) that
would be implemented differently for debug and non-debug.
I think such helper would be useful in other context as well.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 4/6] xen/arm: zynqmp: eemi access control
2018-10-16 2:39 ` Stefano Stabellini
2018-10-16 13:23 ` Julien Grall
@ 2018-10-16 13:29 ` Julien Grall
2018-10-17 14:03 ` Stefano Stabellini
1 sibling, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-10-16 13:29 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: edgar.iglesias, Stefano Stabellini, nd, xen-devel
Hi,
Sorry I forgot to answer to the rest of the e-mail.
On 10/16/2018 03:39 AM, Stefano Stabellini wrote:
> On 15/10/2018 08:25, Julien Grall wrote:
>>>>> + bool hwdom_access; /* HW domain gets access regardless. */
>>>>> +};
>>>>> +
>>>>> +/*
>>>>> + * This table maps a node into a memory address.
>>>>> + * If a guest has access to the address, it has enough control
>>>>> + * over the node to grant it access to EEMI calls for that node.
>>>>> + */
>>>>> +static const struct pm_access pm_node_access[] = {
>>>>
>>>> [...]
>>>>
>>>>> +
>>>>> +/*
>>>>> + * This table maps reset line IDs into a memory address.
>>>>> + * If a guest has access to the address, it has enough control
>>>>> + * over the affected node to grant it access to EEMI calls for
>>>>> + * resetting that node.
>>>>> + */
>>>>> +#define XILPM_RESET_IDX(n) (n - XILPM_RESET_PCIE_CFG)
>>>>> +static const struct pm_access pm_reset_access[] = {
>>>>
>>>> [...]
>>>>
>>>>> +
>>>>> +/*
>>>>> + * This table maps reset line IDs into a memory address.
>>>>> + * If a guest has access to the address, it has enough control
>>>>> + * over the affected node to grant it access to EEMI calls for
>>>>> + * resetting that node.
>>>>> + */
>>>>> +static const struct {
>>>>> + paddr_t start;
>>>>> + paddr_t size;
>>>>> + uint32_t mask; /* Zero means no mask, i.e all bits. */
>>>>> + enum pm_node_id node;
>>>>> + bool hwdom_access;
>>>>> + bool readonly;
>>>>> +} pm_mmio_access[] = {
>>>>
>>>> Those 3 arrays contains a lot of hardcoded value. Can't any of this be
>>>> detected from the device-tree?
>>>
>>> No, the information is not available on device tree unfortunately. >
>>>
>>>> I would be interested to know how this is going to work with upstream
>>>> Linux.
>>>> Do you hardcode all the values there as well?
>>>
>>> Yes: the IDs are specified on an header file, see
>>> include/linux/firmware/xlnx-zynqmp.h on the zynq/firmware branch of the
>>> arm-soc tree. In addition to the IDs, we also have the memory addresses
>>> in Xen to do the permission checks.
>>
>> I am afraid this is not linux upstream. Can you point to the code in Linus's
>> git or explain the state of the review?
>
> It hasn't been pulled into Linux yet, I was told it has already been
> reviewed and is queued in arm-soc for upstreaming at the next merge
> window, which should be imminent.
Looking at that branch, I can see some DT bindings at least for the
clock. I also don't see any hardcoded value for device so far in that
series. Is it going to be sent separately?
>
>
>>>>> +static bool pm_check_access(const struct pm_access *acl, struct
>>>>> domain *d,
>>>>> + uint32_t idx)
>>>>> +{
>>>>> + unsigned long mfn;
>>>>
>>>> mfn_t please. The code is not that nice but at least it add more safety
>>>> in the
>>>> code. Hopefully iommu_access_permitted will be converted to typesafe MFN
>>>> soon.
>>>
>>> Yes, I can make this change without issues.
>>>
>>>
>>>>> +
>>>>> + if ( acl[idx].hwdom_access && is_hardware_domain(d) )
>>>>> + return true;
>>>>> +
>>>>> + if ( !acl[idx].addr )
>>>>> + return false;
>>>>> +
>>>>> + mfn = PFN_DOWN(acl[idx].addr);
>>>>
>>>> maddr_to_mfn(...);
>>>
>>> OK
>>>
>>>
>>>>> + return iomem_access_permitted(d, mfn, mfn);
>>>>
>>>> Is the address something that a guest would be allowed to read/write
>>>> directly?
>>>> If not, then iomem_access_permitted(...) should not be used.
>>>
>>> Yes, the address would be something remapped to the guest using iomem.
>>
>> In the documentation at the beginning of the file you say that memory ranges
>> are typically secure memory. So how a guest can access them directly?
>>
>> I probably need a bit more background here... What is the address correspond
>> to at the end?
>
> The address corresponds to the MMIO region of the device. For instance,
> MM_GEM0 is 0xff0b0000, which is the address of the network card. It is
> accessible. The same goes for MM_CAN0, MM_TTC0, MM_SD0, and all the
> others -- they are all accessible. These are the addresses used in this
> check that should be remapped into the guest.
>
> However, things are different for the pm_mmio_access array used in
> domain_has_mmio_access to check for access rights. (That is also where
> the mask is applied to writes and not to reads, see below.) In that
> case, the addresses correspond to single registers inside the
> zynqmp-psgtr region ("ZynqMP High Speed Processing System Gigabit
> Transceiver") and pin controller region. As you can see, in
> domain_has_mmio_access the access checks are still done on the
> corresponding node address, which is the one that gets remapped. For
> instance in the case of the network card, the remapped address is
> 0xff0b0000, checks are done on it, but the zynqmp-psgtr and pin
> contoller region registers are:
>
> {
> .start = MM_CRL_APB + R_CRL_GEM0_REF_CTRL,
> .size = 4, .node = NODE_ETH_0
> }
> [...]
> {
> .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_RAM_GEM0,
> .size = 4, .node = NODE_ETH_0
> }
>
> The comment comes from Edgar, but I think that he meant that VMs won't
> be able to write to these regions directly. I'll improve the comment.
Thank you for the explanation. Can you add something similar to what you
wrote in the code?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 5/6] xen/arm: zynqmp: implement zynqmp_eemi
2018-10-16 6:48 ` Stefano Stabellini
@ 2018-10-16 13:44 ` Julien Grall
0 siblings, 0 replies; 29+ messages in thread
From: Julien Grall @ 2018-10-16 13:44 UTC (permalink / raw)
To: Stefano Stabellini
Cc: edgar.iglesias, Stefano Stabellini, nd, Stefano Stabellini, xen-devel
Hi,
On 10/16/2018 07:48 AM, Stefano Stabellini wrote:
> On Mon, 15 Oct 2018, Julien Grall wrote:
>> (Resending the e-mail using a different smtp)
>>
>> On 15/10/2018 08:32, Julien Grall wrote:
>>> Hi,
>>>
>>> On 10/10/2018 11:49 PM, Stefano Stabellini wrote:
>>>> On Tue, 28 Aug 2018, Julien Grall wrote:
>>>>> Hi Stefano,
>>>>>
>>>>> On 11/08/18 01:01, Stefano Stabellini wrote:
>>>>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>>>>
>>>>>> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>>>>
>>>>>> zynqmp_eemi uses the defined functions and structs to decide whether
>>>>>> to
>>>>>> make a call to the firmware, or to simply return a predefined value.
>>>>>>
>>>>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>>>>> ---e
>>>>>> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 143
>>>>>> +++++++++++++++++++++++++++-
>>>>>> 1 file changed, 142 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>>>> b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>>>> index 62cc15c..1dbdf04 100644
>>>>>> --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>>>> @@ -808,7 +808,148 @@ static bool domain_has_mmio_access(struct domain
>>>>>> *d,
>>>>>> bool zynqmp_eemi(struct cpu_user_regs *regs)
>>>>>> {
>>>>>> - return false;
>>>>>> + struct arm_smccc_res res;
>>>>>> + bool is_mmio_write = false;
>>>>>> + uint32_t fid = get_user_reg(regs, 0);
>>>>>> + uint32_t nodeid = get_user_reg(regs, 1);
>>>>>> + unsigned int pm_fn = fid & 0xFFFF;
>>>>>
>>>>> Here you will receive all the function ID for the SIP subsystem. Can you
>>>>> confirm the EEMI can be accessed from both SMC32 and SMC64 convention?
>>>>
>>>> Yep
>>>
>>> Can you point me to the documentation? For instance, CALL_COUNT, UID and
>>> VERSION are only accessible via SMC32.
>
> https://www.xilinx.com/support/documentation/user_guides/ug1200-eemi-api.pdf
>
> I am checking if there is a newer version of it.
I am afraid I can't find the function ID in that documentation nor any
mention of the SMC calling convention. I am surprised that EEMI would
ignore tops bits of the ID given they convey different information (e.g
fast/yielding call, 32/64-bit convention).
Looking at the branch you mentioned earlier on, zynqmp_pm_invoke_fn
(drivers/firmware/xilinx/zynqmp.c) is definitely using the SMC64 calling
convention as described in the documentation above the function.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 4/6] xen/arm: zynqmp: eemi access control
2018-10-16 13:23 ` Julien Grall
@ 2018-10-17 13:58 ` Stefano Stabellini
0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2018-10-17 13:58 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, nd, Stefano Stabellini, xen-devel
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2791 bytes --]
On Tue, 16 Oct 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 10/16/2018 03:39 AM, Stefano Stabellini wrote:
> > On 15/10/2018 08:25, Julien Grall wrote:
> > > Hi,
> > >
> > > On 10/10/2018 11:35 PM, Stefano Stabellini wrote:
> > > > On Tue, 28 Aug 2018, Julien Grall wrote:
> > > > > On 11/08/18 01:01, Stefano Stabellini wrote:
> > > > > > #include <xen/iocap.h>
> > > > > > #include <xen/sched.h>
> > > > > > #include <xen/types.h>
> > > > > > @@ -23,6 +91,721 @@
> > > > > > #include <asm/regs.h>
> > > > > > #include <asm/platforms/xilinx-zynqmp-eemi.h>
> > > > > > +struct pm_access
> > > > > > +{
> > > > > > + paddr_t addr;
> > > > >
> > > > > It seems that the address will always page-aligned. So could we store
> > > > > a
> > > > > frame
> > > > > using mfn_t?
> > > >
> > > > Yes we could store just the frame. I started to make the change
> > > > suggested, and all the required corresponding changes to the
> > > > initializations below, for instance:
> > > >
> > > > [NODE_RPU] = { MM_RPU },
> > > >
> > > > needs to become:
> > > >
> > > > [NODE_RPU] = { _mfn(MM_RPU) },
> > > >
> > > > but when I tried to do that, I get a compilation error:
> > > >
> > > > xilinx-zynqmp-eemi.c:106:20: error: initializer element is not
> > > > constant
> > > > [NODE_RPU] = { _mfn(MM_RPU) },
> > > >
> > > > Unfortunately it is not possible to use mfn_t in static initializations,
> > > > because it is a static inline. I could do:
> > > >
> > >
> > > This is a bug in GCC older than 5.0.
> > >
> > > > [NODE_RPU] = { { MM_RPU } },
> > > >
> > > > which would work for DEBUG builds but wouldn't work for non-DEBUG
> > > > builds.
> > > >
> > > > I'll keep paddr_t for the next version of the series.
> > >
> > > What is the issue with that on non-debug build? We are using this
> > > construction in another place without any compiler issue.
> >
> > I modified the code as suggested again and tried with newer GCCs (both
> > 6.3.1 and 7.3.1) but I still get the same error:
> >
> > xilinx-zynqmp-eemi.c:105:20: error: initializer element is not constant
> > [NODE_RPU] = { _mfn(MM_RPU) },
>
> I actually misremembered the problem. _mfn is using a static inline helper
> which is not considered as a constant.
>
> The correct solution would be (mfn_t){ MM_RPU } but GCC 5.0 (and older) does
> not parse correctly the type cast. So we workaround by dropping the cast for
> the initializer.
>
> On your previous e-mail you said that { { MM_RPU } } would not work for debug.
> One solution would be to introduce _mfn_initializer(...) that would be
> implemented differently for debug and non-debug.
>
> I think such helper would be useful in other context as well.
OK, I'll do that
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 4/6] xen/arm: zynqmp: eemi access control
2018-10-16 13:29 ` Julien Grall
@ 2018-10-17 14:03 ` Stefano Stabellini
2018-10-17 14:26 ` Julien Grall
0 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2018-10-17 14:03 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, nd, Stefano Stabellini, xen-devel
[-- Attachment #1: Type: TEXT/PLAIN, Size: 6328 bytes --]
On Tue, 16 Oct 2018, Julien Grall wrote:
> Hi,
>
> Sorry I forgot to answer to the rest of the e-mail.
>
> On 10/16/2018 03:39 AM, Stefano Stabellini wrote:
> > On 15/10/2018 08:25, Julien Grall wrote:
> > > > > > + bool hwdom_access; /* HW domain gets access regardless. */
> > > > > > +};
> > > > > > +
> > > > > > +/*
> > > > > > + * This table maps a node into a memory address.
> > > > > > + * If a guest has access to the address, it has enough control
> > > > > > + * over the node to grant it access to EEMI calls for that node.
> > > > > > + */
> > > > > > +static const struct pm_access pm_node_access[] = {
> > > > >
> > > > > [...]
> > > > >
> > > > > > +
> > > > > > +/*
> > > > > > + * This table maps reset line IDs into a memory address.
> > > > > > + * If a guest has access to the address, it has enough control
> > > > > > + * over the affected node to grant it access to EEMI calls for
> > > > > > + * resetting that node.
> > > > > > + */
> > > > > > +#define XILPM_RESET_IDX(n) (n - XILPM_RESET_PCIE_CFG)
> > > > > > +static const struct pm_access pm_reset_access[] = {
> > > > >
> > > > > [...]
> > > > >
> > > > > > +
> > > > > > +/*
> > > > > > + * This table maps reset line IDs into a memory address.
> > > > > > + * If a guest has access to the address, it has enough control
> > > > > > + * over the affected node to grant it access to EEMI calls for
> > > > > > + * resetting that node.
> > > > > > + */
> > > > > > +static const struct {
> > > > > > + paddr_t start;
> > > > > > + paddr_t size;
> > > > > > + uint32_t mask; /* Zero means no mask, i.e all bits. */
> > > > > > + enum pm_node_id node;
> > > > > > + bool hwdom_access;
> > > > > > + bool readonly;
> > > > > > +} pm_mmio_access[] = {
> > > > >
> > > > > Those 3 arrays contains a lot of hardcoded value. Can't any of this be
> > > > > detected from the device-tree?
> > > >
> > > > No, the information is not available on device tree unfortunately. >
> > > >
> > > > > I would be interested to know how this is going to work with upstream
> > > > > Linux.
> > > > > Do you hardcode all the values there as well?
> > > >
> > > > Yes: the IDs are specified on an header file, see
> > > > include/linux/firmware/xlnx-zynqmp.h on the zynq/firmware branch of the
> > > > arm-soc tree. In addition to the IDs, we also have the memory addresses
> > > > in Xen to do the permission checks.
> > >
> > > I am afraid this is not linux upstream. Can you point to the code in
> > > Linus's
> > > git or explain the state of the review?
> >
> > It hasn't been pulled into Linux yet, I was told it has already been
> > reviewed and is queued in arm-soc for upstreaming at the next merge
> > window, which should be imminent.
>
> Looking at that branch, I can see some DT bindings at least for the clock. I
> also don't see any hardcoded value for device so far in that series. Is it
> going to be sent separately?
If you look at include/linux/firmware/xlnx-zynqmp.h, you'll see some
hardcode values, specifically enum pm_api_id matches numerically the
enum by the same name this series introduces in
xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > > > > > +static bool pm_check_access(const struct pm_access *acl, struct
> > > > > > domain *d,
> > > > > > + uint32_t idx)
> > > > > > +{
> > > > > > + unsigned long mfn;
> > > > >
> > > > > mfn_t please. The code is not that nice but at least it add more
> > > > > safety
> > > > > in the
> > > > > code. Hopefully iommu_access_permitted will be converted to typesafe
> > > > > MFN
> > > > > soon.
> > > >
> > > > Yes, I can make this change without issues.
> > > >
> > > >
> > > > > > +
> > > > > > + if ( acl[idx].hwdom_access && is_hardware_domain(d) )
> > > > > > + return true;
> > > > > > +
> > > > > > + if ( !acl[idx].addr )
> > > > > > + return false;
> > > > > > +
> > > > > > + mfn = PFN_DOWN(acl[idx].addr);
> > > > >
> > > > > maddr_to_mfn(...);
> > > >
> > > > OK
> > > >
> > > >
> > > > > > + return iomem_access_permitted(d, mfn, mfn);
> > > > >
> > > > > Is the address something that a guest would be allowed to read/write
> > > > > directly?
> > > > > If not, then iomem_access_permitted(...) should not be used.
> > > >
> > > > Yes, the address would be something remapped to the guest using iomem.
> > >
> > > In the documentation at the beginning of the file you say that memory
> > > ranges
> > > are typically secure memory. So how a guest can access them directly?
> > >
> > > I probably need a bit more background here... What is the address
> > > correspond
> > > to at the end?
> >
> > The address corresponds to the MMIO region of the device. For instance,
> > MM_GEM0 is 0xff0b0000, which is the address of the network card. It is
> > accessible. The same goes for MM_CAN0, MM_TTC0, MM_SD0, and all the
> > others -- they are all accessible. These are the addresses used in this
> > check that should be remapped into the guest.
> >
> > However, things are different for the pm_mmio_access array used in
> > domain_has_mmio_access to check for access rights. (That is also where
> > the mask is applied to writes and not to reads, see below.) In that
> > case, the addresses correspond to single registers inside the
> > zynqmp-psgtr region ("ZynqMP High Speed Processing System Gigabit
> > Transceiver") and pin controller region. As you can see, in
> > domain_has_mmio_access the access checks are still done on the
> > corresponding node address, which is the one that gets remapped. For
> > instance in the case of the network card, the remapped address is
> > 0xff0b0000, checks are done on it, but the zynqmp-psgtr and pin
> > contoller region registers are:
> >
> > {
> > .start = MM_CRL_APB + R_CRL_GEM0_REF_CTRL,
> > .size = 4, .node = NODE_ETH_0
> > }
> > [...]
> > {
> > .start = MM_IOU_SLCR + R_IOU_SLCR_IOU_RAM_GEM0,
> > .size = 4, .node = NODE_ETH_0
> > }
> >
> > The comment comes from Edgar, but I think that he meant that VMs won't
> > be able to write to these regions directly. I'll improve the comment.
>
> Thank you for the explanation. Can you add something similar to what you wrote
> in the code?
Yes, I'll do that
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 4/6] xen/arm: zynqmp: eemi access control
2018-10-17 14:03 ` Stefano Stabellini
@ 2018-10-17 14:26 ` Julien Grall
0 siblings, 0 replies; 29+ messages in thread
From: Julien Grall @ 2018-10-17 14:26 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: edgar.iglesias, Stefano Stabellini, nd, xen-devel
Hi Stefano,
On 17/10/2018 15:03, Stefano Stabellini wrote:
> On Tue, 16 Oct 2018, Julien Grall wrote:
>> Hi,
>>
>> Sorry I forgot to answer to the rest of the e-mail.
>>
>> On 10/16/2018 03:39 AM, Stefano Stabellini wrote:
>>> On 15/10/2018 08:25, Julien Grall wrote:
>>>>>>> + bool hwdom_access; /* HW domain gets access regardless. */
>>>>>>> +};
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * This table maps a node into a memory address.
>>>>>>> + * If a guest has access to the address, it has enough control
>>>>>>> + * over the node to grant it access to EEMI calls for that node.
>>>>>>> + */
>>>>>>> +static const struct pm_access pm_node_access[] = {
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * This table maps reset line IDs into a memory address.
>>>>>>> + * If a guest has access to the address, it has enough control
>>>>>>> + * over the affected node to grant it access to EEMI calls for
>>>>>>> + * resetting that node.
>>>>>>> + */
>>>>>>> +#define XILPM_RESET_IDX(n) (n - XILPM_RESET_PCIE_CFG)
>>>>>>> +static const struct pm_access pm_reset_access[] = {
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * This table maps reset line IDs into a memory address.
>>>>>>> + * If a guest has access to the address, it has enough control
>>>>>>> + * over the affected node to grant it access to EEMI calls for
>>>>>>> + * resetting that node.
>>>>>>> + */
>>>>>>> +static const struct {
>>>>>>> + paddr_t start;
>>>>>>> + paddr_t size;
>>>>>>> + uint32_t mask; /* Zero means no mask, i.e all bits. */
>>>>>>> + enum pm_node_id node;
>>>>>>> + bool hwdom_access;
>>>>>>> + bool readonly;
>>>>>>> +} pm_mmio_access[] = {
>>>>>>
>>>>>> Those 3 arrays contains a lot of hardcoded value. Can't any of this be
>>>>>> detected from the device-tree?
>>>>>
>>>>> No, the information is not available on device tree unfortunately. >
>>>>>
>>>>>> I would be interested to know how this is going to work with upstream
>>>>>> Linux.
>>>>>> Do you hardcode all the values there as well?
>>>>>
>>>>> Yes: the IDs are specified on an header file, see
>>>>> include/linux/firmware/xlnx-zynqmp.h on the zynq/firmware branch of the
>>>>> arm-soc tree. In addition to the IDs, we also have the memory addresses
>>>>> in Xen to do the permission checks.
>>>>
>>>> I am afraid this is not linux upstream. Can you point to the code in
>>>> Linus's
>>>> git or explain the state of the review?
>>>
>>> It hasn't been pulled into Linux yet, I was told it has already been
>>> reviewed and is queued in arm-soc for upstreaming at the next merge
>>> window, which should be imminent.
>>
>> Looking at that branch, I can see some DT bindings at least for the clock. I
>> also don't see any hardcoded value for device so far in that series. Is it
>> going to be sent separately?
>
> If you look at include/linux/firmware/xlnx-zynqmp.h, you'll see some
> hardcode values, specifically enum pm_api_id matches numerically the
> enum by the same name this series introduces in
> xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
I don't think we are talking the same things. I am talking about
pm_node_id/pm_node_reset (not pm_api_id). I don't see such code in Linux
at the moment and a bit surprised that no DT bindings will be used to
link the value with a device.
So my question stands, how Linux will use pm_node_id/pm_node_reset? Can
you point to code if that exists.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2018-10-17 14:26 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-11 0:01 [PATCH v3 0/6] zynqmp: Add forwarding of platform specific firmware calls Stefano Stabellini
2018-08-11 0:01 ` [PATCH v3 1/6] xen/arm: introduce platform_smc Stefano Stabellini
2018-08-23 15:37 ` Julien Grall
2018-08-23 23:40 ` Stefano Stabellini
2018-08-11 0:01 ` [PATCH v3 2/6] xen/arm: zynqmp: Forward plaform specific firmware calls Stefano Stabellini
2018-08-23 15:41 ` Julien Grall
2018-08-23 23:56 ` Stefano Stabellini
2018-08-24 9:01 ` Julien Grall
2018-10-10 21:50 ` Stefano Stabellini
2018-08-11 0:01 ` [PATCH v3 3/6] xen/arm: zynqmp: introduce zynqmp specific defines Stefano Stabellini
2018-08-11 0:01 ` [PATCH v3 4/6] xen/arm: zynqmp: eemi access control Stefano Stabellini
2018-08-28 16:05 ` Julien Grall
2018-10-10 22:35 ` Stefano Stabellini
2018-10-15 7:25 ` Julien Grall
2018-10-15 13:00 ` Julien Grall
2018-10-16 2:39 ` Stefano Stabellini
2018-10-16 13:23 ` Julien Grall
2018-10-17 13:58 ` Stefano Stabellini
2018-10-16 13:29 ` Julien Grall
2018-10-17 14:03 ` Stefano Stabellini
2018-10-17 14:26 ` Julien Grall
2018-08-11 0:01 ` [PATCH v3 5/6] xen/arm: zynqmp: implement zynqmp_eemi Stefano Stabellini
2018-08-28 16:29 ` Julien Grall
2018-10-10 22:49 ` Stefano Stabellini
2018-10-15 7:32 ` Julien Grall
2018-10-15 13:01 ` Julien Grall
2018-10-16 6:48 ` Stefano Stabellini
2018-10-16 13:44 ` Julien Grall
2018-08-11 0:01 ` [PATCH v3 6/6] xen/arm: zynqmp: Remove blacklist of ZynqMP's PM node Stefano Stabellini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.