Nitheesh Sekar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33425
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
qcs405: Add support for specifying mbn_version
Change-Id: Ic6e269e0f290692871875000586410217c25fc08 Signed-off-by: Nitheesh Sekar nsekar@codeaurora.org --- M src/soc/qualcomm/qcs405/Makefile.inc M util/qualcomm/createxbl.py M util/qualcomm/mbn_tools.py 3 files changed, 62 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33425/1
diff --git a/src/soc/qualcomm/qcs405/Makefile.inc b/src/soc/qualcomm/qcs405/Makefile.inc index cf8268b..4faaffc 100644 --- a/src/soc/qualcomm/qcs405/Makefile.inc +++ b/src/soc/qualcomm/qcs405/Makefile.inc @@ -126,18 +126,12 @@ qc_sec_file := $(shell ls $(QC_SEC_FILE)) ifneq (,$(findstring $(QC_SEC_FILE),$(qc_sec_file))) $(objcbfs)/bootblock.bin: $(objcbfs)/bootblock.elf - python util/qualcomm/createxbl.py -f $(objcbfs)/bootblock.elf \ + python util/qualcomm/createxbl.py --mbn_version 6 -f $(objcbfs)/bootblock.elf \ -x $(QC_SEC_FILE) -o $(objcbfs)/merged_bb_qcsec.mbn \ -a 64 -d 32 -c 32 -ifeq ($(CONFIG_QC_FLASH_SIMULATE_SDCARD),y) - @printf "\nqgpt.py 512 sector size\n" - python util/qualcomm/qgpt.py -s 512 $(objcbfs)/merged_bb_qcsec.mbn \ - $(objcbfs)/bootblock.bin -else @printf "\nqgpt.py 4K sector size\n" python util/qualcomm/qgpt.py $(objcbfs)/merged_bb_qcsec.mbn \ $(objcbfs)/bootblock.bin -endif
else $(objcbfs)/bootblock.bin: $(objcbfs)/bootblock.raw.bin diff --git a/util/qualcomm/createxbl.py b/util/qualcomm/createxbl.py index 4a21854..ad5934d 100755 --- a/util/qualcomm/createxbl.py +++ b/util/qualcomm/createxbl.py @@ -44,6 +44,7 @@ # # when who what, where, why # -------- --- ------------------------------------------------------ +# 05/21/19 rissha Added --mbn_version to add MBN header accordingly # 03/26/18 tv Added -e to enable extended MBNV5 support # 09/04/15 et Added -x and -d to embed xbl_sec ELF # 02/11/15 ck Fixed missing elf type check in ZI OOB feature @@ -119,6 +120,10 @@ help="Removes ZI segments that have addresses greater" + \ " than 32 bits when converting from a 64 to 32 bit ELF")
+ parser.add_option("--mbn_version", + action="store", type="int", dest="mbn_version", + help="Add mbn header in elf image. '5' or '6'") +
(options, args) = parser.parse_args() if not options.elf_inp_file1: @@ -206,11 +211,16 @@ else: zi_oob_enabled = True
- if options.elf_inp_xbl_sec: + header_version = None + + if options.elf_inp_xbl_sec or options.mbn_version == 5: is_ext_mbn_v5 = True + header_version = 5 else: is_ext_mbn_v5 = False
+ if options.mbn_version: + header_version = options.mbn_version
mbn_type = 'elf' header_format = 'reg' @@ -259,7 +269,7 @@ source_elf, target_hash, elf_out_file_name = target_phdr_elf, - secure_type = image_header_secflag) + secure_type = image_header_secflag, header_version = header_version ) if rv: raise RuntimeError, "Failed to run pboot_gen_elf"
@@ -270,7 +280,8 @@ target_hash_hd, image_header_secflag, is_ext_mbn_v5, - elf_file_name = source_elf) + elf_file_name = source_elf, + header_version = header_version) if rv: raise RuntimeError, "Failed to create image header for hash segment"
diff --git a/util/qualcomm/mbn_tools.py b/util/qualcomm/mbn_tools.py index 12dc210..c702f96 100755 --- a/util/qualcomm/mbn_tools.py +++ b/util/qualcomm/mbn_tools.py @@ -41,6 +41,7 @@ # # when who what, where, why # -------- --- --------------------------------------------------------- +# 05/21/18 rissha Added support for extended MBNV6 and Add support for hashing elf segments with SHA384 # 03/22/18 thiru Added support for extended MBNV5. # 06/06/13 yliong CR 497042: Signed and encrypted image is corrupted. MRC features. # 03/18/13 dhaval Add support for hashing elf segments with SHA256 and @@ -69,6 +70,7 @@ SHA256_SIGNATURE_SIZE = 256 # Support SHA256 MAX_NUM_ROOT_CERTS = 4 # Maximum number of OEM root certificates MI_BOOT_IMG_HDR_SIZE = 40 # sizeof(mi_boot_image_header_type) +MI_BOOT_IMG_HDR_SIZE_v6 = 48 # sizeof(mi_boot_image_header_type) for v6 header MI_BOOT_SBL_HDR_SIZE = 80 # sizeof(sbl_header) BOOT_HEADER_LENGTH = 20 # Boot Header Number of Elements SBL_HEADER_LENGTH = 20 # SBL Header Number of Elements @@ -551,7 +553,7 @@ def getLength(self): return BOOT_HEADER_LENGTH
- def writePackedData(self, target, write_full_hdr): + def writePackedData(self, target, write_full_hdr, header_version = None): values = [self.image_id, self.flash_parti_ver, self.image_src, @@ -573,6 +575,10 @@ self.reserved_2, self.reserved_3 ]
+ if header_version == 6: + values.insert(10, self.metadata_size_qti) + values.insert(11, self.metadata_size) + if self.image_dest_ptr >= 0x100000000: values[3] = 0xFFFFFFFF
@@ -584,8 +590,12 @@
# Write 10 entries(40B) or 20 entries(80B) of boot header if write_full_hdr is False: - s = struct.Struct('I'* 10) - values = values[:10] + if header_version == 6: + s = struct.Struct('I'* 12) + values = values[:12] + else: + s = struct.Struct('I'* 10) + values = values[:10] else: s = struct.Struct('I' * self.getLength())
@@ -912,7 +922,8 @@ write_full_hdr = False, in_code_size = None, cert_chain_size_in = CERT_CHAIN_ONEROOT_MAXSIZE, - num_of_pages = None): + num_of_pages = None, + header_version = None):
# Preliminary checks if (requires_preamble is True) and (preamble_file_name is None): @@ -947,7 +958,7 @@
# For ELF or hashed images, image destination will be determined from an ELF input file if gen_dict['IMAGE_KEY_MBN_TYPE'] == 'elf': - image_dest = get_hash_address(elf_file_name) + MI_BOOT_IMG_HDR_SIZE + image_dest = get_hash_address(elf_file_name) + (MI_BOOT_IMG_HDR_SIZE_v6 if header_version == 6 else MI_BOOT_IMG_HDR_SIZE) elif gen_dict['IMAGE_KEY_MBN_TYPE'] == 'bin': image_dest = gen_dict['IMAGE_KEY_IMAGE_DEST'] image_source = gen_dict['IMAGE_KEY_IMAGE_SOURCE'] @@ -993,17 +1004,24 @@ boot_header.cert_chain_size = cert_chain_size
if is_ext_mbn_v5 == True: - # If platform image integrity check is enabled - boot_header.flash_parti_ver = 5 # version - boot_header.image_src = 0 # sig_size_qc - boot_header.image_dest_ptr = 0 # cert_chain_size_qc + # If platform image integrity check is enabled + boot_header.flash_parti_ver = 5 # version + boot_header.image_src = 0 # sig_size_qc + boot_header.image_dest_ptr = 0 # cert_chain_size_qc + + if header_version == 6: + boot_header.flash_parti_ver = 6 # version + boot_header.image_src = 0 # sig_size_qc + boot_header.image_dest_ptr =0 # cert_chain_size_qc + boot_header.metadata_size_qti = 0 # qti_metadata size + boot_header.metadata_size = 0 # oem_metadata size
# If preamble is required, output the preamble file and update the boot_header if requires_preamble is True: boot_header = image_preamble(gen_dict, preamble_file_name, boot_header, num_of_pages)
# Package up the header and write to output file - boot_header.writePackedData(target = output_file_name, write_full_hdr = write_full_hdr) + boot_header.writePackedData(target = output_file_name, write_full_hdr = write_full_hdr, header_version = header_version)
else: raise RuntimeError, "Header format not supported: " + str(header_format) @@ -1021,13 +1039,23 @@ last_phys_addr = None, append_xml_hdr = False, is_sha256_algo = True, - cert_chain_size_in = CERT_CHAIN_ONEROOT_MAXSIZE): + cert_chain_size_in = CERT_CHAIN_ONEROOT_MAXSIZE, + header_version = None): + is_sha384_algo = False + if header_version == 6: + is_sha384_algo = True global MI_PROG_BOOT_DIGEST_SIZE + image_header_size = MI_BOOT_IMG_HDR_SIZE + if (is_sha256_algo is True): MI_PROG_BOOT_DIGEST_SIZE = 32 else: MI_PROG_BOOT_DIGEST_SIZE = 20
+ if is_sha384_algo: + MI_PROG_BOOT_DIGEST_SIZE = 48 + image_header_size = MI_BOOT_IMG_HDR_SIZE_v6 + # Open Files elf_in_fp = OPEN(elf_in_file_name, "rb") hash_out_fp = OPEN(hash_out_file_name, "wb+") @@ -1110,7 +1138,7 @@ fbuf = elf_in_fp.read(hash_size)
if MI_PBT_CHECK_FLAG_TYPE(curr_phdr.p_flags) is True: - hash = generate_hash(fbuf, is_sha256_algo) + hash = generate_hash(fbuf, is_sha256_algo, is_sha384_algo) else: hash = '\0' * MI_PROG_BOOT_DIGEST_SIZE
@@ -1129,7 +1157,7 @@ file_buff = elf_in_fp.read(data_len)
if (MI_PBT_CHECK_FLAG_TYPE(curr_phdr.p_flags) is True) and (data_len > 0): - hash = generate_hash(file_buff, is_sha256_algo) + hash = generate_hash(file_buff, is_sha256_algo, is_sha384_algo) else: hash = '\0' * MI_PROG_BOOT_DIGEST_SIZE
@@ -1151,7 +1179,7 @@
# Initialize the hash table program header [hash_Phdr, pad_hash_segment, hash_tbl_end_addr, hash_tbl_offset] = \ - initialize_hash_phdr(elf_in_file_name, hashtable_size, MI_BOOT_IMG_HDR_SIZE, ELF_BLOCK_ALIGN, is_elf64) + initialize_hash_phdr(elf_in_file_name, hashtable_size, image_header_size, ELF_BLOCK_ALIGN, is_elf64)
# Check if hash segment max size parameter was passed if (hash_seg_max_size is not None): @@ -1252,7 +1280,7 @@ # Read the program header and compute hash proghdr_buff = elf_out_fp.read(elf_header.e_phnum * phdr_size)
- hash = generate_hash(elfhdr_buff + proghdr_buff, is_sha256_algo) + hash = generate_hash(elfhdr_buff + proghdr_buff, is_sha256_algo, is_sha384_algo)
# Write hash to file as first hash table entry hash_out_fp.seek(0) @@ -2101,9 +2129,11 @@ #---------------------------------------------------------------------------- # sha1/sha256 hash routine wrapper #---------------------------------------------------------------------------- -def generate_hash(in_buf, is_sha256_algo): +def generate_hash(in_buf, is_sha256_algo, is_sha384_algo = False): # Initialize a SHA1 object from the Python hash library - if (is_sha256_algo is True): + if is_sha384_algo: + m = hashlib.sha384() + elif (is_sha256_algo is True): m = hashlib.sha256() else: m = hashlib.sha1()
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
Patch Set 1: Code-Review+2
Hello build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33425
to look at the new patch set (#2).
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
qcs405: Add support for specifying mbn_version
Change-Id: Ic6e269e0f290692871875000586410217c25fc08 Signed-off-by: Nitheesh Sekar nsekar@codeaurora.org Signed-off-by: Rishabh Sharma rissha@qti.qualcomm.com --- M src/soc/qualcomm/qcs405/Makefile.inc M util/qualcomm/createxbl.py M util/qualcomm/mbn_tools.py 3 files changed, 62 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33425/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
Patch Set 2:
(8 comments)
https://review.coreboot.org/#/c/33425/2/util/qualcomm/createxbl.py File util/qualcomm/createxbl.py:
https://review.coreboot.org/#/c/33425/2/util/qualcomm/createxbl.py@125 PS2, Line 125: help="Add mbn header in elf image. '5' or '6'") I think for boards where elf_inp_xbl_sec is False (i.e. IPQ boards), the MBN version is actually 3?
https://review.coreboot.org/#/c/33425/2/util/qualcomm/createxbl.py@214 PS2, Line 214: None As per above, this should probably default to 3? Or maybe we should just make mbn_version non-optional and change all the other Makefiles to pass what they need.
https://review.coreboot.org/#/c/33425/2/util/qualcomm/createxbl.py@217 PS2, Line 217: is_ext_mbn_v5 I think all this means is that the MBN version is 5, so we should combine it with your new variable rather than adding a new one. So I'd just change this to
if options.elf_inp_xbl_sec and not options.mbn_version: header_version = 5
(or, as mentioned above, force all boards to pass in mbn_version rather than heuristicking it out like this).
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py File util/qualcomm/mbn_tools.py:
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@73 PS2, Line 73: MI_BOOT_IMG_HDR_SIZE_v6 = 48 # sizeof(mi_boot_image_header_type) for v6 header You need this a lot so maybe better to make a header_size(header_version) helper function to be able to retrieve it where you need it?
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@77 PS2, Line 77: FLASH_PARTI_VERSION = 3 # Flash Partition Version Number I think this is the "default" mbn version? If so, we should remove this constant, make sure header_version is initialized for all code paths, and use that instead.
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@556 PS2, Line 556: , header_version = None Does this need to be passed separately if we already have it in flash_parti_ver?
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@1017 PS2, Line 1017: boot_header.metadata_size = 0 # oem_metadata size Do these last two really need to be guarded? Why not set them unconditionally, and writePackedData will just ignore them if MBN version is too low?
Then you can write the rest as
boot_header.flash_parti_ver = header_versio if header_version >= 5: boot_header.image_src = 0 boot_header.image_dest_ptr = 0
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@2132 PS2, Line 2132: is_sha256_algo, is_sha384_algo Please just have one 'algo' parameter that can be SHA1, SHA256 or SHA384
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
Patch Set 2:
Regarding Julius' comments: are these scripts maintained for coreboot or do they come from somewhere else and just end up here because we need them, too?
While he's right on all his comments, we considered similar scripts "vendorcode" that we import wholesale.
Rishabh Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
Patch Set 2:
(8 comments)
https://review.coreboot.org/#/c/33425/2/util/qualcomm/createxbl.py File util/qualcomm/createxbl.py:
https://review.coreboot.org/#/c/33425/2/util/qualcomm/createxbl.py@125 PS2, Line 125: help="Add mbn header in elf image. '5' or '6'")
I think for boards where elf_inp_xbl_sec is False (i.e. […]
Yes correct, if xbl_sec_elf input is not provided then version will be default 3, that why I make this --mbn_version as optional argument
https://review.coreboot.org/#/c/33425/2/util/qualcomm/createxbl.py@214 PS2, Line 214: None
As per above, this should probably default to 3? Or maybe we should just make mbn_version non-option […]
I cant make --mbn_version non optional as it will break mbn_tools for other targets. So according to this logic header_version is optional and there can be following case which will preserve the backward compatibility- 1.) if no input xbl_sec and no --mbn_version then header_version will be None and mbn_tools will add default version 3 to the image 2.) if xbl_sec is given as input and no --mbn_version then header_version will be 5 3.) if --mbn_version is given, this will take precedence over all parameters
https://review.coreboot.org/#/c/33425/2/util/qualcomm/createxbl.py@217 PS2, Line 217: is_ext_mbn_v5
I think all this means is that the MBN version is 5, so we should combine it with your new variable […]
Cant force all boards to pass mbn_version because it can break mbn_tools invocation for other chipsets
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py File util/qualcomm/mbn_tools.py:
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@73 PS2, Line 73: MI_BOOT_IMG_HDR_SIZE_v6 = 48 # sizeof(mi_boot_image_header_type) for v6 header
You need this a lot so maybe better to make a header_size(header_version) helper function to be able […]
Done
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@77 PS2, Line 77: FLASH_PARTI_VERSION = 3 # Flash Partition Version Number
I think this is the "default" mbn version? If so, we should remove this constant, make sure header_v […]
Removing this and and make header_version initialized for all code path may be risky it will break the older functionality and backward compatibility
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@556 PS2, Line 556: , header_version = None
Does this need to be passed separately if we already have it in flash_parti_ver?
Done
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@1017 PS2, Line 1017: boot_header.metadata_size = 0 # oem_metadata size
Do these last two really need to be guarded? Why not set them unconditionally, and writePackedData w […]
It will be confusion to a newer person who is not aware of the mbn header structure so this condition clarifies the content of the particular header version
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@2132 PS2, Line 2132: is_sha256_algo, is_sha384_algo
Please just have one 'algo' parameter that can be SHA1, SHA256 or SHA384
Done
Rishabh Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py File util/qualcomm/mbn_tools.py:
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@73 PS2, Line 73: MI_BOOT_IMG_HDR_SIZE_v6 = 48 # sizeof(mi_boot_image_header_type) for v6 header
Done
I will create a function but I will not remove these contants because I dont know weather some older chipsets are using this constants from mbn_tool after import
Hello build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33425
to look at the new patch set (#3).
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
qcs405: Add support for specifying mbn_version
Change-Id: Ic6e269e0f290692871875000586410217c25fc08 Signed-off-by: Nitheesh Sekar nsekar@codeaurora.org Signed-off-by: Rishabh Sharma rissha@qti.qualcomm.com --- M src/soc/qualcomm/qcs405/Makefile.inc M util/qualcomm/createxbl.py M util/qualcomm/mbn_tools.py 3 files changed, 77 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33425/3
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
Patch Set 4: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
Patch Set 4: Code-Review-1
(10 comments)
https://review.coreboot.org/#/c/33425/2/util/qualcomm/createxbl.py File util/qualcomm/createxbl.py:
https://review.coreboot.org/#/c/33425/2/util/qualcomm/createxbl.py@214 PS2, Line 214: None
I cant make --mbn_version non optional as it will break mbn_tools for other targets. […]
Well, you can just change the Makefiles of the other targets to pass --mbn_version as well. We don't have out-of-tree boards in coreboot, all users of this tool are in this repository and can be changed. Or are you also using this same script elsewhere and want the backwards-compatibility there?
I'm not opposed to leaving a heuristic for backwards-compatibility if you want to, though. Although we should maybe still update the Makefiles anyway because it makes them easier to read.
https://review.coreboot.org/#/c/33425/4/util/qualcomm/createxbl.py File util/qualcomm/createxbl.py:
https://review.coreboot.org/#/c/33425/4/util/qualcomm/createxbl.py@214 PS4, Line 214: None Didn't we agree this should be 3 if it is not set to anything else?
https://review.coreboot.org/#/c/33425/4/util/qualcomm/createxbl.py@217 PS4, Line 217: is_ext_mbn_v5 = True Again, you should remove this variable because header_version == 5 already means the same thing.
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py File util/qualcomm/mbn_tools.py:
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@73 PS2, Line 73: MI_BOOT_IMG_HDR_SIZE_v6 = 48 # sizeof(mi_boot_image_header_type) for v6 header
I will create a function but I will not remove these contants because I dont know weather some older […]
Again, not really sure what chipsets you're talking about. The only coreboot code using any of this are the three Makefiles of SDM845, QCS405 and IPQ40xx. All of them call createxbl.py. Nothing else uses mbn_tools.py.
https://review.coreboot.org/#/c/33425/2/util/qualcomm/mbn_tools.py@77 PS2, Line 77: FLASH_PARTI_VERSION = 3 # Flash Partition Version Number
Removing this and and make header_version initialized for all code path may be risky it will break t […]
Why? If you make sure that header_version is always initialized to some correct default in createxbl.py, you can assume that it is set here. Having this constant is redundant with the header_version option and incorrect whenever that option is not set to 3. It makes the code confusing.
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py File util/qualcomm/mbn_tools.py:
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py@73 PS4, Line 73: MI_BOOT_IMG_HDR_SIZE_v6 = 48 # sizeof(mi_boot_image_header_type) for v6 header I think both of these constants are unused not and can be removed. You're using the header_size() function instead.
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py@76 PS4, Line 76: VERSION_6 = 6 # Mbn header_version 6 These constants seem kinda pointless, and you're mostly not using them anyway.
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py@1010 PS4, Line 1010: # Please align comment correctly
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py@1018 PS4, Line 1018: =0 spacing
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py@1058 PS4, Line 1058: if (sha_algo is 'SHA384'): Please use == for string comparison
Rishabh Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/#/c/33425/4/util/qualcomm/createxbl.py File util/qualcomm/createxbl.py:
https://review.coreboot.org/#/c/33425/4/util/qualcomm/createxbl.py@214 PS4, Line 214: None
Didn't we agree this should be 3 if it is not set to anything else?
Yes default can be 3. I am agree with this.
https://review.coreboot.org/#/c/33425/4/util/qualcomm/createxbl.py@217 PS4, Line 217: is_ext_mbn_v5 = True
Again, you should remove this variable because header_version == 5 already means the same thing.
Done
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py File util/qualcomm/mbn_tools.py:
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py@1010 PS4, Line 1010: #
Please align comment correctly
Done
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py@1018 PS4, Line 1018: =0
spacing
Done
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py@1058 PS4, Line 1058: if (sha_algo is 'SHA384'):
Please use == for string comparison
Done
Rishabh Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py File util/qualcomm/mbn_tools.py:
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py@73 PS4, Line 73: MI_BOOT_IMG_HDR_SIZE_v6 = 48 # sizeof(mi_boot_image_header_type) for v6 header
I think both of these constants are unused not and can be removed. […]
Done
https://review.coreboot.org/#/c/33425/4/util/qualcomm/mbn_tools.py@76 PS4, Line 76: VERSION_6 = 6 # Mbn header_version 6
These constants seem kinda pointless, and you're mostly not using them anyway.
in new patch all version 3,5 and 6 will use these constants
Hello Julius Werner, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33425
to look at the new patch set (#5).
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
qcs405: Add support for specifying mbn_version
Change-Id: Ic6e269e0f290692871875000586410217c25fc08 Signed-off-by: Nitheesh Sekar nsekar@codeaurora.org Signed-off-by: Rishabh Sharma rissha@qti.qualcomm.com --- M src/soc/qualcomm/qcs405/Makefile.inc M util/qualcomm/createxbl.py M util/qualcomm/mbn_tools.py 3 files changed, 75 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33425/5
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
Patch Set 5:
(7 comments)
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/createxbl.py File util/qualcomm/createxbl.py:
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/createxbl.py@... PS5, Line 125: '5' or '6'" or '3'
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/createxbl.py@... PS5, Line 217: is_ext_mbn_v5 = True Again, you can remove this variable now. (You said "Done" but it's still here.)
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py File util/qualcomm/mbn_tools.py:
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py@... PS5, Line 78: FLASH_PARTI_VERSION = 3 # Flash Partition Version Number Should remove this and use header_version everywhere.
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py@... PS5, Line 1006: Should we maybe also assert that header version is one of the supported values (3, 5 or 6) somewhere around here, just to be sure?
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py@... PS5, Line 1007: if is_ext_mbn_v5 == True: This should check header_version == VERSION_5 instead.
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py@... PS5, Line 1009: boot_header.flash_parti_ver = VERSION_5 # version Just do flash_parti_ver = header_version once outside the conditional.
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py@... PS5, Line 1050: is_sha384_algo = False Unused?
Patrick Georgi has uploaded a new patch set (#7) to the change originally created by Nitheesh Sekar. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
qcs405: Add support for specifying mbn_version
Change-Id: Ic6e269e0f290692871875000586410217c25fc08 Signed-off-by: Nitheesh Sekar nsekar@codeaurora.org Signed-off-by: Rishabh Sharma rissha@qti.qualcomm.com --- M src/soc/qualcomm/qcs405/Makefile.inc M util/qualcomm/createxbl.py M util/qualcomm/mbn_tools.py 3 files changed, 75 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33425/7
Rishabh Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/createxbl.py File util/qualcomm/createxbl.py:
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/createxbl.py@... PS5, Line 125: '5' or '6'"
or '3'
Done
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/createxbl.py@... PS5, Line 217: is_ext_mbn_v5 = True
Again, you can remove this variable now. (You said "Done" but it's still here. […]
This variable is used at many places. I can't see any value add by deleting this variable.
Rishabh Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py File util/qualcomm/mbn_tools.py:
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py@... PS5, Line 78: FLASH_PARTI_VERSION = 3 # Flash Partition Version Number
Should remove this and use header_version everywhere.
Done
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py@... PS5, Line 1006:
Should we maybe also assert that header version is one of the supported values (3, 5 or 6) somewhere […]
Done
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py@... PS5, Line 1007: if is_ext_mbn_v5 == True:
This should check header_version == VERSION_5 instead.
If I modify this, mbn_tools will not remain backward compatible where some board is using is_ext_mbn_v5 to add mbn header version 5. So its better to keep it as unchanged so that backward compatibility is maintained.
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py@... PS5, Line 1009: boot_header.flash_parti_ver = VERSION_5 # version
Just do flash_parti_ver = header_version once outside the conditional.
Again same comment as above. We have not enforced createxbl to provide header_version. Header_version is optional to mbn_tools. So this type of assignment is better to maintain backward compatibility where createxbl is invoking mbn_tools with is_ext_mbn_v5 as True without header_version.
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py@... PS5, Line 1050: is_sha384_algo = False
Unused?
Done
Hello Julius Werner, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33425
to look at the new patch set (#8).
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
qcs405: Add support for specifying mbn_version
Change-Id: Ic6e269e0f290692871875000586410217c25fc08 Signed-off-by: Nitheesh Sekar nsekar@codeaurora.org Signed-off-by: Rishabh Sharma rissha@qti.qualcomm.com --- M src/soc/qualcomm/qcs405/Makefile.inc M util/qualcomm/createxbl.py M util/qualcomm/mbn_tools.py 3 files changed, 91 insertions(+), 37 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/33425/8
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/createxbl.py File util/qualcomm/createxbl.py:
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/createxbl.py@... PS5, Line 217: is_ext_mbn_v5 = True
This variable is used at many places. I can't see any value add by deleting this variable.
The point is that header_version == 5 means exactly the same as is_ext_mbn_v5 == True now. It is redundant. You're introducing a new variable that can serve all purposes of what this variable did (and more), so please replace existing instances of this variable with that. We don't need the same information X times encoded in X different ways in the code, that just makes things confusing.
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py File util/qualcomm/mbn_tools.py:
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py@... PS5, Line 1007: if is_ext_mbn_v5 == True:
If I modify this, mbn_tools will not remain backward compatible where some board is using is_ext_mbn […]
I don't follow. is_ext_mbn_v5 is just an implementation detail of this script, you can change it without affecting externally visible behavior. Those boards just pass the --xbl_sec_filepath argument to this script, which used to only set is_ext_mbn_v5 to True, but with your change it also sets header_version to 5. So you can use header_version == 5 as a 100% equivalent check to is_ext_mbn_v5 == True (and therefore get rid of the is_ext_mbn_v5 vairable).
https://review.coreboot.org/c/coreboot/+/33425/5/util/qualcomm/mbn_tools.py@... PS5, Line 1009: boot_header.flash_parti_ver = VERSION_5 # version
We have not enforced createxbl to provide header_version.
...but you did? That's why I asked you to initialize header_version to 3 by default (and overwrite it to 5 or 6 where appropriate). So that you will always have a defined header_version you can rely on here and don't need that other variable anymore. header_version should not be optional, it should be required now.
Patrick Georgi has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33425 )
Change subject: qcs405: Add support for specifying mbn_version ......................................................................
Abandoned
won't be finished here
-
Julius Werner (Code Review) -
Nitheesh Sekar (Code Review)
-
Patrick Georgi (Code Review)
-
Rishabh Sharma (Code Review)