|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi
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@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 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |