Re: [Xen-devel] [PATCH v6 4/6] xen/arm: zynqmp: implement zynqmp_eemi

[Julien Grall <julien.grall@xxxxxxx>]

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).
> +    {
> +        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.
> +    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