|
[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 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@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.
>
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |