[PATCH v5 0/7] zynqmp: Add forwarding of platform specific firmware calls

[PATCH v5 0/7] zynqmp: Add forwarding of platform specific firmware calls

[Stefano Stabellini]

Hi all,

Compared to v4, I removed the amount of #defines and used mfn_t more
widely across the series. Some #defines remain, see my comments on patch
#3 and patch #5.

Cheers,

Stefano


The following changes since commit 162fc8295f31240dc3670190a91e9bbc03b0d7be:

  libxl: Restore scheduling parameters after migrate in best-effort fashion (2018-10-10 12:36:25 +0100)

are available in the git repository at:

  http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git zynqmp-v5

for you to fetch changes up to e981b3a81f2283f7377712d0784cd5ee5adc599c:

  xen/arm: zynqmp: Remove blacklist of ZynqMP's PM node (2018-12-03 13:00:40 -0800)

----------------------------------------------------------------
Edgar E. Iglesias (6):
      xen/arm: introduce platform_smc
      xen/arm: zynqmp: Forward plaform specific firmware calls
      xen/arm: zynqmp: introduce zynqmp specific defines
      xen/arm: zynqmp: eemi access control
      xen/arm: zynqmp: implement zynqmp_eemi
      xen/arm: zynqmp: Remove blacklist of ZynqMP's PM node

Stefano Stabellini (1):
      xen: introduce mfn_init macro

 xen/arch/arm/platform.c                            |   8 +
 xen/arch/arm/platforms/Makefile                    |   1 +
 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c        | 451 +++++++++++++++++++++
 xen/arch/arm/platforms/xilinx-zynqmp.c             |  15 +-
 xen/arch/arm/vsmc.c                                |   4 +
 xen/include/asm-arm/platform.h                     |   3 +
 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 323 +++++++++++++++
 xen/include/xen/mm.h                               |   6 +
 8 files changed, 805 insertions(+), 6 deletions(-)
 create mode 100644 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
 create mode 100644 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v5 1/7] xen/arm: introduce platform_smc

[Stefano Stabellini]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Introduce platform_smc as a way to handle firmware calls that Xen does
not know about in a platform specific way. This is particularly useful
for implementing the SiP (SoC implementation specific) service calls.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>

---
Changes in v4:
- add likely
---
 xen/arch/arm/platform.c        | 8 ++++++++
 xen/arch/arm/vsmc.c            | 4 ++++
 xen/include/asm-arm/platform.h | 3 +++
 3 files changed, 15 insertions(+)

diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index 6989e58..3426056 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -127,6 +127,14 @@ void platform_poweroff(void)
         platform->poweroff();
 }
 
+bool platform_smc(struct cpu_user_regs *regs)
+{
+    if ( likely(platform && platform->smc) )
+        return platform->smc(regs);
+
+    return false;
+}
+
 bool platform_has_quirk(uint32_t quirk)
 {
     uint32_t quirks = 0;
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index c4ccae6..c72b9a0 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -25,6 +25,7 @@
 #include <asm/smccc.h>
 #include <asm/traps.h>
 #include <asm/vpsci.h>
+#include <asm/platform.h>
 
 /* Number of functions currently supported by Hypervisor Service. */
 #define XEN_SMCCC_FUNCTION_COUNT 3
@@ -272,6 +273,9 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
         case ARM_SMCCC_OWNER_STANDARD:
             handled = handle_sssc(regs);
             break;
+        case ARM_SMCCC_OWNER_SIP:
+            handled = platform_smc(regs);
+            break;
         }
     }
 
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index 2591d7b..dc55b6d 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -26,6 +26,8 @@ struct platform_desc {
     void (*reset)(void);
     /* Platform power-off */
     void (*poweroff)(void);
+    /* Platform specific SMC handler */
+    bool (*smc)(struct cpu_user_regs *regs);
     /*
      * Platform quirks
      * Defined has a function because a platform can support multiple
@@ -55,6 +57,7 @@ int platform_cpu_up(int cpu);
 #endif
 void platform_reset(void);
 void platform_poweroff(void);
+bool platform_smc(struct cpu_user_regs *regs);
 bool platform_has_quirk(uint32_t quirk);
 bool platform_device_is_blacklisted(const struct dt_device_node *node);
 
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 1/7] xen/arm: introduce platform_smc

[Stefano Stabellini]

On Mon, 3 Dec 2018, Stefano Stabellini wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Sorry about this, I haven't found a way to deal with this problem
automatically with git and guilt yet.



> Introduce platform_smc as a way to handle firmware calls that Xen does
> not know about in a platform specific way. This is particularly useful
> for implementing the SiP (SoC implementation specific) service calls.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> 
> ---
> Changes in v4:
> - add likely
> ---
>  xen/arch/arm/platform.c        | 8 ++++++++
>  xen/arch/arm/vsmc.c            | 4 ++++
>  xen/include/asm-arm/platform.h | 3 +++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
> index 6989e58..3426056 100644
> --- a/xen/arch/arm/platform.c
> +++ b/xen/arch/arm/platform.c
> @@ -127,6 +127,14 @@ void platform_poweroff(void)
>          platform->poweroff();
>  }
>  
> +bool platform_smc(struct cpu_user_regs *regs)
> +{
> +    if ( likely(platform && platform->smc) )
> +        return platform->smc(regs);
> +
> +    return false;
> +}
> +
>  bool platform_has_quirk(uint32_t quirk)
>  {
>      uint32_t quirks = 0;
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index c4ccae6..c72b9a0 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -25,6 +25,7 @@
>  #include <asm/smccc.h>
>  #include <asm/traps.h>
>  #include <asm/vpsci.h>
> +#include <asm/platform.h>
>  
>  /* Number of functions currently supported by Hypervisor Service. */
>  #define XEN_SMCCC_FUNCTION_COUNT 3
> @@ -272,6 +273,9 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
>          case ARM_SMCCC_OWNER_STANDARD:
>              handled = handle_sssc(regs);
>              break;
> +        case ARM_SMCCC_OWNER_SIP:
> +            handled = platform_smc(regs);
> +            break;
>          }
>      }
>  
> diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
> index 2591d7b..dc55b6d 100644
> --- a/xen/include/asm-arm/platform.h
> +++ b/xen/include/asm-arm/platform.h
> @@ -26,6 +26,8 @@ struct platform_desc {
>      void (*reset)(void);
>      /* Platform power-off */
>      void (*poweroff)(void);
> +    /* Platform specific SMC handler */
> +    bool (*smc)(struct cpu_user_regs *regs);
>      /*
>       * Platform quirks
>       * Defined has a function because a platform can support multiple
> @@ -55,6 +57,7 @@ int platform_cpu_up(int cpu);
>  #endif
>  void platform_reset(void);
>  void platform_poweroff(void);
> +bool platform_smc(struct cpu_user_regs *regs);
>  bool platform_has_quirk(uint32_t quirk);
>  bool platform_device_is_blacklisted(const struct dt_device_node *node);
>  
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 1/7] xen/arm: introduce platform_smc

[Julien Grall]

Hi Stefano,

On 03/12/2018 21:03, Stefano Stabellini wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> 
> Introduce platform_smc as a way to handle firmware calls that Xen does
> not know about in a platform specific way. This is particularly useful
> for implementing the SiP (SoC implementation specific) service calls.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> 
> ---
> Changes in v4:
> - add likely
> ---
>   xen/arch/arm/platform.c        | 8 ++++++++
>   xen/arch/arm/vsmc.c            | 4 ++++
>   xen/include/asm-arm/platform.h | 3 +++
>   3 files changed, 15 insertions(+)
> 
> diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
> index 6989e58..3426056 100644
> --- a/xen/arch/arm/platform.c
> +++ b/xen/arch/arm/platform.c
> @@ -127,6 +127,14 @@ void platform_poweroff(void)
>           platform->poweroff();
>   }
>   
> +bool platform_smc(struct cpu_user_regs *regs)
> +{
> +    if ( likely(platform && platform->smc) )
> +        return platform->smc(regs);
> +
> +    return false;
> +}
> +
>   bool platform_has_quirk(uint32_t quirk)
>   {
>       uint32_t quirks = 0;
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index c4ccae6..c72b9a0 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -25,6 +25,7 @@
>   #include <asm/smccc.h>
>   #include <asm/traps.h>
>   #include <asm/vpsci.h>
> +#include <asm/platform.h>
>   
>   /* Number of functions currently supported by Hypervisor Service. */
>   #define XEN_SMCCC_FUNCTION_COUNT 3
> @@ -272,6 +273,9 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
>           case ARM_SMCCC_OWNER_STANDARD:
>               handled = handle_sssc(regs);
>               break;
> +        case ARM_SMCCC_OWNER_SIP:
> +            handled = platform_smc(regs);
> +            break;
>           }
>       }
>   
> diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
> index 2591d7b..dc55b6d 100644
> --- a/xen/include/asm-arm/platform.h
> +++ b/xen/include/asm-arm/platform.h
> @@ -26,6 +26,8 @@ struct platform_desc {
>       void (*reset)(void);
>       /* Platform power-off */
>       void (*poweroff)(void);
> +    /* Platform specific SMC handler */
> +    bool (*smc)(struct cpu_user_regs *regs);
>       /*
>        * Platform quirks
>        * Defined has a function because a platform can support multiple
> @@ -55,6 +57,7 @@ int platform_cpu_up(int cpu);
>   #endif
>   void platform_reset(void);
>   void platform_poweroff(void);
> +bool platform_smc(struct cpu_user_regs *regs);
>   bool platform_has_quirk(uint32_t quirk);
>   bool platform_device_is_blacklisted(const struct dt_device_node *node);
>   
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls

[Stefano Stabellini]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Introduce zynqmp_eemi: a function responsible for implementing access
controls over the firmware calls. Only calls that are allowed are
forwarded to the firmware.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>

---
Changes in v4:
- fix typo
- add header guard
- add emacs magic
- remove #includes that will only be used later
- add copyright notice to header
- remove SMCCC 1.1 check
---
 xen/arch/arm/platforms/Makefile                    |  1 +
 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c        | 34 ++++++++++++++++++++++
 xen/arch/arm/platforms/xilinx-zynqmp.c             | 11 +++++++
 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 30 +++++++++++++++++++
 4 files changed, 76 insertions(+)
 create mode 100644 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
 create mode 100644 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h

diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index a79bdb9..fe8e0c7 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -9,3 +9,4 @@ obj-y += sunxi.o
 obj-$(CONFIG_ARM_64) += thunderx.o
 obj-$(CONFIG_ARM_64) += xgene-storm.o
 obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
+obj-$(CONFIG_MPSOC_PLATFORM)  += 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..369bb3f
--- /dev/null
+++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
@@ -0,0 +1,34 @@
+/*
+ * xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
+ *
+ * Xilinx ZynqMP EEMI API
+ *
+ * Copyright (c) 2018 Xilinx Inc.
+ * Written by Edgar E. Iglesias <edgar.iglesias@xilinx.com>
+ *
+ * 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.
+ */
+
+#include <asm/regs.h>
+#include <asm/platforms/xilinx-zynqmp-eemi.h>
+
+bool zynqmp_eemi(struct cpu_user_regs *regs)
+{
+    return false;
+}
+
+/*
+ * 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 d8ceded..9c174d2 100644
--- a/xen/arch/arm/platforms/xilinx-zynqmp.c
+++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
@@ -18,6 +18,8 @@
  */
 
 #include <asm/platform.h>
+#include <asm/platforms/xilinx-zynqmp-eemi.h>
+#include <asm/smccc.h>
 
 static const char * const zynqmp_dt_compat[] __initconst =
 {
@@ -32,8 +34,17 @@ static const struct dt_device_match zynqmp_blacklist_dev[] __initconst =
     { /* sentinel */ },
 };
 
+static bool zynqmp_smc(struct cpu_user_regs *regs)
+{
+    if ( !is_64bit_domain(current->domain) )
+        return false;
+
+    return  zynqmp_eemi(regs);
+}
+
 PLATFORM_START(xilinx_zynqmp, "Xilinx ZynqMP")
     .compatible = zynqmp_dt_compat,
+    .smc = zynqmp_smc,
     .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..43cefb5
--- /dev/null
+++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
@@ -0,0 +1,30 @@
+/*
+ * Copyright (c) 2018 Xilinx Inc.
+ *
+ * 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.
+ */
+
+#ifndef __ASM_ARM_PLATFORMS_ZYNQMP_H
+#define __ASM_ASM_PLATFORMS_ZYNQMP_H
+
+#include <asm/processor.h>
+
+extern bool zynqmp_eemi(struct cpu_user_regs *regs);
+
+#endif /* __ASM_ARM_PLATFORMS_ZYNQMP_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls

[Julien Grall]

Hi Stefano,

On 03/12/2018 21:03, Stefano Stabellini wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> 
> Introduce zynqmp_eemi: a function responsible for implementing access
> controls over the firmware calls. Only calls that are allowed are
> forwarded to the firmware.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> 
> ---
> Changes in v4:
> - fix typo
> - add header guard
> - add emacs magic
> - remove #includes that will only be used later
> - add copyright notice to header
> - remove SMCCC 1.1 check
> ---
>   xen/arch/arm/platforms/Makefile                    |  1 +
>   xen/arch/arm/platforms/xilinx-zynqmp-eemi.c        | 34 ++++++++++++++++++++++
>   xen/arch/arm/platforms/xilinx-zynqmp.c             | 11 +++++++
>   xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 30 +++++++++++++++++++
>   4 files changed, 76 insertions(+)
>   create mode 100644 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>   create mode 100644 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> 
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index a79bdb9..fe8e0c7 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -9,3 +9,4 @@ obj-y += sunxi.o
>   obj-$(CONFIG_ARM_64) += thunderx.o
>   obj-$(CONFIG_ARM_64) += xgene-storm.o
>   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
> +obj-$(CONFIG_MPSOC_PLATFORM)  += 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..369bb3f
> --- /dev/null
> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> @@ -0,0 +1,34 @@
> +/*
> + * xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> + *
> + * Xilinx ZynqMP EEMI API
> + *
> + * Copyright (c) 2018 Xilinx Inc.
> + * Written by Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> + *
> + * 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.
> + */
> +
> +#include <asm/regs.h>
> +#include <asm/platforms/xilinx-zynqmp-eemi.h>
> +
> +bool zynqmp_eemi(struct cpu_user_regs *regs)
> +{
> +    return false;
> +}
> +
> +/*
> + * 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 d8ceded..9c174d2 100644
> --- a/xen/arch/arm/platforms/xilinx-zynqmp.c
> +++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
> @@ -18,6 +18,8 @@
>    */
>   
>   #include <asm/platform.h>
> +#include <asm/platforms/xilinx-zynqmp-eemi.h>
> +#include <asm/smccc.h>
>   
>   static const char * const zynqmp_dt_compat[] __initconst =
>   {
> @@ -32,8 +34,17 @@ static const struct dt_device_match zynqmp_blacklist_dev[] __initconst =
>       { /* sentinel */ },
>   };
>   
> +static bool zynqmp_smc(struct cpu_user_regs *regs)
> +{
> +    if ( !is_64bit_domain(current->domain) )

Please document why you only expose eemi to 64-bit domain. What if the user 
start with 32-bit Dom0?

> +        return false;
> +
> +    return  zynqmp_eemi(regs);

double space.

> +}
> +
>   PLATFORM_START(xilinx_zynqmp, "Xilinx ZynqMP")
>       .compatible = zynqmp_dt_compat,
> +    .smc = zynqmp_smc,
>       .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..43cefb5
> --- /dev/null
> +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> @@ -0,0 +1,30 @@
> +/*
> + * Copyright (c) 2018 Xilinx Inc.
> + *
> + * 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.
> + */
> +
> +#ifndef __ASM_ARM_PLATFORMS_ZYNQMP_H
> +#define __ASM_ASM_PLATFORMS_ZYNQMP_H
> +
> +#include <asm/processor.h>
> +
> +extern bool zynqmp_eemi(struct cpu_user_regs *regs);
> +
> +#endif /* __ASM_ARM_PLATFORMS_ZYNQMP_H */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls

[Stefano Stabellini]

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@xilinx.com>
> > 
> > From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > 
> > Introduce zynqmp_eemi: a function responsible for implementing access
> > controls over the firmware calls. Only calls that are allowed are
> > forwarded to the firmware.
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > 
> > ---
> > Changes in v4:
> > - fix typo
> > - add header guard
> > - add emacs magic
> > - remove #includes that will only be used later
> > - add copyright notice to header
> > - remove SMCCC 1.1 check
> > ---
> >   xen/arch/arm/platforms/Makefile                    |  1 +
> >   xen/arch/arm/platforms/xilinx-zynqmp-eemi.c        | 34
> > ++++++++++++++++++++++
> >   xen/arch/arm/platforms/xilinx-zynqmp.c             | 11 +++++++
> >   xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 30
> > +++++++++++++++++++
> >   4 files changed, 76 insertions(+)
> >   create mode 100644 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> >   create mode 100644 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > 
> > diff --git a/xen/arch/arm/platforms/Makefile
> > b/xen/arch/arm/platforms/Makefile
> > index a79bdb9..fe8e0c7 100644
> > --- a/xen/arch/arm/platforms/Makefile
> > +++ b/xen/arch/arm/platforms/Makefile
> > @@ -9,3 +9,4 @@ obj-y += sunxi.o
> >   obj-$(CONFIG_ARM_64) += thunderx.o
> >   obj-$(CONFIG_ARM_64) += xgene-storm.o
> >   obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
> > +obj-$(CONFIG_MPSOC_PLATFORM)  += 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..369bb3f
> > --- /dev/null
> > +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > @@ -0,0 +1,34 @@
> > +/*
> > + * xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > + *
> > + * Xilinx ZynqMP EEMI API
> > + *
> > + * Copyright (c) 2018 Xilinx Inc.
> > + * Written by Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > + *
> > + * 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.
> > + */
> > +
> > +#include <asm/regs.h>
> > +#include <asm/platforms/xilinx-zynqmp-eemi.h>
> > +
> > +bool zynqmp_eemi(struct cpu_user_regs *regs)
> > +{
> > +    return false;
> > +}
> > +
> > +/*
> > + * 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 d8ceded..9c174d2 100644
> > --- a/xen/arch/arm/platforms/xilinx-zynqmp.c
> > +++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
> > @@ -18,6 +18,8 @@
> >    */
> >     #include <asm/platform.h>
> > +#include <asm/platforms/xilinx-zynqmp-eemi.h>
> > +#include <asm/smccc.h>
> >     static const char * const zynqmp_dt_compat[] __initconst =
> >   {
> > @@ -32,8 +34,17 @@ static const struct dt_device_match
> > zynqmp_blacklist_dev[] __initconst =
> >       { /* sentinel */ },
> >   };
> >   +static bool zynqmp_smc(struct cpu_user_regs *regs)
> > +{
> > +    if ( !is_64bit_domain(current->domain) )
> 
> Please document why you only expose eemi to 64-bit domain. What if the user
> start with 32-bit Dom0?

I'll add a in-code comment saying that only 64-bit guests are supported
by the current implementation -- the 32 bit EEMI ABI is not yet covered.


> > +        return false;
> > +
> > +    return  zynqmp_eemi(regs);
> 
> double space.

I'll fix


> > +}
> > +
> >   PLATFORM_START(xilinx_zynqmp, "Xilinx ZynqMP")
> >       .compatible = zynqmp_dt_compat,
> > +    .smc = zynqmp_smc,
> >       .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..43cefb5
> > --- /dev/null
> > +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > @@ -0,0 +1,30 @@
> > +/*
> > + * Copyright (c) 2018 Xilinx Inc.
> > + *
> > + * 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.
> > + */
> > +
> > +#ifndef __ASM_ARM_PLATFORMS_ZYNQMP_H
> > +#define __ASM_ASM_PLATFORMS_ZYNQMP_H
> > +
> > +#include <asm/processor.h>
> > +
> > +extern bool zynqmp_eemi(struct cpu_user_regs *regs);
> > +
> > +#endif /* __ASM_ARM_PLATFORMS_ZYNQMP_H */
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls

[Julien Grall]

Hi Stefano,

On 11/12/2018 18:50, 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@xilinx.com>
>>>
>>> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>
>>> Introduce zynqmp_eemi: a function responsible for implementing access
>>> controls over the firmware calls. Only calls that are allowed are
>>> forwarded to the firmware.
>>>
>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>>
>>> ---
>>> Changes in v4:
>>> - fix typo
>>> - add header guard
>>> - add emacs magic
>>> - remove #includes that will only be used later
>>> - add copyright notice to header
>>> - remove SMCCC 1.1 check
>>> ---
>>>    xen/arch/arm/platforms/Makefile                    |  1 +
>>>    xen/arch/arm/platforms/xilinx-zynqmp-eemi.c        | 34
>>> ++++++++++++++++++++++
>>>    xen/arch/arm/platforms/xilinx-zynqmp.c             | 11 +++++++
>>>    xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 30
>>> +++++++++++++++++++
>>>    4 files changed, 76 insertions(+)
>>>    create mode 100644 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>    create mode 100644 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
>>>
>>> diff --git a/xen/arch/arm/platforms/Makefile
>>> b/xen/arch/arm/platforms/Makefile
>>> index a79bdb9..fe8e0c7 100644
>>> --- a/xen/arch/arm/platforms/Makefile
>>> +++ b/xen/arch/arm/platforms/Makefile
>>> @@ -9,3 +9,4 @@ obj-y += sunxi.o
>>>    obj-$(CONFIG_ARM_64) += thunderx.o
>>>    obj-$(CONFIG_ARM_64) += xgene-storm.o
>>>    obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
>>> +obj-$(CONFIG_MPSOC_PLATFORM)  += 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..369bb3f
>>> --- /dev/null
>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> @@ -0,0 +1,34 @@
>>> +/*
>>> + * xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> + *
>>> + * Xilinx ZynqMP EEMI API
>>> + *
>>> + * Copyright (c) 2018 Xilinx Inc.
>>> + * Written by Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +#include <asm/regs.h>
>>> +#include <asm/platforms/xilinx-zynqmp-eemi.h>
>>> +
>>> +bool zynqmp_eemi(struct cpu_user_regs *regs)
>>> +{
>>> +    return false;
>>> +}
>>> +
>>> +/*
>>> + * 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 d8ceded..9c174d2 100644
>>> --- a/xen/arch/arm/platforms/xilinx-zynqmp.c
>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
>>> @@ -18,6 +18,8 @@
>>>     */
>>>      #include <asm/platform.h>
>>> +#include <asm/platforms/xilinx-zynqmp-eemi.h>
>>> +#include <asm/smccc.h>
>>>      static const char * const zynqmp_dt_compat[] __initconst =
>>>    {
>>> @@ -32,8 +34,17 @@ static const struct dt_device_match
>>> zynqmp_blacklist_dev[] __initconst =
>>>        { /* sentinel */ },
>>>    };
>>>    +static bool zynqmp_smc(struct cpu_user_regs *regs)
>>> +{
>>> +    if ( !is_64bit_domain(current->domain) )
>>
>> Please document why you only expose eemi to 64-bit domain. What if the user
>> start with 32-bit Dom0?
> 
> I'll add a in-code comment saying that only 64-bit guests are supported
> by the current implementation -- the 32 bit EEMI ABI is not yet covered.

64-bit guests is allowed to use both SMC32 and SMC64. There are actually some 
call that can only be done using SMC32 convention (see patch #6). So why forbid 
32-bit domain when you allow 64-bit using SMC32?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls

[Stefano Stabellini]

On Tue, 11 Dec 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 11/12/2018 18:50, 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@xilinx.com>
> > > > 
> > > > From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > > > 
> > > > Introduce zynqmp_eemi: a function responsible for implementing access
> > > > controls over the firmware calls. Only calls that are allowed are
> > > > forwarded to the firmware.
> > > > 
> > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > > > 
> > > > ---
> > > > Changes in v4:
> > > > - fix typo
> > > > - add header guard
> > > > - add emacs magic
> > > > - remove #includes that will only be used later
> > > > - add copyright notice to header
> > > > - remove SMCCC 1.1 check
> > > > ---
> > > >    xen/arch/arm/platforms/Makefile                    |  1 +
> > > >    xen/arch/arm/platforms/xilinx-zynqmp-eemi.c        | 34
> > > > ++++++++++++++++++++++
> > > >    xen/arch/arm/platforms/xilinx-zynqmp.c             | 11 +++++++
> > > >    xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 30
> > > > +++++++++++++++++++
> > > >    4 files changed, 76 insertions(+)
> > > >    create mode 100644 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > > >    create mode 100644 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > > > 
> > > > diff --git a/xen/arch/arm/platforms/Makefile
> > > > b/xen/arch/arm/platforms/Makefile
> > > > index a79bdb9..fe8e0c7 100644
> > > > --- a/xen/arch/arm/platforms/Makefile
> > > > +++ b/xen/arch/arm/platforms/Makefile
> > > > @@ -9,3 +9,4 @@ obj-y += sunxi.o
> > > >    obj-$(CONFIG_ARM_64) += thunderx.o
> > > >    obj-$(CONFIG_ARM_64) += xgene-storm.o
> > > >    obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
> > > > +obj-$(CONFIG_MPSOC_PLATFORM)  += 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..369bb3f
> > > > --- /dev/null
> > > > +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > > > @@ -0,0 +1,34 @@
> > > > +/*
> > > > + * xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > > > + *
> > > > + * Xilinx ZynqMP EEMI API
> > > > + *
> > > > + * Copyright (c) 2018 Xilinx Inc.
> > > > + * Written by Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > > > + *
> > > > + * 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.
> > > > + */
> > > > +
> > > > +#include <asm/regs.h>
> > > > +#include <asm/platforms/xilinx-zynqmp-eemi.h>
> > > > +
> > > > +bool zynqmp_eemi(struct cpu_user_regs *regs)
> > > > +{
> > > > +    return false;
> > > > +}
> > > > +
> > > > +/*
> > > > + * 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 d8ceded..9c174d2 100644
> > > > --- a/xen/arch/arm/platforms/xilinx-zynqmp.c
> > > > +++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
> > > > @@ -18,6 +18,8 @@
> > > >     */
> > > >      #include <asm/platform.h>
> > > > +#include <asm/platforms/xilinx-zynqmp-eemi.h>
> > > > +#include <asm/smccc.h>
> > > >      static const char * const zynqmp_dt_compat[] __initconst =
> > > >    {
> > > > @@ -32,8 +34,17 @@ static const struct dt_device_match
> > > > zynqmp_blacklist_dev[] __initconst =
> > > >        { /* sentinel */ },
> > > >    };
> > > >    +static bool zynqmp_smc(struct cpu_user_regs *regs)
> > > > +{
> > > > +    if ( !is_64bit_domain(current->domain) )
> > > 
> > > Please document why you only expose eemi to 64-bit domain. What if the
> > > user
> > > start with 32-bit Dom0?
> > 
> > I'll add a in-code comment saying that only 64-bit guests are supported
> > by the current implementation -- the 32 bit EEMI ABI is not yet covered.
> 
> 64-bit guests is allowed to use both SMC32 and SMC64. There are actually some
> call that can only be done using SMC32 convention (see patch #6). So why
> forbid 32-bit domain when you allow 64-bit using SMC32?

I understand. It really looks like this check is a mistake. In fact, it
doesn't matter if a domain is 32bit or 64bit, the only thing that
matters for this code is if the EEMI calls are SMC32 and SMC64. So
instead of is_64bit_domain, I should check that SMC64 is used. Right?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 2/7] xen/arm: zynqmp: Forward plaform specific firmware calls

[Julien Grall]

On 11/12/2018 20:19, Stefano Stabellini wrote:
> On Tue, 11 Dec 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 11/12/2018 18:50, 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@xilinx.com>
>>>>>
>>>>> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>>>
>>>>> Introduce zynqmp_eemi: a function responsible for implementing access
>>>>> controls over the firmware calls. Only calls that are allowed are
>>>>> forwarded to the firmware.
>>>>>
>>>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>>>>
>>>>> ---
>>>>> Changes in v4:
>>>>> - fix typo
>>>>> - add header guard
>>>>> - add emacs magic
>>>>> - remove #includes that will only be used later
>>>>> - add copyright notice to header
>>>>> - remove SMCCC 1.1 check
>>>>> ---
>>>>>     xen/arch/arm/platforms/Makefile                    |  1 +
>>>>>     xen/arch/arm/platforms/xilinx-zynqmp-eemi.c        | 34
>>>>> ++++++++++++++++++++++
>>>>>     xen/arch/arm/platforms/xilinx-zynqmp.c             | 11 +++++++
>>>>>     xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 30
>>>>> +++++++++++++++++++
>>>>>     4 files changed, 76 insertions(+)
>>>>>     create mode 100644 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>>>     create mode 100644 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
>>>>>
>>>>> diff --git a/xen/arch/arm/platforms/Makefile
>>>>> b/xen/arch/arm/platforms/Makefile
>>>>> index a79bdb9..fe8e0c7 100644
>>>>> --- a/xen/arch/arm/platforms/Makefile
>>>>> +++ b/xen/arch/arm/platforms/Makefile
>>>>> @@ -9,3 +9,4 @@ obj-y += sunxi.o
>>>>>     obj-$(CONFIG_ARM_64) += thunderx.o
>>>>>     obj-$(CONFIG_ARM_64) += xgene-storm.o
>>>>>     obj-$(CONFIG_MPSOC_PLATFORM)  += xilinx-zynqmp.o
>>>>> +obj-$(CONFIG_MPSOC_PLATFORM)  += 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..369bb3f
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>>> @@ -0,0 +1,34 @@
>>>>> +/*
>>>>> + * xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>>>> + *
>>>>> + * Xilinx ZynqMP EEMI API
>>>>> + *
>>>>> + * Copyright (c) 2018 Xilinx Inc.
>>>>> + * Written by Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>>> + *
>>>>> + * 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.
>>>>> + */
>>>>> +
>>>>> +#include <asm/regs.h>
>>>>> +#include <asm/platforms/xilinx-zynqmp-eemi.h>
>>>>> +
>>>>> +bool zynqmp_eemi(struct cpu_user_regs *regs)
>>>>> +{
>>>>> +    return false;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * 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 d8ceded..9c174d2 100644
>>>>> --- a/xen/arch/arm/platforms/xilinx-zynqmp.c
>>>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
>>>>> @@ -18,6 +18,8 @@
>>>>>      */
>>>>>       #include <asm/platform.h>
>>>>> +#include <asm/platforms/xilinx-zynqmp-eemi.h>
>>>>> +#include <asm/smccc.h>
>>>>>       static const char * const zynqmp_dt_compat[] __initconst =
>>>>>     {
>>>>> @@ -32,8 +34,17 @@ static const struct dt_device_match
>>>>> zynqmp_blacklist_dev[] __initconst =
>>>>>         { /* sentinel */ },
>>>>>     };
>>>>>     +static bool zynqmp_smc(struct cpu_user_regs *regs)
>>>>> +{
>>>>> +    if ( !is_64bit_domain(current->domain) )
>>>>
>>>> Please document why you only expose eemi to 64-bit domain. What if the
>>>> user
>>>> start with 32-bit Dom0?
>>>
>>> I'll add a in-code comment saying that only 64-bit guests are supported
>>> by the current implementation -- the 32 bit EEMI ABI is not yet covered.
>>
>> 64-bit guests is allowed to use both SMC32 and SMC64. There are actually some
>> call that can only be done using SMC32 convention (see patch #6). So why
>> forbid 32-bit domain when you allow 64-bit using SMC32?
> 
> I understand. It really looks like this check is a mistake. In fact, it
> doesn't matter if a domain is 32bit or 64bit, the only thing that
> matters for this code is if the EEMI calls are SMC32 and SMC64. So
> instead of is_64bit_domain, I should check that SMC64 is used. Right?

This would be wrong because you at least need to handle UID, Call count 
and version using SMC32 as mandated by the SMCCC.

The right solution is to deal with the full identifier in patch #6 
rather than just the function ID.

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines

[Stefano Stabellini]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Introduce zynqmp specific defines for the firmware calls.
See EEMI:
https://www.xilinx.com/support/documentation/user_guides/ug1200-eemi-api.pdf

The error codes are described, under XIlPM Error Codes:
https://www.xilinx.com/support/documentation/user_guides/ug1137-zynq-ultrascale-mpsoc-swdev.pdf

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>

---

Although the amount of #defines has been significantly reduced in v5,
there is still a significant amount of static definitions:

- MM_*
These are the MMIO addresses of each resource to do permission checks.
Technically, they are also present on device tree, but using device tree
to get the mmio regions is not simple and not done in this series.
Xilinx plan to send a patch series in the future to extend the EEMI
functionalities and as part of that work, more device tree based
permission checking will be done, solving this problem. These
definitions could also be removed if we used a trivial "if dom0 -> yes,
otherwise no" permission checking.

- pm_api_id
These are the EEMI function IDs. Unavoidable.

- pm_ret_status
These are the EEMI return statuses. Unavoidable.

- pm_node_id
These are the EEMI function parameters for power management operations.
Today, it is not possible to get them from device tree as there is no
such information there. Even in the future when we add more power
domains info to device tree, the EEMI function parameters might remain
unique and different, requiring a table like this one.

- pm_reset
These are the EEMI function parameters for reset operations. Same as
pm_node_id.

---
Changes in v5:
- remove MMIO access related definitions

Changes in v4:
- define PM_MMIO_SHIFT
---
 xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 293 +++++++++++++++++++++
 1 file changed, 293 insertions(+)

diff --git a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
index 43cefb5..f6ad03b 100644
--- a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
+++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
@@ -16,6 +16,299 @@
 
 #include <asm/processor.h>
 
+#define MM_RPU	0xff9a0
+#define MM_RTC	0xffa60
+#define MM_ADMA_CH0	0xffa80
+
+#define MM_USB3_0_XHCI  0xfe200
+#define MM_USB3_1_XHCI  0xfe300
+
+#define MM_SATA_AHCI_HBA	0xfd0c0
+#define MM_AXIPCIE_MAIN	0xfd0e0
+#define MM_CRF_APB	0xfd1a0000
+#define MM_PCIE_ATTRIB	0xfd480
+#define MM_DP	0xfd4a0
+#define MM_GPU	0xfd4b0
+#define MM_GDMA_CH0	0xfd500
+
+#define MM_UART0	0xff000
+#define MM_UART1	0xff010
+#define MM_I2C0	0xff020
+#define MM_I2C1	0xff030
+#define MM_SPI0	0xff040
+#define MM_SPI1	0xff050
+#define MM_CAN0	0xff060
+#define MM_CAN1	0xff070
+#define MM_GPIO	0xff0a0
+#define MM_GEM0	0xff0b0
+#define MM_GEM1	0xff0c0
+#define MM_GEM2	0xff0d0
+#define MM_GEM3	0xff0e0
+#define MM_QSPI	0xff0f0
+#define MM_NAND	0xff100
+#define MM_TTC0	0xff110
+#define MM_TTC1	0xff120
+#define MM_TTC2	0xff130
+#define MM_TTC3	0xff140
+#define MM_SWDT	0xff150
+#define MM_SD0	0xff160
+#define MM_SD1	0xff170
+
+/* Service calls.  */
+#define PM_GET_TRUSTZONE_VERSION	0xa03
+
+/* SMC function IDs for SiP Service queries */
+#define ZYNQMP_SIP_SVC_CALL_COUNT       0xff00
+#define ZYNQMP_SIP_SVC_UID              0xff01
+#define ZYNQMP_SIP_SVC_VERSION          0xff03
+
+#define PM_MMIO_SHIFT                   32
+
+enum pm_api_id {
+	/* Miscellaneous API functions: */
+	PM_GET_API_VERSION = 1, /* Do not change or move */
+	PM_SET_CONFIGURATION,
+	PM_GET_NODE_STATUS,
+	PM_GET_OP_CHARACTERISTIC,
+	PM_REGISTER_NOTIFIER,
+	/* API for suspending of PUs: */
+	PM_REQ_SUSPEND,
+	PM_SELF_SUSPEND,
+	PM_FORCE_POWERDOWN,
+	PM_ABORT_SUSPEND,
+	PM_REQ_WAKEUP,
+	PM_SET_WAKEUP_SOURCE,
+	PM_SYSTEM_SHUTDOWN,
+	/* API for managing PM slaves: */
+	PM_REQ_NODE,
+	PM_RELEASE_NODE,
+	PM_SET_REQUIREMENT,
+	PM_SET_MAX_LATENCY,
+	/* Direct control API functions: */
+	PM_RESET_ASSERT,
+	PM_RESET_GET_STATUS,
+	PM_MMIO_WRITE,
+	PM_MMIO_READ,
+	PM_INIT,
+	PM_FPGA_LOAD,
+	PM_FPGA_GET_STATUS,
+	PM_GET_CHIPID,
+	/* ID 25 is been used by U-boot to process secure boot images */
+	/* Secure library generic API functions */
+	PM_SECURE_SHA = 26,
+	PM_SECURE_RSA,
+	/* Pin control API functions */
+	PM_PINCTRL_REQUEST,
+	PM_PINCTRL_RELEASE,
+	PM_PINCTRL_GET_FUNCTION,
+	PM_PINCTRL_SET_FUNCTION,
+	PM_PINCTRL_CONFIG_PARAM_GET,
+	PM_PINCTRL_CONFIG_PARAM_SET,
+	/* PM IOCTL API */
+	PM_IOCTL,
+	/* API to query information from firmware */
+	PM_QUERY_DATA,
+	/* Clock control API functions */
+	PM_CLOCK_ENABLE,
+	PM_CLOCK_DISABLE,
+	PM_CLOCK_GETSTATE,
+	PM_CLOCK_SETDIVIDER,
+	PM_CLOCK_GETDIVIDER,
+	PM_CLOCK_SETRATE,
+	PM_CLOCK_GETRATE,
+	PM_CLOCK_SETPARENT,
+	PM_CLOCK_GETPARENT,
+	PM_API_MAX
+};
+
+enum pm_node_id {
+	NODE_RPU = 6,
+	NODE_RPU_0,
+	NODE_RPU_1,
+	NODE_GPU_PP_0 = 20,
+	NODE_GPU_PP_1,
+	NODE_USB_0,
+	NODE_USB_1,
+	NODE_TTC_0,
+	NODE_TTC_1,
+	NODE_TTC_2,
+	NODE_TTC_3,
+	NODE_SATA,
+	NODE_ETH_0,
+	NODE_ETH_1,
+	NODE_ETH_2,
+	NODE_ETH_3,
+	NODE_UART_0,
+	NODE_UART_1,
+	NODE_SPI_0,
+	NODE_SPI_1,
+	NODE_I2C_0,
+	NODE_I2C_1,
+	NODE_SD_0,
+	NODE_SD_1,
+	NODE_DP,
+	NODE_GDMA,
+	NODE_ADMA,
+	NODE_NAND,
+	NODE_QSPI,
+	NODE_GPIO,
+	NODE_CAN_0,
+	NODE_CAN_1,
+	NODE_AFI,
+	NODE_APLL,
+	NODE_VPLL,
+	NODE_DPLL,
+	NODE_RPLL,
+	NODE_IOPLL,
+	NODE_DDR,
+	NODE_IPI_APU,
+	NODE_IPI_RPU_0,
+	NODE_GPU,
+	NODE_PCIE,
+	NODE_PCAP,
+	NODE_RTC,
+};
+
+/**
+ * @XST_PM_SUCCESS:		Success
+ * @XST_PM_INTERNAL:	Unexpected error
+ * @XST_PM_CONFLICT:	Conflicting requirements
+ * @XST_PM_NO_ACCESS:	Access rights violation
+ * @XST_PM_INVALID_NODE:	Does not apply to node passed as argument
+ * @XST_PM_DOUBLE_REQ:	Duplicate request
+ * @XST_PM_ABORT_SUSPEND:	Target has aborted suspend
+ */
+enum pm_ret_status {
+	XST_PM_SUCCESS = 0,
+	XST_PM_INTERNAL = 2000,
+	XST_PM_CONFLICT,
+	XST_PM_NO_ACCESS,
+	XST_PM_INVALID_NODE,
+	XST_PM_DOUBLE_REQ,
+	XST_PM_ABORT_SUSPEND,
+};
+
+enum pm_reset {
+	XILPM_RESET_START = 999,
+	XILPM_RESET_PCIE_CFG,
+	XILPM_RESET_PCIE_BRIDGE,
+	XILPM_RESET_PCIE_CTRL,
+	XILPM_RESET_DP,
+	XILPM_RESET_SWDT_CRF,
+	XILPM_RESET_AFI_FM5,
+	XILPM_RESET_AFI_FM4,
+	XILPM_RESET_AFI_FM3,
+	XILPM_RESET_AFI_FM2,
+	XILPM_RESET_AFI_FM1,
+	XILPM_RESET_AFI_FM0,
+	XILPM_RESET_GDMA,
+	XILPM_RESET_GPU_PP1,
+	XILPM_RESET_GPU_PP0,
+	XILPM_RESET_GPU,
+	XILPM_RESET_GT,
+	XILPM_RESET_SATA,
+	XILPM_RESET_ACPU3_PWRON,
+	XILPM_RESET_ACPU2_PWRON,
+	XILPM_RESET_ACPU1_PWRON,
+	XILPM_RESET_ACPU0_PWRON,
+	XILPM_RESET_APU_L2,
+	XILPM_RESET_ACPU3,
+	XILPM_RESET_ACPU2,
+	XILPM_RESET_ACPU1,
+	XILPM_RESET_ACPU0,
+	XILPM_RESET_DDR,
+	XILPM_RESET_APM_FPD,
+	XILPM_RESET_SOFT,
+	XILPM_RESET_GEM0,
+	XILPM_RESET_GEM1,
+	XILPM_RESET_GEM2,
+	XILPM_RESET_GEM3,
+	XILPM_RESET_QSPI,
+	XILPM_RESET_UART0,
+	XILPM_RESET_UART1,
+	XILPM_RESET_SPI0,
+	XILPM_RESET_SPI1,
+	XILPM_RESET_SDIO0,
+	XILPM_RESET_SDIO1,
+	XILPM_RESET_CAN0,
+	XILPM_RESET_CAN1,
+	XILPM_RESET_I2C0,
+	XILPM_RESET_I2C1,
+	XILPM_RESET_TTC0,
+	XILPM_RESET_TTC1,
+	XILPM_RESET_TTC2,
+	XILPM_RESET_TTC3,
+	XILPM_RESET_SWDT_CRL,
+	XILPM_RESET_NAND,
+	XILPM_RESET_ADMA,
+	XILPM_RESET_GPIO,
+	XILPM_RESET_IOU_CC,
+	XILPM_RESET_TIMESTAMP,
+	XILPM_RESET_RPU_R50,
+	XILPM_RESET_RPU_R51,
+	XILPM_RESET_RPU_AMBA,
+	XILPM_RESET_OCM,
+	XILPM_RESET_RPU_PGE,
+	XILPM_RESET_USB0_CORERESET,
+	XILPM_RESET_USB1_CORERESET,
+	XILPM_RESET_USB0_HIBERRESET,
+	XILPM_RESET_USB1_HIBERRESET,
+	XILPM_RESET_USB0_APB,
+	XILPM_RESET_USB1_APB,
+	XILPM_RESET_IPI,
+	XILPM_RESET_APM_LPD,
+	XILPM_RESET_RTC,
+	XILPM_RESET_SYSMON,
+	XILPM_RESET_AFI_FM6,
+	XILPM_RESET_LPD_SWDT,
+	XILPM_RESET_FPD,
+	XILPM_RESET_RPU_DBG1,
+	XILPM_RESET_RPU_DBG0,
+	XILPM_RESET_DBG_LPD,
+	XILPM_RESET_DBG_FPD,
+	XILPM_RESET_APLL,
+	XILPM_RESET_DPLL,
+	XILPM_RESET_VPLL,
+	XILPM_RESET_IOPLL,
+	XILPM_RESET_RPLL,
+	XILPM_RESET_GPO3_PL_0,
+	XILPM_RESET_GPO3_PL_1,
+	XILPM_RESET_GPO3_PL_2,
+	XILPM_RESET_GPO3_PL_3,
+	XILPM_RESET_GPO3_PL_4,
+	XILPM_RESET_GPO3_PL_5,
+	XILPM_RESET_GPO3_PL_6,
+	XILPM_RESET_GPO3_PL_7,
+	XILPM_RESET_GPO3_PL_8,
+	XILPM_RESET_GPO3_PL_9,
+	XILPM_RESET_GPO3_PL_10,
+	XILPM_RESET_GPO3_PL_11,
+	XILPM_RESET_GPO3_PL_12,
+	XILPM_RESET_GPO3_PL_13,
+	XILPM_RESET_GPO3_PL_14,
+	XILPM_RESET_GPO3_PL_15,
+	XILPM_RESET_GPO3_PL_16,
+	XILPM_RESET_GPO3_PL_17,
+	XILPM_RESET_GPO3_PL_18,
+	XILPM_RESET_GPO3_PL_19,
+	XILPM_RESET_GPO3_PL_20,
+	XILPM_RESET_GPO3_PL_21,
+	XILPM_RESET_GPO3_PL_22,
+	XILPM_RESET_GPO3_PL_23,
+	XILPM_RESET_GPO3_PL_24,
+	XILPM_RESET_GPO3_PL_25,
+	XILPM_RESET_GPO3_PL_26,
+	XILPM_RESET_GPO3_PL_27,
+	XILPM_RESET_GPO3_PL_28,
+	XILPM_RESET_GPO3_PL_29,
+	XILPM_RESET_GPO3_PL_30,
+	XILPM_RESET_GPO3_PL_31,
+	XILPM_RESET_RPU_LS,
+	XILPM_RESET_PS_ONLY,
+	XILPM_RESET_PL,
+	XILPM_RESET_END
+};
+
 extern bool zynqmp_eemi(struct cpu_user_regs *regs);
 
 #endif /* __ASM_ARM_PLATFORMS_ZYNQMP_H */
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines

[Julien Grall]

Hi,

On 03/12/2018 21:03, Stefano Stabellini wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> 
> Introduce zynqmp specific defines for the firmware calls.
> See EEMI:
> https://www.xilinx.com/support/documentation/user_guides/ug1200-eemi-api.pdf
> 
> The error codes are described, under XIlPM Error Codes:
> https://www.xilinx.com/support/documentation/user_guides/ug1137-zynq-ultrascale-mpsoc-swdev.pdf
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> 
> ---
> 
> Although the amount of #defines has been significantly reduced in v5,
> there is still a significant amount of static definitions:

I think the description below would be useful to have in the commit message.

> 
> - MM_*
> These are the MMIO addresses of each resource to do permission checks.
> Technically, they are also present on device tree, but using device tree
> to get the mmio regions is not simple and not done in this series.
> Xilinx plan to send a patch series in the future to extend the EEMI
> functionalities and as part of that work, more device tree based
> permission checking will be done, solving this problem.

As this is already present in the Device-Tree, there would be no issue to remove 
the hardcoded value, correct?

> These
> definitions could also be removed if we used a trivial "if dom0 -> yes,
> otherwise no" permission checking.
> 
> - pm_api_id
> These are the EEMI function IDs. Unavoidable.
> 
> - pm_ret_status
> These are the EEMI return statuses. Unavoidable.
> 
> - pm_node_id
> These are the EEMI function parameters for power management operations.
> Today, it is not possible to get them from device tree as there is no
> such information there. Even in the future when we add more power
> domains info to device tree, the EEMI function parameters might remain
> unique and different, requiring a table like this one.

Does it mean each Linux driver will have to hardcode the pm_node_id as well? 
What is the plan there?

> 
> - pm_reset
> These are the EEMI function parameters for reset operations. Same as
> pm_node_id.

Ditto.

Cheers,


> 
> ---
> Changes in v5:
> - remove MMIO access related definitions
> 
> Changes in v4:
> - define PM_MMIO_SHIFT
> ---
>   xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 293 +++++++++++++++++++++
>   1 file changed, 293 insertions(+)
> 
> diff --git a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> index 43cefb5..f6ad03b 100644
> --- a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> @@ -16,6 +16,299 @@
>   
>   #include <asm/processor.h>
>   
> +#define MM_RPU	0xff9a0
> +#define MM_RTC	0xffa60
> +#define MM_ADMA_CH0	0xffa80
> +
> +#define MM_USB3_0_XHCI  0xfe200
> +#define MM_USB3_1_XHCI  0xfe300
> +
> +#define MM_SATA_AHCI_HBA	0xfd0c0
> +#define MM_AXIPCIE_MAIN	0xfd0e0
> +#define MM_CRF_APB	0xfd1a0000
> +#define MM_PCIE_ATTRIB	0xfd480
> +#define MM_DP	0xfd4a0
> +#define MM_GPU	0xfd4b0
> +#define MM_GDMA_CH0	0xfd500
> +
> +#define MM_UART0	0xff000
> +#define MM_UART1	0xff010
> +#define MM_I2C0	0xff020
> +#define MM_I2C1	0xff030
> +#define MM_SPI0	0xff040
> +#define MM_SPI1	0xff050
> +#define MM_CAN0	0xff060
> +#define MM_CAN1	0xff070
> +#define MM_GPIO	0xff0a0
> +#define MM_GEM0	0xff0b0
> +#define MM_GEM1	0xff0c0
> +#define MM_GEM2	0xff0d0
> +#define MM_GEM3	0xff0e0
> +#define MM_QSPI	0xff0f0
> +#define MM_NAND	0xff100
> +#define MM_TTC0	0xff110
> +#define MM_TTC1	0xff120
> +#define MM_TTC2	0xff130
> +#define MM_TTC3	0xff140
> +#define MM_SWDT	0xff150
> +#define MM_SD0	0xff160
> +#define MM_SD1	0xff170
> +
> +/* Service calls.  */
> +#define PM_GET_TRUSTZONE_VERSION	0xa03
> +
> +/* SMC function IDs for SiP Service queries */
> +#define ZYNQMP_SIP_SVC_CALL_COUNT       0xff00
> +#define ZYNQMP_SIP_SVC_UID              0xff01
> +#define ZYNQMP_SIP_SVC_VERSION          0xff03

There are no need to define those. You can directly use ARM_SMCCC_CALL_* from 
asm-arm/smccc.h.

> +
> +#define PM_MMIO_SHIFT                   32

You don't seem to use it at all. Did I miss anything?

> +
> +enum pm_api_id {
> +	/* Miscellaneous API functions: */
> +	PM_GET_API_VERSION = 1, /* Do not change or move */
> +	PM_SET_CONFIGURATION,
> +	PM_GET_NODE_STATUS,
> +	PM_GET_OP_CHARACTERISTIC,
> +	PM_REGISTER_NOTIFIER,
> +	/* API for suspending of PUs: */
> +	PM_REQ_SUSPEND,
> +	PM_SELF_SUSPEND,
> +	PM_FORCE_POWERDOWN,
> +	PM_ABORT_SUSPEND,
> +	PM_REQ_WAKEUP,
> +	PM_SET_WAKEUP_SOURCE,
> +	PM_SYSTEM_SHUTDOWN,
> +	/* API for managing PM slaves: */
> +	PM_REQ_NODE,
> +	PM_RELEASE_NODE,
> +	PM_SET_REQUIREMENT,
> +	PM_SET_MAX_LATENCY,
> +	/* Direct control API functions: */
> +	PM_RESET_ASSERT,
> +	PM_RESET_GET_STATUS,
> +	PM_MMIO_WRITE,
> +	PM_MMIO_READ,
> +	PM_INIT,
> +	PM_FPGA_LOAD,
> +	PM_FPGA_GET_STATUS,
> +	PM_GET_CHIPID,
> +	/* ID 25 is been used by U-boot to process secure boot images */
> +	/* Secure library generic API functions */
> +	PM_SECURE_SHA = 26,
> +	PM_SECURE_RSA,
> +	/* Pin control API functions */
> +	PM_PINCTRL_REQUEST,
> +	PM_PINCTRL_RELEASE,
> +	PM_PINCTRL_GET_FUNCTION,
> +	PM_PINCTRL_SET_FUNCTION,
> +	PM_PINCTRL_CONFIG_PARAM_GET,
> +	PM_PINCTRL_CONFIG_PARAM_SET,
> +	/* PM IOCTL API */
> +	PM_IOCTL,
> +	/* API to query information from firmware */
> +	PM_QUERY_DATA,
> +	/* Clock control API functions */
> +	PM_CLOCK_ENABLE,
> +	PM_CLOCK_DISABLE,
> +	PM_CLOCK_GETSTATE,
> +	PM_CLOCK_SETDIVIDER,
> +	PM_CLOCK_GETDIVIDER,
> +	PM_CLOCK_SETRATE,
> +	PM_CLOCK_GETRATE,
> +	PM_CLOCK_SETPARENT,
> +	PM_CLOCK_GETPARENT,
> +	PM_API_MAX
> +};
> +
> +enum pm_node_id {
> +	NODE_RPU = 6,
> +	NODE_RPU_0,
> +	NODE_RPU_1,
> +	NODE_GPU_PP_0 = 20,
> +	NODE_GPU_PP_1,
> +	NODE_USB_0,
> +	NODE_USB_1,
> +	NODE_TTC_0,
> +	NODE_TTC_1,
> +	NODE_TTC_2,
> +	NODE_TTC_3,
> +	NODE_SATA,
> +	NODE_ETH_0,
> +	NODE_ETH_1,
> +	NODE_ETH_2,
> +	NODE_ETH_3,
> +	NODE_UART_0,
> +	NODE_UART_1,
> +	NODE_SPI_0,
> +	NODE_SPI_1,
> +	NODE_I2C_0,
> +	NODE_I2C_1,
> +	NODE_SD_0,
> +	NODE_SD_1,
> +	NODE_DP,
> +	NODE_GDMA,
> +	NODE_ADMA,
> +	NODE_NAND,
> +	NODE_QSPI,
> +	NODE_GPIO,
> +	NODE_CAN_0,
> +	NODE_CAN_1,
> +	NODE_AFI,
> +	NODE_APLL,
> +	NODE_VPLL,
> +	NODE_DPLL,
> +	NODE_RPLL,
> +	NODE_IOPLL,
> +	NODE_DDR,
> +	NODE_IPI_APU,
> +	NODE_IPI_RPU_0,
> +	NODE_GPU,
> +	NODE_PCIE,
> +	NODE_PCAP,
> +	NODE_RTC,
> +};
> +
> +/**
> + * @XST_PM_SUCCESS:		Success
> + * @XST_PM_INTERNAL:	Unexpected error
> + * @XST_PM_CONFLICT:	Conflicting requirements
> + * @XST_PM_NO_ACCESS:	Access rights violation
> + * @XST_PM_INVALID_NODE:	Does not apply to node passed as argument
> + * @XST_PM_DOUBLE_REQ:	Duplicate request
> + * @XST_PM_ABORT_SUSPEND:	Target has aborted suspend
> + */
> +enum pm_ret_status {
> +	XST_PM_SUCCESS = 0,
> +	XST_PM_INTERNAL = 2000,
> +	XST_PM_CONFLICT,
> +	XST_PM_NO_ACCESS,
> +	XST_PM_INVALID_NODE,
> +	XST_PM_DOUBLE_REQ,
> +	XST_PM_ABORT_SUSPEND,
> +};
> +
> +enum pm_reset {
> +	XILPM_RESET_START = 999,
> +	XILPM_RESET_PCIE_CFG,
> +	XILPM_RESET_PCIE_BRIDGE,
> +	XILPM_RESET_PCIE_CTRL,
> +	XILPM_RESET_DP,
> +	XILPM_RESET_SWDT_CRF,
> +	XILPM_RESET_AFI_FM5,
> +	XILPM_RESET_AFI_FM4,
> +	XILPM_RESET_AFI_FM3,
> +	XILPM_RESET_AFI_FM2,
> +	XILPM_RESET_AFI_FM1,
> +	XILPM_RESET_AFI_FM0,
> +	XILPM_RESET_GDMA,
> +	XILPM_RESET_GPU_PP1,
> +	XILPM_RESET_GPU_PP0,
> +	XILPM_RESET_GPU,
> +	XILPM_RESET_GT,
> +	XILPM_RESET_SATA,
> +	XILPM_RESET_ACPU3_PWRON,
> +	XILPM_RESET_ACPU2_PWRON,
> +	XILPM_RESET_ACPU1_PWRON,
> +	XILPM_RESET_ACPU0_PWRON,
> +	XILPM_RESET_APU_L2,
> +	XILPM_RESET_ACPU3,
> +	XILPM_RESET_ACPU2,
> +	XILPM_RESET_ACPU1,
> +	XILPM_RESET_ACPU0,
> +	XILPM_RESET_DDR,
> +	XILPM_RESET_APM_FPD,
> +	XILPM_RESET_SOFT,
> +	XILPM_RESET_GEM0,
> +	XILPM_RESET_GEM1,
> +	XILPM_RESET_GEM2,
> +	XILPM_RESET_GEM3,
> +	XILPM_RESET_QSPI,
> +	XILPM_RESET_UART0,
> +	XILPM_RESET_UART1,
> +	XILPM_RESET_SPI0,
> +	XILPM_RESET_SPI1,
> +	XILPM_RESET_SDIO0,
> +	XILPM_RESET_SDIO1,
> +	XILPM_RESET_CAN0,
> +	XILPM_RESET_CAN1,
> +	XILPM_RESET_I2C0,
> +	XILPM_RESET_I2C1,
> +	XILPM_RESET_TTC0,
> +	XILPM_RESET_TTC1,
> +	XILPM_RESET_TTC2,
> +	XILPM_RESET_TTC3,
> +	XILPM_RESET_SWDT_CRL,
> +	XILPM_RESET_NAND,
> +	XILPM_RESET_ADMA,
> +	XILPM_RESET_GPIO,
> +	XILPM_RESET_IOU_CC,
> +	XILPM_RESET_TIMESTAMP,
> +	XILPM_RESET_RPU_R50,
> +	XILPM_RESET_RPU_R51,
> +	XILPM_RESET_RPU_AMBA,
> +	XILPM_RESET_OCM,
> +	XILPM_RESET_RPU_PGE,
> +	XILPM_RESET_USB0_CORERESET,
> +	XILPM_RESET_USB1_CORERESET,
> +	XILPM_RESET_USB0_HIBERRESET,
> +	XILPM_RESET_USB1_HIBERRESET,
> +	XILPM_RESET_USB0_APB,
> +	XILPM_RESET_USB1_APB,
> +	XILPM_RESET_IPI,
> +	XILPM_RESET_APM_LPD,
> +	XILPM_RESET_RTC,
> +	XILPM_RESET_SYSMON,
> +	XILPM_RESET_AFI_FM6,
> +	XILPM_RESET_LPD_SWDT,
> +	XILPM_RESET_FPD,
> +	XILPM_RESET_RPU_DBG1,
> +	XILPM_RESET_RPU_DBG0,
> +	XILPM_RESET_DBG_LPD,
> +	XILPM_RESET_DBG_FPD,
> +	XILPM_RESET_APLL,
> +	XILPM_RESET_DPLL,
> +	XILPM_RESET_VPLL,
> +	XILPM_RESET_IOPLL,
> +	XILPM_RESET_RPLL,
> +	XILPM_RESET_GPO3_PL_0,
> +	XILPM_RESET_GPO3_PL_1,
> +	XILPM_RESET_GPO3_PL_2,
> +	XILPM_RESET_GPO3_PL_3,
> +	XILPM_RESET_GPO3_PL_4,
> +	XILPM_RESET_GPO3_PL_5,
> +	XILPM_RESET_GPO3_PL_6,
> +	XILPM_RESET_GPO3_PL_7,
> +	XILPM_RESET_GPO3_PL_8,
> +	XILPM_RESET_GPO3_PL_9,
> +	XILPM_RESET_GPO3_PL_10,
> +	XILPM_RESET_GPO3_PL_11,
> +	XILPM_RESET_GPO3_PL_12,
> +	XILPM_RESET_GPO3_PL_13,
> +	XILPM_RESET_GPO3_PL_14,
> +	XILPM_RESET_GPO3_PL_15,
> +	XILPM_RESET_GPO3_PL_16,
> +	XILPM_RESET_GPO3_PL_17,
> +	XILPM_RESET_GPO3_PL_18,
> +	XILPM_RESET_GPO3_PL_19,
> +	XILPM_RESET_GPO3_PL_20,
> +	XILPM_RESET_GPO3_PL_21,
> +	XILPM_RESET_GPO3_PL_22,
> +	XILPM_RESET_GPO3_PL_23,
> +	XILPM_RESET_GPO3_PL_24,
> +	XILPM_RESET_GPO3_PL_25,
> +	XILPM_RESET_GPO3_PL_26,
> +	XILPM_RESET_GPO3_PL_27,
> +	XILPM_RESET_GPO3_PL_28,
> +	XILPM_RESET_GPO3_PL_29,
> +	XILPM_RESET_GPO3_PL_30,
> +	XILPM_RESET_GPO3_PL_31,
> +	XILPM_RESET_RPU_LS,
> +	XILPM_RESET_PS_ONLY,
> +	XILPM_RESET_PL,
> +	XILPM_RESET_END
> +};
> +
>   extern bool zynqmp_eemi(struct cpu_user_regs *regs);
>   
>   #endif /* __ASM_ARM_PLATFORMS_ZYNQMP_H */
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines

[Stefano Stabellini]

On Tue, 11 Dec 2018, Julien Grall wrote:
> Hi,
> 
> On 03/12/2018 21:03, Stefano Stabellini wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > 
> > From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > 
> > Introduce zynqmp specific defines for the firmware calls.
> > See EEMI:
> > https://www.xilinx.com/support/documentation/user_guides/ug1200-eemi-api.pdf
> > 
> > The error codes are described, under XIlPM Error Codes:
> > https://www.xilinx.com/support/documentation/user_guides/ug1137-zynq-ultrascale-mpsoc-swdev.pdf
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > 
> > ---
> > 
> > Although the amount of #defines has been significantly reduced in v5,
> > there is still a significant amount of static definitions:
> 
> I think the description below would be useful to have in the commit message.

I can add it.


> > - MM_*
> > These are the MMIO addresses of each resource to do permission checks.
> > Technically, they are also present on device tree, but using device tree
> > to get the mmio regions is not simple and not done in this series.
> > Xilinx plan to send a patch series in the future to extend the EEMI
> > functionalities and as part of that work, more device tree based
> > permission checking will be done, solving this problem.
> 
> As this is already present in the Device-Tree, there would be no issue to
> remove the hardcoded value, correct?

Yes, that is correct. Saeed took it on as part of his next patch series
to rework these checks, removing the MM_* #defines.


> > These
> > definitions could also be removed if we used a trivial "if dom0 -> yes,
> > otherwise no" permission checking.
> > 
> > - pm_api_id
> > These are the EEMI function IDs. Unavoidable.
> > 
> > - pm_ret_status
> > These are the EEMI return statuses. Unavoidable.
> > 
> > - pm_node_id
> > These are the EEMI function parameters for power management operations.
> > Today, it is not possible to get them from device tree as there is no
> > such information there. Even in the future when we add more power
> > domains info to device tree, the EEMI function parameters might remain
> > unique and different, requiring a table like this one.
> 
> Does it mean each Linux driver will have to hardcode the pm_node_id as well?

The EEMI functionalities upstream in Linux today don't need any node_id
knowledge: there is one ioctl that takes a node_id from userspace and
pass it to the firmware without any checks. There are few clock API
calls that take a clock id, but those are different from node_ids and
are discoverable via EEMI calls (they will be discoverable in Xen too,
patch series in development.) In other words, Linux doesn't do anything
with node_ids today.


> What is the plan there?

The plan is to extract the node_id from a power-domain node on device
tree. Each device would have a phandler to link to the right
power-domain node which contains a power-domain-id attribute. The
power-domain-id attribute is the node_id here.

The power-domain-id changes to the Xilinx MPSoC device tree are under
discussion with the device tree community.


> > - pm_reset
> > These are the EEMI function parameters for reset operations. Same as
> > pm_node_id.
> 
> Ditto.

Similar to above


> > 
> > ---
> > Changes in v5:
> > - remove MMIO access related definitions
> > 
> > Changes in v4:
> > - define PM_MMIO_SHIFT
> > ---
> >   xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h | 293
> > +++++++++++++++++++++
> >   1 file changed, 293 insertions(+)
> > 
> > diff --git a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > index 43cefb5..f6ad03b 100644
> > --- a/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > +++ b/xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
> > @@ -16,6 +16,299 @@
> >     #include <asm/processor.h>
> >   +#define MM_RPU	0xff9a0
> > +#define MM_RTC	0xffa60
> > +#define MM_ADMA_CH0	0xffa80
> > +
> > +#define MM_USB3_0_XHCI  0xfe200
> > +#define MM_USB3_1_XHCI  0xfe300
> > +
> > +#define MM_SATA_AHCI_HBA	0xfd0c0
> > +#define MM_AXIPCIE_MAIN	0xfd0e0
> > +#define MM_CRF_APB	0xfd1a0000
> > +#define MM_PCIE_ATTRIB	0xfd480
> > +#define MM_DP	0xfd4a0
> > +#define MM_GPU	0xfd4b0
> > +#define MM_GDMA_CH0	0xfd500
> > +
> > +#define MM_UART0	0xff000
> > +#define MM_UART1	0xff010
> > +#define MM_I2C0	0xff020
> > +#define MM_I2C1	0xff030
> > +#define MM_SPI0	0xff040
> > +#define MM_SPI1	0xff050
> > +#define MM_CAN0	0xff060
> > +#define MM_CAN1	0xff070
> > +#define MM_GPIO	0xff0a0
> > +#define MM_GEM0	0xff0b0
> > +#define MM_GEM1	0xff0c0
> > +#define MM_GEM2	0xff0d0
> > +#define MM_GEM3	0xff0e0
> > +#define MM_QSPI	0xff0f0
> > +#define MM_NAND	0xff100
> > +#define MM_TTC0	0xff110
> > +#define MM_TTC1	0xff120
> > +#define MM_TTC2	0xff130
> > +#define MM_TTC3	0xff140
> > +#define MM_SWDT	0xff150
> > +#define MM_SD0	0xff160
> > +#define MM_SD1	0xff170
> > +
> > +/* Service calls.  */
> > +#define PM_GET_TRUSTZONE_VERSION	0xa03
> > +
> > +/* SMC function IDs for SiP Service queries */
> > +#define ZYNQMP_SIP_SVC_CALL_COUNT       0xff00
> > +#define ZYNQMP_SIP_SVC_UID              0xff01
> > +#define ZYNQMP_SIP_SVC_VERSION          0xff03
> 
> There are no need to define those. You can directly use ARM_SMCCC_CALL_* from
> asm-arm/smccc.h.
> 
> > +
> > +#define PM_MMIO_SHIFT                   32
> 
> You don't seem to use it at all. Did I miss anything?
> 
> > +
> > +enum pm_api_id {
> > +	/* Miscellaneous API functions: */
> > +	PM_GET_API_VERSION = 1, /* Do not change or move */
> > +	PM_SET_CONFIGURATION,
> > +	PM_GET_NODE_STATUS,
> > +	PM_GET_OP_CHARACTERISTIC,
> > +	PM_REGISTER_NOTIFIER,
> > +	/* API for suspending of PUs: */
> > +	PM_REQ_SUSPEND,
> > +	PM_SELF_SUSPEND,
> > +	PM_FORCE_POWERDOWN,
> > +	PM_ABORT_SUSPEND,
> > +	PM_REQ_WAKEUP,
> > +	PM_SET_WAKEUP_SOURCE,
> > +	PM_SYSTEM_SHUTDOWN,
> > +	/* API for managing PM slaves: */
> > +	PM_REQ_NODE,
> > +	PM_RELEASE_NODE,
> > +	PM_SET_REQUIREMENT,
> > +	PM_SET_MAX_LATENCY,
> > +	/* Direct control API functions: */
> > +	PM_RESET_ASSERT,
> > +	PM_RESET_GET_STATUS,
> > +	PM_MMIO_WRITE,
> > +	PM_MMIO_READ,
> > +	PM_INIT,
> > +	PM_FPGA_LOAD,
> > +	PM_FPGA_GET_STATUS,
> > +	PM_GET_CHIPID,
> > +	/* ID 25 is been used by U-boot to process secure boot images */
> > +	/* Secure library generic API functions */
> > +	PM_SECURE_SHA = 26,
> > +	PM_SECURE_RSA,
> > +	/* Pin control API functions */
> > +	PM_PINCTRL_REQUEST,
> > +	PM_PINCTRL_RELEASE,
> > +	PM_PINCTRL_GET_FUNCTION,
> > +	PM_PINCTRL_SET_FUNCTION,
> > +	PM_PINCTRL_CONFIG_PARAM_GET,
> > +	PM_PINCTRL_CONFIG_PARAM_SET,
> > +	/* PM IOCTL API */
> > +	PM_IOCTL,
> > +	/* API to query information from firmware */
> > +	PM_QUERY_DATA,
> > +	/* Clock control API functions */
> > +	PM_CLOCK_ENABLE,
> > +	PM_CLOCK_DISABLE,
> > +	PM_CLOCK_GETSTATE,
> > +	PM_CLOCK_SETDIVIDER,
> > +	PM_CLOCK_GETDIVIDER,
> > +	PM_CLOCK_SETRATE,
> > +	PM_CLOCK_GETRATE,
> > +	PM_CLOCK_SETPARENT,
> > +	PM_CLOCK_GETPARENT,
> > +	PM_API_MAX
> > +};
> > +
> > +enum pm_node_id {
> > +	NODE_RPU = 6,
> > +	NODE_RPU_0,
> > +	NODE_RPU_1,
> > +	NODE_GPU_PP_0 = 20,
> > +	NODE_GPU_PP_1,
> > +	NODE_USB_0,
> > +	NODE_USB_1,
> > +	NODE_TTC_0,
> > +	NODE_TTC_1,
> > +	NODE_TTC_2,
> > +	NODE_TTC_3,
> > +	NODE_SATA,
> > +	NODE_ETH_0,
> > +	NODE_ETH_1,
> > +	NODE_ETH_2,
> > +	NODE_ETH_3,
> > +	NODE_UART_0,
> > +	NODE_UART_1,
> > +	NODE_SPI_0,
> > +	NODE_SPI_1,
> > +	NODE_I2C_0,
> > +	NODE_I2C_1,
> > +	NODE_SD_0,
> > +	NODE_SD_1,
> > +	NODE_DP,
> > +	NODE_GDMA,
> > +	NODE_ADMA,
> > +	NODE_NAND,
> > +	NODE_QSPI,
> > +	NODE_GPIO,
> > +	NODE_CAN_0,
> > +	NODE_CAN_1,
> > +	NODE_AFI,
> > +	NODE_APLL,
> > +	NODE_VPLL,
> > +	NODE_DPLL,
> > +	NODE_RPLL,
> > +	NODE_IOPLL,
> > +	NODE_DDR,
> > +	NODE_IPI_APU,
> > +	NODE_IPI_RPU_0,
> > +	NODE_GPU,
> > +	NODE_PCIE,
> > +	NODE_PCAP,
> > +	NODE_RTC,
> > +};
> > +
> > +/**
> > + * @XST_PM_SUCCESS:		Success
> > + * @XST_PM_INTERNAL:	Unexpected error
> > + * @XST_PM_CONFLICT:	Conflicting requirements
> > + * @XST_PM_NO_ACCESS:	Access rights violation
> > + * @XST_PM_INVALID_NODE:	Does not apply to node passed as argument
> > + * @XST_PM_DOUBLE_REQ:	Duplicate request
> > + * @XST_PM_ABORT_SUSPEND:	Target has aborted suspend
> > + */
> > +enum pm_ret_status {
> > +	XST_PM_SUCCESS = 0,
> > +	XST_PM_INTERNAL = 2000,
> > +	XST_PM_CONFLICT,
> > +	XST_PM_NO_ACCESS,
> > +	XST_PM_INVALID_NODE,
> > +	XST_PM_DOUBLE_REQ,
> > +	XST_PM_ABORT_SUSPEND,
> > +};
> > +
> > +enum pm_reset {
> > +	XILPM_RESET_START = 999,
> > +	XILPM_RESET_PCIE_CFG,
> > +	XILPM_RESET_PCIE_BRIDGE,
> > +	XILPM_RESET_PCIE_CTRL,
> > +	XILPM_RESET_DP,
> > +	XILPM_RESET_SWDT_CRF,
> > +	XILPM_RESET_AFI_FM5,
> > +	XILPM_RESET_AFI_FM4,
> > +	XILPM_RESET_AFI_FM3,
> > +	XILPM_RESET_AFI_FM2,
> > +	XILPM_RESET_AFI_FM1,
> > +	XILPM_RESET_AFI_FM0,
> > +	XILPM_RESET_GDMA,
> > +	XILPM_RESET_GPU_PP1,
> > +	XILPM_RESET_GPU_PP0,
> > +	XILPM_RESET_GPU,
> > +	XILPM_RESET_GT,
> > +	XILPM_RESET_SATA,
> > +	XILPM_RESET_ACPU3_PWRON,
> > +	XILPM_RESET_ACPU2_PWRON,
> > +	XILPM_RESET_ACPU1_PWRON,
> > +	XILPM_RESET_ACPU0_PWRON,
> > +	XILPM_RESET_APU_L2,
> > +	XILPM_RESET_ACPU3,
> > +	XILPM_RESET_ACPU2,
> > +	XILPM_RESET_ACPU1,
> > +	XILPM_RESET_ACPU0,
> > +	XILPM_RESET_DDR,
> > +	XILPM_RESET_APM_FPD,
> > +	XILPM_RESET_SOFT,
> > +	XILPM_RESET_GEM0,
> > +	XILPM_RESET_GEM1,
> > +	XILPM_RESET_GEM2,
> > +	XILPM_RESET_GEM3,
> > +	XILPM_RESET_QSPI,
> > +	XILPM_RESET_UART0,
> > +	XILPM_RESET_UART1,
> > +	XILPM_RESET_SPI0,
> > +	XILPM_RESET_SPI1,
> > +	XILPM_RESET_SDIO0,
> > +	XILPM_RESET_SDIO1,
> > +	XILPM_RESET_CAN0,
> > +	XILPM_RESET_CAN1,
> > +	XILPM_RESET_I2C0,
> > +	XILPM_RESET_I2C1,
> > +	XILPM_RESET_TTC0,
> > +	XILPM_RESET_TTC1,
> > +	XILPM_RESET_TTC2,
> > +	XILPM_RESET_TTC3,
> > +	XILPM_RESET_SWDT_CRL,
> > +	XILPM_RESET_NAND,
> > +	XILPM_RESET_ADMA,
> > +	XILPM_RESET_GPIO,
> > +	XILPM_RESET_IOU_CC,
> > +	XILPM_RESET_TIMESTAMP,
> > +	XILPM_RESET_RPU_R50,
> > +	XILPM_RESET_RPU_R51,
> > +	XILPM_RESET_RPU_AMBA,
> > +	XILPM_RESET_OCM,
> > +	XILPM_RESET_RPU_PGE,
> > +	XILPM_RESET_USB0_CORERESET,
> > +	XILPM_RESET_USB1_CORERESET,
> > +	XILPM_RESET_USB0_HIBERRESET,
> > +	XILPM_RESET_USB1_HIBERRESET,
> > +	XILPM_RESET_USB0_APB,
> > +	XILPM_RESET_USB1_APB,
> > +	XILPM_RESET_IPI,
> > +	XILPM_RESET_APM_LPD,
> > +	XILPM_RESET_RTC,
> > +	XILPM_RESET_SYSMON,
> > +	XILPM_RESET_AFI_FM6,
> > +	XILPM_RESET_LPD_SWDT,
> > +	XILPM_RESET_FPD,
> > +	XILPM_RESET_RPU_DBG1,
> > +	XILPM_RESET_RPU_DBG0,
> > +	XILPM_RESET_DBG_LPD,
> > +	XILPM_RESET_DBG_FPD,
> > +	XILPM_RESET_APLL,
> > +	XILPM_RESET_DPLL,
> > +	XILPM_RESET_VPLL,
> > +	XILPM_RESET_IOPLL,
> > +	XILPM_RESET_RPLL,
> > +	XILPM_RESET_GPO3_PL_0,
> > +	XILPM_RESET_GPO3_PL_1,
> > +	XILPM_RESET_GPO3_PL_2,
> > +	XILPM_RESET_GPO3_PL_3,
> > +	XILPM_RESET_GPO3_PL_4,
> > +	XILPM_RESET_GPO3_PL_5,
> > +	XILPM_RESET_GPO3_PL_6,
> > +	XILPM_RESET_GPO3_PL_7,
> > +	XILPM_RESET_GPO3_PL_8,
> > +	XILPM_RESET_GPO3_PL_9,
> > +	XILPM_RESET_GPO3_PL_10,
> > +	XILPM_RESET_GPO3_PL_11,
> > +	XILPM_RESET_GPO3_PL_12,
> > +	XILPM_RESET_GPO3_PL_13,
> > +	XILPM_RESET_GPO3_PL_14,
> > +	XILPM_RESET_GPO3_PL_15,
> > +	XILPM_RESET_GPO3_PL_16,
> > +	XILPM_RESET_GPO3_PL_17,
> > +	XILPM_RESET_GPO3_PL_18,
> > +	XILPM_RESET_GPO3_PL_19,
> > +	XILPM_RESET_GPO3_PL_20,
> > +	XILPM_RESET_GPO3_PL_21,
> > +	XILPM_RESET_GPO3_PL_22,
> > +	XILPM_RESET_GPO3_PL_23,
> > +	XILPM_RESET_GPO3_PL_24,
> > +	XILPM_RESET_GPO3_PL_25,
> > +	XILPM_RESET_GPO3_PL_26,
> > +	XILPM_RESET_GPO3_PL_27,
> > +	XILPM_RESET_GPO3_PL_28,
> > +	XILPM_RESET_GPO3_PL_29,
> > +	XILPM_RESET_GPO3_PL_30,
> > +	XILPM_RESET_GPO3_PL_31,
> > +	XILPM_RESET_RPU_LS,
> > +	XILPM_RESET_PS_ONLY,
> > +	XILPM_RESET_PL,
> > +	XILPM_RESET_END
> > +};
> > +
> >   extern bool zynqmp_eemi(struct cpu_user_regs *regs);
> >     #endif /* __ASM_ARM_PLATFORMS_ZYNQMP_H */
> > 
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines

[Julien Grall]

Hi Stefano,

On 11/12/2018 19:22, Stefano Stabellini wrote:
> On Tue, 11 Dec 2018, Julien Grall wrote:
>> Hi,
>>
>> On 03/12/2018 21:03, Stefano Stabellini wrote:
>> What is the plan there?
> 
> The plan is to extract the node_id from a power-domain node on device
> tree. Each device would have a phandler to link to the right
> power-domain node which contains a power-domain-id attribute. The
> power-domain-id attribute is the node_id here.
> 
> The power-domain-id changes to the Xilinx MPSoC device tree are under
> discussion with the device tree community.

If I understand correctly, we will never be able to remove the hardcoded values 
in Xen. This is because some device-tree may not have the bindings. Am I correct?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines

[Stefano Stabellini]

On Wed, 12 Dec 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 11/12/2018 19:22, Stefano Stabellini wrote:
> > On Tue, 11 Dec 2018, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 03/12/2018 21:03, Stefano Stabellini wrote:
> > > What is the plan there?
> > 
> > The plan is to extract the node_id from a power-domain node on device
> > tree. Each device would have a phandler to link to the right
> > power-domain node which contains a power-domain-id attribute. The
> > power-domain-id attribute is the node_id here.
> > 
> > The power-domain-id changes to the Xilinx MPSoC device tree are under
> > discussion with the device tree community.
> 
> If I understand correctly, we will never be able to remove the hardcoded
> values in Xen. This is because some device-tree may not have the bindings. Am
> I correct?

If we want to support running on existing hardware and firmware releases,
then you are correct. We won't be able to remove the hardcoded values.
That ship has sailed, not much we can do about it.

If in the future we decide to drop support for older firmware releases
and ask users to update their firmare/devicetrees, then we'll be able to
remove the hardcoded values. I think it is something we can consider.
In fact, I would rather break compatibility than not providing support
for basic power management functionalities at all.

I think it is better to introduce basic EEMI support now, even if in a
couple of Xen releases from now we'll make the decision to drop the
hardcoded values and require new Xilinx device trees. Xilinx users are
used to updating firmware on these boards every 6 months, and it would
beneficial for them to be able to take a Xen Project release rather than
be forced to use a Xilinx Xen release to have support for power
management. When the new bindings become available, as we introduce
support for them in Xen, we can decide whether to keep the hardcoded
values or whether we should take the opportunity to drop them.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 3/7] xen/arm: zynqmp: introduce zynqmp specific defines

[Julien Grall]

Hi Stefano,

On 12/12/18 11:55 PM, Stefano Stabellini wrote:
> On Wed, 12 Dec 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 11/12/2018 19:22, Stefano Stabellini wrote:
>>> On Tue, 11 Dec 2018, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 03/12/2018 21:03, Stefano Stabellini wrote:
>>>> What is the plan there?
>>>
>>> The plan is to extract the node_id from a power-domain node on device
>>> tree. Each device would have a phandler to link to the right
>>> power-domain node which contains a power-domain-id attribute. The
>>> power-domain-id attribute is the node_id here.
>>>
>>> The power-domain-id changes to the Xilinx MPSoC device tree are under
>>> discussion with the device tree community.
>>
>> If I understand correctly, we will never be able to remove the hardcoded
>> values in Xen. This is because some device-tree may not have the bindings. Am
>> I correct?
> 
> If we want to support running on existing hardware and firmware releases,
> then you are correct. We won't be able to remove the hardcoded values.
> That ship has sailed, not much we can do about it.
> 
> If in the future we decide to drop support for older firmware releases
> and ask users to update their firmare/devicetrees, then we'll be able to
> remove the hardcoded values. I think it is something we can consider.
> In fact, I would rather break compatibility than not providing support
> for basic power management functionalities at all.

With your suggestion this means that Xen and the firmware are tied 
together. So you can't update Xen without updating the firmware.

We already ruled out that behavior for x-gene. See the partial revert 
you did 420596c868 to revert back the quirk.

I don't think this is very different here. We are introducing a feature, 
that we know will be broken afterwards unless you update your firmware. 
It is not like it was not planned... We should aim to support most of 
the official firmware unless there are a strong argument not to do.

Basic power management for other than Dom0 is not a valid enough reason 
for me to break compatibility.

> 
> I think it is better to introduce basic EEMI support now, even if in a
> couple of Xen releases from now we'll make the decision to drop the
> hardcoded values and require new Xilinx device trees. Xilinx users are
> used to updating firmware on these boards every 6 months, and it would
> beneficial for them to be able to take a Xen Project release rather than
> be forced to use a Xilinx Xen release to have support for power
> management. When the new bindings become available, as we introduce
> support for them in Xen, we can decide whether to keep the hardcoded
> values or whether we should take the opportunity to drop them.
This is the right moment to discuss about it. It will be too late once 
we get merged. I don't want to see 2 solutions in Xen if we know that 
bindings are work-in-progress.

I am happy to consider EEMI for Dom0. For the guests, it will have to 
wait the device-tree bindings.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v5 4/7] xen: introduce mfn_init macro

[Stefano Stabellini]

To be used in constant initializations of mfn_t variables, such as:

static mfn_t node = mfn_init(MM_ADDR);

It is necessary because static inline functions cannot be used as static
initializers.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien.grall@arm.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/include/xen/mm.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 054d02e..692967e 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -77,6 +77,12 @@ TYPE_SAFE(unsigned long, mfn);
 #undef mfn_x
 #endif
 
+#ifndef NDEBUG
+#define mfn_init(x) { x }
+#else
+#define mfn_init(x) x
+#endif
+
 static inline mfn_t mfn_add(mfn_t mfn, unsigned long i)
 {
     return _mfn(mfn_x(mfn) + i);
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 4/7] xen: introduce mfn_init macro

[Jan Beulich]

>>> On 03.12.18 at 22:03, <sstabellini@kernel.org> wrote:
> To be used in constant initializations of mfn_t variables, such as:
> 
> static mfn_t node = mfn_init(MM_ADDR);
> 
> It is necessary because static inline functions cannot be used as static
> initializers.

We had been at this point once (quite some time ago), and got
away without such an addition. Did you try to find that old
discussion? Are there any new reasons to have such a construct?
Do you need this for other than setting a value to INVALID_MFN,
in which case INVALID_MFN_INITIALIZER ought to be suitable?

This is not to say I'm entirely opposed.

If we were to have such a construct, I wonder though whether
mfn_init() is suitable as a name. Simply MFN() perhaps, and then
also consistently have GFN() and DFN()?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 4/7] xen: introduce mfn_init macro

[Stefano Stabellini]

On Tue, 4 Dec 2018, Jan Beulich wrote:
> >>> On 03.12.18 at 22:03, <sstabellini@kernel.org> wrote:
> > To be used in constant initializations of mfn_t variables, such as:
> > 
> > static mfn_t node = mfn_init(MM_ADDR);
> > 
> > It is necessary because static inline functions cannot be used as static
> > initializers.
> 
> We had been at this point once (quite some time ago), and got
> away without such an addition. Did you try to find that old
> discussion? Are there any new reasons to have such a construct?
> Do you need this for other than setting a value to INVALID_MFN,
> in which case INVALID_MFN_INITIALIZER ought to be suitable?
> 
> This is not to say I'm entirely opposed.
> 
> If we were to have such a construct, I wonder though whether
> mfn_init() is suitable as a name. Simply MFN() perhaps, and then
> also consistently have GFN() and DFN()?

Hi Jan,

I am happy with any name, and MFN() together with GFN() and DFN() look
like a good option.

The reason why it is needed is that without it I cannot introduce a
statically initialized array of mfn_t type like the one in the following
patch in the series:

+static const struct pm_access pm_node_access[] = {
+    /* MM_RPU grants access to all RPU Nodes.  */
+    [NODE_RPU] = { mfn_init(MM_RPU) },
+    [NODE_RPU_0] = { mfn_init(MM_RPU) },
+    [NODE_RPU_1] = { mfn_init(MM_RPU) },
+    [NODE_IPI_RPU_0] = { mfn_init(MM_RPU) },

[...]

Where MM_RPU is a mfn, and the NODE_* are IDs defined as enum:

#define MM_RPU  0xff9a0

enum pm_node_id {
	NODE_RPU = 6,
	NODE_RPU_0,
	NODE_RPU_1,

[...]


Originally I had:

  [NODE_RPU] = { MM_RPU },

but I changed the type to be mfn_t to address one of Julien's comments.
You might get a better idea of the issue if you give a look at this
branch:

http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git zynqmp-v5

See:

xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h
xen/arch/arm/platforms/xilinx-zynqmp-eemi.c

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 4/7] xen: introduce mfn_init macro

[Jan Beulich]

>>> On 04.12.18 at 20:38, <sstabellini@kernel.org> wrote:
> On Tue, 4 Dec 2018, Jan Beulich wrote:
>> >>> On 03.12.18 at 22:03, <sstabellini@kernel.org> wrote:
>> > To be used in constant initializations of mfn_t variables, such as:
>> > 
>> > static mfn_t node = mfn_init(MM_ADDR);
>> > 
>> > It is necessary because static inline functions cannot be used as static
>> > initializers.
>> 
>> We had been at this point once (quite some time ago), and got
>> away without such an addition. Did you try to find that old
>> discussion? Are there any new reasons to have such a construct?
>> Do you need this for other than setting a value to INVALID_MFN,
>> in which case INVALID_MFN_INITIALIZER ought to be suitable?
>> 
>> This is not to say I'm entirely opposed.
>> 
>> If we were to have such a construct, I wonder though whether
>> mfn_init() is suitable as a name. Simply MFN() perhaps, and then
>> also consistently have GFN() and DFN()?
> 
> Hi Jan,
> 
> I am happy with any name, and MFN() together with GFN() and DFN() look
> like a good option.
> 
> The reason why it is needed is that without it I cannot introduce a
> statically initialized array of mfn_t type like the one in the following
> patch in the series:
> 
> +static const struct pm_access pm_node_access[] = {
> +    /* MM_RPU grants access to all RPU Nodes.  */
> +    [NODE_RPU] = { mfn_init(MM_RPU) },
> +    [NODE_RPU_0] = { mfn_init(MM_RPU) },
> +    [NODE_RPU_1] = { mfn_init(MM_RPU) },
> +    [NODE_IPI_RPU_0] = { mfn_init(MM_RPU) },
> 
> [...]
> 
> Where MM_RPU is a mfn, and the NODE_* are IDs defined as enum:
> 
> #define MM_RPU  0xff9a0
> 
> enum pm_node_id {
> 	NODE_RPU = 6,
> 	NODE_RPU_0,
> 	NODE_RPU_1,
> 
> [...]
> 
> 
> Originally I had:
> 
>   [NODE_RPU] = { MM_RPU },
> 
> but I changed the type to be mfn_t to address one of Julien's comments.
> You might get a better idea of the issue if you give a look at this
> branch:
> 
> http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git zynqmp-v5

Well, I have to admit that I'd rather not see ways to embed hard-coded
MFNs into code made available generically. May I suggest that you use
a macro with a name to your liking just locally to that one source file?
As a side note, I'm also puzzled by there being entries in the table which
don't have their MFNs specified. Oddly enough it looks as if
.hwdom_access was true if and only if no MFN is specified. The term
"node" of course is confusing too, considering its NUMA meaning
elsewhere in the hypervisor.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 4/7] xen: introduce mfn_init macro

[Stefano Stabellini]

On Wed, 5 Dec 2018, Jan Beulich wrote:
> >>> On 04.12.18 at 20:38, <sstabellini@kernel.org> wrote:
> > On Tue, 4 Dec 2018, Jan Beulich wrote:
> >> >>> On 03.12.18 at 22:03, <sstabellini@kernel.org> wrote:
> >> > To be used in constant initializations of mfn_t variables, such as:
> >> > 
> >> > static mfn_t node = mfn_init(MM_ADDR);
> >> > 
> >> > It is necessary because static inline functions cannot be used as static
> >> > initializers.
> >> 
> >> We had been at this point once (quite some time ago), and got
> >> away without such an addition. Did you try to find that old
> >> discussion? Are there any new reasons to have such a construct?
> >> Do you need this for other than setting a value to INVALID_MFN,
> >> in which case INVALID_MFN_INITIALIZER ought to be suitable?
> >> 
> >> This is not to say I'm entirely opposed.
> >> 
> >> If we were to have such a construct, I wonder though whether
> >> mfn_init() is suitable as a name. Simply MFN() perhaps, and then
> >> also consistently have GFN() and DFN()?
> > 
> > Hi Jan,
> > 
> > I am happy with any name, and MFN() together with GFN() and DFN() look
> > like a good option.
> > 
> > The reason why it is needed is that without it I cannot introduce a
> > statically initialized array of mfn_t type like the one in the following
> > patch in the series:
> > 
> > +static const struct pm_access pm_node_access[] = {
> > +    /* MM_RPU grants access to all RPU Nodes.  */
> > +    [NODE_RPU] = { mfn_init(MM_RPU) },
> > +    [NODE_RPU_0] = { mfn_init(MM_RPU) },
> > +    [NODE_RPU_1] = { mfn_init(MM_RPU) },
> > +    [NODE_IPI_RPU_0] = { mfn_init(MM_RPU) },
> > 
> > [...]
> > 
> > Where MM_RPU is a mfn, and the NODE_* are IDs defined as enum:
> > 
> > #define MM_RPU  0xff9a0
> > 
> > enum pm_node_id {
> > 	NODE_RPU = 6,
> > 	NODE_RPU_0,
> > 	NODE_RPU_1,
> > 
> > [...]
> > 
> > 
> > Originally I had:
> > 
> >   [NODE_RPU] = { MM_RPU },
> > 
> > but I changed the type to be mfn_t to address one of Julien's comments.
> > You might get a better idea of the issue if you give a look at this
> > branch:
> > 
> > http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git zynqmp-v5
> 
> Well, I have to admit that I'd rather not see ways to embed hard-coded
> MFNs into code made available generically. May I suggest that you use
> a macro with a name to your liking just locally to that one source file?

OK, no problem


> As a side note, I'm also puzzled by there being entries in the table which
> don't have their MFNs specified. Oddly enough it looks as if
> .hwdom_access was true if and only if no MFN is specified.

Yes, the hwdom_access check could be turned into a check for
MFN(INVALID_MFN). I'll do that it will actually make the array size
smaller.


> The term "node" of course is confusing too, considering its NUMA
> meaning elsewhere in the hypervisor.
 
That comes from the EEMI firmware specification: they use the term
"node" to address a power domain "unit".

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v5 5/7] xen/arm: zynqmp: eemi access control

[Stefano Stabellini]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Introduce data structs to implement basic access controls.
Introduce the following three functions:

domain_has_node_access: check access to the node
domain_has_reset_access: check access to the reset line
domain_has_mmio_access: check access to the register

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>

---
Statically defines:

- pm_node_access
It encodes the relationship between a node id and the start of the MMIO
region of a device in the corresponding power domain. It is used for
permission checking. Although the MMIO region start address is available
on device tree and could be derived from there (we plan to improve that
in the future), the relationship between a node id and corresponding
devices is not described and needs to be hardcoded.

- pm_reset_access
Same as pm_node_access for reset lines.

---
Changes in v5:
- improve in-code comments
- use mfn_t in struct pm_access
- remove mmio_access table

Changes in v4:
- add #include as needed
- add #if 0 for bisectability
- use mfn_t in pm_check_access
- add wrap-around ASSERT in domain_has_mmio_access
- use GENMASK in domain_has_mmio_access
- proper bound checks (== ARRAY_SIZE is out of bound)
---
 xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 348 ++++++++++++++++++++++++++++
 1 file changed, 348 insertions(+)

diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
index 369bb3f..92a02df 100644
--- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
+++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
@@ -16,9 +16,357 @@
  * GNU General Public License for more details.
  */
 
+/*
+ *  EEMI Power Management API access
+ *
+ * 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
+ *
+ * Although the details of the setup are configurable, in the common case
+ * 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.
+ *
+ * In order to correctly virtualize 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 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().
+ */
+
+#include <xen/iocap.h>
+#include <xen/sched.h>
 #include <asm/regs.h>
 #include <asm/platforms/xilinx-zynqmp-eemi.h>
 
+#if 0
+struct pm_access
+{
+    mfn_t mfn;
+    bool hwdom_access;    /* HW domain gets access regardless.  */
+};
+
+/*
+ * This table maps a node into a memory address.
+ * If a guest has access to the address, it has enough control
+ * over the node to grant it access to EEMI calls for that node.
+ */
+static const struct pm_access pm_node_access[] = {
+    /* MM_RPU grants access to all RPU Nodes.  */
+    [NODE_RPU] = { mfn_init(MM_RPU) },
+    [NODE_RPU_0] = { mfn_init(MM_RPU) },
+    [NODE_RPU_1] = { mfn_init(MM_RPU) },
+    [NODE_IPI_RPU_0] = { mfn_init(MM_RPU) },
+
+    /* GPU nodes.  */
+    [NODE_GPU] = { mfn_init(MM_GPU) },
+    [NODE_GPU_PP_0] = { mfn_init(MM_GPU) },
+    [NODE_GPU_PP_1] = { mfn_init(MM_GPU) },
+
+    [NODE_USB_0] = { mfn_init(MM_USB3_0_XHCI) },
+    [NODE_USB_1] = { mfn_init(MM_USB3_1_XHCI) },
+    [NODE_TTC_0] = { mfn_init(MM_TTC0) },
+    [NODE_TTC_1] = { mfn_init(MM_TTC1) },
+    [NODE_TTC_2] = { mfn_init(MM_TTC2) },
+    [NODE_TTC_3] = { mfn_init(MM_TTC3) },
+    [NODE_SATA] = { mfn_init(MM_SATA_AHCI_HBA) },
+    [NODE_ETH_0] = { mfn_init(MM_GEM0) },
+    [NODE_ETH_1] = { mfn_init(MM_GEM1) },
+    [NODE_ETH_2] = { mfn_init(MM_GEM2) },
+    [NODE_ETH_3] = { mfn_init(MM_GEM3) },
+    [NODE_UART_0] = { mfn_init(MM_UART0) },
+    [NODE_UART_1] = { mfn_init(MM_UART1) },
+    [NODE_SPI_0] = { mfn_init(MM_SPI0) },
+    [NODE_SPI_1] = { mfn_init(MM_SPI1) },
+    [NODE_I2C_0] = { mfn_init(MM_I2C0) },
+    [NODE_I2C_1] = { mfn_init(MM_I2C1) },
+    [NODE_SD_0] = { mfn_init(MM_SD0) },
+    [NODE_SD_1] = { mfn_init(MM_SD1) },
+    [NODE_DP] = { mfn_init(MM_DP) },
+
+    /* Guest with GDMA Channel 0 gets PM access. Other guests don't.  */
+    [NODE_GDMA] = { mfn_init(MM_GDMA_CH0) },
+    /* Guest with ADMA Channel 0 gets PM access. Other guests don't.  */
+    [NODE_ADMA] = { mfn_init(MM_ADMA_CH0) },
+
+    [NODE_NAND] = { mfn_init(MM_NAND) },
+    [NODE_QSPI] = { mfn_init(MM_QSPI) },
+    [NODE_GPIO] = { mfn_init(MM_GPIO) },
+    [NODE_CAN_0] = { mfn_init(MM_CAN0) },
+    [NODE_CAN_1] = { mfn_init(MM_CAN1) },
+
+    /* Only for the hardware domain.  */
+    [NODE_AFI] = { .hwdom_access = true },
+    [NODE_APLL] = { .hwdom_access = true },
+    [NODE_VPLL] = { .hwdom_access = true },
+    [NODE_DPLL] = { .hwdom_access = true },
+    [NODE_RPLL] = { .hwdom_access = true },
+    [NODE_IOPLL] = { .hwdom_access = true },
+    [NODE_DDR] = { .hwdom_access = true },
+    [NODE_IPI_APU] = { .hwdom_access = true },
+    [NODE_PCAP] = { .hwdom_access = true },
+
+    [NODE_PCIE] = { mfn_init(MM_PCIE_ATTRIB) },
+    [NODE_RTC] = { mfn_init(MM_RTC) },
+};
+
+/*
+ * 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.
+ */
+#define XILPM_RESET_IDX(n) (n - XILPM_RESET_PCIE_CFG)
+static const struct pm_access pm_reset_access[] = {
+    [XILPM_RESET_IDX(XILPM_RESET_PCIE_CFG)] = { mfn_init(MM_AXIPCIE_MAIN) },
+    [XILPM_RESET_IDX(XILPM_RESET_PCIE_BRIDGE)] = { mfn_init(MM_PCIE_ATTRIB) },
+    [XILPM_RESET_IDX(XILPM_RESET_PCIE_CTRL)] = { mfn_init(MM_PCIE_ATTRIB) },
+
+    [XILPM_RESET_IDX(XILPM_RESET_DP)] = { mfn_init(MM_DP) },
+    [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRF)] = { mfn_init(MM_SWDT) },
+    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM5)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM4)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM3)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM2)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM1)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM0)] = { .hwdom_access = true },
+
+    /* Channel 0 grants PM access.  */
+    [XILPM_RESET_IDX(XILPM_RESET_GDMA)] = { mfn_init(MM_GDMA_CH0) },
+    [XILPM_RESET_IDX(XILPM_RESET_GPU_PP1)] = { mfn_init(MM_GPU) },
+    [XILPM_RESET_IDX(XILPM_RESET_GPU_PP0)] = { mfn_init(MM_GPU) },
+    [XILPM_RESET_IDX(XILPM_RESET_GT)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_SATA)] = { mfn_init(MM_SATA_AHCI_HBA) },
+
+    [XILPM_RESET_IDX(XILPM_RESET_APM_FPD)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_SOFT)] = { .hwdom_access = true },
+
+    [XILPM_RESET_IDX(XILPM_RESET_GEM0)] = { mfn_init(MM_GEM0) },
+    [XILPM_RESET_IDX(XILPM_RESET_GEM1)] = { mfn_init(MM_GEM1) },
+    [XILPM_RESET_IDX(XILPM_RESET_GEM2)] = { mfn_init(MM_GEM2) },
+    [XILPM_RESET_IDX(XILPM_RESET_GEM3)] = { mfn_init(MM_GEM3) },
+
+    [XILPM_RESET_IDX(XILPM_RESET_QSPI)] = { mfn_init(MM_QSPI) },
+    [XILPM_RESET_IDX(XILPM_RESET_UART0)] = { mfn_init(MM_UART0) },
+    [XILPM_RESET_IDX(XILPM_RESET_UART1)] = { mfn_init(MM_UART1) },
+    [XILPM_RESET_IDX(XILPM_RESET_SPI0)] = { mfn_init(MM_SPI0) },
+    [XILPM_RESET_IDX(XILPM_RESET_SPI1)] = { mfn_init(MM_SPI1) },
+    [XILPM_RESET_IDX(XILPM_RESET_SDIO0)] = { mfn_init(MM_SD0) },
+    [XILPM_RESET_IDX(XILPM_RESET_SDIO1)] = { mfn_init(MM_SD1) },
+    [XILPM_RESET_IDX(XILPM_RESET_CAN0)] = { mfn_init(MM_CAN0) },
+    [XILPM_RESET_IDX(XILPM_RESET_CAN1)] = { mfn_init(MM_CAN1) },
+    [XILPM_RESET_IDX(XILPM_RESET_I2C0)] = { mfn_init(MM_I2C0) },
+    [XILPM_RESET_IDX(XILPM_RESET_I2C1)] = { mfn_init(MM_I2C1) },
+    [XILPM_RESET_IDX(XILPM_RESET_TTC0)] = { mfn_init(MM_TTC0) },
+    [XILPM_RESET_IDX(XILPM_RESET_TTC1)] = { mfn_init(MM_TTC1) },
+    [XILPM_RESET_IDX(XILPM_RESET_TTC2)] = { mfn_init(MM_TTC2) },
+    [XILPM_RESET_IDX(XILPM_RESET_TTC3)] = { mfn_init(MM_TTC3) },
+    [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRL)] = { mfn_init(MM_SWDT) },
+    [XILPM_RESET_IDX(XILPM_RESET_NAND)] = { mfn_init(MM_NAND) },
+    [XILPM_RESET_IDX(XILPM_RESET_ADMA)] = { mfn_init(MM_ADMA_CH0) },
+    [XILPM_RESET_IDX(XILPM_RESET_GPIO)] = { mfn_init(MM_GPIO) },
+    [XILPM_RESET_IDX(XILPM_RESET_IOU_CC)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_TIMESTAMP)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_RPU_R50)] = { mfn_init(MM_RPU) },
+    [XILPM_RESET_IDX(XILPM_RESET_RPU_R51)] = { mfn_init(MM_RPU) },
+    [XILPM_RESET_IDX(XILPM_RESET_RPU_AMBA)] = { mfn_init(MM_RPU) },
+    [XILPM_RESET_IDX(XILPM_RESET_OCM)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_RPU_PGE)] = { mfn_init(MM_RPU) },
+
+    [XILPM_RESET_IDX(XILPM_RESET_USB0_CORERESET)] = { mfn_init(MM_USB3_0_XHCI) },
+    [XILPM_RESET_IDX(XILPM_RESET_USB0_HIBERRESET)] = { mfn_init(MM_USB3_0_XHCI) },
+    [XILPM_RESET_IDX(XILPM_RESET_USB0_APB)] = { mfn_init(MM_USB3_0_XHCI) },
+
+    [XILPM_RESET_IDX(XILPM_RESET_USB1_CORERESET)] = { mfn_init(MM_USB3_1_XHCI) },
+    [XILPM_RESET_IDX(XILPM_RESET_USB1_HIBERRESET)] = { mfn_init(MM_USB3_1_XHCI) },
+    [XILPM_RESET_IDX(XILPM_RESET_USB1_APB)] = { mfn_init(MM_USB3_1_XHCI) },
+
+    [XILPM_RESET_IDX(XILPM_RESET_IPI)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_APM_LPD)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_RTC)] = { mfn_init(MM_RTC) },
+    [XILPM_RESET_IDX(XILPM_RESET_SYSMON)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM6)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_LPD_SWDT)] = { mfn_init(MM_SWDT) },
+    [XILPM_RESET_IDX(XILPM_RESET_FPD)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG1)] = { mfn_init(MM_RPU) },
+    [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG0)] = { mfn_init(MM_RPU) },
+    [XILPM_RESET_IDX(XILPM_RESET_DBG_LPD)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_DBG_FPD)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_APLL)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_DPLL)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_VPLL)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_IOPLL)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_RPLL)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_0)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_1)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_2)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_3)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_4)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_5)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_6)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_7)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_8)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_9)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_10)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_11)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_12)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_13)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_14)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_15)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_16)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_17)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_18)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_19)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_20)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_21)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_22)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_23)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_24)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_25)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_26)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_27)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_28)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_29)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_30)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_31)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_RPU_LS)] = { mfn_init(MM_RPU) },
+    [XILPM_RESET_IDX(XILPM_RESET_PS_ONLY)] = { .hwdom_access = true },
+    [XILPM_RESET_IDX(XILPM_RESET_PL)] = { .hwdom_access = true },
+};
+
+static bool pm_check_access(const struct pm_access *acl, struct domain *d,
+                            uint32_t idx)
+{
+    if ( acl[idx].hwdom_access && is_hardware_domain(d) )
+        return true;
+
+    if ( !mfn_x(acl[idx].mfn) )
+        return false;
+
+    return iomem_access_permitted(d, mfn_x(acl[idx].mfn), mfn_x(acl[idx].mfn));
+}
+
+/* Check if a domain has access to a node.  */
+static bool domain_has_node_access(struct domain *d, uint32_t nodeid)
+{
+    if ( nodeid >= ARRAY_SIZE(pm_node_access) )
+        return false;
+
+    return pm_check_access(pm_node_access, d, nodeid);
+}
+
+/* Check if a domain has access to a reset line.  */
+static bool domain_has_reset_access(struct domain *d, uint32_t rst)
+{
+    if ( rst < XILPM_RESET_PCIE_CFG )
+        return false;
+
+    rst -= XILPM_RESET_PCIE_CFG;
+
+    if ( rst >= ARRAY_SIZE(pm_reset_access) )
+        return false;
+
+    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)
+{
+    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;
+
+    /* Scan the ACL.  */
+    for ( i = 0; i < ARRAY_SIZE(pm_mmio_access); i++ )
+    {
+        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.
+     */
+    if ( write )
+        *mask &= prot_mask;
+
+    return ret;
+}
+#endif
+
 bool zynqmp_eemi(struct cpu_user_regs *regs)
 {
     return false;
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 5/7] xen/arm: zynqmp: eemi access control

[Julien Grall]

Hi,

On 03/12/2018 21:03, Stefano Stabellini wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> 
> Introduce data structs to implement basic access controls.
> Introduce the following three functions:
> 
> domain_has_node_access: check access to the node
> domain_has_reset_access: check access to the reset line
> domain_has_mmio_access: check access to the register
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> 
> ---
> Statically defines:
> 
> - pm_node_access
> It encodes the relationship between a node id and the start of the MMIO
> region of a device in the corresponding power domain. It is used for
> permission checking. Although the MMIO region start address is available
> on device tree and could be derived from there (we plan to improve that
> in the future), the relationship between a node id and corresponding
> devices is not described and needs to be hardcoded.
> 
> - pm_reset_access
> Same as pm_node_access for reset lines.
> 
> ---
> Changes in v5:
> - improve in-code comments
> - use mfn_t in struct pm_access
> - remove mmio_access table
> 
> Changes in v4:
> - add #include as needed
> - add #if 0 for bisectability
> - use mfn_t in pm_check_access
> - add wrap-around ASSERT in domain_has_mmio_access
> - use GENMASK in domain_has_mmio_access
> - proper bound checks (== ARRAY_SIZE is out of bound)
> ---
>   xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 348 ++++++++++++++++++++++++++++
>   1 file changed, 348 insertions(+)
> 
> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> index 369bb3f..92a02df 100644
> --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> @@ -16,9 +16,357 @@
>    * GNU General Public License for more details.
>    */
>   
> +/*
> + *  EEMI Power Management API access
> + *
> + * 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
> + *
> + * Although the details of the setup are configurable, in the common case
> + * 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.
> + *
> + * In order to correctly virtualize 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 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().
> + */
> +
> +#include <xen/iocap.h>
> +#include <xen/sched.h>
>   #include <asm/regs.h>
>   #include <asm/platforms/xilinx-zynqmp-eemi.h>
>   
> +#if 0
> +struct pm_access
> +{
> +    mfn_t mfn;
> +    bool hwdom_access;    /* HW domain gets access regardless.  */
> +};
> +
> +/*
> + * This table maps a node into a memory address.

Some of the nodes below don't have memory address. So this comment has to be 
updated.

> + * If a guest has access to the address, it has enough control
> + * over the node to grant it access to EEMI calls for that node.
> + */
> +static const struct pm_access pm_node_access[] = {
> +    /* MM_RPU grants access to all RPU Nodes.  */
> +    [NODE_RPU] = { mfn_init(MM_RPU) },
> +    [NODE_RPU_0] = { mfn_init(MM_RPU) },
> +    [NODE_RPU_1] = { mfn_init(MM_RPU) },
> +    [NODE_IPI_RPU_0] = { mfn_init(MM_RPU) },
> +
> +    /* GPU nodes.  */
> +    [NODE_GPU] = { mfn_init(MM_GPU) },
> +    [NODE_GPU_PP_0] = { mfn_init(MM_GPU) },
> +    [NODE_GPU_PP_1] = { mfn_init(MM_GPU) },
> +
> +    [NODE_USB_0] = { mfn_init(MM_USB3_0_XHCI) },
> +    [NODE_USB_1] = { mfn_init(MM_USB3_1_XHCI) },
> +    [NODE_TTC_0] = { mfn_init(MM_TTC0) },
> +    [NODE_TTC_1] = { mfn_init(MM_TTC1) },
> +    [NODE_TTC_2] = { mfn_init(MM_TTC2) },
> +    [NODE_TTC_3] = { mfn_init(MM_TTC3) },
> +    [NODE_SATA] = { mfn_init(MM_SATA_AHCI_HBA) },
> +    [NODE_ETH_0] = { mfn_init(MM_GEM0) },
> +    [NODE_ETH_1] = { mfn_init(MM_GEM1) },
> +    [NODE_ETH_2] = { mfn_init(MM_GEM2) },
> +    [NODE_ETH_3] = { mfn_init(MM_GEM3) },
> +    [NODE_UART_0] = { mfn_init(MM_UART0) },
> +    [NODE_UART_1] = { mfn_init(MM_UART1) },
> +    [NODE_SPI_0] = { mfn_init(MM_SPI0) },
> +    [NODE_SPI_1] = { mfn_init(MM_SPI1) },
> +    [NODE_I2C_0] = { mfn_init(MM_I2C0) },
> +    [NODE_I2C_1] = { mfn_init(MM_I2C1) },
> +    [NODE_SD_0] = { mfn_init(MM_SD0) },
> +    [NODE_SD_1] = { mfn_init(MM_SD1) },
> +    [NODE_DP] = { mfn_init(MM_DP) },
> +
> +    /* Guest with GDMA Channel 0 gets PM access. Other guests don't.  */
> +    [NODE_GDMA] = { mfn_init(MM_GDMA_CH0) },
> +    /* Guest with ADMA Channel 0 gets PM access. Other guests don't.  */
> +    [NODE_ADMA] = { mfn_init(MM_ADMA_CH0) },
> +
> +    [NODE_NAND] = { mfn_init(MM_NAND) },
> +    [NODE_QSPI] = { mfn_init(MM_QSPI) },
> +    [NODE_GPIO] = { mfn_init(MM_GPIO) },
> +    [NODE_CAN_0] = { mfn_init(MM_CAN0) },
> +    [NODE_CAN_1] = { mfn_init(MM_CAN1) },
> +
> +    /* Only for the hardware domain.  */
> +    [NODE_AFI] = { .hwdom_access = true },
> +    [NODE_APLL] = { .hwdom_access = true },
> +    [NODE_VPLL] = { .hwdom_access = true },
> +    [NODE_DPLL] = { .hwdom_access = true },
> +    [NODE_RPLL] = { .hwdom_access = true },
> +    [NODE_IOPLL] = { .hwdom_access = true },
> +    [NODE_DDR] = { .hwdom_access = true },
> +    [NODE_IPI_APU] = { .hwdom_access = true },
> +    [NODE_PCAP] = { .hwdom_access = true },
> +
> +    [NODE_PCIE] = { mfn_init(MM_PCIE_ATTRIB) },
> +    [NODE_RTC] = { mfn_init(MM_RTC) },
> +};
> +
> +/*
> + * This table maps reset line IDs into a memory address.

Same here.

> + * 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.
> + */
> +#define XILPM_RESET_IDX(n) (n - XILPM_RESET_PCIE_CFG)
> +static const struct pm_access pm_reset_access[] = {
> +    [XILPM_RESET_IDX(XILPM_RESET_PCIE_CFG)] = { mfn_init(MM_AXIPCIE_MAIN) },
> +    [XILPM_RESET_IDX(XILPM_RESET_PCIE_BRIDGE)] = { mfn_init(MM_PCIE_ATTRIB) },
> +    [XILPM_RESET_IDX(XILPM_RESET_PCIE_CTRL)] = { mfn_init(MM_PCIE_ATTRIB) },
> +
> +    [XILPM_RESET_IDX(XILPM_RESET_DP)] = { mfn_init(MM_DP) },
> +    [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRF)] = { mfn_init(MM_SWDT) },
> +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM5)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM4)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM3)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM2)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM1)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM0)] = { .hwdom_access = true },
> +
> +    /* Channel 0 grants PM access.  */
> +    [XILPM_RESET_IDX(XILPM_RESET_GDMA)] = { mfn_init(MM_GDMA_CH0) },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPU_PP1)] = { mfn_init(MM_GPU) },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPU_PP0)] = { mfn_init(MM_GPU) },
> +    [XILPM_RESET_IDX(XILPM_RESET_GT)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_SATA)] = { mfn_init(MM_SATA_AHCI_HBA) },
> +
> +    [XILPM_RESET_IDX(XILPM_RESET_APM_FPD)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_SOFT)] = { .hwdom_access = true },
> +
> +    [XILPM_RESET_IDX(XILPM_RESET_GEM0)] = { mfn_init(MM_GEM0) },
> +    [XILPM_RESET_IDX(XILPM_RESET_GEM1)] = { mfn_init(MM_GEM1) },
> +    [XILPM_RESET_IDX(XILPM_RESET_GEM2)] = { mfn_init(MM_GEM2) },
> +    [XILPM_RESET_IDX(XILPM_RESET_GEM3)] = { mfn_init(MM_GEM3) },
> +
> +    [XILPM_RESET_IDX(XILPM_RESET_QSPI)] = { mfn_init(MM_QSPI) },
> +    [XILPM_RESET_IDX(XILPM_RESET_UART0)] = { mfn_init(MM_UART0) },
> +    [XILPM_RESET_IDX(XILPM_RESET_UART1)] = { mfn_init(MM_UART1) },
> +    [XILPM_RESET_IDX(XILPM_RESET_SPI0)] = { mfn_init(MM_SPI0) },
> +    [XILPM_RESET_IDX(XILPM_RESET_SPI1)] = { mfn_init(MM_SPI1) },
> +    [XILPM_RESET_IDX(XILPM_RESET_SDIO0)] = { mfn_init(MM_SD0) },
> +    [XILPM_RESET_IDX(XILPM_RESET_SDIO1)] = { mfn_init(MM_SD1) },
> +    [XILPM_RESET_IDX(XILPM_RESET_CAN0)] = { mfn_init(MM_CAN0) },
> +    [XILPM_RESET_IDX(XILPM_RESET_CAN1)] = { mfn_init(MM_CAN1) },
> +    [XILPM_RESET_IDX(XILPM_RESET_I2C0)] = { mfn_init(MM_I2C0) },
> +    [XILPM_RESET_IDX(XILPM_RESET_I2C1)] = { mfn_init(MM_I2C1) },
> +    [XILPM_RESET_IDX(XILPM_RESET_TTC0)] = { mfn_init(MM_TTC0) },
> +    [XILPM_RESET_IDX(XILPM_RESET_TTC1)] = { mfn_init(MM_TTC1) },
> +    [XILPM_RESET_IDX(XILPM_RESET_TTC2)] = { mfn_init(MM_TTC2) },
> +    [XILPM_RESET_IDX(XILPM_RESET_TTC3)] = { mfn_init(MM_TTC3) },
> +    [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRL)] = { mfn_init(MM_SWDT) },
> +    [XILPM_RESET_IDX(XILPM_RESET_NAND)] = { mfn_init(MM_NAND) },
> +    [XILPM_RESET_IDX(XILPM_RESET_ADMA)] = { mfn_init(MM_ADMA_CH0) },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPIO)] = { mfn_init(MM_GPIO) },
> +    [XILPM_RESET_IDX(XILPM_RESET_IOU_CC)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_TIMESTAMP)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_RPU_R50)] = { mfn_init(MM_RPU) },
> +    [XILPM_RESET_IDX(XILPM_RESET_RPU_R51)] = { mfn_init(MM_RPU) },
> +    [XILPM_RESET_IDX(XILPM_RESET_RPU_AMBA)] = { mfn_init(MM_RPU) },
> +    [XILPM_RESET_IDX(XILPM_RESET_OCM)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_RPU_PGE)] = { mfn_init(MM_RPU) },
> +
> +    [XILPM_RESET_IDX(XILPM_RESET_USB0_CORERESET)] = { mfn_init(MM_USB3_0_XHCI) },
> +    [XILPM_RESET_IDX(XILPM_RESET_USB0_HIBERRESET)] = { mfn_init(MM_USB3_0_XHCI) },
> +    [XILPM_RESET_IDX(XILPM_RESET_USB0_APB)] = { mfn_init(MM_USB3_0_XHCI) },
> +
> +    [XILPM_RESET_IDX(XILPM_RESET_USB1_CORERESET)] = { mfn_init(MM_USB3_1_XHCI) },
> +    [XILPM_RESET_IDX(XILPM_RESET_USB1_HIBERRESET)] = { mfn_init(MM_USB3_1_XHCI) },
> +    [XILPM_RESET_IDX(XILPM_RESET_USB1_APB)] = { mfn_init(MM_USB3_1_XHCI) },
> +
> +    [XILPM_RESET_IDX(XILPM_RESET_IPI)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_APM_LPD)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_RTC)] = { mfn_init(MM_RTC) },
> +    [XILPM_RESET_IDX(XILPM_RESET_SYSMON)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM6)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_LPD_SWDT)] = { mfn_init(MM_SWDT) },
> +    [XILPM_RESET_IDX(XILPM_RESET_FPD)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG1)] = { mfn_init(MM_RPU) },
> +    [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG0)] = { mfn_init(MM_RPU) },
> +    [XILPM_RESET_IDX(XILPM_RESET_DBG_LPD)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_DBG_FPD)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_APLL)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_DPLL)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_VPLL)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_IOPLL)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_RPLL)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_0)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_1)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_2)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_3)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_4)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_5)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_6)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_7)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_8)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_9)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_10)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_11)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_12)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_13)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_14)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_15)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_16)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_17)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_18)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_19)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_20)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_21)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_22)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_23)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_24)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_25)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_26)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_27)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_28)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_29)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_30)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_31)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_RPU_LS)] = { mfn_init(MM_RPU) },
> +    [XILPM_RESET_IDX(XILPM_RESET_PS_ONLY)] = { .hwdom_access = true },
> +    [XILPM_RESET_IDX(XILPM_RESET_PL)] = { .hwdom_access = true },
> +};
> +
> +static bool pm_check_access(const struct pm_access *acl, struct domain *d,
> +                            uint32_t idx)
> +{
> +    if ( acl[idx].hwdom_access && is_hardware_domain(d) )
> +        return true;
> +
> +    if ( !mfn_x(acl[idx].mfn) )

Technically 0 is a valid mfn. If you want to encode an invalid value then 
MFN_INVALID is safer.

But what are you trying to prevent? Are the node IDs not allocated contiguously?

> +        return false;
> +
> +    return iomem_access_permitted(d, mfn_x(acl[idx].mfn), mfn_x(acl[idx].mfn));
> +}
> +
> +/* Check if a domain has access to a node.  */
> +static bool domain_has_node_access(struct domain *d, uint32_t nodeid)
> +{
> +    if ( nodeid >= ARRAY_SIZE(pm_node_access) )
> +        return false;
> +
> +    return pm_check_access(pm_node_access, d, nodeid);
> +}
> +
> +/* Check if a domain has access to a reset line.  */
> +static bool domain_has_reset_access(struct domain *d, uint32_t rst)
> +{
> +    if ( rst < XILPM_RESET_PCIE_CFG )
> +        return false;
> +
> +    rst -= XILPM_RESET_PCIE_CFG;
> +
> +    if ( rst >= ARRAY_SIZE(pm_reset_access) )
> +        return false;

I think you can make this code no rely at all on XILPM_RESET_PCIE_CFG by using 
XILPM_RESET_IDX.

if ( XILPM_RESET_IDX(rst) >= ARRAY_SIZE(pm_reset_access) )
    return false;

rst = XILPM_RESET_IDX(rst);

We rely on the unsigned underflow to catch value below XILPM_RESET_PCIE_CFG and 
make the code less error prone to change the array without the code here.

> +
> +    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)
> +{
> +    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;
> +
> +    /* Scan the ACL.  */
> +    for ( i = 0; i < ARRAY_SIZE(pm_mmio_access); i++ )
> +    {
> +        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);

The field mask does not seem to exist.

> +        break;
> +    }
> +
> +    /*
> +     * Masking only applies to writes: values are safe to read, but not
> +     * all bits are writeable.
> +     */
> +    if ( write )
> +        *mask &= prot_mask;
> +
> +    return ret;
> +}
> +#endif
> +
>   bool zynqmp_eemi(struct cpu_user_regs *regs)
>   {
>       return false;
> 

Cheers,


-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 5/7] xen/arm: zynqmp: eemi access control

[Stefano Stabellini]

On Tue, 11 Dec 2018, Julien Grall wrote:
> Hi,
> 
> On 03/12/2018 21:03, Stefano Stabellini wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > 
> > From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > 
> > Introduce data structs to implement basic access controls.
> > Introduce the following three functions:
> > 
> > domain_has_node_access: check access to the node
> > domain_has_reset_access: check access to the reset line
> > domain_has_mmio_access: check access to the register
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > 
> > ---
> > Statically defines:
> > 
> > - pm_node_access
> > It encodes the relationship between a node id and the start of the MMIO
> > region of a device in the corresponding power domain. It is used for
> > permission checking. Although the MMIO region start address is available
> > on device tree and could be derived from there (we plan to improve that
> > in the future), the relationship between a node id and corresponding
> > devices is not described and needs to be hardcoded.
> > 
> > - pm_reset_access
> > Same as pm_node_access for reset lines.
> > 
> > ---
> > Changes in v5:
> > - improve in-code comments
> > - use mfn_t in struct pm_access
> > - remove mmio_access table
> > 
> > Changes in v4:
> > - add #include as needed
> > - add #if 0 for bisectability
> > - use mfn_t in pm_check_access
> > - add wrap-around ASSERT in domain_has_mmio_access
> > - use GENMASK in domain_has_mmio_access
> > - proper bound checks (== ARRAY_SIZE is out of bound)
> > ---
> >   xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 348
> > ++++++++++++++++++++++++++++
> >   1 file changed, 348 insertions(+)
> > 
> > diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > index 369bb3f..92a02df 100644
> > --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
> > @@ -16,9 +16,357 @@
> >    * GNU General Public License for more details.
> >    */
> >   +/*
> > + *  EEMI Power Management API access
> > + *
> > + * 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
> > + *
> > + * Although the details of the setup are configurable, in the common case
> > + * 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.
> > + *
> > + * In order to correctly virtualize 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 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().
> > + */
> > +
> > +#include <xen/iocap.h>
> > +#include <xen/sched.h>
> >   #include <asm/regs.h>
> >   #include <asm/platforms/xilinx-zynqmp-eemi.h>
> >   +#if 0
> > +struct pm_access
> > +{
> > +    mfn_t mfn;
> > +    bool hwdom_access;    /* HW domain gets access regardless.  */
> > +};
> > +
> > +/*
> > + * This table maps a node into a memory address.
> 
> Some of the nodes below don't have memory address. So this comment has to be
> updated.

Yes, I'll update and improve the comment


> > + * If a guest has access to the address, it has enough control
> > + * over the node to grant it access to EEMI calls for that node.
> > + */
> > +static const struct pm_access pm_node_access[] = {
> > +    /* MM_RPU grants access to all RPU Nodes.  */
> > +    [NODE_RPU] = { mfn_init(MM_RPU) },
> > +    [NODE_RPU_0] = { mfn_init(MM_RPU) },
> > +    [NODE_RPU_1] = { mfn_init(MM_RPU) },
> > +    [NODE_IPI_RPU_0] = { mfn_init(MM_RPU) },
> > +
> > +    /* GPU nodes.  */
> > +    [NODE_GPU] = { mfn_init(MM_GPU) },
> > +    [NODE_GPU_PP_0] = { mfn_init(MM_GPU) },
> > +    [NODE_GPU_PP_1] = { mfn_init(MM_GPU) },
> > +
> > +    [NODE_USB_0] = { mfn_init(MM_USB3_0_XHCI) },
> > +    [NODE_USB_1] = { mfn_init(MM_USB3_1_XHCI) },
> > +    [NODE_TTC_0] = { mfn_init(MM_TTC0) },
> > +    [NODE_TTC_1] = { mfn_init(MM_TTC1) },
> > +    [NODE_TTC_2] = { mfn_init(MM_TTC2) },
> > +    [NODE_TTC_3] = { mfn_init(MM_TTC3) },
> > +    [NODE_SATA] = { mfn_init(MM_SATA_AHCI_HBA) },
> > +    [NODE_ETH_0] = { mfn_init(MM_GEM0) },
> > +    [NODE_ETH_1] = { mfn_init(MM_GEM1) },
> > +    [NODE_ETH_2] = { mfn_init(MM_GEM2) },
> > +    [NODE_ETH_3] = { mfn_init(MM_GEM3) },
> > +    [NODE_UART_0] = { mfn_init(MM_UART0) },
> > +    [NODE_UART_1] = { mfn_init(MM_UART1) },
> > +    [NODE_SPI_0] = { mfn_init(MM_SPI0) },
> > +    [NODE_SPI_1] = { mfn_init(MM_SPI1) },
> > +    [NODE_I2C_0] = { mfn_init(MM_I2C0) },
> > +    [NODE_I2C_1] = { mfn_init(MM_I2C1) },
> > +    [NODE_SD_0] = { mfn_init(MM_SD0) },
> > +    [NODE_SD_1] = { mfn_init(MM_SD1) },
> > +    [NODE_DP] = { mfn_init(MM_DP) },
> > +
> > +    /* Guest with GDMA Channel 0 gets PM access. Other guests don't.  */
> > +    [NODE_GDMA] = { mfn_init(MM_GDMA_CH0) },
> > +    /* Guest with ADMA Channel 0 gets PM access. Other guests don't.  */
> > +    [NODE_ADMA] = { mfn_init(MM_ADMA_CH0) },
> > +
> > +    [NODE_NAND] = { mfn_init(MM_NAND) },
> > +    [NODE_QSPI] = { mfn_init(MM_QSPI) },
> > +    [NODE_GPIO] = { mfn_init(MM_GPIO) },
> > +    [NODE_CAN_0] = { mfn_init(MM_CAN0) },
> > +    [NODE_CAN_1] = { mfn_init(MM_CAN1) },
> > +
> > +    /* Only for the hardware domain.  */
> > +    [NODE_AFI] = { .hwdom_access = true },
> > +    [NODE_APLL] = { .hwdom_access = true },
> > +    [NODE_VPLL] = { .hwdom_access = true },
> > +    [NODE_DPLL] = { .hwdom_access = true },
> > +    [NODE_RPLL] = { .hwdom_access = true },
> > +    [NODE_IOPLL] = { .hwdom_access = true },
> > +    [NODE_DDR] = { .hwdom_access = true },
> > +    [NODE_IPI_APU] = { .hwdom_access = true },
> > +    [NODE_PCAP] = { .hwdom_access = true },
> > +
> > +    [NODE_PCIE] = { mfn_init(MM_PCIE_ATTRIB) },
> > +    [NODE_RTC] = { mfn_init(MM_RTC) },
> > +};
> > +
> > +/*
> > + * This table maps reset line IDs into a memory address.
> 
> Same here.
> 
> > + * 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.
> > + */
> > +#define XILPM_RESET_IDX(n) (n - XILPM_RESET_PCIE_CFG)
> > +static const struct pm_access pm_reset_access[] = {
> > +    [XILPM_RESET_IDX(XILPM_RESET_PCIE_CFG)] = { mfn_init(MM_AXIPCIE_MAIN)
> > },
> > +    [XILPM_RESET_IDX(XILPM_RESET_PCIE_BRIDGE)] = { mfn_init(MM_PCIE_ATTRIB)
> > },
> > +    [XILPM_RESET_IDX(XILPM_RESET_PCIE_CTRL)] = { mfn_init(MM_PCIE_ATTRIB)
> > },
> > +
> > +    [XILPM_RESET_IDX(XILPM_RESET_DP)] = { mfn_init(MM_DP) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRF)] = { mfn_init(MM_SWDT) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM5)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM4)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM3)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM2)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM1)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM0)] = { .hwdom_access = true },
> > +
> > +    /* Channel 0 grants PM access.  */
> > +    [XILPM_RESET_IDX(XILPM_RESET_GDMA)] = { mfn_init(MM_GDMA_CH0) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPU_PP1)] = { mfn_init(MM_GPU) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPU_PP0)] = { mfn_init(MM_GPU) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GT)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_SATA)] = { mfn_init(MM_SATA_AHCI_HBA) },
> > +
> > +    [XILPM_RESET_IDX(XILPM_RESET_APM_FPD)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_SOFT)] = { .hwdom_access = true },
> > +
> > +    [XILPM_RESET_IDX(XILPM_RESET_GEM0)] = { mfn_init(MM_GEM0) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GEM1)] = { mfn_init(MM_GEM1) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GEM2)] = { mfn_init(MM_GEM2) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GEM3)] = { mfn_init(MM_GEM3) },
> > +
> > +    [XILPM_RESET_IDX(XILPM_RESET_QSPI)] = { mfn_init(MM_QSPI) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_UART0)] = { mfn_init(MM_UART0) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_UART1)] = { mfn_init(MM_UART1) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_SPI0)] = { mfn_init(MM_SPI0) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_SPI1)] = { mfn_init(MM_SPI1) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_SDIO0)] = { mfn_init(MM_SD0) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_SDIO1)] = { mfn_init(MM_SD1) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_CAN0)] = { mfn_init(MM_CAN0) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_CAN1)] = { mfn_init(MM_CAN1) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_I2C0)] = { mfn_init(MM_I2C0) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_I2C1)] = { mfn_init(MM_I2C1) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_TTC0)] = { mfn_init(MM_TTC0) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_TTC1)] = { mfn_init(MM_TTC1) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_TTC2)] = { mfn_init(MM_TTC2) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_TTC3)] = { mfn_init(MM_TTC3) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRL)] = { mfn_init(MM_SWDT) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_NAND)] = { mfn_init(MM_NAND) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_ADMA)] = { mfn_init(MM_ADMA_CH0) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPIO)] = { mfn_init(MM_GPIO) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_IOU_CC)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_TIMESTAMP)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_RPU_R50)] = { mfn_init(MM_RPU) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_RPU_R51)] = { mfn_init(MM_RPU) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_RPU_AMBA)] = { mfn_init(MM_RPU) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_OCM)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_RPU_PGE)] = { mfn_init(MM_RPU) },
> > +
> > +    [XILPM_RESET_IDX(XILPM_RESET_USB0_CORERESET)] = {
> > mfn_init(MM_USB3_0_XHCI) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_USB0_HIBERRESET)] = {
> > mfn_init(MM_USB3_0_XHCI) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_USB0_APB)] = { mfn_init(MM_USB3_0_XHCI) },
> > +
> > +    [XILPM_RESET_IDX(XILPM_RESET_USB1_CORERESET)] = {
> > mfn_init(MM_USB3_1_XHCI) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_USB1_HIBERRESET)] = {
> > mfn_init(MM_USB3_1_XHCI) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_USB1_APB)] = { mfn_init(MM_USB3_1_XHCI) },
> > +
> > +    [XILPM_RESET_IDX(XILPM_RESET_IPI)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_APM_LPD)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_RTC)] = { mfn_init(MM_RTC) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_SYSMON)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM6)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_LPD_SWDT)] = { mfn_init(MM_SWDT) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_FPD)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG1)] = { mfn_init(MM_RPU) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG0)] = { mfn_init(MM_RPU) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_DBG_LPD)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_DBG_FPD)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_APLL)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_DPLL)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_VPLL)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_IOPLL)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_RPLL)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_0)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_1)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_2)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_3)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_4)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_5)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_6)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_7)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_8)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_9)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_10)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_11)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_12)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_13)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_14)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_15)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_16)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_17)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_18)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_19)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_20)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_21)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_22)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_23)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_24)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_25)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_26)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_27)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_28)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_29)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_30)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_31)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_RPU_LS)] = { mfn_init(MM_RPU) },
> > +    [XILPM_RESET_IDX(XILPM_RESET_PS_ONLY)] = { .hwdom_access = true },
> > +    [XILPM_RESET_IDX(XILPM_RESET_PL)] = { .hwdom_access = true },
> > +};
> > +
> > +static bool pm_check_access(const struct pm_access *acl, struct domain *d,
> > +                            uint32_t idx)
> > +{
> > +    if ( acl[idx].hwdom_access && is_hardware_domain(d) )
> > +        return true;
> > +
> > +    if ( !mfn_x(acl[idx].mfn) )
> 
> Technically 0 is a valid mfn. If you want to encode an invalid value then
> MFN_INVALID is safer.
> 
> But what are you trying to prevent? Are the node IDs not allocated
> contiguously?

I improved the comments above now. But the idea is that a zero address
means nobody has access. None of those resources have actually a zero
address, so there are no risks of conflicts with any valid address zero.

Following a comment by Jan, I have also started using MFN_INVALID to
encode only dom0 has access, getting rid of .hwdom_access completely.

So in the next series:

- address -> regular check
- 0 -> NO
- INVALID_MFN -> dom0 only


> > +        return false;
> > +
> > +    return iomem_access_permitted(d, mfn_x(acl[idx].mfn),
> > mfn_x(acl[idx].mfn));
> > +}
> > +
> > +/* Check if a domain has access to a node.  */
> > +static bool domain_has_node_access(struct domain *d, uint32_t nodeid)
> > +{
> > +    if ( nodeid >= ARRAY_SIZE(pm_node_access) )
> > +        return false;
> > +
> > +    return pm_check_access(pm_node_access, d, nodeid);
> > +}
> > +
> > +/* Check if a domain has access to a reset line.  */
> > +static bool domain_has_reset_access(struct domain *d, uint32_t rst)
> > +{
> > +    if ( rst < XILPM_RESET_PCIE_CFG )
> > +        return false;
> > +
> > +    rst -= XILPM_RESET_PCIE_CFG;
> > +
> > +    if ( rst >= ARRAY_SIZE(pm_reset_access) )
> > +        return false;
> 
> I think you can make this code no rely at all on XILPM_RESET_PCIE_CFG by using
> XILPM_RESET_IDX.
> 
> if ( XILPM_RESET_IDX(rst) >= ARRAY_SIZE(pm_reset_access) )
>    return false;
> 
> rst = XILPM_RESET_IDX(rst);
> 
> We rely on the unsigned underflow to catch value below XILPM_RESET_PCIE_CFG
> and make the code less error prone to change the array without the code here.

Good idea, I'll do that


> > +
> > +    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)
> > +{
> > +    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;
> > +
> > +    /* Scan the ACL.  */
> > +    for ( i = 0; i < ARRAY_SIZE(pm_mmio_access); i++ )
> > +    {
> > +        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);
> 
> The field mask does not seem to exist.

This code is fully gone now


> > +        break;
> > +    }
> > +
> > +    /*
> > +     * Masking only applies to writes: values are safe to read, but not
> > +     * all bits are writeable.
> > +     */
> > +    if ( write )
> > +        *mask &= prot_mask;
> > +
> > +    return ret;
> > +}
> > +#endif
> > +
> >   bool zynqmp_eemi(struct cpu_user_regs *regs)
> >   {
> >       return false;

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 5/7] xen/arm: zynqmp: eemi access control

[Julien Grall]

Hi Stefano,

On 12/12/18 11:57 PM, Stefano Stabellini wrote:
> On Tue, 11 Dec 2018, Julien Grall wrote:
>> Hi,
>>
>> On 03/12/2018 21:03, Stefano Stabellini wrote:
>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>
>>> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>
>>> Introduce data structs to implement basic access controls.
>>> Introduce the following three functions:
>>>
>>> domain_has_node_access: check access to the node
>>> domain_has_reset_access: check access to the reset line
>>> domain_has_mmio_access: check access to the register
>>>
>>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>>
>>> ---
>>> Statically defines:
>>>
>>> - pm_node_access
>>> It encodes the relationship between a node id and the start of the MMIO
>>> region of a device in the corresponding power domain. It is used for
>>> permission checking. Although the MMIO region start address is available
>>> on device tree and could be derived from there (we plan to improve that
>>> in the future), the relationship between a node id and corresponding
>>> devices is not described and needs to be hardcoded.
>>>
>>> - pm_reset_access
>>> Same as pm_node_access for reset lines.
>>>
>>> ---
>>> Changes in v5:
>>> - improve in-code comments
>>> - use mfn_t in struct pm_access
>>> - remove mmio_access table
>>>
>>> Changes in v4:
>>> - add #include as needed
>>> - add #if 0 for bisectability
>>> - use mfn_t in pm_check_access
>>> - add wrap-around ASSERT in domain_has_mmio_access
>>> - use GENMASK in domain_has_mmio_access
>>> - proper bound checks (== ARRAY_SIZE is out of bound)
>>> ---
>>>    xen/arch/arm/platforms/xilinx-zynqmp-eemi.c | 348
>>> ++++++++++++++++++++++++++++
>>>    1 file changed, 348 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> index 369bb3f..92a02df 100644
>>> --- a/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> +++ b/xen/arch/arm/platforms/xilinx-zynqmp-eemi.c
>>> @@ -16,9 +16,357 @@
>>>     * GNU General Public License for more details.
>>>     */
>>>    +/*
>>> + *  EEMI Power Management API access
>>> + *
>>> + * 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
>>> + *
>>> + * Although the details of the setup are configurable, in the common case
>>> + * 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.
>>> + *
>>> + * In order to correctly virtualize 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 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().
>>> + */
>>> +
>>> +#include <xen/iocap.h>
>>> +#include <xen/sched.h>
>>>    #include <asm/regs.h>
>>>    #include <asm/platforms/xilinx-zynqmp-eemi.h>
>>>    +#if 0
>>> +struct pm_access
>>> +{
>>> +    mfn_t mfn;
>>> +    bool hwdom_access;    /* HW domain gets access regardless.  */
>>> +};
>>> +
>>> +/*
>>> + * This table maps a node into a memory address.
>>
>> Some of the nodes below don't have memory address. So this comment has to be
>> updated.
> 
> Yes, I'll update and improve the comment
> 
> 
>>> + * If a guest has access to the address, it has enough control
>>> + * over the node to grant it access to EEMI calls for that node.
>>> + */
>>> +static const struct pm_access pm_node_access[] = {
>>> +    /* MM_RPU grants access to all RPU Nodes.  */
>>> +    [NODE_RPU] = { mfn_init(MM_RPU) },
>>> +    [NODE_RPU_0] = { mfn_init(MM_RPU) },
>>> +    [NODE_RPU_1] = { mfn_init(MM_RPU) },
>>> +    [NODE_IPI_RPU_0] = { mfn_init(MM_RPU) },
>>> +
>>> +    /* GPU nodes.  */
>>> +    [NODE_GPU] = { mfn_init(MM_GPU) },
>>> +    [NODE_GPU_PP_0] = { mfn_init(MM_GPU) },
>>> +    [NODE_GPU_PP_1] = { mfn_init(MM_GPU) },
>>> +
>>> +    [NODE_USB_0] = { mfn_init(MM_USB3_0_XHCI) },
>>> +    [NODE_USB_1] = { mfn_init(MM_USB3_1_XHCI) },
>>> +    [NODE_TTC_0] = { mfn_init(MM_TTC0) },
>>> +    [NODE_TTC_1] = { mfn_init(MM_TTC1) },
>>> +    [NODE_TTC_2] = { mfn_init(MM_TTC2) },
>>> +    [NODE_TTC_3] = { mfn_init(MM_TTC3) },
>>> +    [NODE_SATA] = { mfn_init(MM_SATA_AHCI_HBA) },
>>> +    [NODE_ETH_0] = { mfn_init(MM_GEM0) },
>>> +    [NODE_ETH_1] = { mfn_init(MM_GEM1) },
>>> +    [NODE_ETH_2] = { mfn_init(MM_GEM2) },
>>> +    [NODE_ETH_3] = { mfn_init(MM_GEM3) },
>>> +    [NODE_UART_0] = { mfn_init(MM_UART0) },
>>> +    [NODE_UART_1] = { mfn_init(MM_UART1) },
>>> +    [NODE_SPI_0] = { mfn_init(MM_SPI0) },
>>> +    [NODE_SPI_1] = { mfn_init(MM_SPI1) },
>>> +    [NODE_I2C_0] = { mfn_init(MM_I2C0) },
>>> +    [NODE_I2C_1] = { mfn_init(MM_I2C1) },
>>> +    [NODE_SD_0] = { mfn_init(MM_SD0) },
>>> +    [NODE_SD_1] = { mfn_init(MM_SD1) },
>>> +    [NODE_DP] = { mfn_init(MM_DP) },
>>> +
>>> +    /* Guest with GDMA Channel 0 gets PM access. Other guests don't.  */
>>> +    [NODE_GDMA] = { mfn_init(MM_GDMA_CH0) },
>>> +    /* Guest with ADMA Channel 0 gets PM access. Other guests don't.  */
>>> +    [NODE_ADMA] = { mfn_init(MM_ADMA_CH0) },
>>> +
>>> +    [NODE_NAND] = { mfn_init(MM_NAND) },
>>> +    [NODE_QSPI] = { mfn_init(MM_QSPI) },
>>> +    [NODE_GPIO] = { mfn_init(MM_GPIO) },
>>> +    [NODE_CAN_0] = { mfn_init(MM_CAN0) },
>>> +    [NODE_CAN_1] = { mfn_init(MM_CAN1) },
>>> +
>>> +    /* Only for the hardware domain.  */
>>> +    [NODE_AFI] = { .hwdom_access = true },
>>> +    [NODE_APLL] = { .hwdom_access = true },
>>> +    [NODE_VPLL] = { .hwdom_access = true },
>>> +    [NODE_DPLL] = { .hwdom_access = true },
>>> +    [NODE_RPLL] = { .hwdom_access = true },
>>> +    [NODE_IOPLL] = { .hwdom_access = true },
>>> +    [NODE_DDR] = { .hwdom_access = true },
>>> +    [NODE_IPI_APU] = { .hwdom_access = true },
>>> +    [NODE_PCAP] = { .hwdom_access = true },
>>> +
>>> +    [NODE_PCIE] = { mfn_init(MM_PCIE_ATTRIB) },
>>> +    [NODE_RTC] = { mfn_init(MM_RTC) },
>>> +};
>>> +
>>> +/*
>>> + * This table maps reset line IDs into a memory address.
>>
>> Same here.
>>
>>> + * 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.
>>> + */
>>> +#define XILPM_RESET_IDX(n) (n - XILPM_RESET_PCIE_CFG)
>>> +static const struct pm_access pm_reset_access[] = {
>>> +    [XILPM_RESET_IDX(XILPM_RESET_PCIE_CFG)] = { mfn_init(MM_AXIPCIE_MAIN)
>>> },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_PCIE_BRIDGE)] = { mfn_init(MM_PCIE_ATTRIB)
>>> },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_PCIE_CTRL)] = { mfn_init(MM_PCIE_ATTRIB)
>>> },
>>> +
>>> +    [XILPM_RESET_IDX(XILPM_RESET_DP)] = { mfn_init(MM_DP) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRF)] = { mfn_init(MM_SWDT) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM5)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM4)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM3)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM2)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM1)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM0)] = { .hwdom_access = true },
>>> +
>>> +    /* Channel 0 grants PM access.  */
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GDMA)] = { mfn_init(MM_GDMA_CH0) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPU_PP1)] = { mfn_init(MM_GPU) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPU_PP0)] = { mfn_init(MM_GPU) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GT)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_SATA)] = { mfn_init(MM_SATA_AHCI_HBA) },
>>> +
>>> +    [XILPM_RESET_IDX(XILPM_RESET_APM_FPD)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_SOFT)] = { .hwdom_access = true },
>>> +
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GEM0)] = { mfn_init(MM_GEM0) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GEM1)] = { mfn_init(MM_GEM1) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GEM2)] = { mfn_init(MM_GEM2) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GEM3)] = { mfn_init(MM_GEM3) },
>>> +
>>> +    [XILPM_RESET_IDX(XILPM_RESET_QSPI)] = { mfn_init(MM_QSPI) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_UART0)] = { mfn_init(MM_UART0) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_UART1)] = { mfn_init(MM_UART1) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_SPI0)] = { mfn_init(MM_SPI0) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_SPI1)] = { mfn_init(MM_SPI1) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_SDIO0)] = { mfn_init(MM_SD0) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_SDIO1)] = { mfn_init(MM_SD1) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_CAN0)] = { mfn_init(MM_CAN0) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_CAN1)] = { mfn_init(MM_CAN1) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_I2C0)] = { mfn_init(MM_I2C0) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_I2C1)] = { mfn_init(MM_I2C1) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_TTC0)] = { mfn_init(MM_TTC0) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_TTC1)] = { mfn_init(MM_TTC1) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_TTC2)] = { mfn_init(MM_TTC2) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_TTC3)] = { mfn_init(MM_TTC3) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_SWDT_CRL)] = { mfn_init(MM_SWDT) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_NAND)] = { mfn_init(MM_NAND) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_ADMA)] = { mfn_init(MM_ADMA_CH0) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPIO)] = { mfn_init(MM_GPIO) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_IOU_CC)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_TIMESTAMP)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_RPU_R50)] = { mfn_init(MM_RPU) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_RPU_R51)] = { mfn_init(MM_RPU) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_RPU_AMBA)] = { mfn_init(MM_RPU) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_OCM)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_RPU_PGE)] = { mfn_init(MM_RPU) },
>>> +
>>> +    [XILPM_RESET_IDX(XILPM_RESET_USB0_CORERESET)] = {
>>> mfn_init(MM_USB3_0_XHCI) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_USB0_HIBERRESET)] = {
>>> mfn_init(MM_USB3_0_XHCI) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_USB0_APB)] = { mfn_init(MM_USB3_0_XHCI) },
>>> +
>>> +    [XILPM_RESET_IDX(XILPM_RESET_USB1_CORERESET)] = {
>>> mfn_init(MM_USB3_1_XHCI) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_USB1_HIBERRESET)] = {
>>> mfn_init(MM_USB3_1_XHCI) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_USB1_APB)] = { mfn_init(MM_USB3_1_XHCI) },
>>> +
>>> +    [XILPM_RESET_IDX(XILPM_RESET_IPI)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_APM_LPD)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_RTC)] = { mfn_init(MM_RTC) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_SYSMON)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_AFI_FM6)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_LPD_SWDT)] = { mfn_init(MM_SWDT) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_FPD)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG1)] = { mfn_init(MM_RPU) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_RPU_DBG0)] = { mfn_init(MM_RPU) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_DBG_LPD)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_DBG_FPD)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_APLL)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_DPLL)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_VPLL)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_IOPLL)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_RPLL)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_0)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_1)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_2)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_3)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_4)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_5)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_6)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_7)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_8)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_9)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_10)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_11)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_12)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_13)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_14)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_15)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_16)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_17)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_18)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_19)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_20)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_21)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_22)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_23)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_24)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_25)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_26)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_27)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_28)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_29)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_30)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_GPO3_PL_31)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_RPU_LS)] = { mfn_init(MM_RPU) },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_PS_ONLY)] = { .hwdom_access = true },
>>> +    [XILPM_RESET_IDX(XILPM_RESET_PL)] = { .hwdom_access = true },
>>> +};
>>> +
>>> +static bool pm_check_access(const struct pm_access *acl, struct domain *d,
>>> +                            uint32_t idx)
>>> +{
>>> +    if ( acl[idx].hwdom_access && is_hardware_domain(d) )
>>> +        return true;
>>> +
>>> +    if ( !mfn_x(acl[idx].mfn) )
>>
>> Technically 0 is a valid mfn. If you want to encode an invalid value then
>> MFN_INVALID is safer.
>>
>> But what are you trying to prevent? Are the node IDs not allocated
>> contiguously?
> 
> I improved the comments above now. But the idea is that a zero address
> means nobody has access.

The hardware domain has technically full access to the hardware and we 
trust it enough to do the right things. So why would you need to deny 
access here?

> None of those resources have actually a zero
> address, so there are no risks of conflicts with any valid address zero.
> 
> Following a comment by Jan, I have also started using MFN_INVALID to
> encode only dom0 has access, getting rid of .hwdom_access completely.
> 
> So in the next series:
> 
> - address -> regular check
> - 0 -> NO
> - INVALID_MFN -> dom0 only

IHMO this is a confusing solution. I would actually prefer if we keep 2 
fields but potentially inverting the condition for hwdom_access.

.guest_access = <boolean>
.mfn = <machine frame number>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi

[Stefano Stabellini]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

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@xilinx.com>
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
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)
+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);
+    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),
+                      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;
 }
 
 /*
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi

[Julien Grall]

Hi Stefano,

On 03/12/2018 21:03, Stefano Stabellini wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> 
> 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@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> 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?

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


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

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?

> +
> +    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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi

[Stefano Stabellini]

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@xilinx.com>
> > 
> > From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > 
> > 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@xilinx.com>
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> > 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi

[Julien Grall]

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@xilinx.com>
>>>
>>> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>>>
>>> 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@xilinx.com>
>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>> ---
>>> 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).

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

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi

[Stefano Stabellini]

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@xilinx.com>
> > > > 
> > > > From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > > > 
> > > > 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@xilinx.com>
> > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > > > ---
> > > > 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v5 6/7] xen/arm: zynqmp: implement zynqmp_eemi

[Julien Grall]

Hi,

On 12/12/18 11:56 PM, Stefano Stabellini wrote:
> 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:
>>>> 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.

Please write it down in the code so we know why we don't need to worry 
on parameters.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v5 7/7] xen/arm: zynqmp: Remove blacklist of ZynqMP's PM node

[Stefano Stabellini]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Stop blacklisting ZynqMP's power management node. It is now possible
since we allow the hardware domain to issue HVC/SMC calls to firmware.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 xen/arch/arm/platforms/xilinx-zynqmp.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/xen/arch/arm/platforms/xilinx-zynqmp.c b/xen/arch/arm/platforms/xilinx-zynqmp.c
index 9c174d2..c49555b 100644
--- a/xen/arch/arm/platforms/xilinx-zynqmp.c
+++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
@@ -27,13 +27,6 @@ static const char * const zynqmp_dt_compat[] __initconst =
     NULL
 };
 
-static const struct dt_device_match zynqmp_blacklist_dev[] __initconst =
-{
-    /* Power management is not yet supported.  */
-    DT_MATCH_COMPATIBLE("xlnx,zynqmp-pm"),
-    { /* sentinel */ },
-};
-
 static bool zynqmp_smc(struct cpu_user_regs *regs)
 {
     if ( !is_64bit_domain(current->domain) )
@@ -45,7 +38,6 @@ static bool zynqmp_smc(struct cpu_user_regs *regs)
 PLATFORM_START(xilinx_zynqmp, "Xilinx ZynqMP")
     .compatible = zynqmp_dt_compat,
     .smc = zynqmp_smc,
-    .blacklist_dev = zynqmp_blacklist_dev,
 PLATFORM_END
 
 /*
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel