[U-Boot] [PATCH] xilinx: zynq: Add support to secure images
Stefan Herbrechtsmeier
stefan at herbrechtsmeier.net
Thu May 31 17:26:33 UTC 2018
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. 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?
Best regards
Stefan
More information about the U-Boot
mailing list