Hi Stefan, > -----Original Message----- > From: Stefan Herbrechtsmeier [mailto:ste...@herbrechtsmeier.net] > Sent: Thursday, May 31, 2018 10:57 PM > To: Siva Durga Prasad Paladugu <siva...@xilinx.com>; u- > b...@lists.denx.de > Cc: michal.si...@xilinx.com > Subject: Re: [U-Boot] [PATCH] xilinx: zynq: Add support to secure images > > Hi Siva, > > Am 31.05.2018 um 12:37 schrieb Siva Durga Prasad Paladugu: > >> -----Original Message----- > >> From: Stefan Herbrechtsmeier [mailto:ste...@herbrechtsmeier.net] > >> Sent: Thursday, May 31, 2018 3:43 PM > >> To: Siva Durga Prasad Paladugu <siva...@xilinx.com>; u- > >> b...@lists.denx.de > >> Cc: michal.si...@xilinx.com > >> Subject: Re: [U-Boot] [PATCH] xilinx: zynq: Add support to secure > >> images > >> > >> Am 31.05.2018 um 11:25 schrieb Siva Durga Prasad Paladugu: > >>> This patch basically adds two new commands for loadig secure > >>> images/bitstreams. > >>> 1. zynq rsa adds support to load secure image which can be both > >>> authenticated or encrypted or both authenticated and encrypted > >>> image in xilinx bootimage(BOOT.bin) format. > >>> 2. zynq aes command adds support to decrypted and load encrypted > >>> image either back to DDR or it can load an encrypted bitsream > >>> to PL directly by decrypting it. The image has to be encrypted > >>> using xilinx bootgen tool and to get only the encrypted > >>> image from tool use -split option while invoking bootgen. > >>> > >>> Signed-off-by: Siva Durga Prasad Paladugu > >>> <siva.durga.palad...@xilinx.com> > >>> --- > >>> Changes from RFC: > >>> - Moved zynqaes to board/xilinx/zynq/cmds.c and renamed as > >>> "zynq aes". > >>> - Moved boot image parsing code to a separate file. > >>> - Squashed in to a single patch. > >>> - Fixed coding style comments. > >>> --- > >>> arch/arm/Kconfig | 1 + > >>> board/xilinx/zynq/Kconfig | 26 +++ > >>> board/xilinx/zynq/Makefile | 5 + > >>> board/xilinx/zynq/bootimg.c | 143 ++++++++++++ > >>> board/xilinx/zynq/cmds.c | 527 > >> +++++++++++++++++++++++++++++++++++++++++++ > >>> drivers/fpga/zynqpl.c | 65 ++++++ > >>> include/u-boot/rsa-mod-exp.h | 4 + > >>> include/zynq_bootimg.h | 33 +++ > >>> include/zynqpl.h | 5 + > >>> lib/rsa/rsa-mod-exp.c | 51 +++++ > >>> 10 files changed, 860 insertions(+) > >>> create mode 100644 board/xilinx/zynq/Kconfig > >>> create mode 100644 board/xilinx/zynq/bootimg.c > >>> create mode 100644 board/xilinx/zynq/cmds.c > >>> create mode 100644 include/zynq_bootimg.h > >>> > >> [snip] > >> > >>> diff --git a/board/xilinx/zynq/cmds.c b/board/xilinx/zynq/cmds.c new > >>> file mode 100644 index 0000000..6aebec1 > >>> --- /dev/null > >>> +++ b/board/xilinx/zynq/cmds.c > >>> @@ -0,0 +1,527 @@ > >> [snip] > >> > >>> +#ifdef CONFIG_SYS_LONGHELP > >>> +static char zynq_help_text[] = > >>> + "rsa <baseaddr> - Verifies the authenticated and encrypted\n" > >>> + " zynq images and loads them back to load\n" > >>> + " addresses as specified in BOOT > >>> image(BOOT.BIN)\n" > >>> + "aes [operation type] <srcaddr> <srclen> <dstaddr> <dstlen>\n" > >>> + "Decrypts the encrypted image present in source address\n" > >>> + "and places the decrypted image at destination address\n" > >>> + "zynqaes operations:\n" > >>> + " zynqaes <srcaddr> <srclen> <dstaddr> <dstlen>\n" > >>> + " zynqaes load <srcaddr> <srclen>\n" > >>> + " zynqaes loadp <srcaddr> <srclen>\n" > >>> + "if operation type is load or loadp, it loads the encrypted\n" > >>> + "full or partial bitstream on to PL respectively. If no valid\n" > >>> + "operation type specified then it loads decrypted image back\n" > >>> + "to memory and it doesn't support loading PL bistsream\n"; > >>> +#endif > >>> + > >>> +U_BOOT_CMD(zynq, 6, 0, do_zynq, > >>> + "Zynq specific commands RSA/AES verification ", > >>> +zynq_help_text ); > >> Why don't you integrate the encrypted fpga image support into the > >> fpga command? > >> > >> The encrypted fpga image could be detected at run time and the only > >> difference is a reduced pcap rate. > > Its not just handles encrypted fpga images, indeed it handles all > > encrypted Images so, that’s why it is here. > > But the encrypted fpga image is handled total different as it isn't copied > back to the memory. The encrypted fpga image command has more > similarity with the fpga command than the aes command. You introduce a > second command to program the fpga outside of the fpga framework. > Furthermore this command isn't supported by the fit image nor the spl. > > I think the main different between the encrypted and not encrypted fpga > image is the lower pcap rate. Because the encrypted fpga image is marked > as encrypted inside the image the software could automatically enable the > lower pcap rate.
I got your point, Please point me to such marker inside the fpga image if you are aware of. Till now, I thought we don’t have such mechanism and hence like this. Meanwhile, I will also try to find out on this internally. >This enables the spl to use an encrypted fpga image inside > a fit image. > > >>> diff --git a/drivers/fpga/zynqpl.c b/drivers/fpga/zynqpl.c index > >>> fd37d18..bdac49e 100644 > >>> --- a/drivers/fpga/zynqpl.c > >>> +++ b/drivers/fpga/zynqpl.c > >>> @@ -17,6 +17,7 @@ > >>> > >>> #define DEVCFG_CTRL_PCFG_PROG_B 0x40000000 > >>> #define DEVCFG_CTRL_PCFG_AES_EFUSE_MASK 0x00001000 > >>> +#define DEVCFG_CTRL_PCAP_RATE_EN_MASK 0x02000000 > >>> #define DEVCFG_ISR_FATAL_ERROR_MASK 0x00740040 > >>> #define DEVCFG_ISR_ERROR_FLAGS_MASK 0x00340840 > >>> #define DEVCFG_ISR_RX_FIFO_OV 0x00040000 > >>> @@ -497,3 +498,67 @@ struct xilinx_fpga_op zynq_op = { > >>> .loadfs = zynq_loadfs, > >>> #endif > >>> }; > >>> + > >>> +#ifdef CONFIG_CMD_ZYNQ_AES > >>> +/* > >>> + * Load the encrypted image from src addr and decrypt the image and > >>> + * place it back the decrypted image into dstaddr. > >>> + */ > >>> +int zynq_decrypt_load(u32 srcaddr, u32 srclen, u32 dstaddr, u32 > dstlen, > >>> + u8 bstype) > >>> +{ > >>> + u32 isr_status, ts; > >>> + > >>> + if (srcaddr < SZ_1M || dstaddr < SZ_1M) { > >>> + printf("%s: src and dst addr should be > 1M\n", > >>> + __func__); > >>> + return FPGA_FAIL; > >>> + } > >>> + > >>> + if (zynq_dma_xfer_init(bstype)) { > >>> + printf("%s: zynq_dma_xfer_init FAIL\n", __func__); > >>> + return FPGA_FAIL; > >>> + } > >>> + > >>> + writel((readl(&devcfg_base->ctrl) | > >> DEVCFG_CTRL_PCAP_RATE_EN_MASK), > >>> + &devcfg_base->ctrl); > >>> + > >>> + debug("%s: Source = 0x%08X\n", __func__, (u32)srcaddr); > >>> + debug("%s: Size = %zu\n", __func__, srclen); > >>> + > >>> + /* flush(clean & invalidate) d-cache range buf */ > >>> + flush_dcache_range((u32)srcaddr, (u32)srcaddr + > >>> + roundup(srclen << 2, ARCH_DMA_MINALIGN)); > >>> + /* > >>> + * Flush destination address range only if image is not > >>> + * bitstream. > >>> + */ > >>> + if (bstype == BIT_NONE) > >>> + flush_dcache_range((u32)dstaddr, (u32)dstaddr + > >>> + roundup(dstlen << 2, > >>> + ARCH_DMA_MINALIGN)); > >>> + > >>> + if (zynq_dma_transfer(srcaddr | 1, srclen, dstaddr | 1, dstlen)) > >>> + return FPGA_FAIL; > >>> + > >>> + if (bstype == BIT_FULL) { > >>> + isr_status = readl(&devcfg_base->int_sts); > >>> + /* Check FPGA configuration completion */ > >>> + ts = get_timer(0); > >>> + while (!(isr_status & DEVCFG_ISR_PCFG_DONE)) { > >>> + if (get_timer(ts) > CONFIG_SYS_FPGA_WAIT) { > >>> + printf("%s: Timeout wait for FPGA to > >>> config\n", > >>> + __func__); > >>> + return FPGA_FAIL; > >>> + } > >>> + isr_status = readl(&devcfg_base->int_sts); > >>> + } > >>> + > >>> + printf("%s: FPGA config done\n", __func__); > >>> + > >>> + if (bstype != BIT_PARTIAL) > >>> + zynq_slcr_devcfg_enable(); > >>> + } > >>> + > >>> + return FPGA_SUCCESS; > >>> +} > >>> +#endif > >> This function introduces a lot of duplicated code because it mostly > >> copies the zynq_load function. > > Yeah, but not much. I felt its better to be a separate code than handling > with lot of if's just to be clean. > > Also, most of functionality was anyway carried out in separate > > functions like dma init and xfer. So, not a big duplicate. > > Don't you duplicate the whole "check fpga configuration completion" code? We can move that code also to a routine and invoke that routine here. Anyway, lets first sort out the first comment as its more related to functionality and will come back to this later. Thanks, Siva > > Best regards > Stefan _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot