Re: [Xen-devel] [RFC v2 5/6] xen/arm: zynqmp: Forward plaform specific firmware calls

[Julien Grall <julien.grall@xxxxxxx>]

Hi Edgar,

On 07/02/17 19:42, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxxx>
>
> Implement an EEMI mediator and forward platform specific
> firmware calls from guests to firmware.
>
> The EEMI mediator is responsible for implementing access
> controls modifying or blocking calls that try to operate
> on setup for devices that are not under the calling guest's
> control.
>
> EEMI:
> https://www.xilinx.com/support/documentation/user_guides/ug1200-eemi-api.pdf
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxxxxx>
> ---
>  xen/arch/arm/platforms/Makefile                    |   1 +
>  xen/arch/arm/platforms/xilinx-zynqmp-eemi.c        | 794 +++++++++++++++++++++
>  xen/arch/arm/platforms/xilinx-zynqmp.c             |  18 +
>  xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 337 +++++++++
>  xen/include/asm-arm/platforms/xilinx-zynqmp-mm.h   | 284 ++++++++
>  5 files changed, 1434 insertions(+)
>  create mode 100644 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>  create mode 100644 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
>  create mode 100644 xen/include/asm-arm/platforms/xilinx-zynqmp-mm.h
>
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index 49fa683..b58b71f 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_ARM_64) += seattle.o
>  obj-$(CONFIG_ARM_32) += sunxi.o
>  obj-$(CONFIG_ARM_64) += xgene-storm.o
>  obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
> +obj-$(CONFIG_ARM_64) += 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..c2c184d
> --- /dev/null
> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
As you introduce another file for Xilinx, it might be worth considering 
a new directory xilinx in the platforms.
> @@ -0,0 +1,794 @@
> +/*
> + * xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> + *
> + * Xilinx ZynqMP EEMI API mediator.
> + *
> + * Copyright (c) 2017 Xilinx Inc.
> + * Written by Edgar E. Iglesias <edgar.iglesias@xxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
We had a bad habit on ARM to use "either version 2 of the License, or 
(at your option) any later version". However Xen is GPLv2 only (see 
CONTRIBUTING). Could you update the license to:
"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.
> + */
> +
> +/*
> + * Mediator for the EEMI Power Mangement API.
s/Mangement/Management/

> + *
> + * 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
> + *
> + * Allthough the details of the setup are configurable, in the common case
s/Allthough/although/

> + * 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.
> + *
> + * Other calls are more problematic. Here're some:
I don't think you can the contraction "here're".

> + * * Power requests for nodes (i.e turn on or off a given device)
> + * * Reset line control (i.e reset a given device)
> + * * MMIO access (i.e directly access clock and reset registers)
> + *
> + * Power requests for nodes:
> + * In order to correctly mediate 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 a 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().
> + *
> + * Reset lines:
> + * Reset lines are identified by a list of identifiers that do not relate
> + * to device nodes. We can use the same approach here as for nodes, except
> + * that we need a separate table mapping reset lines to base addresses.
> + * This is implemented by pm_reset_access.
> + *
> + * MMIO access:
> + * These calls allow guests to access certain memory ranges. These ranges
> + * are typically protected for secure-world access only and also from
> + * certain masters only, so guests cannot access them directly.
> + * Registers within the memory regions affect certain nodes. In this case,
> + * our imput is an address and we map that address into a node. If the
> + * guest has ownership of that node, the access is allowed.
> + * Some registers contain bitfields and a single register may contain
> + * bits that affect multiple nodes.
> + *
> + * Some of the calls act on more global state. For example acting on
> + * NODE_PS, which affects many a lot of nodes. This higher level
> + * orchestrating is left for the hardware-domain only.
> + */
> +
> +#include <xen/iocap.h>
> +#include <asm/platform.h>
> +#include <asm/platforms/xilinx-zynqmp-eemi.h>
> +#include <asm/platforms/xilinx-zynqmp-mm.h>
> +
> +struct pm_access
> +{
> +    uint64_t addr;
Please use paddr_t here.

> +    bool hwdom_access;    /* HW domain gets access regardless.  */
> +};

[...]

> +/*
> + * 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.
> + */
> +static const struct {
> +    uint64_t start;
> +    uint64_t end;    /* Inclusive. If not set, single reg entry.  */
For both, s/uint64_t/paddr_t/

> +    uint32_t mask;   /* Zero means no mask, i.e all bits.  */
> +    enum pm_node_id node;
> +    bool hwdom_access;
> +    bool readonly;
> +} pm_mmio_access[] = {
Could not we find all those information from the device-tree?

> +static bool pm_check_access(const struct pm_access *acl, struct domain *d, int 
> idx)
> +{
> +    unsigned long mfn;
> +
> +    if ( acl[idx].hwdom_access && is_hardware_domain(d) )
> +        return true;
> +
> +    mfn = paddr_to_pfn(acl[idx].addr);
acl[idx].addr may not be page aligned, so is there any possibility to 
have a same page permitted in multiple domain?
> +    if ( !mfn )
> +        return false;
> +
> +    return iomem_access_permitted(d, mfn, mfn);
Who will give access to the MMIO region? Is it the toolstack via 
iomem="..."?
Also, giving access to the MMIO also means the guest will be able to map 
it. After reading the comment at the top of the file, I am not sure this 
is what you want.
> +}
> +
> +/* Check if a domain has access to a node.  */
> +static bool domain_has_node_access(struct domain *d, enum pm_node_id node)
> +{
> +    if ( node <= 0 || node > ARRAY_SIZE(pm_node_access) )
Some compiler will complain of the first check because the enum is 
always positive.
Also, what are you trying to check given the node is an enum and should 
fall into the array?
> +        return false;
> +
> +    /* NODE_UNKNOWN is treated as a wildcard.  */
> +    if ( node == NODE_UNKNOWN )
> +        return true;
Which means every domain will access to it. I think this is easily 
error-prone because NODE_UNKOWN is 0. So you may give access to anyone 
by mistake.
Looking at the pm_mmio_access list, my understanding is NODE_UNKNOWN 
will be used when only the hardware domain can access it or the region 
is read-only.
So I would use the read-only property to know whether any domain has access.

> +
> +    return pm_check_access(pm_node_access, d, node);
> +}
> +
> +/* Check if a domain has access to a reset line.  */
> +static bool domain_has_reset_access(struct domain *d, enum pm_reset rst)
> +{
> +    int rst_idx = PM_RESET_IDX(rst);
> +
> +    if ( rst_idx < 0 || rst_idx > ARRAY_SIZE(pm_reset_access) )
Same question as above for the bound check.

> +        return false;
> +
> +    return pm_check_access(pm_reset_access, d, rst_idx);
> +}
> +
> +/*
> + * 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_mediate_mmio_access(struct domain *d,
> +                                       bool write, uint64_t addr,
s/uint64_t/paddr_t/

> +                                       uint32_t *mask)
> +{
> +    unsigned int i;
> +    bool ret = false;
> +    uint32_t prot_mask = 0;
> +
> +    ASSERT(mask);
> +
> +    /*
> +     * 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++ ) {
> +        bool r;
> +
> +        /* Check if the address is OK.  */
> +        if ( pm_mmio_access[i].end ) {
> +            /* Memory range.  */
> +            if ( addr < pm_mmio_access[i].start )
> +                continue;
> +            if ( addr > pm_mmio_access[i].end )
> +                continue;
> +        } else {
> +            /* Single register.  */
Are the single register only a byte? If not, in the case of a region is 
it safe to access in a middle of a register?
> +            if ( addr != pm_mmio_access[i].start )
> +                continue;
> +        }
I would drop the single register case and use make .end = .start. This 
would avoid potentially mistake in the code.
If you are concern about the readability of the array, you could use a 
macro.
> +
> +        if ( write && pm_mmio_access[i].readonly )
> +            continue;
> +        if ( pm_mmio_access[i].hwdom_access && !is_hardware_domain(d) )
> +            continue;
> +
> +        /* Unlimited access is represented by a zero mask.  */
> +        ASSERT( pm_mmio_access[i].mask != 0xFFFFFFFF );
Why? Would not it make the logic easier to handle unlimited access using 
0xFFFFFFFF?
You could use macro for that purpose.

> +
> +        r = domain_has_node_access(d, pm_mmio_access[i].node);
> +        if ( r ) {
> +            /* We've got access to this reg (or parts of it).  */
> +            ret = true;
> +            /* Masking only applies to writes, so reads can exit here.  */
If you do that a guest could potentially read some memory it is not 
allowed. Don't you expect sensitive data in the MMIO?
> +            if ( !write )
> +                break;
> +
> +            /* Permit write access to selected bits.  */
> +            prot_mask |= pm_mmio_access[i].mask ? pm_mmio_access[i].mask : ~0;
> +            printk("match: mask=%x prot_mask=%x\n",
> +                   pm_mmio_access[i].mask, prot_mask);
> +            if ( prot_mask == 0xFFFFFFFF )
> +                break; /* Full access, we're done.  */
> +        } else {
> +            if ( !pm_mmio_access[i].mask ) {
> +                /*
> +                 * The entire reg is tied to a device that we don't have
> +                 * access to. No point in continuing.
> +                 */
I am not sure to understand this, above you loop over until you may get 
prot_mask full. But here you exist as soon as one mask may not be valid.
> +                return false;
> +            }
> +        }
> +    }
> +
> +    /* Masking only applies to writes.  */
> +    if ( write )
> +        *mask &= prot_mask;
Newline here please.

> +    return ret;
> +}
> +
> +bool zynqmp_eemi_mediate(register_t fid,
> +                         register_t a0,
> +                         register_t a1,
> +                         register_t a2,
> +                         register_t a3,
> +                         register_t a4,
> +                         register_t a5,
> +                         register_t *ret)
> +{
> +    bool is_mmio_write = false;
> +    unsigned int pm_fn = fid & 0xFFFF;
> +    uint32_t pm_arg[4];
> +
> +    /* Decode pm args.  */
> +    pm_arg[0] = a0;
> +    pm_arg[1] = a0 >> 32;
> +    pm_arg[2] = a1;
> +    pm_arg[3] = a1 >> 32;
> +
> +    ASSERT(ret);
> +
> +    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 which Xen will trap and act accordingly upon.
> +     */
> +    case PM_SELF_SUSPEND:
> +        ret[0] = PM_RET_SUCCESS;
> +        goto done;
> +
> +    case PM_GET_NODE_STATUS:
> +    /* API for PUs.  */
What PUs stand for?

> +    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, pm_arg[0]) ) {
Coding style:

if ( ... )
{

> +            printk("zynqmp-pm: fn=%d No access to node %d\n", pm_fn, 
> pm_arg[0]);
Printk is not rate-limited, this means a guest could flood Xen with 
error message if it does not have access to the region.
Please use gprintk() here, it will print the domain ID and also
rate-limit the log.

Also, pm_fn and pm_arg are unsigned, so please use %u.


> +            ret[0] = PM_RET_ERROR_ACCESS;
> +            goto done;
> +        }
> +        goto forward_to_fw;
> +
> +    case PM_RESET_ASSERT:
> +    case PM_RESET_GET_STATUS:
> +        if ( !domain_has_reset_access(current->domain, pm_arg[0]) ) {
Coding style.

> +            printk("zynqmp-pm: fn=%d No access to reset %d\n", pm_fn, 
> pm_arg[0]);
Same as above for the printk.

> +            ret[0] = PM_RET_ERROR_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_API_VERSION:
> +    case PM_GET_CHIPID:
> +        goto forward_to_fw;
> +
> +    /* Mediated MMIO access.  */
> +    case PM_MMIO_WRITE:
> +        is_mmio_write = true;
> +    /* Fallthrough.  */
> +    case PM_MMIO_READ:
> +        if ( !domain_mediate_mmio_access(current->domain,
> +                                         is_mmio_write,
> +                                         pm_arg[0], &pm_arg[1]) ) {
See above for the coding style.

> +            printk("eemi: fn=%d No access to MMIO %s %x\n",
> +                   pm_fn, is_mmio_write ? "write" : "read", pm_arg[0]);
See above for the printk. Also, please use %#x if you print hexa to not 
confuse with decimal. But why do you print pm_arg in hexa here and 
decimal above?
> +            ret[0] = PM_RET_ERROR_ACCESS;
> +            goto done;
> +        }
> +        goto forward_to_fw;
> +
> +    /* Exclusive to Dom0.  */
> +    case PM_INIT:
> +    case PM_SET_CONFIGURATION:
> +    case PM_FPGA_LOAD:
> +    case PM_FPGA_GET_STATUS:
> +        if ( !is_hardware_domain(current->domain) ) {
See above for the coding style.

> +            printk("eemi: fn=%d No access", pm_fn);
See above for the printk.

> +            ret[0] = PM_RET_ERROR_ACCESS;
> +            goto done;
> +        }
> +        goto forward_to_fw;
> +
> +    /* These calls are never allowed.  */
> +    case PM_SYSTEM_SHUTDOWN:
> +        ret[0] = PM_RET_ERROR_ACCESS;
> +        goto done;
> +
> +    default:
> +        printk("zynqmp-pm: Unhandled PM Call: %d\n", (u32)fid);
See above for the printk. Also why do you cast fid?

> +        return false;
> +    }
> +
> +forward_to_fw:
> +    /* Re-encode pm args.  */
> +    a0 = ((uint64_t)pm_arg[1] << 32) | pm_arg[0];
> +    a1 = ((uint64_t)pm_arg[3] << 32) | pm_arg[2];
> +    call_smcc64(fid, a0, a1, a2, a3, a4, a5, ret);
> +done:
> +    return true;
> +}
> +
> +/*
> + * 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 2adee91..d826282 100644
> --- a/xen/arch/arm/platforms/xilinx-zynqmp.c
> +++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
> @@ -18,6 +18,7 @@
>   */
>
>  #include <asm/platform.h>
> +#include <asm/platforms/xilinx-zynqmp-eemi.h>
>
>  static const char * const zynqmp_dt_compat[] __initconst =
>  {
> @@ -32,8 +33,25 @@ static const struct dt_device_match zynqmp_blacklist_dev[] 
> __initconst =
>      { /* sentinel */ },
>  };
>
> +bool zynqmp_hvc(struct cpu_user_regs *regs)
This function should be static.

> +{
> +    register_t ret[4] = { 0 };
> +
> +    if ( !zynqmp_eemi_mediate(regs->x0, regs->x1, regs->x2, regs->x3,
> +                              regs->x4, regs->x5, regs->x6, ret) )
> +        return false;
> +
> +    /* Transfer return values into guest registers.  */
> +    regs->x0 = ret[0];
> +    regs->x1 = ret[1];
> +    regs->x2 = ret[2];
> +    regs->x3 = ret[3];
Newline here please.

> +    return true;
> +}
> +
>  PLATFORM_START(xgene_storm, "Xilinx ZynqMP")
>      .compatible = zynqmp_dt_compat,
> +    .hvc = zynqmp_hvc,
>      .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..774593c
> --- /dev/null
> +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
[...]

> +/**
> + * @PM_RET_SUCCESS:            success
> + * @PM_RET_ERROR_ARGS:         illegal arguments provided
> + * @PM_RET_ERROR_ACCESS:       access rights violation
> + * @PM_RET_ERROR_TIMEOUT:      timeout in communication with PMU
> + * @PM_RET_ERROR_NOTSUPPORTED: feature not supported
> + * @PM_RET_ERROR_PROC:         node is not a processor node
> + * @PM_RET_ERROR_API_ID:       illegal API ID
> + * @PM_RET_ERROR_OTHER:                other error
> + */
> +enum pm_ret_status {
> +       PM_RET_SUCCESS,
I looked at the spec you provide in the commit message and I cannot find 
those name. Is it related to EEMI_SUCCESS? If so, I would prefer if we 
use the name from the spec.
Furthermore, it does not seem that values are provided in the spec.

> +       PM_RET_ERROR_ARGS,
> +       PM_RET_ERROR_ACCESS,
> +       PM_RET_ERROR_TIMEOUT,
> +       PM_RET_ERROR_NOTSUPPORTED,
> +       PM_RET_ERROR_PROC,
> +       PM_RET_ERROR_API_ID,
> +       PM_RET_ERROR_FAILURE,
> +       PM_RET_ERROR_COMMUNIC,
> +       PM_RET_ERROR_DOUBLEREQ,
> +       PM_RET_ERROR_OTHER,
> +};
[...]

> diff --git a/xen/include/asm-arm/platforms/xilinx-zynqmp-mm.h 
> b/xen/include/asm-arm/platforms/xilinx-zynqmp-mm.h
> new file mode 100644
> index 0000000..4fb1695
> --- /dev/null
> +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-mm.h
I feel a bit unsure to see that many hardcoded values in Xen. Is it the 
only way to retrieve the mmio/power information?
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel