[PATCH] arm64: zynqmp: Print the secure boot status information in EL3

[PATCH] arm64: zynqmp: Print the secure boot status information in EL3

[Jorge Ramirez-Ortiz]

Confirm the secure boot configuration on the console.

Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
---
 arch/arm/mach-zynqmp/include/mach/hardware.h |  3 ++-
 board/xilinx/zynqmp/zynqmp.c                 | 16 +++++++++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-zynqmp/include/mach/hardware.h b/arch/arm/mach-zynqmp/include/mach/hardware.h
index 3776499070..3d3ffa086e 100644
--- a/arch/arm/mach-zynqmp/include/mach/hardware.h
+++ b/arch/arm/mach-zynqmp/include/mach/hardware.h
@@ -139,7 +139,8 @@ struct apu_regs {
 #define ZYNQMP_SILICON_VER_SHIFT	0
 
 struct csu_regs {
-	u32 reserved0[4];
+	u32 status;
+	u32 reserved0[3];
 	u32 multi_boot;
 	u32 reserved1[11];
 	u32 idcode;
diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
index 1748fec2e4..b7d11630d1 100644
--- a/board/xilinx/zynqmp/zynqmp.c
+++ b/board/xilinx/zynqmp/zynqmp.c
@@ -355,6 +355,18 @@ static int multi_boot(void)
 	return 0;
 }
 
+static void secure_boot(void)
+{
+	u32 status;
+
+	status = readl(&csu_base->status);
+	if (status & (BIT(0) | BIT(1))) {
+		printf("Secure Boot:\t%s%s\n",
+		       status & BIT(0) ? "authenticated" : "not authenticated",
+		       status & BIT(1) ? ", encrypted" : ", not encrypted");
+	}
+}
+
 #define PS_SYSMON_ANALOG_BUS_VAL	0x3210
 #define PS_SYSMON_ANALOG_BUS_REG	0xFFA50914
 
@@ -391,8 +403,10 @@ int board_init(void)
 	fpga_add(fpga_xilinx, &zynqmppl);
 #endif
 
-	if (current_el() == 3)
+	if (current_el() == 3) {
 		multi_boot();
+		secure_boot();
+	}
 
 	return 0;
 }
-- 
2.31.1

[RFC PATCH 1/2] fpga_load: pass compatible string

[Jorge Ramirez-Ortiz]

Instead of ignoring the mandatory fpga compatible string, let the
different implementations decide how to handle it

Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
---
 cmd/fpga.c           |  8 ++++----
 common/image.c       |  4 ++--
 common/spl/spl_fit.c |  4 +---
 drivers/fpga/fpga.c  | 11 +++++++++--
 include/fpga.h       |  2 +-
 5 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/cmd/fpga.c b/cmd/fpga.c
index 3fdd0b35e8..f3ed1fcdff 100644
--- a/cmd/fpga.c
+++ b/cmd/fpga.c
@@ -178,7 +178,7 @@ static int do_fpga_load(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (ret)
 		return ret;
 
-	return fpga_load(dev, (void *)fpga_data, data_size, BIT_FULL);
+	return fpga_load(dev, (void *)fpga_data, data_size, BIT_FULL, NULL);
 }
 
 static int do_fpga_loadb(struct cmd_tbl *cmdtp, int flag, int argc,
@@ -209,7 +209,7 @@ static int do_fpga_loadp(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (ret)
 		return ret;
 
-	return fpga_load(dev, (void *)fpga_data, data_size, BIT_PARTIAL);
+	return fpga_load(dev, (void *)fpga_data, data_size, BIT_PARTIAL, NULL);
 }
 #endif
 
@@ -315,7 +315,7 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int flag, int argc,
 			data_size = image_get_data_size(hdr);
 		}
 		return fpga_load(dev, (void *)data, data_size,
-				  BIT_FULL);
+				  BIT_FULL, NULL);
 	}
 #endif
 #if defined(CONFIG_FIT)
@@ -355,7 +355,7 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int flag, int argc,
 			return CMD_RET_FAILURE;
 		}
 
-		return fpga_load(dev, fit_data, data_size, BIT_FULL);
+		return fpga_load(dev, fit_data, data_size, BIT_FULL, NULL);
 	}
 #endif
 	default:
diff --git a/common/image.c b/common/image.c
index e199d61a4c..97f3deda24 100644
--- a/common/image.c
+++ b/common/image.c
@@ -1516,14 +1516,14 @@ int boot_get_fpga(int argc, char *const argv[], bootm_headers_t *images,
 						 img_len, BIT_FULL);
 			if (err)
 				err = fpga_load(devnum, (const void *)img_data,
-						img_len, BIT_FULL);
+						img_len, BIT_FULL, NULL);
 		} else {
 			name = "partial";
 			err = fpga_loadbitstream(devnum, (char *)img_data,
 						 img_len, BIT_PARTIAL);
 			if (err)
 				err = fpga_load(devnum, (const void *)img_data,
-						img_len, BIT_PARTIAL);
+						img_len, BIT_PARTIAL, NULL);
 		}
 
 		if (err)
diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index f41abca0cc..4db22efd8c 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -566,11 +566,9 @@ static int spl_fit_upload_fpga(struct spl_fit_info *ctx, int node,
 	compatible = fdt_getprop(ctx->fit, node, "compatible", NULL);
 	if (!compatible)
 		warn_deprecated("'fpga' image without 'compatible' property");
-	else if (strcmp(compatible, "u-boot,fpga-legacy"))
-		printf("Ignoring compatible = %s property\n", compatible);
 
 	ret = fpga_load(0, (void *)fpga_image->load_addr, fpga_image->size,
-			BIT_FULL);
+			BIT_FULL, compatible);
 	if (ret) {
 		printf("%s: Cannot load the image to the FPGA\n", __func__);
 		return ret;
diff --git a/drivers/fpga/fpga.c b/drivers/fpga/fpga.c
index fe3dfa1233..00aa4190b4 100644
--- a/drivers/fpga/fpga.c
+++ b/drivers/fpga/fpga.c
@@ -252,7 +252,8 @@ int fpga_loads(int devnum, const void *buf, size_t size,
 /*
  * Generic multiplexing code
  */
-int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype)
+int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype,
+	      const char *compatible)
 {
 	int ret_val = FPGA_FAIL;           /* assume failure */
 	const fpga_desc *desc = fpga_validate(devnum, buf, bsize,
@@ -263,13 +264,16 @@ int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype)
 		case fpga_xilinx:
 #if defined(CONFIG_FPGA_XILINX)
 			ret_val = xilinx_load(desc->devdesc, buf, bsize,
-					      bstype);
+					      bstype, compatible);
 #else
 			fpga_no_sup((char *)__func__, "Xilinx devices");
 #endif
 			break;
 		case fpga_altera:
 #if defined(CONFIG_FPGA_ALTERA)
+			if (strcmp(compatible, "u-boot,fpga-legacy"))
+				printf("Ignoring compatible = %s property\n",
+				       compatible);
 			ret_val = altera_load(desc->devdesc, buf, bsize);
 #else
 			fpga_no_sup((char *)__func__, "Altera devices");
@@ -277,6 +281,9 @@ int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype)
 			break;
 		case fpga_lattice:
 #if defined(CONFIG_FPGA_LATTICE)
+			if (strcmp(compatible, "u-boot,fpga-legacy"))
+				printf("Ignoring compatible = %s property\n",
+				       compatible);
 			ret_val = lattice_load(desc->devdesc, buf, bsize);
 #else
 			fpga_no_sup((char *)__func__, "Lattice devices");
diff --git a/include/fpga.h b/include/fpga.h
index ec5144334d..d85ef2cf18 100644
--- a/include/fpga.h
+++ b/include/fpga.h
@@ -64,7 +64,7 @@ int fpga_count(void);
 const fpga_desc *const fpga_get_desc(int devnum);
 int fpga_is_partial_data(int devnum, size_t img_len);
 int fpga_load(int devnum, const void *buf, size_t bsize,
-	      bitstream_type bstype);
+	      bitstream_type bstype, const char *compatible);
 int fpga_fsload(int devnum, const void *buf, size_t size,
 		fpga_fs_info *fpga_fsinfo);
 int fpga_loads(int devnum, const void *buf, size_t size,
-- 
2.31.1

[RFC PATCH 2/2] fpga: xilinx: allow loading authenticated images (DDR)

[Jorge Ramirez-Ortiz]

Add new compatible string u-boot,zynqmp-fpga-ddrauth to handle this
use case.

Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
---
 drivers/fpga/xilinx.c   | 29 +++++++++++++++++++++++++----
 drivers/fpga/zynqmppl.c |  4 ++--
 include/xilinx.h        |  2 +-
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/fpga/xilinx.c b/drivers/fpga/xilinx.c
index cbebefb55f..c8035105e2 100644
--- a/drivers/fpga/xilinx.c
+++ b/drivers/fpga/xilinx.c
@@ -135,23 +135,44 @@ int fpga_loadbitstream(int devnum, char *fpgadata, size_t size,
 	dataptr += 4;
 	printf("  bytes in bitstream = %d\n", swapsize);
 
-	return fpga_load(devnum, dataptr, swapsize, bstype);
+	return fpga_load(devnum, dataptr, swapsize, bstype, NULL);
 }
 
 int xilinx_load(xilinx_desc *desc, const void *buf, size_t bsize,
-		bitstream_type bstype)
+		bitstream_type bstype, const char *compatible)
 {
+	struct fpga_secure_info info = { 0 };
+
 	if (!xilinx_validate (desc, (char *)__FUNCTION__)) {
 		printf ("%s: Invalid device descriptor\n", __FUNCTION__);
 		return FPGA_FAIL;
 	}
 
-	if (!desc->operations || !desc->operations->load) {
+	if (!desc->operations) {
 		printf("%s: Missing load operation\n", __func__);
 		return FPGA_FAIL;
 	}
 
-	return desc->operations->load(desc, buf, bsize, bstype);
+	if (!compatible || !strcmp(compatible, "u-boot,fpga-legacy")) {
+		if (!desc->operations->load) {
+			printf("%s: Missing load operation\n", __func__);
+			return FPGA_FAIL;
+		}
+		return desc->operations->load(desc, buf, bsize, bstype);
+	}
+
+	if (!strcmp(compatible, "u-boot,zynqmp-fpga-ddrauth")) {
+		if (!desc->operations->loads) {
+			printf("%s: Missing load operation\n", __func__);
+			return FPGA_FAIL;
+		}
+		/* DDR authentication */
+		info.authflag = 1;
+		return desc->operations->loads(desc, buf, bsize, &info);
+	}
+
+	printf("%s: compatible support not implemented\n", __func__);
+	return FPGA_FAIL;
 }
 
 #if defined(CONFIG_CMD_FPGA_LOADFS)
diff --git a/drivers/fpga/zynqmppl.c b/drivers/fpga/zynqmppl.c
index 6b394869db..65a9412123 100644
--- a/drivers/fpga/zynqmppl.c
+++ b/drivers/fpga/zynqmppl.c
@@ -245,7 +245,7 @@ static int zynqmp_load(xilinx_desc *desc, const void *buf, size_t bsize,
 	return ret;
 }
 
-#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) && !defined(CONFIG_SPL_BUILD)
+#if defined(CONFIG_CMD_FPGA_LOAD_SECURE)
 static int zynqmp_loads(xilinx_desc *desc, const void *buf, size_t bsize,
 			struct fpga_secure_info *fpga_sec_info)
 {
@@ -306,7 +306,7 @@ static int zynqmp_pcap_info(xilinx_desc *desc)
 
 struct xilinx_fpga_op zynqmp_op = {
 	.load = zynqmp_load,
-#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) && !defined(CONFIG_SPL_BUILD)
+#if defined(CONFIG_CMD_FPGA_LOAD_SECURE)
 	.loads = zynqmp_loads,
 #endif
 	.info = zynqmp_pcap_info,
diff --git a/include/xilinx.h b/include/xilinx.h
index ab4537becf..ae78009e6b 100644
--- a/include/xilinx.h
+++ b/include/xilinx.h
@@ -59,7 +59,7 @@ struct xilinx_fpga_op {
 /* Generic Xilinx Functions
  *********************************************************************/
 int xilinx_load(xilinx_desc *desc, const void *image, size_t size,
-		bitstream_type bstype);
+		bitstream_type bstype, const char *compatible);
 int xilinx_dump(xilinx_desc *desc, const void *buf, size_t bsize);
 int xilinx_info(xilinx_desc *desc);
 int xilinx_loadfs(xilinx_desc *desc, const void *buf, size_t bsize,
-- 
2.31.1

Re: [PATCH] arm64: zynqmp: Print the secure boot status information in EL3

[Igor Opaniuk]

Hi Jorge,

On Tue, Oct 5, 2021 at 2:13 PM Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
>
> Confirm the secure boot configuration on the console.
>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> ---
>  arch/arm/mach-zynqmp/include/mach/hardware.h |  3 ++-
>  board/xilinx/zynqmp/zynqmp.c                 | 16 +++++++++++++++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-zynqmp/include/mach/hardware.h b/arch/arm/mach-zynqmp/include/mach/hardware.h
> index 3776499070..3d3ffa086e 100644
> --- a/arch/arm/mach-zynqmp/include/mach/hardware.h
> +++ b/arch/arm/mach-zynqmp/include/mach/hardware.h
> @@ -139,7 +139,8 @@ struct apu_regs {
>  #define ZYNQMP_SILICON_VER_SHIFT       0
>
>  struct csu_regs {
> -       u32 reserved0[4];
> +       u32 status;
> +       u32 reserved0[3];
>         u32 multi_boot;
>         u32 reserved1[11];
>         u32 idcode;
> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> index 1748fec2e4..b7d11630d1 100644
> --- a/board/xilinx/zynqmp/zynqmp.c
> +++ b/board/xilinx/zynqmp/zynqmp.c
> @@ -355,6 +355,18 @@ static int multi_boot(void)
>         return 0;
>  }
>
> +static void secure_boot(void)
> +{
> +       u32 status;
> +
> +       status = readl(&csu_base->status);
> +       if (status & (BIT(0) | BIT(1))) {
> +               printf("Secure Boot:\t%s%s\n",
> +                      status & BIT(0) ? "authenticated" : "not authenticated",
> +                      status & BIT(1) ? ", encrypted" : ", not encrypted");
> +       }
> +}
> +
>  #define PS_SYSMON_ANALOG_BUS_VAL       0x3210
>  #define PS_SYSMON_ANALOG_BUS_REG       0xFFA50914
>
> @@ -391,8 +403,10 @@ int board_init(void)
>         fpga_add(fpga_xilinx, &zynqmppl);
>  #endif
>
> -       if (current_el() == 3)
> +       if (current_el() == 3) {
>                 multi_boot();
> +               secure_boot();
> +       }
>
>         return 0;
>  }
> --
> 2.31.1
>

Reviewed-by: Igor Opaniuk <igor.opaniuk@foundries.io>

-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk
Embedded Software Engineer
T:  +380 938364067
E: igor.opaniuk@foundries.io
W: www.foundries.io

Re: [PATCH] arm64: zynqmp: Print the secure boot status information in EL3

[Oleksandr Suvorov]

On Tue, Oct 5, 2021 at 2:13 PM Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
>
> Confirm the secure boot configuration on the console.
>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>

Acked-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> ---
>  arch/arm/mach-zynqmp/include/mach/hardware.h |  3 ++-
>  board/xilinx/zynqmp/zynqmp.c                 | 16 +++++++++++++++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-zynqmp/include/mach/hardware.h b/arch/arm/mach-zynqmp/include/mach/hardware.h
> index 3776499070..3d3ffa086e 100644
> --- a/arch/arm/mach-zynqmp/include/mach/hardware.h
> +++ b/arch/arm/mach-zynqmp/include/mach/hardware.h
> @@ -139,7 +139,8 @@ struct apu_regs {
>  #define ZYNQMP_SILICON_VER_SHIFT       0
>
>  struct csu_regs {
> -       u32 reserved0[4];
> +       u32 status;
> +       u32 reserved0[3];
>         u32 multi_boot;
>         u32 reserved1[11];
>         u32 idcode;
> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> index 1748fec2e4..b7d11630d1 100644
> --- a/board/xilinx/zynqmp/zynqmp.c
> +++ b/board/xilinx/zynqmp/zynqmp.c
> @@ -355,6 +355,18 @@ static int multi_boot(void)
>         return 0;
>  }
>
> +static void secure_boot(void)
> +{
> +       u32 status;
> +
> +       status = readl(&csu_base->status);
> +       if (status & (BIT(0) | BIT(1))) {
> +               printf("Secure Boot:\t%s%s\n",
> +                      status & BIT(0) ? "authenticated" : "not authenticated",
> +                      status & BIT(1) ? ", encrypted" : ", not encrypted");
> +       }
> +}
> +
>  #define PS_SYSMON_ANALOG_BUS_VAL       0x3210
>  #define PS_SYSMON_ANALOG_BUS_REG       0xFFA50914
>
> @@ -391,8 +403,10 @@ int board_init(void)
>         fpga_add(fpga_xilinx, &zynqmppl);
>  #endif
>
> -       if (current_el() == 3)
> +       if (current_el() == 3) {
>                 multi_boot();
> +               secure_boot();
> +       }
>
>         return 0;
>  }
> --
> 2.31.1
>


--
Best regards
Oleksandr

Oleksandr Suvorov
cryosay@gmail.com

Re: [PATCH] arm64: zynqmp: Print the secure boot status information in EL3

[Michal Simek]

On 10/5/21 1:13 PM, Jorge Ramirez-Ortiz wrote:
> Confirm the secure boot configuration on the console.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> ---
>  arch/arm/mach-zynqmp/include/mach/hardware.h |  3 ++-
>  board/xilinx/zynqmp/zynqmp.c                 | 16 +++++++++++++++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-zynqmp/include/mach/hardware.h b/arch/arm/mach-zynqmp/include/mach/hardware.h
> index 3776499070..3d3ffa086e 100644
> --- a/arch/arm/mach-zynqmp/include/mach/hardware.h
> +++ b/arch/arm/mach-zynqmp/include/mach/hardware.h
> @@ -139,7 +139,8 @@ struct apu_regs {
>  #define ZYNQMP_SILICON_VER_SHIFT	0
>  
>  struct csu_regs {
> -	u32 reserved0[4];
> +	u32 status;
> +	u32 reserved0[3];
>  	u32 multi_boot;
>  	u32 reserved1[11];
>  	u32 idcode;
> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> index 1748fec2e4..b7d11630d1 100644
> --- a/board/xilinx/zynqmp/zynqmp.c
> +++ b/board/xilinx/zynqmp/zynqmp.c
> @@ -355,6 +355,18 @@ static int multi_boot(void)
>  	return 0;
>  }
>  
> +static void secure_boot(void)
> +{
> +	u32 status;
> +
> +	status = readl(&csu_base->status);

I would prefer to use zynqmp_mmio_read instead to make sure that we can
call this function also from regular u-boot not running in EL3.
For SPL it will be just readl what you have here too.


> +	if (status & (BIT(0) | BIT(1))) {
> +		printf("Secure Boot:\t%s%s\n",
> +		       status & BIT(0) ? "authenticated" : "not authenticated",
> +		       status & BIT(1) ? ", encrypted" : ", not encrypted");

It is pretty much visible that instead of BIT(X) you should use macros.

> +	}
> +}
> +
>  #define PS_SYSMON_ANALOG_BUS_VAL	0x3210
>  #define PS_SYSMON_ANALOG_BUS_REG	0xFFA50914
>  
> @@ -391,8 +403,10 @@ int board_init(void)
>  	fpga_add(fpga_xilinx, &zynqmppl);
>  #endif
>  
> -	if (current_el() == 3)
> +	if (current_el() == 3) {
>  		multi_boot();

This code has changed a little bit. Please use the latest u-boot version.

> +		secure_boot();

Is it useful to get information only for SPL in EL3? Just asking.

> +	}
>  
>  	return 0;
>  }
> 

Thanks,
Michal

Re: [RFC PATCH 2/2] fpga: xilinx: allow loading authenticated images (DDR)

[Michal Simek]

On 10/5/21 1:13 PM, Jorge Ramirez-Ortiz wrote:
> Add new compatible string u-boot,zynqmp-fpga-ddrauth to handle this
> use case.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> ---
>  drivers/fpga/xilinx.c   | 29 +++++++++++++++++++++++++----
>  drivers/fpga/zynqmppl.c |  4 ++--
>  include/xilinx.h        |  2 +-
>  3 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/fpga/xilinx.c b/drivers/fpga/xilinx.c
> index cbebefb55f..c8035105e2 100644
> --- a/drivers/fpga/xilinx.c
> +++ b/drivers/fpga/xilinx.c
> @@ -135,23 +135,44 @@ int fpga_loadbitstream(int devnum, char *fpgadata, size_t size,
>  	dataptr += 4;
>  	printf("  bytes in bitstream = %d\n", swapsize);
>  
> -	return fpga_load(devnum, dataptr, swapsize, bstype);
> +	return fpga_load(devnum, dataptr, swapsize, bstype, NULL);
>  }
>  
>  int xilinx_load(xilinx_desc *desc, const void *buf, size_t bsize,
> -		bitstream_type bstype)
> +		bitstream_type bstype, const char *compatible)
>  {
> +	struct fpga_secure_info info = { 0 };
> +
>  	if (!xilinx_validate (desc, (char *)__FUNCTION__)) {
>  		printf ("%s: Invalid device descriptor\n", __FUNCTION__);
>  		return FPGA_FAIL;
>  	}
>  
> -	if (!desc->operations || !desc->operations->load) {
> +	if (!desc->operations) {
>  		printf("%s: Missing load operation\n", __func__);
>  		return FPGA_FAIL;
>  	}
>  
> -	return desc->operations->load(desc, buf, bsize, bstype);
> +	if (!compatible || !strcmp(compatible, "u-boot,fpga-legacy")) {
> +		if (!desc->operations->load) {
> +			printf("%s: Missing load operation\n", __func__);
> +			return FPGA_FAIL;
> +		}
> +		return desc->operations->load(desc, buf, bsize, bstype);
> +	}
> +
> +	if (!strcmp(compatible, "u-boot,zynqmp-fpga-ddrauth")) {
> +		if (!desc->operations->loads) {
> +			printf("%s: Missing load operation\n", __func__);
> +			return FPGA_FAIL;
> +		}
> +		/* DDR authentication */
> +		info.authflag = 1;
> +		return desc->operations->loads(desc, buf, bsize, &info);
> +	}

I am not sure about this solution. First of all it forces SPL to have
additional code which just extending size. It means there must be a way
to enable/disable it.

The next thing is that you have zynqmp string in generic xilinx file
which doesn't look right. It would be better to deal with image types
directly in driver which is capable to handle it.
It means record fit image compatible string in the driver with
hooks/flags and determined what should be called.

And the same style should work for SPL and also for U-Boot proper.

Thanks,
Michal

Re: [RFC PATCH 2/2] fpga: xilinx: allow loading authenticated images (DDR)

[Jorge Ramirez-Ortiz, Foundries]

On 05/10/21, Michal Simek wrote:
> 
> 
> On 10/5/21 1:13 PM, Jorge Ramirez-Ortiz wrote:
> > Add new compatible string u-boot,zynqmp-fpga-ddrauth to handle this
> > use case.
> > 
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > ---
> >  drivers/fpga/xilinx.c   | 29 +++++++++++++++++++++++++----
> >  drivers/fpga/zynqmppl.c |  4 ++--
> >  include/xilinx.h        |  2 +-
> >  3 files changed, 28 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/fpga/xilinx.c b/drivers/fpga/xilinx.c
> > index cbebefb55f..c8035105e2 100644
> > --- a/drivers/fpga/xilinx.c
> > +++ b/drivers/fpga/xilinx.c
> > @@ -135,23 +135,44 @@ int fpga_loadbitstream(int devnum, char *fpgadata, size_t size,
> >  	dataptr += 4;
> >  	printf("  bytes in bitstream = %d\n", swapsize);
> >  
> > -	return fpga_load(devnum, dataptr, swapsize, bstype);
> > +	return fpga_load(devnum, dataptr, swapsize, bstype, NULL);
> >  }
> >  
> >  int xilinx_load(xilinx_desc *desc, const void *buf, size_t bsize,
> > -		bitstream_type bstype)
> > +		bitstream_type bstype, const char *compatible)
> >  {
> > +	struct fpga_secure_info info = { 0 };
> > +
> >  	if (!xilinx_validate (desc, (char *)__FUNCTION__)) {
> >  		printf ("%s: Invalid device descriptor\n", __FUNCTION__);
> >  		return FPGA_FAIL;
> >  	}
> >  
> > -	if (!desc->operations || !desc->operations->load) {
> > +	if (!desc->operations) {
> >  		printf("%s: Missing load operation\n", __func__);
> >  		return FPGA_FAIL;
> >  	}
> >  
> > -	return desc->operations->load(desc, buf, bsize, bstype);
> > +	if (!compatible || !strcmp(compatible, "u-boot,fpga-legacy")) {
> > +		if (!desc->operations->load) {
> > +			printf("%s: Missing load operation\n", __func__);
> > +			return FPGA_FAIL;
> > +		}
> > +		return desc->operations->load(desc, buf, bsize, bstype);
> > +	}
> > +
> > +	if (!strcmp(compatible, "u-boot,zynqmp-fpga-ddrauth")) {
> > +		if (!desc->operations->loads) {
> > +			printf("%s: Missing load operation\n", __func__);
> > +			return FPGA_FAIL;
> > +		}
> > +		/* DDR authentication */
> > +		info.authflag = 1;
> > +		return desc->operations->loads(desc, buf, bsize, &info);
> > +	}
> 
> I am not sure about this solution. First of all it forces SPL to have
> additional code which just extending size. It means there must be a way
> to enable/disable it.

that can be contained of course...so not really an issue (ie compile
out either fpga_load or fpga_loads depending on needs).

> 
> The next thing is that you have zynqmp string in generic xilinx file
> which doesn't look right. It would be better to deal with image types
> directly in driver which is capable to handle it.

sure that is easy..but the idea is the same

> It means record fit image compatible string in the driver with
> hooks/flags and determined what should be called.

what about the compatible names? will you define those? or should I do
it? 

For our use case, we need DDR authentication so that must be
defined. I can add others and someone else can add the support needed?


> 
> And the same style should work for SPL and also for U-Boot proper.

sure.

thanks for the quick response.

> 
> Thanks,
> Michal

Re: [RFC PATCH 2/2] fpga: xilinx: allow loading authenticated images (DDR)

[Michal Simek]

On 10/5/21 4:03 PM, Jorge Ramirez-Ortiz, Foundries wrote:
> On 05/10/21, Michal Simek wrote:
>>
>>
>> On 10/5/21 1:13 PM, Jorge Ramirez-Ortiz wrote:
>>> Add new compatible string u-boot,zynqmp-fpga-ddrauth to handle this
>>> use case.
>>>
>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
>>> ---
>>>  drivers/fpga/xilinx.c   | 29 +++++++++++++++++++++++++----
>>>  drivers/fpga/zynqmppl.c |  4 ++--
>>>  include/xilinx.h        |  2 +-
>>>  3 files changed, 28 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/fpga/xilinx.c b/drivers/fpga/xilinx.c
>>> index cbebefb55f..c8035105e2 100644
>>> --- a/drivers/fpga/xilinx.c
>>> +++ b/drivers/fpga/xilinx.c
>>> @@ -135,23 +135,44 @@ int fpga_loadbitstream(int devnum, char *fpgadata, size_t size,
>>>  	dataptr += 4;
>>>  	printf("  bytes in bitstream = %d\n", swapsize);
>>>  
>>> -	return fpga_load(devnum, dataptr, swapsize, bstype);
>>> +	return fpga_load(devnum, dataptr, swapsize, bstype, NULL);
>>>  }
>>>  
>>>  int xilinx_load(xilinx_desc *desc, const void *buf, size_t bsize,
>>> -		bitstream_type bstype)
>>> +		bitstream_type bstype, const char *compatible)
>>>  {
>>> +	struct fpga_secure_info info = { 0 };
>>> +
>>>  	if (!xilinx_validate (desc, (char *)__FUNCTION__)) {
>>>  		printf ("%s: Invalid device descriptor\n", __FUNCTION__);
>>>  		return FPGA_FAIL;
>>>  	}
>>>  
>>> -	if (!desc->operations || !desc->operations->load) {
>>> +	if (!desc->operations) {
>>>  		printf("%s: Missing load operation\n", __func__);
>>>  		return FPGA_FAIL;
>>>  	}
>>>  
>>> -	return desc->operations->load(desc, buf, bsize, bstype);
>>> +	if (!compatible || !strcmp(compatible, "u-boot,fpga-legacy")) {
>>> +		if (!desc->operations->load) {
>>> +			printf("%s: Missing load operation\n", __func__);
>>> +			return FPGA_FAIL;
>>> +		}
>>> +		return desc->operations->load(desc, buf, bsize, bstype);
>>> +	}
>>> +
>>> +	if (!strcmp(compatible, "u-boot,zynqmp-fpga-ddrauth")) {
>>> +		if (!desc->operations->loads) {
>>> +			printf("%s: Missing load operation\n", __func__);
>>> +			return FPGA_FAIL;
>>> +		}
>>> +		/* DDR authentication */
>>> +		info.authflag = 1;
>>> +		return desc->operations->loads(desc, buf, bsize, &info);
>>> +	}
>>
>> I am not sure about this solution. First of all it forces SPL to have
>> additional code which just extending size. It means there must be a way
>> to enable/disable it.
> 
> that can be contained of course...so not really an issue (ie compile
> out either fpga_load or fpga_loads depending on needs).
> 
>>
>> The next thing is that you have zynqmp string in generic xilinx file
>> which doesn't look right. It would be better to deal with image types
>> directly in driver which is capable to handle it.
> 
> sure that is easy..but the idea is the same

We just need to find out how this should be properly propagated and stored.

> 
>> It means record fit image compatible string in the driver with
>> hooks/flags and determined what should be called.
> 
> what about the compatible names? will you define those? or should I do
> it? 

Just define it and let's see if this fits.

> 
> For our use case, we need DDR authentication so that must be
> defined. I can add others and someone else can add the support needed?
> 

Sure. None is asking you to support everything. Just add support for
regular bitstreams and your case. And adding support for something else
will be just +1 if interface is right.

Thanks,
Michal

Re: [RFC PATCH 1/2] fpga_load: pass compatible string

[Simon Glass]

Hi Jorge,

On Tue, 5 Oct 2021 at 05:13, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
>
> Instead of ignoring the mandatory fpga compatible string, let the
> different implementations decide how to handle it
>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> ---
>  cmd/fpga.c           |  8 ++++----
>  common/image.c       |  4 ++--
>  common/spl/spl_fit.c |  4 +---
>  drivers/fpga/fpga.c  | 11 +++++++++--
>  include/fpga.h       |  2 +-
>  5 files changed, 17 insertions(+), 12 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c

[..]

> index f41abca0cc..4db22efd8c 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -566,11 +566,9 @@ static int spl_fit_upload_fpga(struct spl_fit_info *ctx, int node,
>         compatible = fdt_getprop(ctx->fit, node, "compatible", NULL);
>         if (!compatible)
>                 warn_deprecated("'fpga' image without 'compatible' property");
> -       else if (strcmp(compatible, "u-boot,fpga-legacy"))
> -               printf("Ignoring compatible = %s property\n", compatible);
>
>         ret = fpga_load(0, (void *)fpga_image->load_addr, fpga_image->size,
> -                       BIT_FULL);
> +                       BIT_FULL, compatible);
>         if (ret) {
>                 printf("%s: Cannot load the image to the FPGA\n", __func__);
>                 return ret;
> diff --git a/drivers/fpga/fpga.c b/drivers/fpga/fpga.c
> index fe3dfa1233..00aa4190b4 100644
> --- a/drivers/fpga/fpga.c
> +++ b/drivers/fpga/fpga.c
> @@ -252,7 +252,8 @@ int fpga_loads(int devnum, const void *buf, size_t size,
>  /*
>   * Generic multiplexing code
>   */
> -int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype)
> +int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype,
> +             const char *compatible)
>  {
>         int ret_val = FPGA_FAIL;           /* assume failure */
>         const fpga_desc *desc = fpga_validate(devnum, buf, bsize,
> @@ -263,13 +264,16 @@ int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype)
>                 case fpga_xilinx:
>  #if defined(CONFIG_FPGA_XILINX)
>                         ret_val = xilinx_load(desc->devdesc, buf, bsize,
> -                                             bstype);
> +                                             bstype, compatible);
>  #else
>                         fpga_no_sup((char *)__func__, "Xilinx devices");
>  #endif
>                         break;
>                 case fpga_altera:
>  #if defined(CONFIG_FPGA_ALTERA)
> +                       if (strcmp(compatible, "u-boot,fpga-legacy"))
> +                               printf("Ignoring compatible = %s property\n",
> +                                      compatible);
>                         ret_val = altera_load(desc->devdesc, buf, bsize);
>  #else
>                         fpga_no_sup((char *)__func__, "Altera devices");
> @@ -277,6 +281,9 @@ int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype)
>                         break;
>                 case fpga_lattice:
>  #if defined(CONFIG_FPGA_LATTICE)
> +                       if (strcmp(compatible, "u-boot,fpga-legacy"))
> +                               printf("Ignoring compatible = %s property\n",
> +                                      compatible);
>                         ret_val = lattice_load(desc->devdesc, buf, bsize);
>  #else
>                         fpga_no_sup((char *)__func__, "Lattice devices");
> diff --git a/include/fpga.h b/include/fpga.h
> index ec5144334d..d85ef2cf18 100644
> --- a/include/fpga.h
> +++ b/include/fpga.h
> @@ -64,7 +64,7 @@ int fpga_count(void);
>  const fpga_desc *const fpga_get_desc(int devnum);
>  int fpga_is_partial_data(int devnum, size_t img_len);
>  int fpga_load(int devnum, const void *buf, size_t bsize,
> -             bitstream_type bstype);
> +             bitstream_type bstype, const char *compatible);

Please can you add a function comment?

>  int fpga_fsload(int devnum, const void *buf, size_t size,
>                 fpga_fs_info *fpga_fsinfo);
>  int fpga_loads(int devnum, const void *buf, size_t size,
> --
> 2.31.1
>

Also the FPGA uclass is missing a sandbox driver/emulator and a test,
if you have time to do that. At present FPGA tests in CI are skipped
(e.g. with make qcheck).

Regards,
Simon

Re: [RFC PATCH 1/2] fpga_load: pass compatible string

[Jorge Ramirez-Ortiz, Foundries]

On 14/10/21, Simon Glass wrote:
> Hi Jorge,
> 
> On Tue, 5 Oct 2021 at 05:13, Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
> >
> > Instead of ignoring the mandatory fpga compatible string, let the
> > different implementations decide how to handle it
> >
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > ---
> >  cmd/fpga.c           |  8 ++++----
> >  common/image.c       |  4 ++--
> >  common/spl/spl_fit.c |  4 +---
> >  drivers/fpga/fpga.c  | 11 +++++++++--
> >  include/fpga.h       |  2 +-
> >  5 files changed, 17 insertions(+), 12 deletions(-)
> >
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

thanks, I'll add your tag

> > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> 
> [..]
> 
> > index f41abca0cc..4db22efd8c 100644
> > --- a/common/spl/spl_fit.c
> > +++ b/common/spl/spl_fit.c
> > @@ -566,11 +566,9 @@ static int spl_fit_upload_fpga(struct spl_fit_info *ctx, int node,
> >         compatible = fdt_getprop(ctx->fit, node, "compatible", NULL);
> >         if (!compatible)
> >                 warn_deprecated("'fpga' image without 'compatible' property");
> > -       else if (strcmp(compatible, "u-boot,fpga-legacy"))
> > -               printf("Ignoring compatible = %s property\n", compatible);
> >
> >         ret = fpga_load(0, (void *)fpga_image->load_addr, fpga_image->size,
> > -                       BIT_FULL);
> > +                       BIT_FULL, compatible);
> >         if (ret) {
> >                 printf("%s: Cannot load the image to the FPGA\n", __func__);
> >                 return ret;
> > diff --git a/drivers/fpga/fpga.c b/drivers/fpga/fpga.c
> > index fe3dfa1233..00aa4190b4 100644
> > --- a/drivers/fpga/fpga.c
> > +++ b/drivers/fpga/fpga.c
> > @@ -252,7 +252,8 @@ int fpga_loads(int devnum, const void *buf, size_t size,
> >  /*
> >   * Generic multiplexing code
> >   */
> > -int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype)
> > +int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype,
> > +             const char *compatible)
> >  {
> >         int ret_val = FPGA_FAIL;           /* assume failure */
> >         const fpga_desc *desc = fpga_validate(devnum, buf, bsize,
> > @@ -263,13 +264,16 @@ int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype)
> >                 case fpga_xilinx:
> >  #if defined(CONFIG_FPGA_XILINX)
> >                         ret_val = xilinx_load(desc->devdesc, buf, bsize,
> > -                                             bstype);
> > +                                             bstype, compatible);
> >  #else
> >                         fpga_no_sup((char *)__func__, "Xilinx devices");
> >  #endif
> >                         break;
> >                 case fpga_altera:
> >  #if defined(CONFIG_FPGA_ALTERA)
> > +                       if (strcmp(compatible, "u-boot,fpga-legacy"))
> > +                               printf("Ignoring compatible = %s property\n",
> > +                                      compatible);
> >                         ret_val = altera_load(desc->devdesc, buf, bsize);
> >  #else
> >                         fpga_no_sup((char *)__func__, "Altera devices");
> > @@ -277,6 +281,9 @@ int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype)
> >                         break;
> >                 case fpga_lattice:
> >  #if defined(CONFIG_FPGA_LATTICE)
> > +                       if (strcmp(compatible, "u-boot,fpga-legacy"))
> > +                               printf("Ignoring compatible = %s property\n",
> > +                                      compatible);
> >                         ret_val = lattice_load(desc->devdesc, buf, bsize);
> >  #else
> >                         fpga_no_sup((char *)__func__, "Lattice devices");
> > diff --git a/include/fpga.h b/include/fpga.h
> > index ec5144334d..d85ef2cf18 100644
> > --- a/include/fpga.h
> > +++ b/include/fpga.h
> > @@ -64,7 +64,7 @@ int fpga_count(void);
> >  const fpga_desc *const fpga_get_desc(int devnum);
> >  int fpga_is_partial_data(int devnum, size_t img_len);
> >  int fpga_load(int devnum, const void *buf, size_t bsize,
> > -             bitstream_type bstype);
> > +             bitstream_type bstype, const char *compatible);
> 
> Please can you add a function comment?

ok

> 
> >  int fpga_fsload(int devnum, const void *buf, size_t size,
> >                 fpga_fs_info *fpga_fsinfo);
> >  int fpga_loads(int devnum, const void *buf, size_t size,
> > --
> > 2.31.1
> >
> 
> Also the FPGA uclass is missing a sandbox driver/emulator and a test,
> if you have time to do that. At present FPGA tests in CI are skipped
> (e.g. with make qcheck).

I am really short of time ATM with another deadline coming (sorry!)
I believe Oleksandr might be willing to pick this up but I'll let him
add further.


> 
> Regards,
> Simon

Re: [PATCH] arm64: zynqmp: Print the secure boot status information in EL3

[Jorge Ramirez-Ortiz, Foundries]

On 12/08/21, Michal Simek wrote:
> 
> 
> On 7/22/21 1:19 PM, Jorge Ramirez-Ortiz wrote:
> > Confirm the secure boot configuration on the console.
> > 
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > ---
> >  arch/arm/mach-zynqmp/include/mach/hardware.h |  3 ++-
> >  board/xilinx/zynqmp/zynqmp.c                 | 16 +++++++++++++++-
> >  2 files changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/mach-zynqmp/include/mach/hardware.h b/arch/arm/mach-zynqmp/include/mach/hardware.h
> > index 3776499070..3d3ffa086e 100644
> > --- a/arch/arm/mach-zynqmp/include/mach/hardware.h
> > +++ b/arch/arm/mach-zynqmp/include/mach/hardware.h
> > @@ -139,7 +139,8 @@ struct apu_regs {
> >  #define ZYNQMP_SILICON_VER_SHIFT	0
> >  
> >  struct csu_regs {
> > -	u32 reserved0[4];
> > +	u32 status;
> > +	u32 reserved0[3];
> >  	u32 multi_boot;
> >  	u32 reserved1[11];
> >  	u32 idcode;
> > diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> > index 1748fec2e4..b7d11630d1 100644
> > --- a/board/xilinx/zynqmp/zynqmp.c
> > +++ b/board/xilinx/zynqmp/zynqmp.c
> > @@ -355,6 +355,18 @@ static int multi_boot(void)
> >  	return 0;
> >  }
> >  
> > +static void secure_boot(void)
> > +{
> > +	u32 status;
> > +
> > +	status = readl(&csu_base->status);
> > +	if (status & (BIT(0) | BIT(1))) {
> 
> please create macros for these bits.

ok
> 
> {} around are not needed.

yep

> 
> 
> > +		printf("Secure Boot:\t%s%s\n",
> > +		       status & BIT(0) ? "authenticated" : "not authenticated",
> > +		       status & BIT(1) ? ", encrypted" : ", not encrypted");
> 
> Isn't this more space efficient?
> printf("Secure Boot:\t%sauthenticated, %sencrypted\n",
> 	       status & BIT(0) ? "" : "not ",
> 	       status & BIT(1) ? "" : "not ");
> 
> And as I see it is.
> aarch64: (for 1/1 boards) all -33.0 rodata -17.0 spl/u-boot-spl:all
> -33.0 spl/u-boot-spl:rodata -17.0 spl/u-boot-spl:text -16.0 text -16.0
>             xilinx_zynqmp_virt: all -33 rodata -17 spl/u-boot-spl:all
> -33 spl/u-boot-spl:rodata -17 spl/u-boot-spl:text -16 text -16
>                spl-u-boot-spl: add: 0/0, grow: 0/-1 bytes: 0/-16 (-16)
> 
>

ok with me

> > +	}
> > +}
> > +
> >  #define PS_SYSMON_ANALOG_BUS_VAL	0x3210
> >  #define PS_SYSMON_ANALOG_BUS_REG	0xFFA50914
> >  
> > @@ -391,8 +403,10 @@ int board_init(void)
> >  	fpga_add(fpga_xilinx, &zynqmppl);
> >  #endif
> >  
> > -	if (current_el() == 3)
> > +	if (current_el() == 3) {
> >  		multi_boot();
> > +		secure_boot();
> > +	}
> 
> Please take a look at
> https://lists.denx.de/pipermail/u-boot/2021-July/456382.html
> I have changed multi_boot function a little bit.

ok

> 
> Thanks,
> Michal
> 
> -- 
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Xilinx Microblaze
> Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
> U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs
> 

Re: [PATCH] arm64: zynqmp: Print the secure boot status information in EL3

[Michal Simek]

On 7/22/21 1:19 PM, Jorge Ramirez-Ortiz wrote:
> Confirm the secure boot configuration on the console.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> ---
>  arch/arm/mach-zynqmp/include/mach/hardware.h |  3 ++-
>  board/xilinx/zynqmp/zynqmp.c                 | 16 +++++++++++++++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-zynqmp/include/mach/hardware.h b/arch/arm/mach-zynqmp/include/mach/hardware.h
> index 3776499070..3d3ffa086e 100644
> --- a/arch/arm/mach-zynqmp/include/mach/hardware.h
> +++ b/arch/arm/mach-zynqmp/include/mach/hardware.h
> @@ -139,7 +139,8 @@ struct apu_regs {
>  #define ZYNQMP_SILICON_VER_SHIFT	0
>  
>  struct csu_regs {
> -	u32 reserved0[4];
> +	u32 status;
> +	u32 reserved0[3];
>  	u32 multi_boot;
>  	u32 reserved1[11];
>  	u32 idcode;
> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> index 1748fec2e4..b7d11630d1 100644
> --- a/board/xilinx/zynqmp/zynqmp.c
> +++ b/board/xilinx/zynqmp/zynqmp.c
> @@ -355,6 +355,18 @@ static int multi_boot(void)
>  	return 0;
>  }
>  
> +static void secure_boot(void)
> +{
> +	u32 status;
> +
> +	status = readl(&csu_base->status);
> +	if (status & (BIT(0) | BIT(1))) {

please create macros for these bits.

{} around are not needed.


> +		printf("Secure Boot:\t%s%s\n",
> +		       status & BIT(0) ? "authenticated" : "not authenticated",
> +		       status & BIT(1) ? ", encrypted" : ", not encrypted");

Isn't this more space efficient?
printf("Secure Boot:\t%sauthenticated, %sencrypted\n",
	       status & BIT(0) ? "" : "not ",
	       status & BIT(1) ? "" : "not ");

And as I see it is.
aarch64: (for 1/1 boards) all -33.0 rodata -17.0 spl/u-boot-spl:all
-33.0 spl/u-boot-spl:rodata -17.0 spl/u-boot-spl:text -16.0 text -16.0
            xilinx_zynqmp_virt: all -33 rodata -17 spl/u-boot-spl:all
-33 spl/u-boot-spl:rodata -17 spl/u-boot-spl:text -16 text -16
               spl-u-boot-spl: add: 0/0, grow: 0/-1 bytes: 0/-16 (-16)


> +	}
> +}
> +
>  #define PS_SYSMON_ANALOG_BUS_VAL	0x3210
>  #define PS_SYSMON_ANALOG_BUS_REG	0xFFA50914
>  
> @@ -391,8 +403,10 @@ int board_init(void)
>  	fpga_add(fpga_xilinx, &zynqmppl);
>  #endif
>  
> -	if (current_el() == 3)
> +	if (current_el() == 3) {
>  		multi_boot();
> +		secure_boot();
> +	}

Please take a look at
https://lists.denx.de/pipermail/u-boot/2021-July/456382.html
I have changed multi_boot function a little bit.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

Re: [PATCH] arm64: zynqmp: Print the secure boot status information in EL3

[Jorge Ramirez-Ortiz, Foundries]

On 22/07/21, Jorge Ramirez-Ortiz wrote:

reminder

> Confirm the secure boot configuration on the console.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> ---
>  arch/arm/mach-zynqmp/include/mach/hardware.h |  3 ++-
>  board/xilinx/zynqmp/zynqmp.c                 | 16 +++++++++++++++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-zynqmp/include/mach/hardware.h b/arch/arm/mach-zynqmp/include/mach/hardware.h
> index 3776499070..3d3ffa086e 100644
> --- a/arch/arm/mach-zynqmp/include/mach/hardware.h
> +++ b/arch/arm/mach-zynqmp/include/mach/hardware.h
> @@ -139,7 +139,8 @@ struct apu_regs {
>  #define ZYNQMP_SILICON_VER_SHIFT	0
>  
>  struct csu_regs {
> -	u32 reserved0[4];
> +	u32 status;
> +	u32 reserved0[3];
>  	u32 multi_boot;
>  	u32 reserved1[11];
>  	u32 idcode;
> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> index 1748fec2e4..b7d11630d1 100644
> --- a/board/xilinx/zynqmp/zynqmp.c
> +++ b/board/xilinx/zynqmp/zynqmp.c
> @@ -355,6 +355,18 @@ static int multi_boot(void)
>  	return 0;
>  }
>  
> +static void secure_boot(void)
> +{
> +	u32 status;
> +
> +	status = readl(&csu_base->status);
> +	if (status & (BIT(0) | BIT(1))) {
> +		printf("Secure Boot:\t%s%s\n",
> +		       status & BIT(0) ? "authenticated" : "not authenticated",
> +		       status & BIT(1) ? ", encrypted" : ", not encrypted");
> +	}
> +}
> +
>  #define PS_SYSMON_ANALOG_BUS_VAL	0x3210
>  #define PS_SYSMON_ANALOG_BUS_REG	0xFFA50914
>  
> @@ -391,8 +403,10 @@ int board_init(void)
>  	fpga_add(fpga_xilinx, &zynqmppl);
>  #endif
>  
> -	if (current_el() == 3)
> +	if (current_el() == 3) {
>  		multi_boot();
> +		secure_boot();
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.31.1
> 

Re: [PATCH] arm64: zynqmp: Print the secure boot status information in EL3

[Igor Opaniuk]

Hi Jorge,

On Thu, Jul 22, 2021 at 2:19 PM Jorge Ramirez-Ortiz <jorge@foundries.io> wrote:
>
> Confirm the secure boot configuration on the console.
>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> ---
>  arch/arm/mach-zynqmp/include/mach/hardware.h |  3 ++-
>  board/xilinx/zynqmp/zynqmp.c                 | 16 +++++++++++++++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-zynqmp/include/mach/hardware.h b/arch/arm/mach-zynqmp/include/mach/hardware.h
> index 3776499070..3d3ffa086e 100644
> --- a/arch/arm/mach-zynqmp/include/mach/hardware.h
> +++ b/arch/arm/mach-zynqmp/include/mach/hardware.h
> @@ -139,7 +139,8 @@ struct apu_regs {
>  #define ZYNQMP_SILICON_VER_SHIFT       0
>
>  struct csu_regs {
> -       u32 reserved0[4];
> +       u32 status;
> +       u32 reserved0[3];
>         u32 multi_boot;
>         u32 reserved1[11];
>         u32 idcode;
> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> index 1748fec2e4..b7d11630d1 100644
> --- a/board/xilinx/zynqmp/zynqmp.c
> +++ b/board/xilinx/zynqmp/zynqmp.c
> @@ -355,6 +355,18 @@ static int multi_boot(void)
>         return 0;
>  }
>
> +static void secure_boot(void)
> +{
> +       u32 status;
> +
> +       status = readl(&csu_base->status);
> +       if (status & (BIT(0) | BIT(1))) {
> +               printf("Secure Boot:\t%s%s\n",
> +                      status & BIT(0) ? "authenticated" : "not authenticated",
> +                      status & BIT(1) ? ", encrypted" : ", not encrypted");
> +       }
> +}
> +
>  #define PS_SYSMON_ANALOG_BUS_VAL       0x3210
>  #define PS_SYSMON_ANALOG_BUS_REG       0xFFA50914
>
> @@ -391,8 +403,10 @@ int board_init(void)
>         fpga_add(fpga_xilinx, &zynqmppl);
>  #endif
>
> -       if (current_el() == 3)
> +       if (current_el() == 3) {
>                 multi_boot();
> +               secure_boot();
> +       }
>
>         return 0;
>  }
> --
> 2.31.1
>

Reviewed-by: Igor Opaniuk <igor.opaniuk@foundries.io>

-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk@gmail.com
skype: igor.opanyuk
+380 (93) 836 40 67
http://ua.linkedin.com/in/iopaniuk

[PATCH] arm64: zynqmp: Print the secure boot status information in EL3

[Jorge Ramirez-Ortiz]

Confirm the secure boot configuration on the console.

Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
---
 arch/arm/mach-zynqmp/include/mach/hardware.h |  3 ++-
 board/xilinx/zynqmp/zynqmp.c                 | 16 +++++++++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-zynqmp/include/mach/hardware.h b/arch/arm/mach-zynqmp/include/mach/hardware.h
index 3776499070..3d3ffa086e 100644
--- a/arch/arm/mach-zynqmp/include/mach/hardware.h
+++ b/arch/arm/mach-zynqmp/include/mach/hardware.h
@@ -139,7 +139,8 @@ struct apu_regs {
 #define ZYNQMP_SILICON_VER_SHIFT	0
 
 struct csu_regs {
-	u32 reserved0[4];
+	u32 status;
+	u32 reserved0[3];
 	u32 multi_boot;
 	u32 reserved1[11];
 	u32 idcode;
diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
index 1748fec2e4..b7d11630d1 100644
--- a/board/xilinx/zynqmp/zynqmp.c
+++ b/board/xilinx/zynqmp/zynqmp.c
@@ -355,6 +355,18 @@ static int multi_boot(void)
 	return 0;
 }
 
+static void secure_boot(void)
+{
+	u32 status;
+
+	status = readl(&csu_base->status);
+	if (status & (BIT(0) | BIT(1))) {
+		printf("Secure Boot:\t%s%s\n",
+		       status & BIT(0) ? "authenticated" : "not authenticated",
+		       status & BIT(1) ? ", encrypted" : ", not encrypted");
+	}
+}
+
 #define PS_SYSMON_ANALOG_BUS_VAL	0x3210
 #define PS_SYSMON_ANALOG_BUS_REG	0xFFA50914
 
@@ -391,8 +403,10 @@ int board_init(void)
 	fpga_add(fpga_xilinx, &zynqmppl);
 #endif
 
-	if (current_el() == 3)
+	if (current_el() == 3) {
 		multi_boot();
+		secure_boot();
+	}
 
 	return 0;
 }
-- 
2.31.1