* [PATCH v5 0/7] zynqmp: Add forwarding of platform specific firmware calls
@ 2018-12-03 21:02 Stefano Stabellini
2018-12-03 21:03 ` [PATCH v5 1/7] xen/arm: introduce platform_smc Stefano Stabellini
` (6 more replies)
0 siblings, 7 replies; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-03 21:02 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, julien.grall, sstabellini, saeed.nowshadi
Hi all,
Compared to v4, I removed the amount of #defines and used mfn_t more
widely across the series. Some #defines remain, see my comments on patch
#3 and patch #5.
Cheers,
Stefano
The following changes since commit 162fc8295f31240dc3670190a91e9bbc03b0d7be:
libxl: Restore scheduling parameters after migrate in best-effort fashion (2018-10-10 12:36:25 +0100)
are available in the git repository at:
http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git zynqmp-v5
for you to fetch changes up to e981b3a81f2283f7377712d0784cd5ee5adc599c:
xen/arm: zynqmp: Remove blacklist of ZynqMP's PM node (2018-12-03 13:00:40 -0800)
----------------------------------------------------------------
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
Stefano Stabellini (1):
xen: introduce mfn_init macro
xen/arch/arm/platform.c | 8 +
xen/arch/arm/platforms/Makefile | 1 +
xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 451 +++++++++++++++++++++
xen/arch/arm/platforms/xilinx-zynqmp.c | 15 +-
xen/arch/arm/vsmc.c | 4 +
xen/include/asm-arm/platform.h | 3 +
xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 323 +++++++++++++++
xen/include/xen/mm.h | 6 +
8 files changed, 805 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] 32+ messages in thread
* [PATCH v5 1/7] xen/arm: introduce platform_smc
2018-12-03 21:02 [PATCH v5 0/7] zynqmp: Add forwarding of platform specific firmware calls Stefano Stabellini
@ 2018-12-03 21:03 ` Stefano Stabellini
2018-12-03 21:07 ` Stefano Stabellini
2018-12-11 14:54 ` Julien Grall
2018-12-03 21:03 ` [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls Stefano Stabellini
` (5 subsequent siblings)
6 siblings, 2 replies; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-03 21:03 UTC (permalink / raw)
To: xen-devel
Cc: edgar.iglesias, Stefano Stabellini, julien.grall, sstabellini,
saeed.nowshadi
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>
---
Changes in v4:
- add likely
---
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 6989e58..3426056 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 ( likely(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] 32+ messages in thread
* [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls
2018-12-03 21:02 [PATCH v5 0/7] zynqmp: Add forwarding of platform specific firmware calls Stefano Stabellini
2018-12-03 21:03 ` [PATCH v5 1/7] xen/arm: introduce platform_smc Stefano Stabellini
@ 2018-12-03 21:03 ` Stefano Stabellini
2018-12-11 15:03 ` Julien Grall
2018-12-03 21:03 ` [PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines Stefano Stabellini
` (4 subsequent siblings)
6 siblings, 1 reply; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-03 21:03 UTC (permalink / raw)
To: xen-devel
Cc: edgar.iglesias, Stefano Stabellini, julien.grall, sstabellini,
saeed.nowshadi
From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Introduce zynqmp_eemi: a function responsible 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>
---
Changes in v4:
- fix typo
- add header guard
- add emacs magic
- remove #includes that will only be used later
- add copyright notice to header
- remove SMCCC 1.1 check
---
xen/arch/arm/platforms/Makefile | 1 +
xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 34 ++++++++++++++++++++++
xen/arch/arm/platforms/xilinx-zynqmp.c | 11 +++++++
xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 30 +++++++++++++++++++
4 files changed, 76 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 a79bdb9..fe8e0c7 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_MPSOC_PLATFORM) += xilinx-zynqmp.o
+obj-$(CONFIG_MPSOC_PLATFORM) += 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..369bb3f
--- /dev/null
+++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
@@ -0,0 +1,34 @@
+/*
+ * 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 <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..9c174d2 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,17 @@ 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;
+
+ 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..43cefb5
--- /dev/null
+++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
@@ -0,0 +1,30 @@
+/*
+ * Copyright (c) 2018 Xilinx Inc.
+ *
+ * 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.
+ */
+
+#ifndef __ASM_ARM_PLATFORMS_ZYNQMP_H
+#define __ASM_ASM_PLATFORMS_ZYNQMP_H
+
+#include <asm/processor.h>
+
+extern bool zynqmp_eemi(struct cpu_user_regs *regs);
+
+#endif /* __ASM_ARM_PLATFORMS_ZYNQMP_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * 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] 32+ messages in thread
* [PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines
2018-12-03 21:02 [PATCH v5 0/7] zynqmp: Add forwarding of platform specific firmware calls Stefano Stabellini
2018-12-03 21:03 ` [PATCH v5 1/7] xen/arm: introduce platform_smc Stefano Stabellini
2018-12-03 21:03 ` [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls Stefano Stabellini
@ 2018-12-03 21:03 ` Stefano Stabellini
2018-12-11 15:21 ` Julien Grall
2018-12-03 21:03 ` [PATCH v5 4/7] xen: introduce mfn_init macro Stefano Stabellini
` (3 subsequent siblings)
6 siblings, 1 reply; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-03 21:03 UTC (permalink / raw)
To: xen-devel
Cc: edgar.iglesias, Stefano Stabellini, julien.grall, sstabellini,
saeed.nowshadi
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>
---
Although the amount of #defines has been significantly reduced in v5,
there is still a significant amount of static definitions:
- MM_*
These are the MMIO addresses of each resource to do permission checks.
Technically, they are also present on device tree, but using device tree
to get the mmio regions is not simple and not done in this series.
Xilinx plan to send a patch series in the future to extend the EEMI
functionalities and as part of that work, more device tree based
permission checking will be done, solving this problem. These
definitions could also be removed if we used a trivial "if dom0 -> yes,
otherwise no" permission checking.
- pm_api_id
These are the EEMI function IDs. Unavoidable.
- pm_ret_status
These are the EEMI return statuses. Unavoidable.
- pm_node_id
These are the EEMI function parameters for power management operations.
Today, it is not possible to get them from device tree as there is no
such information there. Even in the future when we add more power
domains info to device tree, the EEMI function parameters might remain
unique and different, requiring a table like this one.
- pm_reset
These are the EEMI function parameters for reset operations. Same as
pm_node_id.
---
Changes in v5:
- remove MMIO access related definitions
Changes in v4:
- define PM_MMIO_SHIFT
---
xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 293 +++++++++++++++++++++
1 file changed, 293 insertions(+)
diff --git a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
index 43cefb5..f6ad03b 100644
--- a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
+++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
@@ -16,6 +16,299 @@
#include <asm/processor.h>
+#define MM_RPU 0xff9a0
+#define MM_RTC 0xffa60
+#define MM_ADMA_CH0 0xffa80
+
+#define MM_USB3_0_XHCI 0xfe200
+#define MM_USB3_1_XHCI 0xfe300
+
+#define MM_SATA_AHCI_HBA 0xfd0c0
+#define MM_AXIPCIE_MAIN 0xfd0e0
+#define MM_CRF_APB 0xfd1a0000
+#define MM_PCIE_ATTRIB 0xfd480
+#define MM_DP 0xfd4a0
+#define MM_GPU 0xfd4b0
+#define MM_GDMA_CH0 0xfd500
+
+#define MM_UART0 0xff000
+#define MM_UART1 0xff010
+#define MM_I2C0 0xff020
+#define MM_I2C1 0xff030
+#define MM_SPI0 0xff040
+#define MM_SPI1 0xff050
+#define MM_CAN0 0xff060
+#define MM_CAN1 0xff070
+#define MM_GPIO 0xff0a0
+#define MM_GEM0 0xff0b0
+#define MM_GEM1 0xff0c0
+#define MM_GEM2 0xff0d0
+#define MM_GEM3 0xff0e0
+#define MM_QSPI 0xff0f0
+#define MM_NAND 0xff100
+#define MM_TTC0 0xff110
+#define MM_TTC1 0xff120
+#define MM_TTC2 0xff130
+#define MM_TTC3 0xff140
+#define MM_SWDT 0xff150
+#define MM_SD0 0xff160
+#define MM_SD1 0xff170
+
+/* 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
+
+#define PM_MMIO_SHIFT 32
+
+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);
#endif /* __ASM_ARM_PLATFORMS_ZYNQMP_H */
--
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] 32+ messages in thread
* [PATCH v5 4/7] xen: introduce mfn_init macro
2018-12-03 21:02 [PATCH v5 0/7] zynqmp: Add forwarding of platform specific firmware calls Stefano Stabellini
` (2 preceding siblings ...)
2018-12-03 21:03 ` [PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines Stefano Stabellini
@ 2018-12-03 21:03 ` Stefano Stabellini
2018-12-04 10:25 ` Jan Beulich
2018-12-03 21:03 ` [PATCH v5 5/7] xen/arm: zynqmp: eemi access control Stefano Stabellini
` (2 subsequent siblings)
6 siblings, 1 reply; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-03 21:03 UTC (permalink / raw)
To: xen-devel
Cc: edgar.iglesias, Stefano Stabellini, sstabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
saeed.nowshadi, julien.grall, Tim Deegan, Jan Beulich
To be used in constant initializations of mfn_t variables, such as:
static mfn_t node = mfn_init(MM_ADDR);
It is necessary because static inline functions cannot be used as static
initializers.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien.grall@arm.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
---
xen/include/xen/mm.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 054d02e..692967e 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -77,6 +77,12 @@ TYPE_SAFE(unsigned long, mfn);
#undef mfn_x
#endif
+#ifndef NDEBUG
+#define mfn_init(x) { x }
+#else
+#define mfn_init(x) x
+#endif
+
static inline mfn_t mfn_add(mfn_t mfn, unsigned long i)
{
return _mfn(mfn_x(mfn) + i);
--
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] 32+ messages in thread
* [PATCH v5 5/7] xen/arm: zynqmp: eemi access control
2018-12-03 21:02 [PATCH v5 0/7] zynqmp: Add forwarding of platform specific firmware calls Stefano Stabellini
` (3 preceding siblings ...)
2018-12-03 21:03 ` [PATCH v5 4/7] xen: introduce mfn_init macro Stefano Stabellini
@ 2018-12-03 21:03 ` Stefano Stabellini
2018-12-11 15:37 ` Julien Grall
2018-12-03 21:03 ` [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi Stefano Stabellini
2018-12-03 21:03 ` [PATCH v5 7/7] xen/arm: zynqmp: Remove blacklist of ZynqMP's PM node Stefano Stabellini
6 siblings, 1 reply; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-03 21:03 UTC (permalink / raw)
To: xen-devel
Cc: edgar.iglesias, Stefano Stabellini, julien.grall, sstabellini,
saeed.nowshadi
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>
---
Statically defines:
- pm_node_access
It encodes the relationship between a node id and the start of the MMIO
region of a device in the corresponding power domain. It is used for
permission checking. Although the MMIO region start address is available
on device tree and could be derived from there (we plan to improve that
in the future), the relationship between a node id and corresponding
devices is not described and needs to be hardcoded.
- pm_reset_access
Same as pm_node_access for reset lines.
---
Changes in v5:
- improve in-code comments
- use mfn_t in struct pm_access
- remove mmio_access table
Changes in v4:
- add #include as needed
- add #if 0 for bisectability
- use mfn_t in pm_check_access
- add wrap-around ASSERT in domain_has_mmio_access
- use GENMASK in domain_has_mmio_access
- proper bound checks (== ARRAY_SIZE is out of bound)
---
xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 348 ++++++++++++++++++++++++++++
1 file changed, 348 insertions(+)
diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
index 369bb3f..92a02df 100644
--- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
+++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
@@ -16,9 +16,357 @@
* 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().
+ */
+
+#include <xen/iocap.h>
+#include <xen/sched.h>
#include <asm/regs.h>
#include <asm/platforms/xilinx-zynqmp-eemi.h>
+#if 0
+struct pm_access
+{
+ mfn_t mfn;
+ 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] = { mfn_init(MM_RPU) },
+ [NODE_RPU_0] = { mfn_init(MM_RPU) },
+ [NODE_RPU_1] = { mfn_init(MM_RPU) },
+ [NODE_IPI_RPU_0] = { mfn_init(MM_RPU) },
+
+ /* GPU nodes. */
+ [NODE_GPU] = { mfn_init(MM_GPU) },
+ [NODE_GPU_PP_0] = { mfn_init(MM_GPU) },
+ [NODE_GPU_PP_1] = { mfn_init(MM_GPU) },
+
+ [NODE_USB_0] = { mfn_init(MM_USB3_0_XHCI) },
+ [NODE_USB_1] = { mfn_init(MM_USB3_1_XHCI) },
+ [NODE_TTC_0] = { mfn_init(MM_TTC0) },
+ [NODE_TTC_1] = { mfn_init(MM_TTC1) },
+ [NODE_TTC_2] = { mfn_init(MM_TTC2) },
+ [NODE_TTC_3] = { mfn_init(MM_TTC3) },
+ [NODE_SATA] = { mfn_init(MM_SATA_AHCI_HBA) },
+ [NODE_ETH_0] = { mfn_init(MM_GEM0) },
+ [NODE_ETH_1] = { mfn_init(MM_GEM1) },
+ [NODE_ETH_2] = { mfn_init(MM_GEM2) },
+ [NODE_ETH_3] = { mfn_init(MM_GEM3) },
+ [NODE_UART_0] = { mfn_init(MM_UART0) },
+ [NODE_UART_1] = { mfn_init(MM_UART1) },
+ [NODE_SPI_0] = { mfn_init(MM_SPI0) },
+ [NODE_SPI_1] = { mfn_init(MM_SPI1) },
+ [NODE_I2C_0] = { mfn_init(MM_I2C0) },
+ [NODE_I2C_1] = { mfn_init(MM_I2C1) },
+ [NODE_SD_0] = { mfn_init(MM_SD0) },
+ [NODE_SD_1] = { mfn_init(MM_SD1) },
+ [NODE_DP] = { mfn_init(MM_DP) },
+
+ /* Guest with GDMA Channel 0 gets PM access. Other guests don't. */
+ [NODE_GDMA] = { mfn_init(MM_GDMA_CH0) },
+ /* Guest with ADMA Channel 0 gets PM access. Other guests don't. */
+ [NODE_ADMA] = { mfn_init(MM_ADMA_CH0) },
+
+ [NODE_NAND] = { mfn_init(MM_NAND) },
+ [NODE_QSPI] = { mfn_init(MM_QSPI) },
+ [NODE_GPIO] = { mfn_init(MM_GPIO) },
+ [NODE_CAN_0] = { mfn_init(MM_CAN0) },
+ [NODE_CAN_1] = { mfn_init(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] = { mfn_init(MM_PCIE_ATTRIB) },
+ [NODE_RTC] = { mfn_init(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)] = { mfn_init(MM_AXIPCIE_MAIN) },
+ [XILPM_RESET_IDX(XILPM_RESET_PCIE_BRIDGE)] = { mfn_init(MM_PCIE_ATTRIB) },
+ [XILPM_RESET_IDX(XILPM_RESET_PCIE_CTRL)] = { mfn_init(MM_PCIE_ATTRIB) },
+
+ [XILPM_RESET_IDX(XILPM_RESET_DP)] = { mfn_init(MM_DP) },
+ [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRF)] = { mfn_init(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)] = { mfn_init(MM_GDMA_CH0) },
+ [XILPM_RESET_IDX(XILPM_RESET_GPU_PP1)] = { mfn_init(MM_GPU) },
+ [XILPM_RESET_IDX(XILPM_RESET_GPU_PP0)] = { mfn_init(MM_GPU) },
+ [XILPM_RESET_IDX(XILPM_RESET_GT)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_SATA)] = { mfn_init(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)] = { mfn_init(MM_GEM0) },
+ [XILPM_RESET_IDX(XILPM_RESET_GEM1)] = { mfn_init(MM_GEM1) },
+ [XILPM_RESET_IDX(XILPM_RESET_GEM2)] = { mfn_init(MM_GEM2) },
+ [XILPM_RESET_IDX(XILPM_RESET_GEM3)] = { mfn_init(MM_GEM3) },
+
+ [XILPM_RESET_IDX(XILPM_RESET_QSPI)] = { mfn_init(MM_QSPI) },
+ [XILPM_RESET_IDX(XILPM_RESET_UART0)] = { mfn_init(MM_UART0) },
+ [XILPM_RESET_IDX(XILPM_RESET_UART1)] = { mfn_init(MM_UART1) },
+ [XILPM_RESET_IDX(XILPM_RESET_SPI0)] = { mfn_init(MM_SPI0) },
+ [XILPM_RESET_IDX(XILPM_RESET_SPI1)] = { mfn_init(MM_SPI1) },
+ [XILPM_RESET_IDX(XILPM_RESET_SDIO0)] = { mfn_init(MM_SD0) },
+ [XILPM_RESET_IDX(XILPM_RESET_SDIO1)] = { mfn_init(MM_SD1) },
+ [XILPM_RESET_IDX(XILPM_RESET_CAN0)] = { mfn_init(MM_CAN0) },
+ [XILPM_RESET_IDX(XILPM_RESET_CAN1)] = { mfn_init(MM_CAN1) },
+ [XILPM_RESET_IDX(XILPM_RESET_I2C0)] = { mfn_init(MM_I2C0) },
+ [XILPM_RESET_IDX(XILPM_RESET_I2C1)] = { mfn_init(MM_I2C1) },
+ [XILPM_RESET_IDX(XILPM_RESET_TTC0)] = { mfn_init(MM_TTC0) },
+ [XILPM_RESET_IDX(XILPM_RESET_TTC1)] = { mfn_init(MM_TTC1) },
+ [XILPM_RESET_IDX(XILPM_RESET_TTC2)] = { mfn_init(MM_TTC2) },
+ [XILPM_RESET_IDX(XILPM_RESET_TTC3)] = { mfn_init(MM_TTC3) },
+ [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRL)] = { mfn_init(MM_SWDT) },
+ [XILPM_RESET_IDX(XILPM_RESET_NAND)] = { mfn_init(MM_NAND) },
+ [XILPM_RESET_IDX(XILPM_RESET_ADMA)] = { mfn_init(MM_ADMA_CH0) },
+ [XILPM_RESET_IDX(XILPM_RESET_GPIO)] = { mfn_init(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)] = { mfn_init(MM_RPU) },
+ [XILPM_RESET_IDX(XILPM_RESET_RPU_R51)] = { mfn_init(MM_RPU) },
+ [XILPM_RESET_IDX(XILPM_RESET_RPU_AMBA)] = { mfn_init(MM_RPU) },
+ [XILPM_RESET_IDX(XILPM_RESET_OCM)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_RPU_PGE)] = { mfn_init(MM_RPU) },
+
+ [XILPM_RESET_IDX(XILPM_RESET_USB0_CORERESET)] = { mfn_init(MM_USB3_0_XHCI) },
+ [XILPM_RESET_IDX(XILPM_RESET_USB0_HIBERRESET)] = { mfn_init(MM_USB3_0_XHCI) },
+ [XILPM_RESET_IDX(XILPM_RESET_USB0_APB)] = { mfn_init(MM_USB3_0_XHCI) },
+
+ [XILPM_RESET_IDX(XILPM_RESET_USB1_CORERESET)] = { mfn_init(MM_USB3_1_XHCI) },
+ [XILPM_RESET_IDX(XILPM_RESET_USB1_HIBERRESET)] = { mfn_init(MM_USB3_1_XHCI) },
+ [XILPM_RESET_IDX(XILPM_RESET_USB1_APB)] = { mfn_init(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)] = { mfn_init(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)] = { mfn_init(MM_SWDT) },
+ [XILPM_RESET_IDX(XILPM_RESET_FPD)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG1)] = { mfn_init(MM_RPU) },
+ [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG0)] = { mfn_init(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)] = { mfn_init(MM_RPU) },
+ [XILPM_RESET_IDX(XILPM_RESET_PS_ONLY)] = { .hwdom_access = true },
+ [XILPM_RESET_IDX(XILPM_RESET_PL)] = { .hwdom_access = true },
+};
+
+static bool pm_check_access(const struct pm_access *acl, struct domain *d,
+ uint32_t idx)
+{
+ if ( acl[idx].hwdom_access && is_hardware_domain(d) )
+ return true;
+
+ if ( !mfn_x(acl[idx].mfn) )
+ return false;
+
+ return iomem_access_permitted(d, mfn_x(acl[idx].mfn), mfn_x(acl[idx].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++ )
+ {
+ ASSERT(pm_mmio_access[i].start + pm_mmio_access[i].size >=
+ pm_mmio_access[i].start);
+
+ 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 ?: GENMASK(31, 0);
+ break;
+ }
+
+ /*
+ * Masking only applies to writes: values are safe to read, but not
+ * all bits are writeable.
+ */
+ if ( write )
+ *mask &= prot_mask;
+
+ return ret;
+}
+#endif
+
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] 32+ messages in thread
* [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi
2018-12-03 21:02 [PATCH v5 0/7] zynqmp: Add forwarding of platform specific firmware calls Stefano Stabellini
` (4 preceding siblings ...)
2018-12-03 21:03 ` [PATCH v5 5/7] xen/arm: zynqmp: eemi access control Stefano Stabellini
@ 2018-12-03 21:03 ` Stefano Stabellini
2018-12-11 15:55 ` Julien Grall
2018-12-03 21:03 ` [PATCH v5 7/7] xen/arm: zynqmp: Remove blacklist of ZynqMP's PM node Stefano Stabellini
6 siblings, 1 reply; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-03 21:03 UTC (permalink / raw)
To: xen-devel
Cc: edgar.iglesias, Stefano Stabellini, julien.grall, sstabellini,
saeed.nowshadi
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>
---
Changes in v5:
- remove mmio_access handling
Changes in v4:
- add #include as needed
- improve comment
- code style
---
xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 181 +++++++++++++++++++---------
1 file changed, 125 insertions(+), 56 deletions(-)
diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
index 92a02df..9ecf286 100644
--- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
+++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
@@ -76,10 +76,10 @@
#include <xen/iocap.h>
#include <xen/sched.h>
+#include <asm/smccc.h>
#include <asm/regs.h>
#include <asm/platforms/xilinx-zynqmp-eemi.h>
-#if 0
struct pm_access
{
mfn_t mfn;
@@ -309,67 +309,136 @@ static bool domain_has_reset_access(struct domain *d, uint32_t rst)
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)
+bool zynqmp_eemi(struct cpu_user_regs *regs)
{
- 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;
+ struct arm_smccc_res res;
+ 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;
- /* Scan the ACL. */
- for ( i = 0; i < ARRAY_SIZE(pm_mmio_access); i++ )
+ switch ( pm_fn )
{
- ASSERT(pm_mmio_access[i].start + pm_mmio_access[i].size >=
- pm_mmio_access[i].start);
-
- 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 ?: GENMASK(31, 0);
- break;
- }
-
/*
- * Masking only applies to writes: values are safe to read, but not
- * all bits are writeable.
+ * 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 or PSCI call which Xen will trap and act accordingly upon.
*/
- if ( write )
- *mask &= prot_mask;
-
- return ret;
-}
-#endif
+ 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;
+
+ /* No MMIO access is allowed from non-secure domains */
+ case PM_MMIO_WRITE:
+ case PM_MMIO_READ:
+ gprintk(XENLOG_WARNING,
+ "zynqmp-pm: fn=%u No MMIO access to %u\n", pm_fn, nodeid);
+ ret = XST_PM_NO_ACCESS;
+ goto done;
+
+ /* 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;
+ }
-bool zynqmp_eemi(struct cpu_user_regs *regs)
-{
- 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] 32+ messages in thread
* [PATCH v5 7/7] xen/arm: zynqmp: Remove blacklist of ZynqMP's PM node
2018-12-03 21:02 [PATCH v5 0/7] zynqmp: Add forwarding of platform specific firmware calls Stefano Stabellini
` (5 preceding siblings ...)
2018-12-03 21:03 ` [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi Stefano Stabellini
@ 2018-12-03 21:03 ` Stefano Stabellini
6 siblings, 0 replies; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-03 21:03 UTC (permalink / raw)
To: xen-devel
Cc: edgar.iglesias, Stefano Stabellini, julien.grall, sstabellini,
saeed.nowshadi
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 9c174d2..c49555b 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) )
@@ -45,7 +38,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] 32+ messages in thread
* Re: [PATCH v5 1/7] xen/arm: introduce platform_smc
2018-12-03 21:03 ` [PATCH v5 1/7] xen/arm: introduce platform_smc Stefano Stabellini
@ 2018-12-03 21:07 ` Stefano Stabellini
2018-12-11 14:54 ` Julien Grall
1 sibling, 0 replies; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-03 21:07 UTC (permalink / raw)
To: Stefano Stabellini
Cc: edgar.iglesias, Stefano Stabellini, julien.grall, saeed.nowshadi,
xen-devel
On Mon, 3 Dec 2018, Stefano Stabellini wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Sorry about this, I haven't found a way to deal with this problem
automatically with git and guilt yet.
> 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>
>
> ---
> Changes in v4:
> - add likely
> ---
> 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 6989e58..3426056 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 ( likely(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 [flat|nested] 32+ messages in thread
* Re: [PATCH v5 4/7] xen: introduce mfn_init macro
2018-12-03 21:03 ` [PATCH v5 4/7] xen: introduce mfn_init macro Stefano Stabellini
@ 2018-12-04 10:25 ` Jan Beulich
2018-12-04 19:38 ` Stefano Stabellini
0 siblings, 1 reply; 32+ messages in thread
From: Jan Beulich @ 2018-12-04 10:25 UTC (permalink / raw)
To: Stefano Stabellini
Cc: edgar.iglesias, Tim Deegan, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Stefano Stabellini, Ian Jackson,
xen-devel, Julien Grall, saeed.nowshadi
>>> On 03.12.18 at 22:03, <sstabellini@kernel.org> wrote:
> To be used in constant initializations of mfn_t variables, such as:
>
> static mfn_t node = mfn_init(MM_ADDR);
>
> It is necessary because static inline functions cannot be used as static
> initializers.
We had been at this point once (quite some time ago), and got
away without such an addition. Did you try to find that old
discussion? Are there any new reasons to have such a construct?
Do you need this for other than setting a value to INVALID_MFN,
in which case INVALID_MFN_INITIALIZER ought to be suitable?
This is not to say I'm entirely opposed.
If we were to have such a construct, I wonder though whether
mfn_init() is suitable as a name. Simply MFN() perhaps, and then
also consistently have GFN() and DFN()?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v5 4/7] xen: introduce mfn_init macro
2018-12-04 10:25 ` Jan Beulich
@ 2018-12-04 19:38 ` Stefano Stabellini
2018-12-05 8:27 ` Jan Beulich
0 siblings, 1 reply; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-04 19:38 UTC (permalink / raw)
To: Jan Beulich
Cc: edgar.iglesias, Tim Deegan, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper,
Stefano Stabellini, Ian Jackson, xen-devel, Julien Grall,
saeed.nowshadi
On Tue, 4 Dec 2018, Jan Beulich wrote:
> >>> On 03.12.18 at 22:03, <sstabellini@kernel.org> wrote:
> > To be used in constant initializations of mfn_t variables, such as:
> >
> > static mfn_t node = mfn_init(MM_ADDR);
> >
> > It is necessary because static inline functions cannot be used as static
> > initializers.
>
> We had been at this point once (quite some time ago), and got
> away without such an addition. Did you try to find that old
> discussion? Are there any new reasons to have such a construct?
> Do you need this for other than setting a value to INVALID_MFN,
> in which case INVALID_MFN_INITIALIZER ought to be suitable?
>
> This is not to say I'm entirely opposed.
>
> If we were to have such a construct, I wonder though whether
> mfn_init() is suitable as a name. Simply MFN() perhaps, and then
> also consistently have GFN() and DFN()?
Hi Jan,
I am happy with any name, and MFN() together with GFN() and DFN() look
like a good option.
The reason why it is needed is that without it I cannot introduce a
statically initialized array of mfn_t type like the one in the following
patch in the series:
+static const struct pm_access pm_node_access[] = {
+ /* MM_RPU grants access to all RPU Nodes. */
+ [NODE_RPU] = { mfn_init(MM_RPU) },
+ [NODE_RPU_0] = { mfn_init(MM_RPU) },
+ [NODE_RPU_1] = { mfn_init(MM_RPU) },
+ [NODE_IPI_RPU_0] = { mfn_init(MM_RPU) },
[...]
Where MM_RPU is a mfn, and the NODE_* are IDs defined as enum:
#define MM_RPU 0xff9a0
enum pm_node_id {
NODE_RPU = 6,
NODE_RPU_0,
NODE_RPU_1,
[...]
Originally I had:
[NODE_RPU] = { MM_RPU },
but I changed the type to be mfn_t to address one of Julien's comments.
You might get a better idea of the issue if you give a look at this
branch:
http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git zynqmp-v5
See:
xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v5 4/7] xen: introduce mfn_init macro
2018-12-04 19:38 ` Stefano Stabellini
@ 2018-12-05 8:27 ` Jan Beulich
2018-12-12 23:56 ` Stefano Stabellini
0 siblings, 1 reply; 32+ messages in thread
From: Jan Beulich @ 2018-12-05 8:27 UTC (permalink / raw)
To: Stefano Stabellini
Cc: edgar.iglesias, Tim Deegan, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Stefano Stabellini, Ian Jackson,
xen-devel, Julien Grall, saeed.nowshadi
>>> On 04.12.18 at 20:38, <sstabellini@kernel.org> wrote:
> On Tue, 4 Dec 2018, Jan Beulich wrote:
>> >>> On 03.12.18 at 22:03, <sstabellini@kernel.org> wrote:
>> > To be used in constant initializations of mfn_t variables, such as:
>> >
>> > static mfn_t node = mfn_init(MM_ADDR);
>> >
>> > It is necessary because static inline functions cannot be used as static
>> > initializers.
>>
>> We had been at this point once (quite some time ago), and got
>> away without such an addition. Did you try to find that old
>> discussion? Are there any new reasons to have such a construct?
>> Do you need this for other than setting a value to INVALID_MFN,
>> in which case INVALID_MFN_INITIALIZER ought to be suitable?
>>
>> This is not to say I'm entirely opposed.
>>
>> If we were to have such a construct, I wonder though whether
>> mfn_init() is suitable as a name. Simply MFN() perhaps, and then
>> also consistently have GFN() and DFN()?
>
> Hi Jan,
>
> I am happy with any name, and MFN() together with GFN() and DFN() look
> like a good option.
>
> The reason why it is needed is that without it I cannot introduce a
> statically initialized array of mfn_t type like the one in the following
> patch in the series:
>
> +static const struct pm_access pm_node_access[] = {
> + /* MM_RPU grants access to all RPU Nodes. */
> + [NODE_RPU] = { mfn_init(MM_RPU) },
> + [NODE_RPU_0] = { mfn_init(MM_RPU) },
> + [NODE_RPU_1] = { mfn_init(MM_RPU) },
> + [NODE_IPI_RPU_0] = { mfn_init(MM_RPU) },
>
> [...]
>
> Where MM_RPU is a mfn, and the NODE_* are IDs defined as enum:
>
> #define MM_RPU 0xff9a0
>
> enum pm_node_id {
> NODE_RPU = 6,
> NODE_RPU_0,
> NODE_RPU_1,
>
> [...]
>
>
> Originally I had:
>
> [NODE_RPU] = { MM_RPU },
>
> but I changed the type to be mfn_t to address one of Julien's comments.
> You might get a better idea of the issue if you give a look at this
> branch:
>
> http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git zynqmp-v5
Well, I have to admit that I'd rather not see ways to embed hard-coded
MFNs into code made available generically. May I suggest that you use
a macro with a name to your liking just locally to that one source file?
As a side note, I'm also puzzled by there being entries in the table which
don't have their MFNs specified. Oddly enough it looks as if
.hwdom_access was true if and only if no MFN is specified. The term
"node" of course is confusing too, considering its NUMA meaning
elsewhere in the hypervisor.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v5 1/7] xen/arm: introduce platform_smc
2018-12-03 21:03 ` [PATCH v5 1/7] xen/arm: introduce platform_smc Stefano Stabellini
2018-12-03 21:07 ` Stefano Stabellini
@ 2018-12-11 14:54 ` Julien Grall
1 sibling, 0 replies; 32+ messages in thread
From: Julien Grall @ 2018-12-11 14:54 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: edgar.iglesias, Stefano Stabellini, saeed.nowshadi
Hi Stefano,
On 03/12/2018 21:03, Stefano Stabellini wrote:
> 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>
Acked-by: Julien Grall <julien.grall@arm.com>
Cheers,
>
> ---
> Changes in v4:
> - add likely
> ---
> 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 6989e58..3426056 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 ( likely(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);
>
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls
2018-12-03 21:03 ` [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls Stefano Stabellini
@ 2018-12-11 15:03 ` Julien Grall
2018-12-11 18:50 ` Stefano Stabellini
0 siblings, 1 reply; 32+ messages in thread
From: Julien Grall @ 2018-12-11 15:03 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: edgar.iglesias, Stefano Stabellini, saeed.nowshadi
Hi Stefano,
On 03/12/2018 21:03, Stefano Stabellini wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>
> Introduce zynqmp_eemi: a function responsible 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>
>
> ---
> Changes in v4:
> - fix typo
> - add header guard
> - add emacs magic
> - remove #includes that will only be used later
> - add copyright notice to header
> - remove SMCCC 1.1 check
> ---
> xen/arch/arm/platforms/Makefile | 1 +
> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 34 ++++++++++++++++++++++
> xen/arch/arm/platforms/xilinx-zynqmp.c | 11 +++++++
> xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 30 +++++++++++++++++++
> 4 files changed, 76 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 a79bdb9..fe8e0c7 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_MPSOC_PLATFORM) += xilinx-zynqmp.o
> +obj-$(CONFIG_MPSOC_PLATFORM) += 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..369bb3f
> --- /dev/null
> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> @@ -0,0 +1,34 @@
> +/*
> + * 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 <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..9c174d2 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,17 @@ 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) )
Please document why you only expose eemi to 64-bit domain. What if the user
start with 32-bit Dom0?
> + return false;
> +
> + return zynqmp_eemi(regs);
double space.
> +}
> +
> 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..43cefb5
> --- /dev/null
> +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> @@ -0,0 +1,30 @@
> +/*
> + * Copyright (c) 2018 Xilinx Inc.
> + *
> + * 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.
> + */
> +
> +#ifndef __ASM_ARM_PLATFORMS_ZYNQMP_H
> +#define __ASM_ASM_PLATFORMS_ZYNQMP_H
> +
> +#include <asm/processor.h>
> +
> +extern bool zynqmp_eemi(struct cpu_user_regs *regs);
> +
> +#endif /* __ASM_ARM_PLATFORMS_ZYNQMP_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
>
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] 32+ messages in thread
* Re: [PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines
2018-12-03 21:03 ` [PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines Stefano Stabellini
@ 2018-12-11 15:21 ` Julien Grall
2018-12-11 19:22 ` Stefano Stabellini
0 siblings, 1 reply; 32+ messages in thread
From: Julien Grall @ 2018-12-11 15:21 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: edgar.iglesias, Stefano Stabellini, saeed.nowshadi
Hi,
On 03/12/2018 21:03, Stefano Stabellini wrote:
> 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>
>
> ---
>
> Although the amount of #defines has been significantly reduced in v5,
> there is still a significant amount of static definitions:
I think the description below would be useful to have in the commit message.
>
> - MM_*
> These are the MMIO addresses of each resource to do permission checks.
> Technically, they are also present on device tree, but using device tree
> to get the mmio regions is not simple and not done in this series.
> Xilinx plan to send a patch series in the future to extend the EEMI
> functionalities and as part of that work, more device tree based
> permission checking will be done, solving this problem.
As this is already present in the Device-Tree, there would be no issue to remove
the hardcoded value, correct?
> These
> definitions could also be removed if we used a trivial "if dom0 -> yes,
> otherwise no" permission checking.
>
> - pm_api_id
> These are the EEMI function IDs. Unavoidable.
>
> - pm_ret_status
> These are the EEMI return statuses. Unavoidable.
>
> - pm_node_id
> These are the EEMI function parameters for power management operations.
> Today, it is not possible to get them from device tree as there is no
> such information there. Even in the future when we add more power
> domains info to device tree, the EEMI function parameters might remain
> unique and different, requiring a table like this one.
Does it mean each Linux driver will have to hardcode the pm_node_id as well?
What is the plan there?
>
> - pm_reset
> These are the EEMI function parameters for reset operations. Same as
> pm_node_id.
Ditto.
Cheers,
>
> ---
> Changes in v5:
> - remove MMIO access related definitions
>
> Changes in v4:
> - define PM_MMIO_SHIFT
> ---
> xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 293 +++++++++++++++++++++
> 1 file changed, 293 insertions(+)
>
> diff --git a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> index 43cefb5..f6ad03b 100644
> --- a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> @@ -16,6 +16,299 @@
>
> #include <asm/processor.h>
>
> +#define MM_RPU 0xff9a0
> +#define MM_RTC 0xffa60
> +#define MM_ADMA_CH0 0xffa80
> +
> +#define MM_USB3_0_XHCI 0xfe200
> +#define MM_USB3_1_XHCI 0xfe300
> +
> +#define MM_SATA_AHCI_HBA 0xfd0c0
> +#define MM_AXIPCIE_MAIN 0xfd0e0
> +#define MM_CRF_APB 0xfd1a0000
> +#define MM_PCIE_ATTRIB 0xfd480
> +#define MM_DP 0xfd4a0
> +#define MM_GPU 0xfd4b0
> +#define MM_GDMA_CH0 0xfd500
> +
> +#define MM_UART0 0xff000
> +#define MM_UART1 0xff010
> +#define MM_I2C0 0xff020
> +#define MM_I2C1 0xff030
> +#define MM_SPI0 0xff040
> +#define MM_SPI1 0xff050
> +#define MM_CAN0 0xff060
> +#define MM_CAN1 0xff070
> +#define MM_GPIO 0xff0a0
> +#define MM_GEM0 0xff0b0
> +#define MM_GEM1 0xff0c0
> +#define MM_GEM2 0xff0d0
> +#define MM_GEM3 0xff0e0
> +#define MM_QSPI 0xff0f0
> +#define MM_NAND 0xff100
> +#define MM_TTC0 0xff110
> +#define MM_TTC1 0xff120
> +#define MM_TTC2 0xff130
> +#define MM_TTC3 0xff140
> +#define MM_SWDT 0xff150
> +#define MM_SD0 0xff160
> +#define MM_SD1 0xff170
> +
> +/* 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
There are no need to define those. You can directly use ARM_SMCCC_CALL_* from
asm-arm/smccc.h.
> +
> +#define PM_MMIO_SHIFT 32
You don't seem to use it at all. Did I miss anything?
> +
> +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);
>
> #endif /* __ASM_ARM_PLATFORMS_ZYNQMP_H */
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v5 5/7] xen/arm: zynqmp: eemi access control
2018-12-03 21:03 ` [PATCH v5 5/7] xen/arm: zynqmp: eemi access control Stefano Stabellini
@ 2018-12-11 15:37 ` Julien Grall
2018-12-12 23:57 ` Stefano Stabellini
0 siblings, 1 reply; 32+ messages in thread
From: Julien Grall @ 2018-12-11 15:37 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: edgar.iglesias, Stefano Stabellini, saeed.nowshadi
Hi,
On 03/12/2018 21:03, 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>
>
> ---
> Statically defines:
>
> - pm_node_access
> It encodes the relationship between a node id and the start of the MMIO
> region of a device in the corresponding power domain. It is used for
> permission checking. Although the MMIO region start address is available
> on device tree and could be derived from there (we plan to improve that
> in the future), the relationship between a node id and corresponding
> devices is not described and needs to be hardcoded.
>
> - pm_reset_access
> Same as pm_node_access for reset lines.
>
> ---
> Changes in v5:
> - improve in-code comments
> - use mfn_t in struct pm_access
> - remove mmio_access table
>
> Changes in v4:
> - add #include as needed
> - add #if 0 for bisectability
> - use mfn_t in pm_check_access
> - add wrap-around ASSERT in domain_has_mmio_access
> - use GENMASK in domain_has_mmio_access
> - proper bound checks (== ARRAY_SIZE is out of bound)
> ---
> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 348 ++++++++++++++++++++++++++++
> 1 file changed, 348 insertions(+)
>
> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> index 369bb3f..92a02df 100644
> --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> @@ -16,9 +16,357 @@
> * 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().
> + */
> +
> +#include <xen/iocap.h>
> +#include <xen/sched.h>
> #include <asm/regs.h>
> #include <asm/platforms/xilinx-zynqmp-eemi.h>
>
> +#if 0
> +struct pm_access
> +{
> + mfn_t mfn;
> + bool hwdom_access; /* HW domain gets access regardless. */
> +};
> +
> +/*
> + * This table maps a node into a memory address.
Some of the nodes below don't have memory address. So this comment has to be
updated.
> + * 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] = { mfn_init(MM_RPU) },
> + [NODE_RPU_0] = { mfn_init(MM_RPU) },
> + [NODE_RPU_1] = { mfn_init(MM_RPU) },
> + [NODE_IPI_RPU_0] = { mfn_init(MM_RPU) },
> +
> + /* GPU nodes. */
> + [NODE_GPU] = { mfn_init(MM_GPU) },
> + [NODE_GPU_PP_0] = { mfn_init(MM_GPU) },
> + [NODE_GPU_PP_1] = { mfn_init(MM_GPU) },
> +
> + [NODE_USB_0] = { mfn_init(MM_USB3_0_XHCI) },
> + [NODE_USB_1] = { mfn_init(MM_USB3_1_XHCI) },
> + [NODE_TTC_0] = { mfn_init(MM_TTC0) },
> + [NODE_TTC_1] = { mfn_init(MM_TTC1) },
> + [NODE_TTC_2] = { mfn_init(MM_TTC2) },
> + [NODE_TTC_3] = { mfn_init(MM_TTC3) },
> + [NODE_SATA] = { mfn_init(MM_SATA_AHCI_HBA) },
> + [NODE_ETH_0] = { mfn_init(MM_GEM0) },
> + [NODE_ETH_1] = { mfn_init(MM_GEM1) },
> + [NODE_ETH_2] = { mfn_init(MM_GEM2) },
> + [NODE_ETH_3] = { mfn_init(MM_GEM3) },
> + [NODE_UART_0] = { mfn_init(MM_UART0) },
> + [NODE_UART_1] = { mfn_init(MM_UART1) },
> + [NODE_SPI_0] = { mfn_init(MM_SPI0) },
> + [NODE_SPI_1] = { mfn_init(MM_SPI1) },
> + [NODE_I2C_0] = { mfn_init(MM_I2C0) },
> + [NODE_I2C_1] = { mfn_init(MM_I2C1) },
> + [NODE_SD_0] = { mfn_init(MM_SD0) },
> + [NODE_SD_1] = { mfn_init(MM_SD1) },
> + [NODE_DP] = { mfn_init(MM_DP) },
> +
> + /* Guest with GDMA Channel 0 gets PM access. Other guests don't. */
> + [NODE_GDMA] = { mfn_init(MM_GDMA_CH0) },
> + /* Guest with ADMA Channel 0 gets PM access. Other guests don't. */
> + [NODE_ADMA] = { mfn_init(MM_ADMA_CH0) },
> +
> + [NODE_NAND] = { mfn_init(MM_NAND) },
> + [NODE_QSPI] = { mfn_init(MM_QSPI) },
> + [NODE_GPIO] = { mfn_init(MM_GPIO) },
> + [NODE_CAN_0] = { mfn_init(MM_CAN0) },
> + [NODE_CAN_1] = { mfn_init(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] = { mfn_init(MM_PCIE_ATTRIB) },
> + [NODE_RTC] = { mfn_init(MM_RTC) },
> +};
> +
> +/*
> + * This table maps reset line IDs into a memory address.
Same here.
> + * 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)] = { mfn_init(MM_AXIPCIE_MAIN) },
> + [XILPM_RESET_IDX(XILPM_RESET_PCIE_BRIDGE)] = { mfn_init(MM_PCIE_ATTRIB) },
> + [XILPM_RESET_IDX(XILPM_RESET_PCIE_CTRL)] = { mfn_init(MM_PCIE_ATTRIB) },
> +
> + [XILPM_RESET_IDX(XILPM_RESET_DP)] = { mfn_init(MM_DP) },
> + [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRF)] = { mfn_init(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)] = { mfn_init(MM_GDMA_CH0) },
> + [XILPM_RESET_IDX(XILPM_RESET_GPU_PP1)] = { mfn_init(MM_GPU) },
> + [XILPM_RESET_IDX(XILPM_RESET_GPU_PP0)] = { mfn_init(MM_GPU) },
> + [XILPM_RESET_IDX(XILPM_RESET_GT)] = { .hwdom_access = true },
> + [XILPM_RESET_IDX(XILPM_RESET_SATA)] = { mfn_init(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)] = { mfn_init(MM_GEM0) },
> + [XILPM_RESET_IDX(XILPM_RESET_GEM1)] = { mfn_init(MM_GEM1) },
> + [XILPM_RESET_IDX(XILPM_RESET_GEM2)] = { mfn_init(MM_GEM2) },
> + [XILPM_RESET_IDX(XILPM_RESET_GEM3)] = { mfn_init(MM_GEM3) },
> +
> + [XILPM_RESET_IDX(XILPM_RESET_QSPI)] = { mfn_init(MM_QSPI) },
> + [XILPM_RESET_IDX(XILPM_RESET_UART0)] = { mfn_init(MM_UART0) },
> + [XILPM_RESET_IDX(XILPM_RESET_UART1)] = { mfn_init(MM_UART1) },
> + [XILPM_RESET_IDX(XILPM_RESET_SPI0)] = { mfn_init(MM_SPI0) },
> + [XILPM_RESET_IDX(XILPM_RESET_SPI1)] = { mfn_init(MM_SPI1) },
> + [XILPM_RESET_IDX(XILPM_RESET_SDIO0)] = { mfn_init(MM_SD0) },
> + [XILPM_RESET_IDX(XILPM_RESET_SDIO1)] = { mfn_init(MM_SD1) },
> + [XILPM_RESET_IDX(XILPM_RESET_CAN0)] = { mfn_init(MM_CAN0) },
> + [XILPM_RESET_IDX(XILPM_RESET_CAN1)] = { mfn_init(MM_CAN1) },
> + [XILPM_RESET_IDX(XILPM_RESET_I2C0)] = { mfn_init(MM_I2C0) },
> + [XILPM_RESET_IDX(XILPM_RESET_I2C1)] = { mfn_init(MM_I2C1) },
> + [XILPM_RESET_IDX(XILPM_RESET_TTC0)] = { mfn_init(MM_TTC0) },
> + [XILPM_RESET_IDX(XILPM_RESET_TTC1)] = { mfn_init(MM_TTC1) },
> + [XILPM_RESET_IDX(XILPM_RESET_TTC2)] = { mfn_init(MM_TTC2) },
> + [XILPM_RESET_IDX(XILPM_RESET_TTC3)] = { mfn_init(MM_TTC3) },
> + [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRL)] = { mfn_init(MM_SWDT) },
> + [XILPM_RESET_IDX(XILPM_RESET_NAND)] = { mfn_init(MM_NAND) },
> + [XILPM_RESET_IDX(XILPM_RESET_ADMA)] = { mfn_init(MM_ADMA_CH0) },
> + [XILPM_RESET_IDX(XILPM_RESET_GPIO)] = { mfn_init(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)] = { mfn_init(MM_RPU) },
> + [XILPM_RESET_IDX(XILPM_RESET_RPU_R51)] = { mfn_init(MM_RPU) },
> + [XILPM_RESET_IDX(XILPM_RESET_RPU_AMBA)] = { mfn_init(MM_RPU) },
> + [XILPM_RESET_IDX(XILPM_RESET_OCM)] = { .hwdom_access = true },
> + [XILPM_RESET_IDX(XILPM_RESET_RPU_PGE)] = { mfn_init(MM_RPU) },
> +
> + [XILPM_RESET_IDX(XILPM_RESET_USB0_CORERESET)] = { mfn_init(MM_USB3_0_XHCI) },
> + [XILPM_RESET_IDX(XILPM_RESET_USB0_HIBERRESET)] = { mfn_init(MM_USB3_0_XHCI) },
> + [XILPM_RESET_IDX(XILPM_RESET_USB0_APB)] = { mfn_init(MM_USB3_0_XHCI) },
> +
> + [XILPM_RESET_IDX(XILPM_RESET_USB1_CORERESET)] = { mfn_init(MM_USB3_1_XHCI) },
> + [XILPM_RESET_IDX(XILPM_RESET_USB1_HIBERRESET)] = { mfn_init(MM_USB3_1_XHCI) },
> + [XILPM_RESET_IDX(XILPM_RESET_USB1_APB)] = { mfn_init(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)] = { mfn_init(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)] = { mfn_init(MM_SWDT) },
> + [XILPM_RESET_IDX(XILPM_RESET_FPD)] = { .hwdom_access = true },
> + [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG1)] = { mfn_init(MM_RPU) },
> + [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG0)] = { mfn_init(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)] = { mfn_init(MM_RPU) },
> + [XILPM_RESET_IDX(XILPM_RESET_PS_ONLY)] = { .hwdom_access = true },
> + [XILPM_RESET_IDX(XILPM_RESET_PL)] = { .hwdom_access = true },
> +};
> +
> +static bool pm_check_access(const struct pm_access *acl, struct domain *d,
> + uint32_t idx)
> +{
> + if ( acl[idx].hwdom_access && is_hardware_domain(d) )
> + return true;
> +
> + if ( !mfn_x(acl[idx].mfn) )
Technically 0 is a valid mfn. If you want to encode an invalid value then
MFN_INVALID is safer.
But what are you trying to prevent? Are the node IDs not allocated contiguously?
> + return false;
> +
> + return iomem_access_permitted(d, mfn_x(acl[idx].mfn), mfn_x(acl[idx].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;
I think you can make this code no rely at all on XILPM_RESET_PCIE_CFG by using
XILPM_RESET_IDX.
if ( XILPM_RESET_IDX(rst) >= ARRAY_SIZE(pm_reset_access) )
return false;
rst = XILPM_RESET_IDX(rst);
We rely on the unsigned underflow to catch value below XILPM_RESET_PCIE_CFG and
make the code less error prone to change the array without the code here.
> +
> + 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++ )
> + {
> + ASSERT(pm_mmio_access[i].start + pm_mmio_access[i].size >=
> + pm_mmio_access[i].start);
> +
> + 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 ?: GENMASK(31, 0);
The field mask does not seem to exist.
> + break;
> + }
> +
> + /*
> + * Masking only applies to writes: values are safe to read, but not
> + * all bits are writeable.
> + */
> + if ( write )
> + *mask &= prot_mask;
> +
> + return ret;
> +}
> +#endif
> +
> 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] 32+ messages in thread
* Re: [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi
2018-12-03 21:03 ` [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi Stefano Stabellini
@ 2018-12-11 15:55 ` Julien Grall
2018-12-11 22:23 ` Stefano Stabellini
0 siblings, 1 reply; 32+ messages in thread
From: Julien Grall @ 2018-12-11 15:55 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
Cc: edgar.iglesias, Stefano Stabellini, saeed.nowshadi
Hi Stefano,
On 03/12/2018 21:03, 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>
> ---
> Changes in v5:
> - remove mmio_access handling
>
> Changes in v4:
> - add #include as needed
> - improve comment
> - code style
> ---
> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 181 +++++++++++++++++++---------
> 1 file changed, 125 insertions(+), 56 deletions(-)
>
> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> index 92a02df..9ecf286 100644
> --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> @@ -76,10 +76,10 @@
>
> #include <xen/iocap.h>
> #include <xen/sched.h>
> +#include <asm/smccc.h>
> #include <asm/regs.h>
> #include <asm/platforms/xilinx-zynqmp-eemi.h>
>
> -#if 0
> struct pm_access
> {
> mfn_t mfn;
> @@ -309,67 +309,136 @@ static bool domain_has_reset_access(struct domain *d, uint32_t rst)
> 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)
Why do you remove code that you just introduced?
> +bool zynqmp_eemi(struct cpu_user_regs *regs)
> {
> - 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;
> + struct arm_smccc_res res;
> + uint32_t fid = get_user_reg(regs, 0);
> + uint32_t nodeid = get_user_reg(regs, 1);
You didn't address my concern regarding SMC32 vs SMC64 convention. As I said
earlier on, at least CALL_COUNT, UID and VERSION are only accessible using the
SMC32 convention.
I can't tell for the other as the EEMI spec does not seem to specify it. I would
be 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.
So this needs to be fixed properly.
> + unsigned int pm_fn = fid & 0xFFFF;
> + enum pm_ret_status ret;
>
> - /* Scan the ACL. */
> - for ( i = 0; i < ARRAY_SIZE(pm_mmio_access); i++ )
> + switch ( pm_fn )
> {
> - ASSERT(pm_mmio_access[i].start + pm_mmio_access[i].size >=
> - pm_mmio_access[i].start);
> -
> - 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 ?: GENMASK(31, 0);
> - break;
> - }
> -
> /*
> - * Masking only applies to writes: values are safe to read, but not
> - * all bits are writeable.
> + * 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 or PSCI call which Xen will trap and act accordingly upon.
> */
> - if ( write )
> - *mask &= prot_mask;
> -
> - return ret;
> -}
> -#endif
> + 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;
> +
> + /* No MMIO access is allowed from non-secure domains */
> + case PM_MMIO_WRITE:
> + case PM_MMIO_READ:
> + gprintk(XENLOG_WARNING,
> + "zynqmp-pm: fn=%u No MMIO access to %u\n", pm_fn, nodeid);
> + ret = XST_PM_NO_ACCESS;
> + goto done;
> +
> + /* 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;
> + }
>
> -bool zynqmp_eemi(struct cpu_user_regs *regs)
> -{
> - 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),value to
> + 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);
If you use always SMCCC 1.1, then you should add code to deny Xen boot on
platform not supporting SMCCC 1.1 or later.
Furthermore, you are forwarding unsanitized values to the firmware. For
instance, what would happen if the number of parameters of the call are
increased? How are you sure this will not open a hole?
> +
> + 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] 32+ messages in thread
* Re: [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls
2018-12-11 15:03 ` Julien Grall
@ 2018-12-11 18:50 ` Stefano Stabellini
2018-12-11 19:09 ` Julien Grall
0 siblings, 1 reply; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-11 18:50 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, Stefano Stabellini,
saeed.nowshadi, xen-devel
On Tue, 11 Dec 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 03/12/2018 21:03, Stefano Stabellini wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> >
> > Introduce zynqmp_eemi: a function responsible 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>
> >
> > ---
> > Changes in v4:
> > - fix typo
> > - add header guard
> > - add emacs magic
> > - remove #includes that will only be used later
> > - add copyright notice to header
> > - remove SMCCC 1.1 check
> > ---
> > xen/arch/arm/platforms/Makefile | 1 +
> > xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 34
> > ++++++++++++++++++++++
> > xen/arch/arm/platforms/xilinx-zynqmp.c | 11 +++++++
> > xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 30
> > +++++++++++++++++++
> > 4 files changed, 76 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 a79bdb9..fe8e0c7 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_MPSOC_PLATFORM) += xilinx-zynqmp.o
> > +obj-$(CONFIG_MPSOC_PLATFORM) += 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..369bb3f
> > --- /dev/null
> > +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > @@ -0,0 +1,34 @@
> > +/*
> > + * 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 <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..9c174d2 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,17 @@ 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) )
>
> Please document why you only expose eemi to 64-bit domain. What if the user
> start with 32-bit Dom0?
I'll add a in-code comment saying that only 64-bit guests are supported
by the current implementation -- the 32 bit EEMI ABI is not yet covered.
> > + return false;
> > +
> > + return zynqmp_eemi(regs);
>
> double space.
I'll fix
> > +}
> > +
> > 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..43cefb5
> > --- /dev/null
> > +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > @@ -0,0 +1,30 @@
> > +/*
> > + * Copyright (c) 2018 Xilinx Inc.
> > + *
> > + * 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.
> > + */
> > +
> > +#ifndef __ASM_ARM_PLATFORMS_ZYNQMP_H
> > +#define __ASM_ASM_PLATFORMS_ZYNQMP_H
> > +
> > +#include <asm/processor.h>
> > +
> > +extern bool zynqmp_eemi(struct cpu_user_regs *regs);
> > +
> > +#endif /* __ASM_ARM_PLATFORMS_ZYNQMP_H */
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> >
>
> 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] 32+ messages in thread
* Re: [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls
2018-12-11 18:50 ` Stefano Stabellini
@ 2018-12-11 19:09 ` Julien Grall
2018-12-11 20:19 ` Stefano Stabellini
0 siblings, 1 reply; 32+ messages in thread
From: Julien Grall @ 2018-12-11 19:09 UTC (permalink / raw)
To: Stefano Stabellini
Cc: edgar.iglesias, Stefano Stabellini, saeed.nowshadi, xen-devel
Hi Stefano,
On 11/12/2018 18:50, Stefano Stabellini wrote:
> On Tue, 11 Dec 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 03/12/2018 21:03, Stefano Stabellini wrote:
>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>
>>> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>
>>> Introduce zynqmp_eemi: a function responsible 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>
>>>
>>> ---
>>> Changes in v4:
>>> - fix typo
>>> - add header guard
>>> - add emacs magic
>>> - remove #includes that will only be used later
>>> - add copyright notice to header
>>> - remove SMCCC 1.1 check
>>> ---
>>> xen/arch/arm/platforms/Makefile | 1 +
>>> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 34
>>> ++++++++++++++++++++++
>>> xen/arch/arm/platforms/xilinx-zynqmp.c | 11 +++++++
>>> xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 30
>>> +++++++++++++++++++
>>> 4 files changed, 76 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 a79bdb9..fe8e0c7 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_MPSOC_PLATFORM) += xilinx-zynqmp.o
>>> +obj-$(CONFIG_MPSOC_PLATFORM) += 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..369bb3f
>>> --- /dev/null
>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> @@ -0,0 +1,34 @@
>>> +/*
>>> + * 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 <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..9c174d2 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,17 @@ 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) )
>>
>> Please document why you only expose eemi to 64-bit domain. What if the user
>> start with 32-bit Dom0?
>
> I'll add a in-code comment saying that only 64-bit guests are supported
> by the current implementation -- the 32 bit EEMI ABI is not yet covered.
64-bit guests is allowed to use both SMC32 and SMC64. There are actually some
call that can only be done using SMC32 convention (see patch #6). So why forbid
32-bit domain when you allow 64-bit using 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] 32+ messages in thread
* Re: [PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines
2018-12-11 15:21 ` Julien Grall
@ 2018-12-11 19:22 ` Stefano Stabellini
2018-12-12 10:29 ` Julien Grall
0 siblings, 1 reply; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-11 19:22 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, Stefano Stabellini,
saeed.nowshadi, xen-devel
On Tue, 11 Dec 2018, Julien Grall wrote:
> Hi,
>
> On 03/12/2018 21:03, Stefano Stabellini wrote:
> > 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>
> >
> > ---
> >
> > Although the amount of #defines has been significantly reduced in v5,
> > there is still a significant amount of static definitions:
>
> I think the description below would be useful to have in the commit message.
I can add it.
> > - MM_*
> > These are the MMIO addresses of each resource to do permission checks.
> > Technically, they are also present on device tree, but using device tree
> > to get the mmio regions is not simple and not done in this series.
> > Xilinx plan to send a patch series in the future to extend the EEMI
> > functionalities and as part of that work, more device tree based
> > permission checking will be done, solving this problem.
>
> As this is already present in the Device-Tree, there would be no issue to
> remove the hardcoded value, correct?
Yes, that is correct. Saeed took it on as part of his next patch series
to rework these checks, removing the MM_* #defines.
> > These
> > definitions could also be removed if we used a trivial "if dom0 -> yes,
> > otherwise no" permission checking.
> >
> > - pm_api_id
> > These are the EEMI function IDs. Unavoidable.
> >
> > - pm_ret_status
> > These are the EEMI return statuses. Unavoidable.
> >
> > - pm_node_id
> > These are the EEMI function parameters for power management operations.
> > Today, it is not possible to get them from device tree as there is no
> > such information there. Even in the future when we add more power
> > domains info to device tree, the EEMI function parameters might remain
> > unique and different, requiring a table like this one.
>
> Does it mean each Linux driver will have to hardcode the pm_node_id as well?
The EEMI functionalities upstream in Linux today don't need any node_id
knowledge: there is one ioctl that takes a node_id from userspace and
pass it to the firmware without any checks. There are few clock API
calls that take a clock id, but those are different from node_ids and
are discoverable via EEMI calls (they will be discoverable in Xen too,
patch series in development.) In other words, Linux doesn't do anything
with node_ids today.
> What is the plan there?
The plan is to extract the node_id from a power-domain node on device
tree. Each device would have a phandler to link to the right
power-domain node which contains a power-domain-id attribute. The
power-domain-id attribute is the node_id here.
The power-domain-id changes to the Xilinx MPSoC device tree are under
discussion with the device tree community.
> > - pm_reset
> > These are the EEMI function parameters for reset operations. Same as
> > pm_node_id.
>
> Ditto.
Similar to above
> >
> > ---
> > Changes in v5:
> > - remove MMIO access related definitions
> >
> > Changes in v4:
> > - define PM_MMIO_SHIFT
> > ---
> > xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 293
> > +++++++++++++++++++++
> > 1 file changed, 293 insertions(+)
> >
> > diff --git a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > index 43cefb5..f6ad03b 100644
> > --- a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > @@ -16,6 +16,299 @@
> > #include <asm/processor.h>
> > +#define MM_RPU 0xff9a0
> > +#define MM_RTC 0xffa60
> > +#define MM_ADMA_CH0 0xffa80
> > +
> > +#define MM_USB3_0_XHCI 0xfe200
> > +#define MM_USB3_1_XHCI 0xfe300
> > +
> > +#define MM_SATA_AHCI_HBA 0xfd0c0
> > +#define MM_AXIPCIE_MAIN 0xfd0e0
> > +#define MM_CRF_APB 0xfd1a0000
> > +#define MM_PCIE_ATTRIB 0xfd480
> > +#define MM_DP 0xfd4a0
> > +#define MM_GPU 0xfd4b0
> > +#define MM_GDMA_CH0 0xfd500
> > +
> > +#define MM_UART0 0xff000
> > +#define MM_UART1 0xff010
> > +#define MM_I2C0 0xff020
> > +#define MM_I2C1 0xff030
> > +#define MM_SPI0 0xff040
> > +#define MM_SPI1 0xff050
> > +#define MM_CAN0 0xff060
> > +#define MM_CAN1 0xff070
> > +#define MM_GPIO 0xff0a0
> > +#define MM_GEM0 0xff0b0
> > +#define MM_GEM1 0xff0c0
> > +#define MM_GEM2 0xff0d0
> > +#define MM_GEM3 0xff0e0
> > +#define MM_QSPI 0xff0f0
> > +#define MM_NAND 0xff100
> > +#define MM_TTC0 0xff110
> > +#define MM_TTC1 0xff120
> > +#define MM_TTC2 0xff130
> > +#define MM_TTC3 0xff140
> > +#define MM_SWDT 0xff150
> > +#define MM_SD0 0xff160
> > +#define MM_SD1 0xff170
> > +
> > +/* 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
>
> There are no need to define those. You can directly use ARM_SMCCC_CALL_* from
> asm-arm/smccc.h.
>
> > +
> > +#define PM_MMIO_SHIFT 32
>
> You don't seem to use it at all. Did I miss anything?
>
> > +
> > +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);
> > #endif /* __ASM_ARM_PLATFORMS_ZYNQMP_H */
> >
>
> --
> Julien Grall
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls
2018-12-11 19:09 ` Julien Grall
@ 2018-12-11 20:19 ` Stefano Stabellini
2018-12-11 20:27 ` Julien Grall
0 siblings, 1 reply; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-11 20:19 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, Stefano Stabellini,
saeed.nowshadi, xen-devel
On Tue, 11 Dec 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 11/12/2018 18:50, Stefano Stabellini wrote:
> > On Tue, 11 Dec 2018, Julien Grall wrote:
> > > Hi Stefano,
> > >
> > > On 03/12/2018 21:03, Stefano Stabellini wrote:
> > > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > > >
> > > > From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > > >
> > > > Introduce zynqmp_eemi: a function responsible 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>
> > > >
> > > > ---
> > > > Changes in v4:
> > > > - fix typo
> > > > - add header guard
> > > > - add emacs magic
> > > > - remove #includes that will only be used later
> > > > - add copyright notice to header
> > > > - remove SMCCC 1.1 check
> > > > ---
> > > > xen/arch/arm/platforms/Makefile | 1 +
> > > > xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 34
> > > > ++++++++++++++++++++++
> > > > xen/arch/arm/platforms/xilinx-zynqmp.c | 11 +++++++
> > > > xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 30
> > > > +++++++++++++++++++
> > > > 4 files changed, 76 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 a79bdb9..fe8e0c7 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_MPSOC_PLATFORM) += xilinx-zynqmp.o
> > > > +obj-$(CONFIG_MPSOC_PLATFORM) += 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..369bb3f
> > > > --- /dev/null
> > > > +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > > > @@ -0,0 +1,34 @@
> > > > +/*
> > > > + * 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 <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..9c174d2 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,17 @@ 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) )
> > >
> > > Please document why you only expose eemi to 64-bit domain. What if the
> > > user
> > > start with 32-bit Dom0?
> >
> > I'll add a in-code comment saying that only 64-bit guests are supported
> > by the current implementation -- the 32 bit EEMI ABI is not yet covered.
>
> 64-bit guests is allowed to use both SMC32 and SMC64. There are actually some
> call that can only be done using SMC32 convention (see patch #6). So why
> forbid 32-bit domain when you allow 64-bit using SMC32?
I understand. It really looks like this check is a mistake. In fact, it
doesn't matter if a domain is 32bit or 64bit, the only thing that
matters for this code is if the EEMI calls are SMC32 and SMC64. So
instead of is_64bit_domain, I should check that SMC64 is used. Right?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls
2018-12-11 20:19 ` Stefano Stabellini
@ 2018-12-11 20:27 ` Julien Grall
0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2018-12-11 20:27 UTC (permalink / raw)
To: Stefano Stabellini
Cc: edgar.iglesias, Stefano Stabellini, nd, saeed.nowshadi, xen-devel
On 11/12/2018 20:19, Stefano Stabellini wrote:
> On Tue, 11 Dec 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 11/12/2018 18:50, Stefano Stabellini wrote:
>>> On Tue, 11 Dec 2018, Julien Grall wrote:
>>>> Hi Stefano,
>>>>
>>>> On 03/12/2018 21:03, Stefano Stabellini wrote:
>>>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>>>
>>>>> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>>>
>>>>> Introduce zynqmp_eemi: a function responsible 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>
>>>>>
>>>>> ---
>>>>> Changes in v4:
>>>>> - fix typo
>>>>> - add header guard
>>>>> - add emacs magic
>>>>> - remove #includes that will only be used later
>>>>> - add copyright notice to header
>>>>> - remove SMCCC 1.1 check
>>>>> ---
>>>>> xen/arch/arm/platforms/Makefile | 1 +
>>>>> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 34
>>>>> ++++++++++++++++++++++
>>>>> xen/arch/arm/platforms/xilinx-zynqmp.c | 11 +++++++
>>>>> xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 30
>>>>> +++++++++++++++++++
>>>>> 4 files changed, 76 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 a79bdb9..fe8e0c7 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_MPSOC_PLATFORM) += xilinx-zynqmp.o
>>>>> +obj-$(CONFIG_MPSOC_PLATFORM) += 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..369bb3f
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>>> @@ -0,0 +1,34 @@
>>>>> +/*
>>>>> + * 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 <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..9c174d2 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,17 @@ 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) )
>>>>
>>>> Please document why you only expose eemi to 64-bit domain. What if the
>>>> user
>>>> start with 32-bit Dom0?
>>>
>>> I'll add a in-code comment saying that only 64-bit guests are supported
>>> by the current implementation -- the 32 bit EEMI ABI is not yet covered.
>>
>> 64-bit guests is allowed to use both SMC32 and SMC64. There are actually some
>> call that can only be done using SMC32 convention (see patch #6). So why
>> forbid 32-bit domain when you allow 64-bit using SMC32?
>
> I understand. It really looks like this check is a mistake. In fact, it
> doesn't matter if a domain is 32bit or 64bit, the only thing that
> matters for this code is if the EEMI calls are SMC32 and SMC64. So
> instead of is_64bit_domain, I should check that SMC64 is used. Right?
This would be wrong because you at least need to handle UID, Call count
and version using SMC32 as mandated by the SMCCC.
The right solution is to deal with the full identifier in patch #6
rather than just the function ID.
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] 32+ messages in thread
* Re: [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi
2018-12-11 15:55 ` Julien Grall
@ 2018-12-11 22:23 ` Stefano Stabellini
2018-12-12 10:48 ` Julien Grall
0 siblings, 1 reply; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-11 22:23 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, Stefano Stabellini,
saeed.nowshadi, xen-devel
On Tue, 11 Dec 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 03/12/2018 21:03, 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>
> > ---
> > Changes in v5:
> > - remove mmio_access handling
> >
> > Changes in v4:
> > - add #include as needed
> > - improve comment
> > - code style
> > ---
> > xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 181
> > +++++++++++++++++++---------
> > 1 file changed, 125 insertions(+), 56 deletions(-)
> >
> > diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > index 92a02df..9ecf286 100644
> > --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > @@ -76,10 +76,10 @@
> > #include <xen/iocap.h>
> > #include <xen/sched.h>
> > +#include <asm/smccc.h>
> > #include <asm/regs.h>
> > #include <asm/platforms/xilinx-zynqmp-eemi.h>
> > -#if 0
> > struct pm_access
> > {
> > mfn_t mfn;
> > @@ -309,67 +309,136 @@ static bool domain_has_reset_access(struct domain *d,
> > uint32_t rst)
> > 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)
>
> Why do you remove code that you just introduced?
I am really sorry about this, it was error applying a patch. This code
should never have been introduced: the code should be removed from the
previous patch. I'll fix it.
> > +bool zynqmp_eemi(struct cpu_user_regs *regs)
> > {
> > - 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;
> > + struct arm_smccc_res res;
> > + uint32_t fid = get_user_reg(regs, 0);
> > + uint32_t nodeid = get_user_reg(regs, 1);
>
> You didn't address my concern regarding SMC32 vs SMC64 convention. As I said
> earlier on, at least CALL_COUNT, UID and VERSION are only accessible using the
> SMC32 convention.
>
> I can't tell for the other as the EEMI spec does not seem to specify it. I
> would be 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.
>
> So this needs to be fixed properly.
OK, I'll add a check for the mandatory smc32 calls and forward them to
firmware properly.
> > + unsigned int pm_fn = fid & 0xFFFF;
> > + enum pm_ret_status ret;
> > - /* Scan the ACL. */
> > - for ( i = 0; i < ARRAY_SIZE(pm_mmio_access); i++ )
> > + switch ( pm_fn )
> > {
> > - ASSERT(pm_mmio_access[i].start + pm_mmio_access[i].size >=
> > - pm_mmio_access[i].start);
> > -
> > - 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 ?: GENMASK(31, 0);
> > - break;
> > - }
> > -
> > /*
> > - * Masking only applies to writes: values are safe to read, but not
> > - * all bits are writeable.
> > + * 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 or PSCI call which Xen will trap and act accordingly upon.
> > */
> > - if ( write )
> > - *mask &= prot_mask;
> > -
> > - return ret;
> > -}
> > -#endif
> > + 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;
> > +
> > + /* No MMIO access is allowed from non-secure domains */
> > + case PM_MMIO_WRITE:
> > + case PM_MMIO_READ:
> > + gprintk(XENLOG_WARNING,
> > + "zynqmp-pm: fn=%u No MMIO access to %u\n", pm_fn, nodeid);
> > + ret = XST_PM_NO_ACCESS;
> > + goto done;
> > +
> > + /* 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;
> > + }
> > -bool zynqmp_eemi(struct cpu_user_regs *regs)
> > -{
> > - 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),value to
> > + 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);
>
> If you use always SMCCC 1.1, then you should add code to deny Xen boot on
> platform not supporting SMCCC 1.1 or later.
I can do that.
> Furthermore, you are forwarding unsanitized values to the firmware. For
> instance, what would happen if the number of parameters of the call are
> increased? How are you sure this will not open a hole?
EEMI is backward compatible and the implementation is tested with Xen
regularly. A change like the one you describe should be considered a
backward compatibility breakage.
> > + 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] 32+ messages in thread
* Re: [PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines
2018-12-11 19:22 ` Stefano Stabellini
@ 2018-12-12 10:29 ` Julien Grall
2018-12-12 23:55 ` Stefano Stabellini
0 siblings, 1 reply; 32+ messages in thread
From: Julien Grall @ 2018-12-12 10:29 UTC (permalink / raw)
To: Stefano Stabellini
Cc: edgar.iglesias, Stefano Stabellini, saeed.nowshadi, xen-devel
Hi Stefano,
On 11/12/2018 19:22, Stefano Stabellini wrote:
> On Tue, 11 Dec 2018, Julien Grall wrote:
>> Hi,
>>
>> On 03/12/2018 21:03, Stefano Stabellini wrote:
>> What is the plan there?
>
> The plan is to extract the node_id from a power-domain node on device
> tree. Each device would have a phandler to link to the right
> power-domain node which contains a power-domain-id attribute. The
> power-domain-id attribute is the node_id here.
>
> The power-domain-id changes to the Xilinx MPSoC device tree are under
> discussion with the device tree community.
If I understand correctly, we will never be able to remove the hardcoded values
in Xen. This is because some device-tree may not have the bindings. Am I correct?
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] 32+ messages in thread
* Re: [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi
2018-12-11 22:23 ` Stefano Stabellini
@ 2018-12-12 10:48 ` Julien Grall
2018-12-12 23:56 ` Stefano Stabellini
0 siblings, 1 reply; 32+ messages in thread
From: Julien Grall @ 2018-12-12 10:48 UTC (permalink / raw)
To: Stefano Stabellini
Cc: edgar.iglesias, Stefano Stabellini, saeed.nowshadi, xen-devel
On 11/12/2018 22:23, Stefano Stabellini wrote:
> On Tue, 11 Dec 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 03/12/2018 21:03, 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>
>>> ---
>>> Changes in v5:
>>> - remove mmio_access handling
>>>
>>> Changes in v4:
>>> - add #include as needed
>>> - improve comment
>>> - code style
>>> ---
>>> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 181
>>> +++++++++++++++++++---------
>>> 1 file changed, 125 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> index 92a02df..9ecf286 100644
>>> --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> @@ -76,10 +76,10 @@
>>> #include <xen/iocap.h>
>>> #include <xen/sched.h>
>>> +#include <asm/smccc.h>
>>> #include <asm/regs.h>
>>> #include <asm/platforms/xilinx-zynqmp-eemi.h>
>>> -#if 0
>>> struct pm_access
>>> {
>>> mfn_t mfn;
>>> @@ -309,67 +309,136 @@ static bool domain_has_reset_access(struct domain *d,
>>> uint32_t rst)
>>> 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)
>>
>> Why do you remove code that you just introduced?
>
> I am really sorry about this, it was error applying a patch. This code
> should never have been introduced: the code should be removed from the
> previous patch. I'll fix it.
>
>
>>> +bool zynqmp_eemi(struct cpu_user_regs *regs)
>>> {
>>> - 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;
>>> + struct arm_smccc_res res;
>>> + uint32_t fid = get_user_reg(regs, 0);
>>> + uint32_t nodeid = get_user_reg(regs, 1);
>>
>> You didn't address my concern regarding SMC32 vs SMC64 convention. As I said
>> earlier on, at least CALL_COUNT, UID and VERSION are only accessible using the
>> SMC32 convention.
>>
>> I can't tell for the other as the EEMI spec does not seem to specify it. I
>> would be 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.
>>
>> So this needs to be fixed properly.
>
> OK, I'll add a check for the mandatory smc32 calls and forward them to
> firmware properly.
smc32 is only part of the problem I mentioned. The main problem is you only look
at the function number (bits 0-15). This is not enough to know which function is
going to be called. You want to take into account the full function identifier.
We provide helpers to create them (see ARM_SMCCC_CALL_VAL).
>
>
>>> + unsigned int pm_fn = fid & 0xFFFF;
>>> + enum pm_ret_status ret;
>>> - /* Scan the ACL. */
>>> - for ( i = 0; i < ARRAY_SIZE(pm_mmio_access); i++ )
>>> + switch ( pm_fn )
>>> {
>>> - ASSERT(pm_mmio_access[i].start + pm_mmio_access[i].size >=
>>> - pm_mmio_access[i].start);
>>> -
>>> - 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 ?: GENMASK(31, 0);
>>> - break;
>>> - }
>>> -
>>> /*
>>> - * Masking only applies to writes: values are safe to read, but not
>>> - * all bits are writeable.
>>> + * 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 or PSCI call which Xen will trap and act accordingly upon.
>>> */
>>> - if ( write )
>>> - *mask &= prot_mask;
>>> -
>>> - return ret;
>>> -}
>>> -#endif
>>> + 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;
>>> +
>>> + /* No MMIO access is allowed from non-secure domains */
>>> + case PM_MMIO_WRITE:
>>> + case PM_MMIO_READ:
>>> + gprintk(XENLOG_WARNING,
>>> + "zynqmp-pm: fn=%u No MMIO access to %u\n", pm_fn, nodeid);
>>> + ret = XST_PM_NO_ACCESS;
>>> + goto done;
>>> +
>>> + /* 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;
>>> + }
>>> -bool zynqmp_eemi(struct cpu_user_regs *regs)
>>> -{
>>> - 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),value to
>>> + 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);
>>
>> If you use always SMCCC 1.1, then you should add code to deny Xen boot on
>> platform not supporting SMCCC 1.1 or later.
>
> I can do that.
>
>
>> Furthermore, you are forwarding unsanitized values to the firmware. For
>> instance, what would happen if the number of parameters of the call are
>> increased? How are you sure this will not open a hole?
>
> EEMI is backward compatible and the implementation is tested with Xen
> regularly. A change like the one you describe should be considered a
> backward compatibility breakage.
I disagree, you can still make backward compatible. For instance, the new
parameters could be gated by a flag in an existing parameter. Another way is
Xilinx promise that non-existing arguments should always be 0 and then
re-purpose the value for non-zero case.
While it is backward compatible, you may end up passing unsanitized value to the
firmware. Not sure this is what we want...
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] 32+ messages in thread
* Re: [PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines
2018-12-12 10:29 ` Julien Grall
@ 2018-12-12 23:55 ` Stefano Stabellini
2018-12-13 15:50 ` Julien Grall
0 siblings, 1 reply; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-12 23:55 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, Stefano Stabellini,
saeed.nowshadi, xen-devel
On Wed, 12 Dec 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 11/12/2018 19:22, Stefano Stabellini wrote:
> > On Tue, 11 Dec 2018, Julien Grall wrote:
> > > Hi,
> > >
> > > On 03/12/2018 21:03, Stefano Stabellini wrote:
> > > What is the plan there?
> >
> > The plan is to extract the node_id from a power-domain node on device
> > tree. Each device would have a phandler to link to the right
> > power-domain node which contains a power-domain-id attribute. The
> > power-domain-id attribute is the node_id here.
> >
> > The power-domain-id changes to the Xilinx MPSoC device tree are under
> > discussion with the device tree community.
>
> If I understand correctly, we will never be able to remove the hardcoded
> values in Xen. This is because some device-tree may not have the bindings. Am
> I correct?
If we want to support running on existing hardware and firmware releases,
then you are correct. We won't be able to remove the hardcoded values.
That ship has sailed, not much we can do about it.
If in the future we decide to drop support for older firmware releases
and ask users to update their firmare/devicetrees, then we'll be able to
remove the hardcoded values. I think it is something we can consider.
In fact, I would rather break compatibility than not providing support
for basic power management functionalities at all.
I think it is better to introduce basic EEMI support now, even if in a
couple of Xen releases from now we'll make the decision to drop the
hardcoded values and require new Xilinx device trees. Xilinx users are
used to updating firmware on these boards every 6 months, and it would
beneficial for them to be able to take a Xen Project release rather than
be forced to use a Xilinx Xen release to have support for power
management. When the new bindings become available, as we introduce
support for them in Xen, we can decide whether to keep the hardcoded
values or whether we should take the opportunity to drop them.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi
2018-12-12 10:48 ` Julien Grall
@ 2018-12-12 23:56 ` Stefano Stabellini
2018-12-13 15:59 ` Julien Grall
0 siblings, 1 reply; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-12 23:56 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, Stefano Stabellini,
saeed.nowshadi, xen-devel
On Wed, 12 Dec 2018, Julien Grall wrote:
> On 11/12/2018 22:23, Stefano Stabellini wrote:
> > On Tue, 11 Dec 2018, Julien Grall wrote:
> > > Hi Stefano,
> > >
> > > On 03/12/2018 21:03, 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>
> > > > ---
> > > > Changes in v5:
> > > > - remove mmio_access handling
> > > >
> > > > Changes in v4:
> > > > - add #include as needed
> > > > - improve comment
> > > > - code style
> > > > ---
> > > > xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 181
> > > > +++++++++++++++++++---------
> > > > 1 file changed, 125 insertions(+), 56 deletions(-)
> > > >
> > > > diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > > > b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > > > index 92a02df..9ecf286 100644
> > > > --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > > > +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > > > @@ -76,10 +76,10 @@
> > > > #include <xen/iocap.h>
> > > > #include <xen/sched.h>
> > > > +#include <asm/smccc.h>
> > > > #include <asm/regs.h>
> > > > #include <asm/platforms/xilinx-zynqmp-eemi.h>
> > > > -#if 0
> > > > struct pm_access
> > > > {
> > > > mfn_t mfn;
> > > > @@ -309,67 +309,136 @@ static bool domain_has_reset_access(struct domain
> > > > *d,
> > > > uint32_t rst)
> > > > 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)
> > >
> > > Why do you remove code that you just introduced?
> >
> > I am really sorry about this, it was error applying a patch. This code
> > should never have been introduced: the code should be removed from the
> > previous patch. I'll fix it.
> >
> >
> > > > +bool zynqmp_eemi(struct cpu_user_regs *regs)
> > > > {
> > > > - 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;
> > > > + struct arm_smccc_res res;
> > > > + uint32_t fid = get_user_reg(regs, 0);
> > > > + uint32_t nodeid = get_user_reg(regs, 1);
> > >
> > > You didn't address my concern regarding SMC32 vs SMC64 convention. As I
> > > said
> > > earlier on, at least CALL_COUNT, UID and VERSION are only accessible using
> > > the
> > > SMC32 convention.
> > >
> > > I can't tell for the other as the EEMI spec does not seem to specify it. I
> > > would be 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.
> > >
> > > So this needs to be fixed properly.
> >
> > OK, I'll add a check for the mandatory smc32 calls and forward them to
> > firmware properly.
>
> smc32 is only part of the problem I mentioned. The main problem is you only
> look at the function number (bits 0-15). This is not enough to know which
> function is going to be called. You want to take into account the full
> function identifier. We provide helpers to create them (see
> ARM_SMCCC_CALL_VAL).
Yes, I get what you are saying. I made a few changes in that direction.
> > > > + unsigned int pm_fn = fid & 0xFFFF;
> > > > + enum pm_ret_status ret;
> > > > - /* Scan the ACL. */
> > > > - for ( i = 0; i < ARRAY_SIZE(pm_mmio_access); i++ )
> > > > + switch ( pm_fn )
> > > > {
> > > > - ASSERT(pm_mmio_access[i].start + pm_mmio_access[i].size >=
> > > > - pm_mmio_access[i].start);
> > > > -
> > > > - 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 ?: GENMASK(31, 0);
> > > > - break;
> > > > - }
> > > > -
> > > > /*
> > > > - * Masking only applies to writes: values are safe to read, but not
> > > > - * all bits are writeable.
> > > > + * 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 or PSCI call which Xen will trap and act accordingly upon.
> > > > */
> > > > - if ( write )
> > > > - *mask &= prot_mask;
> > > > -
> > > > - return ret;
> > > > -}
> > > > -#endif
> > > > + 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;
> > > > +
> > > > + /* No MMIO access is allowed from non-secure domains */
> > > > + case PM_MMIO_WRITE:
> > > > + case PM_MMIO_READ:
> > > > + gprintk(XENLOG_WARNING,
> > > > + "zynqmp-pm: fn=%u No MMIO access to %u\n", pm_fn,
> > > > nodeid);
> > > > + ret = XST_PM_NO_ACCESS;
> > > > + goto done;
> > > > +
> > > > + /* 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;
> > > > + }
> > > > -bool zynqmp_eemi(struct cpu_user_regs *regs)
> > > > -{
> > > > - 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),value to
> > > > + 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);
> > >
> > > If you use always SMCCC 1.1, then you should add code to deny Xen boot on
> > > platform not supporting SMCCC 1.1 or later.
> >
> > I can do that.
> >
> >
> > > Furthermore, you are forwarding unsanitized values to the firmware. For
> > > instance, what would happen if the number of parameters of the call are
> > > increased? How are you sure this will not open a hole?
> >
> > EEMI is backward compatible and the implementation is tested with Xen
> > regularly. A change like the one you describe should be considered a
> > backward compatibility breakage.
>
> I disagree, you can still make backward compatible. For instance, the new
> parameters could be gated by a flag in an existing parameter. Another way is
> Xilinx promise that non-existing arguments should always be 0 and then
> re-purpose the value for non-zero case.
>
> While it is backward compatible, you may end up passing unsanitized value to
> the
> firmware. Not sure this is what we want...
Backward compatibility is not only about avoiding breaking existing call
parameters and return values. It is also about not breaking the
semantics of the calls.
If a call is deemed guest safe (when a guest has a device assigned) so
Xen forwards the call to firmware, but then firmware introduces a new
parameter (before it was ignored) and with it, allows more extensive
operations that go beyong a single device, I think that is semantics
breakage.
In any case, this is almost impossible to do with EEMI because calls
take a node_id parameter or similar that identifies the area of the
effect of the operation, and we already check on the node_id to see if
the guest is allowed to make the call.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v5 4/7] xen: introduce mfn_init macro
2018-12-05 8:27 ` Jan Beulich
@ 2018-12-12 23:56 ` Stefano Stabellini
0 siblings, 0 replies; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-12 23:56 UTC (permalink / raw)
To: Jan Beulich
Cc: edgar.iglesias, Tim Deegan, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper,
Stefano Stabellini, Ian Jackson, xen-devel, Julien Grall,
saeed.nowshadi
On Wed, 5 Dec 2018, Jan Beulich wrote:
> >>> On 04.12.18 at 20:38, <sstabellini@kernel.org> wrote:
> > On Tue, 4 Dec 2018, Jan Beulich wrote:
> >> >>> On 03.12.18 at 22:03, <sstabellini@kernel.org> wrote:
> >> > To be used in constant initializations of mfn_t variables, such as:
> >> >
> >> > static mfn_t node = mfn_init(MM_ADDR);
> >> >
> >> > It is necessary because static inline functions cannot be used as static
> >> > initializers.
> >>
> >> We had been at this point once (quite some time ago), and got
> >> away without such an addition. Did you try to find that old
> >> discussion? Are there any new reasons to have such a construct?
> >> Do you need this for other than setting a value to INVALID_MFN,
> >> in which case INVALID_MFN_INITIALIZER ought to be suitable?
> >>
> >> This is not to say I'm entirely opposed.
> >>
> >> If we were to have such a construct, I wonder though whether
> >> mfn_init() is suitable as a name. Simply MFN() perhaps, and then
> >> also consistently have GFN() and DFN()?
> >
> > Hi Jan,
> >
> > I am happy with any name, and MFN() together with GFN() and DFN() look
> > like a good option.
> >
> > The reason why it is needed is that without it I cannot introduce a
> > statically initialized array of mfn_t type like the one in the following
> > patch in the series:
> >
> > +static const struct pm_access pm_node_access[] = {
> > + /* MM_RPU grants access to all RPU Nodes. */
> > + [NODE_RPU] = { mfn_init(MM_RPU) },
> > + [NODE_RPU_0] = { mfn_init(MM_RPU) },
> > + [NODE_RPU_1] = { mfn_init(MM_RPU) },
> > + [NODE_IPI_RPU_0] = { mfn_init(MM_RPU) },
> >
> > [...]
> >
> > Where MM_RPU is a mfn, and the NODE_* are IDs defined as enum:
> >
> > #define MM_RPU 0xff9a0
> >
> > enum pm_node_id {
> > NODE_RPU = 6,
> > NODE_RPU_0,
> > NODE_RPU_1,
> >
> > [...]
> >
> >
> > Originally I had:
> >
> > [NODE_RPU] = { MM_RPU },
> >
> > but I changed the type to be mfn_t to address one of Julien's comments.
> > You might get a better idea of the issue if you give a look at this
> > branch:
> >
> > http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git zynqmp-v5
>
> Well, I have to admit that I'd rather not see ways to embed hard-coded
> MFNs into code made available generically. May I suggest that you use
> a macro with a name to your liking just locally to that one source file?
OK, no problem
> As a side note, I'm also puzzled by there being entries in the table which
> don't have their MFNs specified. Oddly enough it looks as if
> .hwdom_access was true if and only if no MFN is specified.
Yes, the hwdom_access check could be turned into a check for
MFN(INVALID_MFN). I'll do that it will actually make the array size
smaller.
> The term "node" of course is confusing too, considering its NUMA
> meaning elsewhere in the hypervisor.
That comes from the EEMI firmware specification: they use the term
"node" to address a power domain "unit".
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v5 5/7] xen/arm: zynqmp: eemi access control
2018-12-11 15:37 ` Julien Grall
@ 2018-12-12 23:57 ` Stefano Stabellini
2018-12-13 15:58 ` Julien Grall
0 siblings, 1 reply; 32+ messages in thread
From: Stefano Stabellini @ 2018-12-12 23:57 UTC (permalink / raw)
To: Julien Grall
Cc: edgar.iglesias, Stefano Stabellini, Stefano Stabellini,
saeed.nowshadi, xen-devel
On Tue, 11 Dec 2018, Julien Grall wrote:
> Hi,
>
> On 03/12/2018 21:03, 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>
> >
> > ---
> > Statically defines:
> >
> > - pm_node_access
> > It encodes the relationship between a node id and the start of the MMIO
> > region of a device in the corresponding power domain. It is used for
> > permission checking. Although the MMIO region start address is available
> > on device tree and could be derived from there (we plan to improve that
> > in the future), the relationship between a node id and corresponding
> > devices is not described and needs to be hardcoded.
> >
> > - pm_reset_access
> > Same as pm_node_access for reset lines.
> >
> > ---
> > Changes in v5:
> > - improve in-code comments
> > - use mfn_t in struct pm_access
> > - remove mmio_access table
> >
> > Changes in v4:
> > - add #include as needed
> > - add #if 0 for bisectability
> > - use mfn_t in pm_check_access
> > - add wrap-around ASSERT in domain_has_mmio_access
> > - use GENMASK in domain_has_mmio_access
> > - proper bound checks (== ARRAY_SIZE is out of bound)
> > ---
> > xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 348
> > ++++++++++++++++++++++++++++
> > 1 file changed, 348 insertions(+)
> >
> > diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > index 369bb3f..92a02df 100644
> > --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > @@ -16,9 +16,357 @@
> > * 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().
> > + */
> > +
> > +#include <xen/iocap.h>
> > +#include <xen/sched.h>
> > #include <asm/regs.h>
> > #include <asm/platforms/xilinx-zynqmp-eemi.h>
> > +#if 0
> > +struct pm_access
> > +{
> > + mfn_t mfn;
> > + bool hwdom_access; /* HW domain gets access regardless. */
> > +};
> > +
> > +/*
> > + * This table maps a node into a memory address.
>
> Some of the nodes below don't have memory address. So this comment has to be
> updated.
Yes, I'll update and improve the comment
> > + * 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] = { mfn_init(MM_RPU) },
> > + [NODE_RPU_0] = { mfn_init(MM_RPU) },
> > + [NODE_RPU_1] = { mfn_init(MM_RPU) },
> > + [NODE_IPI_RPU_0] = { mfn_init(MM_RPU) },
> > +
> > + /* GPU nodes. */
> > + [NODE_GPU] = { mfn_init(MM_GPU) },
> > + [NODE_GPU_PP_0] = { mfn_init(MM_GPU) },
> > + [NODE_GPU_PP_1] = { mfn_init(MM_GPU) },
> > +
> > + [NODE_USB_0] = { mfn_init(MM_USB3_0_XHCI) },
> > + [NODE_USB_1] = { mfn_init(MM_USB3_1_XHCI) },
> > + [NODE_TTC_0] = { mfn_init(MM_TTC0) },
> > + [NODE_TTC_1] = { mfn_init(MM_TTC1) },
> > + [NODE_TTC_2] = { mfn_init(MM_TTC2) },
> > + [NODE_TTC_3] = { mfn_init(MM_TTC3) },
> > + [NODE_SATA] = { mfn_init(MM_SATA_AHCI_HBA) },
> > + [NODE_ETH_0] = { mfn_init(MM_GEM0) },
> > + [NODE_ETH_1] = { mfn_init(MM_GEM1) },
> > + [NODE_ETH_2] = { mfn_init(MM_GEM2) },
> > + [NODE_ETH_3] = { mfn_init(MM_GEM3) },
> > + [NODE_UART_0] = { mfn_init(MM_UART0) },
> > + [NODE_UART_1] = { mfn_init(MM_UART1) },
> > + [NODE_SPI_0] = { mfn_init(MM_SPI0) },
> > + [NODE_SPI_1] = { mfn_init(MM_SPI1) },
> > + [NODE_I2C_0] = { mfn_init(MM_I2C0) },
> > + [NODE_I2C_1] = { mfn_init(MM_I2C1) },
> > + [NODE_SD_0] = { mfn_init(MM_SD0) },
> > + [NODE_SD_1] = { mfn_init(MM_SD1) },
> > + [NODE_DP] = { mfn_init(MM_DP) },
> > +
> > + /* Guest with GDMA Channel 0 gets PM access. Other guests don't. */
> > + [NODE_GDMA] = { mfn_init(MM_GDMA_CH0) },
> > + /* Guest with ADMA Channel 0 gets PM access. Other guests don't. */
> > + [NODE_ADMA] = { mfn_init(MM_ADMA_CH0) },
> > +
> > + [NODE_NAND] = { mfn_init(MM_NAND) },
> > + [NODE_QSPI] = { mfn_init(MM_QSPI) },
> > + [NODE_GPIO] = { mfn_init(MM_GPIO) },
> > + [NODE_CAN_0] = { mfn_init(MM_CAN0) },
> > + [NODE_CAN_1] = { mfn_init(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] = { mfn_init(MM_PCIE_ATTRIB) },
> > + [NODE_RTC] = { mfn_init(MM_RTC) },
> > +};
> > +
> > +/*
> > + * This table maps reset line IDs into a memory address.
>
> Same here.
>
> > + * 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)] = { mfn_init(MM_AXIPCIE_MAIN)
> > },
> > + [XILPM_RESET_IDX(XILPM_RESET_PCIE_BRIDGE)] = { mfn_init(MM_PCIE_ATTRIB)
> > },
> > + [XILPM_RESET_IDX(XILPM_RESET_PCIE_CTRL)] = { mfn_init(MM_PCIE_ATTRIB)
> > },
> > +
> > + [XILPM_RESET_IDX(XILPM_RESET_DP)] = { mfn_init(MM_DP) },
> > + [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRF)] = { mfn_init(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)] = { mfn_init(MM_GDMA_CH0) },
> > + [XILPM_RESET_IDX(XILPM_RESET_GPU_PP1)] = { mfn_init(MM_GPU) },
> > + [XILPM_RESET_IDX(XILPM_RESET_GPU_PP0)] = { mfn_init(MM_GPU) },
> > + [XILPM_RESET_IDX(XILPM_RESET_GT)] = { .hwdom_access = true },
> > + [XILPM_RESET_IDX(XILPM_RESET_SATA)] = { mfn_init(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)] = { mfn_init(MM_GEM0) },
> > + [XILPM_RESET_IDX(XILPM_RESET_GEM1)] = { mfn_init(MM_GEM1) },
> > + [XILPM_RESET_IDX(XILPM_RESET_GEM2)] = { mfn_init(MM_GEM2) },
> > + [XILPM_RESET_IDX(XILPM_RESET_GEM3)] = { mfn_init(MM_GEM3) },
> > +
> > + [XILPM_RESET_IDX(XILPM_RESET_QSPI)] = { mfn_init(MM_QSPI) },
> > + [XILPM_RESET_IDX(XILPM_RESET_UART0)] = { mfn_init(MM_UART0) },
> > + [XILPM_RESET_IDX(XILPM_RESET_UART1)] = { mfn_init(MM_UART1) },
> > + [XILPM_RESET_IDX(XILPM_RESET_SPI0)] = { mfn_init(MM_SPI0) },
> > + [XILPM_RESET_IDX(XILPM_RESET_SPI1)] = { mfn_init(MM_SPI1) },
> > + [XILPM_RESET_IDX(XILPM_RESET_SDIO0)] = { mfn_init(MM_SD0) },
> > + [XILPM_RESET_IDX(XILPM_RESET_SDIO1)] = { mfn_init(MM_SD1) },
> > + [XILPM_RESET_IDX(XILPM_RESET_CAN0)] = { mfn_init(MM_CAN0) },
> > + [XILPM_RESET_IDX(XILPM_RESET_CAN1)] = { mfn_init(MM_CAN1) },
> > + [XILPM_RESET_IDX(XILPM_RESET_I2C0)] = { mfn_init(MM_I2C0) },
> > + [XILPM_RESET_IDX(XILPM_RESET_I2C1)] = { mfn_init(MM_I2C1) },
> > + [XILPM_RESET_IDX(XILPM_RESET_TTC0)] = { mfn_init(MM_TTC0) },
> > + [XILPM_RESET_IDX(XILPM_RESET_TTC1)] = { mfn_init(MM_TTC1) },
> > + [XILPM_RESET_IDX(XILPM_RESET_TTC2)] = { mfn_init(MM_TTC2) },
> > + [XILPM_RESET_IDX(XILPM_RESET_TTC3)] = { mfn_init(MM_TTC3) },
> > + [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRL)] = { mfn_init(MM_SWDT) },
> > + [XILPM_RESET_IDX(XILPM_RESET_NAND)] = { mfn_init(MM_NAND) },
> > + [XILPM_RESET_IDX(XILPM_RESET_ADMA)] = { mfn_init(MM_ADMA_CH0) },
> > + [XILPM_RESET_IDX(XILPM_RESET_GPIO)] = { mfn_init(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)] = { mfn_init(MM_RPU) },
> > + [XILPM_RESET_IDX(XILPM_RESET_RPU_R51)] = { mfn_init(MM_RPU) },
> > + [XILPM_RESET_IDX(XILPM_RESET_RPU_AMBA)] = { mfn_init(MM_RPU) },
> > + [XILPM_RESET_IDX(XILPM_RESET_OCM)] = { .hwdom_access = true },
> > + [XILPM_RESET_IDX(XILPM_RESET_RPU_PGE)] = { mfn_init(MM_RPU) },
> > +
> > + [XILPM_RESET_IDX(XILPM_RESET_USB0_CORERESET)] = {
> > mfn_init(MM_USB3_0_XHCI) },
> > + [XILPM_RESET_IDX(XILPM_RESET_USB0_HIBERRESET)] = {
> > mfn_init(MM_USB3_0_XHCI) },
> > + [XILPM_RESET_IDX(XILPM_RESET_USB0_APB)] = { mfn_init(MM_USB3_0_XHCI) },
> > +
> > + [XILPM_RESET_IDX(XILPM_RESET_USB1_CORERESET)] = {
> > mfn_init(MM_USB3_1_XHCI) },
> > + [XILPM_RESET_IDX(XILPM_RESET_USB1_HIBERRESET)] = {
> > mfn_init(MM_USB3_1_XHCI) },
> > + [XILPM_RESET_IDX(XILPM_RESET_USB1_APB)] = { mfn_init(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)] = { mfn_init(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)] = { mfn_init(MM_SWDT) },
> > + [XILPM_RESET_IDX(XILPM_RESET_FPD)] = { .hwdom_access = true },
> > + [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG1)] = { mfn_init(MM_RPU) },
> > + [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG0)] = { mfn_init(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)] = { mfn_init(MM_RPU) },
> > + [XILPM_RESET_IDX(XILPM_RESET_PS_ONLY)] = { .hwdom_access = true },
> > + [XILPM_RESET_IDX(XILPM_RESET_PL)] = { .hwdom_access = true },
> > +};
> > +
> > +static bool pm_check_access(const struct pm_access *acl, struct domain *d,
> > + uint32_t idx)
> > +{
> > + if ( acl[idx].hwdom_access && is_hardware_domain(d) )
> > + return true;
> > +
> > + if ( !mfn_x(acl[idx].mfn) )
>
> Technically 0 is a valid mfn. If you want to encode an invalid value then
> MFN_INVALID is safer.
>
> But what are you trying to prevent? Are the node IDs not allocated
> contiguously?
I improved the comments above now. But the idea is that a zero address
means nobody has access. None of those resources have actually a zero
address, so there are no risks of conflicts with any valid address zero.
Following a comment by Jan, I have also started using MFN_INVALID to
encode only dom0 has access, getting rid of .hwdom_access completely.
So in the next series:
- address -> regular check
- 0 -> NO
- INVALID_MFN -> dom0 only
> > + return false;
> > +
> > + return iomem_access_permitted(d, mfn_x(acl[idx].mfn),
> > mfn_x(acl[idx].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;
>
> I think you can make this code no rely at all on XILPM_RESET_PCIE_CFG by using
> XILPM_RESET_IDX.
>
> if ( XILPM_RESET_IDX(rst) >= ARRAY_SIZE(pm_reset_access) )
> return false;
>
> rst = XILPM_RESET_IDX(rst);
>
> We rely on the unsigned underflow to catch value below XILPM_RESET_PCIE_CFG
> and make the code less error prone to change the array without the code here.
Good idea, I'll do that
> > +
> > + 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++ )
> > + {
> > + ASSERT(pm_mmio_access[i].start + pm_mmio_access[i].size >=
> > + pm_mmio_access[i].start);
> > +
> > + 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 ?: GENMASK(31, 0);
>
> The field mask does not seem to exist.
This code is fully gone now
> > + break;
> > + }
> > +
> > + /*
> > + * Masking only applies to writes: values are safe to read, but not
> > + * all bits are writeable.
> > + */
> > + if ( write )
> > + *mask &= prot_mask;
> > +
> > + return ret;
> > +}
> > +#endif
> > +
> > bool zynqmp_eemi(struct cpu_user_regs *regs)
> > {
> > return false;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines
2018-12-12 23:55 ` Stefano Stabellini
@ 2018-12-13 15:50 ` Julien Grall
0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2018-12-13 15:50 UTC (permalink / raw)
To: Stefano Stabellini
Cc: edgar.iglesias, Stefano Stabellini, saeed.nowshadi, xen-devel
Hi Stefano,
On 12/12/18 11:55 PM, Stefano Stabellini wrote:
> On Wed, 12 Dec 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 11/12/2018 19:22, Stefano Stabellini wrote:
>>> On Tue, 11 Dec 2018, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 03/12/2018 21:03, Stefano Stabellini wrote:
>>>> What is the plan there?
>>>
>>> The plan is to extract the node_id from a power-domain node on device
>>> tree. Each device would have a phandler to link to the right
>>> power-domain node which contains a power-domain-id attribute. The
>>> power-domain-id attribute is the node_id here.
>>>
>>> The power-domain-id changes to the Xilinx MPSoC device tree are under
>>> discussion with the device tree community.
>>
>> If I understand correctly, we will never be able to remove the hardcoded
>> values in Xen. This is because some device-tree may not have the bindings. Am
>> I correct?
>
> If we want to support running on existing hardware and firmware releases,
> then you are correct. We won't be able to remove the hardcoded values.
> That ship has sailed, not much we can do about it.
>
> If in the future we decide to drop support for older firmware releases
> and ask users to update their firmare/devicetrees, then we'll be able to
> remove the hardcoded values. I think it is something we can consider.
> In fact, I would rather break compatibility than not providing support
> for basic power management functionalities at all.
With your suggestion this means that Xen and the firmware are tied
together. So you can't update Xen without updating the firmware.
We already ruled out that behavior for x-gene. See the partial revert
you did 420596c868 to revert back the quirk.
I don't think this is very different here. We are introducing a feature,
that we know will be broken afterwards unless you update your firmware.
It is not like it was not planned... We should aim to support most of
the official firmware unless there are a strong argument not to do.
Basic power management for other than Dom0 is not a valid enough reason
for me to break compatibility.
>
> I think it is better to introduce basic EEMI support now, even if in a
> couple of Xen releases from now we'll make the decision to drop the
> hardcoded values and require new Xilinx device trees. Xilinx users are
> used to updating firmware on these boards every 6 months, and it would
> beneficial for them to be able to take a Xen Project release rather than
> be forced to use a Xilinx Xen release to have support for power
> management. When the new bindings become available, as we introduce
> support for them in Xen, we can decide whether to keep the hardcoded
> values or whether we should take the opportunity to drop them.
This is the right moment to discuss about it. It will be too late once
we get merged. I don't want to see 2 solutions in Xen if we know that
bindings are work-in-progress.
I am happy to consider EEMI for Dom0. For the guests, it will have to
wait the device-tree bindings.
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] 32+ messages in thread
* Re: [PATCH v5 5/7] xen/arm: zynqmp: eemi access control
2018-12-12 23:57 ` Stefano Stabellini
@ 2018-12-13 15:58 ` Julien Grall
0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2018-12-13 15:58 UTC (permalink / raw)
To: Stefano Stabellini
Cc: edgar.iglesias, Stefano Stabellini, saeed.nowshadi, xen-devel
Hi Stefano,
On 12/12/18 11:57 PM, Stefano Stabellini wrote:
> On Tue, 11 Dec 2018, Julien Grall wrote:
>> Hi,
>>
>> On 03/12/2018 21:03, 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>
>>>
>>> ---
>>> Statically defines:
>>>
>>> - pm_node_access
>>> It encodes the relationship between a node id and the start of the MMIO
>>> region of a device in the corresponding power domain. It is used for
>>> permission checking. Although the MMIO region start address is available
>>> on device tree and could be derived from there (we plan to improve that
>>> in the future), the relationship between a node id and corresponding
>>> devices is not described and needs to be hardcoded.
>>>
>>> - pm_reset_access
>>> Same as pm_node_access for reset lines.
>>>
>>> ---
>>> Changes in v5:
>>> - improve in-code comments
>>> - use mfn_t in struct pm_access
>>> - remove mmio_access table
>>>
>>> Changes in v4:
>>> - add #include as needed
>>> - add #if 0 for bisectability
>>> - use mfn_t in pm_check_access
>>> - add wrap-around ASSERT in domain_has_mmio_access
>>> - use GENMASK in domain_has_mmio_access
>>> - proper bound checks (== ARRAY_SIZE is out of bound)
>>> ---
>>> xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 348
>>> ++++++++++++++++++++++++++++
>>> 1 file changed, 348 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> index 369bb3f..92a02df 100644
>>> --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> @@ -16,9 +16,357 @@
>>> * 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().
>>> + */
>>> +
>>> +#include <xen/iocap.h>
>>> +#include <xen/sched.h>
>>> #include <asm/regs.h>
>>> #include <asm/platforms/xilinx-zynqmp-eemi.h>
>>> +#if 0
>>> +struct pm_access
>>> +{
>>> + mfn_t mfn;
>>> + bool hwdom_access; /* HW domain gets access regardless. */
>>> +};
>>> +
>>> +/*
>>> + * This table maps a node into a memory address.
>>
>> Some of the nodes below don't have memory address. So this comment has to be
>> updated.
>
> Yes, I'll update and improve the comment
>
>
>>> + * 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] = { mfn_init(MM_RPU) },
>>> + [NODE_RPU_0] = { mfn_init(MM_RPU) },
>>> + [NODE_RPU_1] = { mfn_init(MM_RPU) },
>>> + [NODE_IPI_RPU_0] = { mfn_init(MM_RPU) },
>>> +
>>> + /* GPU nodes. */
>>> + [NODE_GPU] = { mfn_init(MM_GPU) },
>>> + [NODE_GPU_PP_0] = { mfn_init(MM_GPU) },
>>> + [NODE_GPU_PP_1] = { mfn_init(MM_GPU) },
>>> +
>>> + [NODE_USB_0] = { mfn_init(MM_USB3_0_XHCI) },
>>> + [NODE_USB_1] = { mfn_init(MM_USB3_1_XHCI) },
>>> + [NODE_TTC_0] = { mfn_init(MM_TTC0) },
>>> + [NODE_TTC_1] = { mfn_init(MM_TTC1) },
>>> + [NODE_TTC_2] = { mfn_init(MM_TTC2) },
>>> + [NODE_TTC_3] = { mfn_init(MM_TTC3) },
>>> + [NODE_SATA] = { mfn_init(MM_SATA_AHCI_HBA) },
>>> + [NODE_ETH_0] = { mfn_init(MM_GEM0) },
>>> + [NODE_ETH_1] = { mfn_init(MM_GEM1) },
>>> + [NODE_ETH_2] = { mfn_init(MM_GEM2) },
>>> + [NODE_ETH_3] = { mfn_init(MM_GEM3) },
>>> + [NODE_UART_0] = { mfn_init(MM_UART0) },
>>> + [NODE_UART_1] = { mfn_init(MM_UART1) },
>>> + [NODE_SPI_0] = { mfn_init(MM_SPI0) },
>>> + [NODE_SPI_1] = { mfn_init(MM_SPI1) },
>>> + [NODE_I2C_0] = { mfn_init(MM_I2C0) },
>>> + [NODE_I2C_1] = { mfn_init(MM_I2C1) },
>>> + [NODE_SD_0] = { mfn_init(MM_SD0) },
>>> + [NODE_SD_1] = { mfn_init(MM_SD1) },
>>> + [NODE_DP] = { mfn_init(MM_DP) },
>>> +
>>> + /* Guest with GDMA Channel 0 gets PM access. Other guests don't. */
>>> + [NODE_GDMA] = { mfn_init(MM_GDMA_CH0) },
>>> + /* Guest with ADMA Channel 0 gets PM access. Other guests don't. */
>>> + [NODE_ADMA] = { mfn_init(MM_ADMA_CH0) },
>>> +
>>> + [NODE_NAND] = { mfn_init(MM_NAND) },
>>> + [NODE_QSPI] = { mfn_init(MM_QSPI) },
>>> + [NODE_GPIO] = { mfn_init(MM_GPIO) },
>>> + [NODE_CAN_0] = { mfn_init(MM_CAN0) },
>>> + [NODE_CAN_1] = { mfn_init(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] = { mfn_init(MM_PCIE_ATTRIB) },
>>> + [NODE_RTC] = { mfn_init(MM_RTC) },
>>> +};
>>> +
>>> +/*
>>> + * This table maps reset line IDs into a memory address.
>>
>> Same here.
>>
>>> + * 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)] = { mfn_init(MM_AXIPCIE_MAIN)
>>> },
>>> + [XILPM_RESET_IDX(XILPM_RESET_PCIE_BRIDGE)] = { mfn_init(MM_PCIE_ATTRIB)
>>> },
>>> + [XILPM_RESET_IDX(XILPM_RESET_PCIE_CTRL)] = { mfn_init(MM_PCIE_ATTRIB)
>>> },
>>> +
>>> + [XILPM_RESET_IDX(XILPM_RESET_DP)] = { mfn_init(MM_DP) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRF)] = { mfn_init(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)] = { mfn_init(MM_GDMA_CH0) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_GPU_PP1)] = { mfn_init(MM_GPU) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_GPU_PP0)] = { mfn_init(MM_GPU) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_GT)] = { .hwdom_access = true },
>>> + [XILPM_RESET_IDX(XILPM_RESET_SATA)] = { mfn_init(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)] = { mfn_init(MM_GEM0) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_GEM1)] = { mfn_init(MM_GEM1) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_GEM2)] = { mfn_init(MM_GEM2) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_GEM3)] = { mfn_init(MM_GEM3) },
>>> +
>>> + [XILPM_RESET_IDX(XILPM_RESET_QSPI)] = { mfn_init(MM_QSPI) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_UART0)] = { mfn_init(MM_UART0) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_UART1)] = { mfn_init(MM_UART1) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_SPI0)] = { mfn_init(MM_SPI0) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_SPI1)] = { mfn_init(MM_SPI1) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_SDIO0)] = { mfn_init(MM_SD0) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_SDIO1)] = { mfn_init(MM_SD1) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_CAN0)] = { mfn_init(MM_CAN0) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_CAN1)] = { mfn_init(MM_CAN1) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_I2C0)] = { mfn_init(MM_I2C0) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_I2C1)] = { mfn_init(MM_I2C1) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_TTC0)] = { mfn_init(MM_TTC0) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_TTC1)] = { mfn_init(MM_TTC1) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_TTC2)] = { mfn_init(MM_TTC2) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_TTC3)] = { mfn_init(MM_TTC3) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRL)] = { mfn_init(MM_SWDT) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_NAND)] = { mfn_init(MM_NAND) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_ADMA)] = { mfn_init(MM_ADMA_CH0) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_GPIO)] = { mfn_init(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)] = { mfn_init(MM_RPU) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_RPU_R51)] = { mfn_init(MM_RPU) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_RPU_AMBA)] = { mfn_init(MM_RPU) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_OCM)] = { .hwdom_access = true },
>>> + [XILPM_RESET_IDX(XILPM_RESET_RPU_PGE)] = { mfn_init(MM_RPU) },
>>> +
>>> + [XILPM_RESET_IDX(XILPM_RESET_USB0_CORERESET)] = {
>>> mfn_init(MM_USB3_0_XHCI) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_USB0_HIBERRESET)] = {
>>> mfn_init(MM_USB3_0_XHCI) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_USB0_APB)] = { mfn_init(MM_USB3_0_XHCI) },
>>> +
>>> + [XILPM_RESET_IDX(XILPM_RESET_USB1_CORERESET)] = {
>>> mfn_init(MM_USB3_1_XHCI) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_USB1_HIBERRESET)] = {
>>> mfn_init(MM_USB3_1_XHCI) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_USB1_APB)] = { mfn_init(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)] = { mfn_init(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)] = { mfn_init(MM_SWDT) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_FPD)] = { .hwdom_access = true },
>>> + [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG1)] = { mfn_init(MM_RPU) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG0)] = { mfn_init(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)] = { mfn_init(MM_RPU) },
>>> + [XILPM_RESET_IDX(XILPM_RESET_PS_ONLY)] = { .hwdom_access = true },
>>> + [XILPM_RESET_IDX(XILPM_RESET_PL)] = { .hwdom_access = true },
>>> +};
>>> +
>>> +static bool pm_check_access(const struct pm_access *acl, struct domain *d,
>>> + uint32_t idx)
>>> +{
>>> + if ( acl[idx].hwdom_access && is_hardware_domain(d) )
>>> + return true;
>>> +
>>> + if ( !mfn_x(acl[idx].mfn) )
>>
>> Technically 0 is a valid mfn. If you want to encode an invalid value then
>> MFN_INVALID is safer.
>>
>> But what are you trying to prevent? Are the node IDs not allocated
>> contiguously?
>
> I improved the comments above now. But the idea is that a zero address
> means nobody has access.
The hardware domain has technically full access to the hardware and we
trust it enough to do the right things. So why would you need to deny
access here?
> None of those resources have actually a zero
> address, so there are no risks of conflicts with any valid address zero.
>
> Following a comment by Jan, I have also started using MFN_INVALID to
> encode only dom0 has access, getting rid of .hwdom_access completely.
>
> So in the next series:
>
> - address -> regular check
> - 0 -> NO
> - INVALID_MFN -> dom0 only
IHMO this is a confusing solution. I would actually prefer if we keep 2
fields but potentially inverting the condition for hwdom_access.
.guest_access = <boolean>
.mfn = <machine frame number>
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] 32+ messages in thread
* Re: [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi
2018-12-12 23:56 ` Stefano Stabellini
@ 2018-12-13 15:59 ` Julien Grall
0 siblings, 0 replies; 32+ messages in thread
From: Julien Grall @ 2018-12-13 15:59 UTC (permalink / raw)
To: Stefano Stabellini
Cc: edgar.iglesias, Stefano Stabellini, saeed.nowshadi, xen-devel
Hi,
On 12/12/18 11:56 PM, Stefano Stabellini wrote:
> On Wed, 12 Dec 2018, Julien Grall wrote:
>> On 11/12/2018 22:23, Stefano Stabellini wrote:
>>> On Tue, 11 Dec 2018, Julien Grall wrote:
>>>> Furthermore, you are forwarding unsanitized values to the firmware. For
>>>> instance, what would happen if the number of parameters of the call are
>>>> increased? How are you sure this will not open a hole?
>>>
>>> EEMI is backward compatible and the implementation is tested with Xen
>>> regularly. A change like the one you describe should be considered a
>>> backward compatibility breakage.
>>
>> I disagree, you can still make backward compatible. For instance, the new
>> parameters could be gated by a flag in an existing parameter. Another way is
>> Xilinx promise that non-existing arguments should always be 0 and then
>> re-purpose the value for non-zero case.
>>
>> While it is backward compatible, you may end up passing unsanitized value to
>> the
>> firmware. Not sure this is what we want...
>
> Backward compatibility is not only about avoiding breaking existing call
> parameters and return values. It is also about not breaking the
> semantics of the calls.
>
> If a call is deemed guest safe (when a guest has a device assigned) so
> Xen forwards the call to firmware, but then firmware introduces a new
> parameter (before it was ignored) and with it, allows more extensive
> operations that go beyong a single device, I think that is semantics
> breakage.
>
> In any case, this is almost impossible to do with EEMI because calls
> take a node_id parameter or similar that identifies the area of the
> effect of the operation, and we already check on the node_id to see if
> the guest is allowed to make the call.
Please write it down in the code so we know why we don't need to worry
on parameters.
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] 32+ messages in thread
end of thread, other threads:[~2018-12-13 15:59 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-03 21:02 [PATCH v5 0/7] zynqmp: Add forwarding of platform specific firmware calls Stefano Stabellini
2018-12-03 21:03 ` [PATCH v5 1/7] xen/arm: introduce platform_smc Stefano Stabellini
2018-12-03 21:07 ` Stefano Stabellini
2018-12-11 14:54 ` Julien Grall
2018-12-03 21:03 ` [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls Stefano Stabellini
2018-12-11 15:03 ` Julien Grall
2018-12-11 18:50 ` Stefano Stabellini
2018-12-11 19:09 ` Julien Grall
2018-12-11 20:19 ` Stefano Stabellini
2018-12-11 20:27 ` Julien Grall
2018-12-03 21:03 ` [PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines Stefano Stabellini
2018-12-11 15:21 ` Julien Grall
2018-12-11 19:22 ` Stefano Stabellini
2018-12-12 10:29 ` Julien Grall
2018-12-12 23:55 ` Stefano Stabellini
2018-12-13 15:50 ` Julien Grall
2018-12-03 21:03 ` [PATCH v5 4/7] xen: introduce mfn_init macro Stefano Stabellini
2018-12-04 10:25 ` Jan Beulich
2018-12-04 19:38 ` Stefano Stabellini
2018-12-05 8:27 ` Jan Beulich
2018-12-12 23:56 ` Stefano Stabellini
2018-12-03 21:03 ` [PATCH v5 5/7] xen/arm: zynqmp: eemi access control Stefano Stabellini
2018-12-11 15:37 ` Julien Grall
2018-12-12 23:57 ` Stefano Stabellini
2018-12-13 15:58 ` Julien Grall
2018-12-03 21:03 ` [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi Stefano Stabellini
2018-12-11 15:55 ` Julien Grall
2018-12-11 22:23 ` Stefano Stabellini
2018-12-12 10:48 ` Julien Grall
2018-12-12 23:56 ` Stefano Stabellini
2018-12-13 15:59 ` Julien Grall
2018-12-03 21:03 ` [PATCH v5 7/7] xen/arm: zynqmp: Remove blacklist of ZynqMP's PM node Stefano Stabellini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).