|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 4/6] xen/arm: zynqmp: implement zynqmp_eemi
On Mon, 17 Dec 2018, Julien Grall wrote: > Hi, > > On 14/12/2018 19:11, Stefano Stabellini wrote: > > From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxxx> > > > > From: Edgar E. Iglesias <edgar.iglesias@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx> > > --- > > Changes in v6: > > - mmio_access removal moved to previous patch > > - forward to firmware mandatory smc32 calls > > - check that the function id belongs to the right range before > > proceeding > > - basic is_hardware_domain implementation for domain_has_node_access and > > domain_has_reset_access > > > > 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 | 180 > > +++++++++++++++++++++++++++- > > 1 file changed, 179 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c > > b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c > > index 369bb3f..e0456ae 100644 > > --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c > > +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c > > @@ -17,11 +17,189 @@ > > */ > > #include <asm/regs.h> > > +#include <xen/sched.h> > > +#include <asm/smccc.h> > > #include <asm/platforms/xilinx-zynqmp-eemi.h> > > +/* > > + * EEMI firmware API: > > + * > > https://www.xilinx.com/support/documentation/user_guides/ug1200-eemi-api.pdf > > + * > > + * Power domain node_ids identify the area of effect of the power > > + * management operations. They are the first parameter passed to power > > + * management EEMI calls. > > + * > > + * Reset IDs identify the area of effect of a reset operation. They are > > + * the first parameter passed to reset EEMI calls. > > + * > > + * For now, let the hardware domain access to all power domain nodes and > > + * all reset lines. In the future, we'll check for ownership of > > + * resources by specific virtual machines. > > + */ > > +static inline bool domain_has_node_access(struct domain *d, uint32_t > > nodeid) > > +{ > > + return is_hardware_domain(d); > > +} > > + > > +static inline bool domain_has_reset_access(struct domain *d, uint32_t rst) > > +{ > > + return is_hardware_domain(d); > > +} > > + > > bool zynqmp_eemi(struct cpu_user_regs *regs) > > { > > - return false; > > + struct arm_smccc_res res; > > + uint32_t fid = get_user_reg(regs, 0); > > + uint32_t nodeid; > > + unsigned int pm_fn; > > + enum pm_ret_status ret; > > + > > + /* Check for the mandatory SMC32 functions first */ > > + switch ( fid ) > > + { > > + case ARM_SMCCC_CALL_COUNT_FID(SIP): > > + case ARM_SMCCC_CALL_UID_FID(SIP): > > + case ARM_SMCCC_REVISION_FID(SIP): > > + goto forward_to_fw; > > + default: > > + break; > > + } > > + > > + /* EEMI calls are SMC64 SIP Fast Calls */ > > + if ( !(fid & ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, > > + ARM_SMCCC_CONV_64, > > + ARM_SMCCC_OWNER_SIP, > > + 0x0)) ) > > I am afraid this does not work as you expect. This only check that at least > one bit is set, it does not check the bits have the correct value. > > But this is merely a hack to avoid having everywhere: > > ARM_SMCCC_CALL_VAL(..., ..., ..., 0x0) > > This is mostly due to how you define the IDs. I can see two solutions > 1) Stop using the enum and define use IDs using > ARM_SMCCC_CALL_VAL(...) > 2) Introduce EEMI_FID to wrap the call and use everywhere > > 1) is probably the best but I am happy with 2) as well. This also has the > advantage to avoid handling SMCCC32 functions aside and match how the other > SMCC subsystem are implemented in Xen (see vpsci and vsmc). Yes, I can see how either 1) or 2) would improve the code. I'll choose 2) because because I kind of like the enums by now. It will also highlight a difference I hadn't even noticed myself: the IPI Mailbox calls (last patch) are actually ARM_SMCCC_CONV_32 calls. Good call on using the full FID. > > + { > > + ret = ARM_SMCCC_NOT_SUPPORTED; > > + goto done; > > + } > > + > > + nodeid = get_user_reg(regs, 1); > > + pm_fn = fid & 0xFFFF; > > + > > + switch ( pm_fn ) > > + { > > + /* > > + * We can't allow CPUs to suspend without Xen knowing about it. > > + * We accept but ignore the request and wait for the guest to issue > > + * a WFI or PSCI call which Xen will trap and act accordingly upon. > > + */ > > + case PM_SELF_SUSPEND: > > + ret = XST_PM_SUCCESS; > > + goto done; > > + > > + case PM_GET_NODE_STATUS: > > + /* API for PUs. */ > > + case PM_REQ_SUSPEND: > > + case PM_FORCE_POWERDOWN: > > + case PM_ABORT_SUSPEND: > > + case PM_REQ_WAKEUP: > > + case PM_SET_WAKEUP_SOURCE: > > + /* API for slaves. */ > > + case PM_REQ_NODE: > > + case PM_RELEASE_NODE: > > + case PM_SET_REQUIREMENT: > > + case PM_SET_MAX_LATENCY: > > + if ( !domain_has_node_access(current->domain, nodeid) ) > > + { > > + gprintk(XENLOG_WARNING, > > + "zynqmp-pm: fn=%u No access to node %u\n", pm_fn, > > nodeid); > > + ret = XST_PM_NO_ACCESS; > > + goto done; > > + } > > + goto forward_to_fw; > > + > > + case PM_RESET_ASSERT: > > + case PM_RESET_GET_STATUS: > > + if ( !domain_has_reset_access(current->domain, nodeid) ) > > + { > > + gprintk(XENLOG_WARNING, > > + "zynqmp-pm: fn=%u No access to reset %u\n", pm_fn, > > nodeid); > > + ret = XST_PM_NO_ACCESS; > > + goto done; > > + } > > + goto forward_to_fw; > > + > > + /* These calls are safe and always allowed. */ > > + case ZYNQMP_SIP_SVC_CALL_COUNT: > > + case ZYNQMP_SIP_SVC_UID: > > + case ZYNQMP_SIP_SVC_VERSION: > > + case PM_GET_TRUSTZONE_VERSION: > > + case PM_GET_API_VERSION: > > + case PM_GET_CHIPID: > > + goto forward_to_fw; > > + > > + /* 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; > > + } > > + > > +forward_to_fw: > > On the previous version, I have requested a comment in the code explaining why > forward the commands without sanity check. I added something above but I can add something here too and make it clearer. > > + arm_smccc_1_1_smc(get_user_reg(regs, 0), > > + get_user_reg(regs, 1), > > + get_user_reg(regs, 2), > > + get_user_reg(regs, 3), > > + get_user_reg(regs, 4), > > + get_user_reg(regs, 5), > > + get_user_reg(regs, 6), > > + get_user_reg(regs, 7), > > + &res); > > + > > + set_user_reg(regs, 0, res.a0); > > + set_user_reg(regs, 1, res.a1); > > + set_user_reg(regs, 2, res.a2); > > + set_user_reg(regs, 3, res.a3); > > + return true; > > + > > +done: > > + set_user_reg(regs, 0, ret); > > + return true; > > } > > /* > > > > Cheers, > > -- > Julien Grall > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |