Hi Sughosh, On Sat, 5 Aug 2023 at 13:12, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > hi Simon, > > On Sun, 6 Aug 2023 at 00:06, Simon Glass <s...@chromium.org> wrote: > > > > Hi Sughosh, > > > > On Sat, 5 Aug 2023 at 12:01, Sughosh Ganu <sughosh.g...@linaro.org> wrote: > > > > > > hi Simon, > > > > > > On Sat, 5 Aug 2023 at 20:34, Simon Glass <s...@chromium.org> wrote: > > > > > > > > Hi Sughosh, > > > > > > > > On Sat, 5 Aug 2023 at 05:35, Sughosh Ganu <sughosh.g...@linaro.org> > > > > wrote: > > > > > > > > > > The EFI capsule files can now be generated as part of u-boot > > > > > build through binman. Add capsule entry nodes in the u-boot.dtsi for > > > > > the sandbox architecture for generating the capsules. These capsules > > > > > are then used for testing the EFI capsule update functionality on the > > > > > sandbox platforms. Also add binman nodes for generating the input > > > > > files used for these capsules. > > > > > > > > > > Remove the corresponding logic which was used for generation of these > > > > > input files which is now superfluous. > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.g...@linaro.org> > > > > > --- > > > > > Changes since V6: > > > > > * Use macros defined in sandbox_efi_capsule for GUIDs and capsule > > > > > input filenames. > > > > > * Generate the capsule input files through binman text entries. > > > > > > > > > > arch/sandbox/dts/u-boot.dtsi | 363 > > > > > ++++++++++++++++++ > > > > > include/sandbox_efi_capsule.h | 11 + > > > > > test/py/tests/test_efi_capsule/conftest.py | 168 +------- > > > > > test/py/tests/test_efi_capsule/signature.dts | 10 - > > > > > .../tests/test_efi_capsule/uboot_bin_env.its | 36 -- > > > > > 5 files changed, 385 insertions(+), 203 deletions(-) > > > > > delete mode 100644 test/py/tests/test_efi_capsule/signature.dts > > > > > delete mode 100644 test/py/tests/test_efi_capsule/uboot_bin_env.its > > > > > > > > I think you are still getting confused with using filenames when you > > > > don't need to. See below... > > > > > > No, I don't think so. This is being done on purpose, since I want > > > these files to be created. These are then copied to the efi capsule > > > update tests' data directory, and are then used in testing the > > > feature. Maybe if you want, I can put a comment to that effect. > > > > I just traced through this code. I really think this needs to be > > simplified. You can do it in a patch at the end if you like. > > > > But you have the 'u-boot.bin.old' and 'Old' strings in > > test_efi_capsule_auth2, for example. > > > > In the binman world you define UBOOT_BIN_IMAGE_OLD as > > "u-boot.bin.old", then use that in the sandbox.dtsi > > > > Then binman creates the u-boot.bin.old file (unnecessarily in my view) > > Then in efi_capsule_data() you copy the file to the test directory. > > > > So for the last step, you could just create the file again, rather > > than copying it from the U-Boot build directory. After all, you know > > the contents. If you like you could put the different contents as > > binary strings in capsule_defs.py > > > > Then you don't need to create the files. > > Okay. TBH, I thought you would ask me to reuse the files created in > binman for the tests as well, which is why I put this logic. I will > create these files as part of the tests then. > > > > > There is a lot more I can say about the EFI capsule tests. For now I > > think we'll have to live with it creating 19 different binman images > > on sandbox. I think we could put them in an efi_capsules subdir, but > > that would need to be created somewhere, since binman doesn't do it. I > > suppose we could make binman automatically create a directory if an > > entry filename needs it? > > I think this can be taken up as a follow-up. I also was thinking of > adding a flag/property to not generate all the map files. I don't > think such a property exists currently. The map files really are not > needed for the capsules.
Sure, but if you implemented SetImagePos() then it would work. Something to think about when you create the tool to dump capsules. > > > > > Anyway, for tests, primarily we need to split things up, so we have, > > for example: > > > > process_capsule_file() > > > > which processes the capsule in U-Boot, e.g. using an 'efi' command, > > then outputs what it did. Then the test can plant the capsule, run > > that function and check that the output is as expected. This can > > actually be a unit test, i.e. written in C. > > > > Most of the tests can do this. Then we can have one test that checks > > the whole flow, but it doesn't need to be done for every case, as now. > > I believe even Ilias thinks that these tests should be in C. But this > is a separate effort, not related to this series. I also have doubts > about your observation on not using so many capsule files for tests, > but that can be investigated separately. For now, I want to focus on > getting these changes in, followed by the generation of capsules > through the config file. And FWIW, I am able to use relative paths in > the binman tests for generating and testing the capsules generated > through the config file -- so that takes care of your objection to > using absolute paths. I will send that series once this gets merged. OK, yes it is best to get this series in and worry about tdy-ups after that. Regards, Simon