Re: [PATCH] xsm: refactor and optimize policy loading

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 5/24/22 12:17, Jason Andryuk wrote:
> On Mon, May 23, 2022 at 11:40 AM Daniel P. Smith
> <dpsmith@xxxxxxxxxxxxxxxxxxxx> wrote:
>>
>> It is possible to select a few different build configurations that results in
>> the unnecessary walking of the boot module list looking for a policy module.
>> This specifically occurs when the flask policy is enabled but either the 
>> dummy
>> or the SILO policy is selected as the enforcing policy. This is not ideal for
>> configurations like hyperlaunch and dom0less when there could be a number of
>> modules to be walked or unnecessary device tree lookups
>>
>> This patch does two things, it moves all policy initialization logic under 
>> the
>> xsm_XXXX_policy_init() functions and introduces the init_policy flag.  The
>> init_policy flag will be set based on which enforcing policy is selected and
>> gates whether the boot modules should be checked for a policy file.
> 
> I can see the use of init_policy to skip the search.  (I'm not the
> biggest fan of the name, need_policy/uses_policy/has_policy?, but
> that's not a big deal).  That part seems fine.

Yep, I went through that and several other variations trying to find the
one that would "read" best at the places it was set and checked. If
anyone has a strong stance for a preferred naming, I have no objections.

> I don't care for the movement of `policy_buffer =
> xsm_flask_init_policy;` since it replaces the single location with two
> locations.  I prefer leaving the built-in policy fallback in
> xsm_core_init since it is multiboot/devicetree agnostic.  i.e. the
> boot-method specific code passes a policy if it finds one, and
> xsm_core_init can fallback to the built-in policy if none is supplied.
> Since a built-in policy is flask specific, it could potentially be
> pushed down in flask_init.
> 
> Is there a need for the xsm_flask_init_policy movement I am missing?

I would be willing to bet that the de-duplication is the reason it was
initially done this way. But as I explained in the response to Jan,
doing so means that xsm_XXXX_policy_init() will have to return success
even if it did not successfully initialize the policy buffer. I
considered making a static inline function to point the policy buffer at
the built-in policy, but quickly walked back from it. The reason being
is that it is not any real logic duplications, just two lines of
variable setting. IMHO a single repeat of setting a pair of variables is
better than the bifurcating of the policy buffer initialization.

v/r,
dps