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@xxxxxxx> > >> 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.