[U-Boot] [PATCH] xilinx: zynq: Add support to secure images
Siva Durga Prasad Paladugu
sivadur at xilinx.com
Fri Jun 1 04:06:21 UTC 2018
Hi Stefan,
> -----Original Message-----
> From: Stefan Herbrechtsmeier [mailto:stefan at herbrechtsmeier.net]
> Sent: Thursday, May 31, 2018 10:57 PM
> To: Siva Durga Prasad Paladugu <sivadur at xilinx.com>; u-
> boot at lists.denx.de
> Cc: michal.simek at 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:stefan at herbrechtsmeier.net]
> >> Sent: Thursday, May 31, 2018 3:43 PM
> >> To: Siva Durga Prasad Paladugu <sivadur at xilinx.com>; u-
> >> boot at lists.denx.de
> >> Cc: michal.simek at 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.paladugu at 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
More information about the U-Boot
mailing list