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.
