Re: [PATCH v2 04/16] platform/x86/amd/pmf: Add support for PMF Policy Binary

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6145 bytes --]

On Mon, 9 Oct 2023, Shyam Sundar S K wrote:
> On 10/4/2023 5:30 PM, Ilpo Järvinen wrote:
> > On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> > 
> >> PMF Policy binary is a encrypted and signed binary that will be part
> >> of the BIOS. PMF driver via the ACPI interface checks the existence
> >> of Smart PC bit. If the advertised bit is found, PMF driver walks
> >> the acpi namespace to find out the policy binary size and the address
> >> which has to be passed to the TA during the TA init sequence.
> >>
> >> The policy binary is comprised of inputs (or the events) and outputs
> >> (or the actions). With the PMF ecosystem, OEMs generate the policy
> >> binary (or could be multiple binaries) that contains a supported set
> >> of inputs and outputs which could be specifically carved out for each
> >> usage segment (or for each user also) that could influence the system
> >> behavior either by enriching the user experience or/and boost/throttle
> >> power limits.
> >>
> >> Once the TA init command succeeds, the PMF driver sends the changing
> >> events in the current environment to the TA for a constant sampling
> >> frequency time (the event here could be a lid close or open) and
> >> if the policy binary has corresponding action built within it, the
> >> TA sends the action for it in the subsequent enact command.
> >>
> >> If the inputs sent to the TA has no output defined in the policy
> >> binary generated by OEMs, there will be no action to be performed
> >> by the PMF driver.
> >>
> >> Example policies:
> >>
> >> 1) if slider is performance ; set the SPL to 40W
> >> Here PMF driver registers with the platform profile interface and
> >> when the slider position is changed, PMF driver lets the TA know
> >> about this. TA sends back an action to update the Sustained
> >> Power Limit (SPL). PMF driver updates this limit via the PMFW mailbox.
> >>
> >> 2) if user_away ; then lock the system
> >> Here PMF driver hooks to the AMD SFH driver to know the user presence
> >> and send the inputs to TA and if the condition is met, the TA sends
> >> the action of locking the system. PMF driver generates a uevent and
> >> based on the udev rule in the userland the system gets locked with
> >> systemctl.
> >>
> >> The intent here is to provide the OEM's to make a policy to lock the
> >> system when the user is away ; but the userland can make a choice to
> >> ignore it.
> >>
> >> and so on.
> >>
> >> The OEMs will have an utility to create numerous such policies and
> >> the policies shall be reviewed by AMD before signing and encrypting
> >> them. Policies are shared between operating systems to have seemless user
> >> experience.
> >>
> >> Since all this action has to happen via the "amdtee" driver, currently
> >> there is no caller for it in the kernel which can load the amdtee driver.
> >> Without amdtee driver loading onto the system the "tee" calls shall fail
> >> from the PMF driver. Hence an explicit "request_module" has been added
> >> to address this.
> >>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

> >> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> >> index 678dce4fea08..787f25511191 100644
> >> --- a/drivers/platform/x86/amd/pmf/core.c
> >> +++ b/drivers/platform/x86/amd/pmf/core.c
> >> @@ -384,6 +384,18 @@ static int amd_pmf_probe(struct platform_device *pdev)
> >>  		return -ENOMEM;
> >>  
> >>  	dev->dev = &pdev->dev;
> >> +	err = apmf_check_smart_pc(dev);
> >> +	if (!err) {
> >> +		/* in order for Smart PC solution to work it has a hard dependency
> >> +		 * on the amdtee driver to be loaded first even before the PMF driver
> >> +		 * loads. PMF ASL has a _CRS method that advertises the existence
> >> +		 * of Smart PC bit. If this information is present, use this to
> >> +		 * explicitly probe the amdtee driver, so that "tee" plumbing is done
> >> +		 * before the PMF Smart PC init happens.
> >> +		 */
> > 
> > But please follow no-text on /* line formatting for multiline comments. 
> > Also start with a capital letter.
> > 
> > 
> >> +		if (request_module("amdtee"))
> > 
> > Are you aware that this won't give you very strong guarantees about 
> > anything if request_module()'s function comments is to be believed?
> > 
> > If that's all what you're after, MODULE_SOFTDEP("pre: amdtee"); is 
> > probably enough (and I unfortunately don't know the answer how to do it if 
> > you want something stronger than that when you don't directly depend on 
> > the symbols of the other module).
> 
> MODULE_SOFTDEP("pre: amdtee"); did not help.

So how was this module loaded then? I suppose if the user does insmod, the 
softdep wouldn't be honored but modprobe should load the dependencies 
first.

> There is no consumer loading the 'amdtee' driver today in the kernel.
> Even now with this change, the pmf driver calls the TEE subsystem APIs
> that will eventually land in amdtee code.
>
> So the call flow would be:
> pmf driver-> tee subsystem -> amdtee driver -> ASP

Right, and that indirect route is why it won't be made as hard 
dependency.

> IMO, in order to make this link work request_module() would be
> required. Is that OK to retain request_module() in v3?

Fine.

> >> +	struct ta_pmf_enact_result policy_apply_table;
> >> +	u32 rsvd[906];
> > 
> > This is some size (SZ_1K?) - sizeof(ta_pmf_enact_result)? I don't know if 
> > compiler would like such a construct though in the array declaration. If 
> > the compiler isn't complaining it would be the most informative way to 
> > state the size but if it's not happy, a comment might be useful.
> 
> This is a reserved space for future use. And that's the same way
> defined in the FW as well. If I play with the sizes, the FW (PMF TA)
> starts to misbehave and does not provide outputs all the time.
> 
> Like you mentioned, are you Ok if I just put a comment as "reserved
> space for future use"?

You can put the comment if you want but I understood that even without 
the comment. I was just interested in how that magic number 906 was 
derived and if it could have been better defined.

I think you can leave the line as is.

-- 
 i.