On Tue, 2020-11-03 at 11:15 -0800, Lakshmi Ramasubramanian wrote: > On 11/3/20 6:55 AM, Mimi Zohar wrote: > > Hi Mimi, > > Thanks for reviewing the patches. > > > On Fri, 2020-10-30 at 10:44 -0700, Lakshmi Ramasubramanian wrote: > >> The functions remove_ima_buffer() and delete_fdt_mem_rsv() that handle > >> carrying forward the IMA measurement logs on kexec for powerpc do not > >> have architecture specific code, but they are currently defined for > >> powerpc only. > > > > ^ ... logs on kexec, do not have architecture specific code, but are > > currently limited to powerpc. > Will make this change. > > > > >> > >> remove_ima_buffer() and delete_fdt_mem_rsv() are used to remove > >> the IMA log entry from the device tree and free the memory reserved > >> for the log. These functions need to be defined even if the current > >> kernel does not support carrying forward IMA log across kexec since > >> the previous kernel could have supported that and therefore the current > >> kernel needs to free the allocation. > > > > The first paragraph describes these function as "handle carrying > > forward the IMA measurement logs on kexec", while this paragraph says > > "are used to remove the IMA log entry". Consider listing all of the > > functions being moved in the first paragrah, then "handle carrying > > forward" could be expanded to "carrying ... and removing". > Sure. Sorry, even with naming do_get_kexec_buffer(), the measurement list isn't being carried across kexec. Please adjust the wording. > > > > >> > >> Rename remove_ima_buffer() to remove_ima_kexec_buffer(). > >> Define remove_ima_kexec_buffer() and delete_fdt_mem_rsv() in > >> drivers/of/fdt.c. A later patch in this series will use these functions > >> to free the allocation, if any, made by the previous kernel for ARM64. > > > > - ^Define -> Move > > - Three functions are being moved, but only two are listed. > > "do_get_kexec_buffer" is not mentioned. > > - Don't refer to a later patch, but explain the purpose here. For > > example, "Move ... , making them accessible to other archs." > Sure. > > > > >> > >> Define FDT_PROP_IMA_KEXEC_BUFFER for the chosen node, namely > >> "linux,ima-kexec-buffer", that is added to the DTB to hold > >> the address and the size of the memory reserved to carry > >> the IMA measurement log. > > > > The above two paragraphs describe renaming a function and defining a > > chosen node. These two preparatory changes should be made, > > independently of each other, prior to this patch. This patch should be > > limited to moving code, with the subject line truncated to "move arch > > independent code to drivers/of". > > > > Just to be clear - > > Split this patch into 3 parts as listed below: > > PATCH #1: Rename remove_ima_buffer() to remove_ima_kexec_buffer() > PATCH #2: Define the chosen node > PATCH #3: Move the functions to drivers/of/fdt.c yes, thanks. Mimi > > Sure - I'll make the above changes and update patch descriptions > accordingly.
