Re: [PATCH net-next v7 3/7] arch: x86: add IPC mailbox accessor function and add SoC register access

David E. Box
2025-02-19T17:22:50Z

On Thu, 2025-02-06 at 08:46 -0800, Dave Hansen wrote:
> On 2/6/25 05:18, Choong Yong Liang wrote:
> >
> > - Exports intel_pmc_ipc() for host access to the PMC IPC mailbox
> > - Add support to use IPC command allows host to access SoC registers
> > through PMC firmware that are otherwise inaccessible to the host due
> > to security policies.
> I'm not quite parsing that second bullet as a complete sentence.
>
> But that sounds scary! Why is the fact that they are "otherwise
> inaccessible" relevant here?

The PMC IPC mailbox is a host interface to the PMC. Its purpose is to allow the
host to access certain areas of the PMC that are restricted from direct MMIO
access due to security policies. Other parts of the PMC are accessible via MMIO
(most of what the intel_pmc_core driver touches with is MMIO), so I think the
original statement was trying to explain why the mailbox is needed instead of
MMIO in this case. However, I agree that the mention of security policies is not
relevant to the change itself.

> ...
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 87198d957e2f..631c1f10776c 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -688,6 +688,15 @@ config X86_AMD_PLATFORM_DEVICE
> >     I2C and UART depend on COMMON_CLK to set clock. GPIO driver is
> >     implemented under PINCTRL subsystem.
> >
> > +config INTEL_PMC_IPC
> > + tristate "Intel Core SoC Power Management Controller IPC mailbox"
> > + depends on ACPI
> > + help
> > +   This option enables sideband register access support for Intel
> > SoC
> > +   power management controller IPC mailbox.
> > +
> > +   If you don't require the option or are in doubt, say N.
>
> Could we perhaps beef this up a bit to help users figure out if they
> want to turn this on? Really the only word in the entire help text
> that's useful is "Intel".
>
> I'm not even sure we *want* to expose this to users. Can we just leave
> it as:
>
> config INTEL_PMC_IPC
> def_tristate n
> depends on ACPI
>
> so that it only gets enabled by the "select" in the other patches?

I agree with this change Choong. This can be selected by the driver that's using
it. There's no need to expose it to users.

>
> > + * Authors: Choong Yong Liang <yong.liang.choong@linux.intel.com>
> > + *          David E. Box <david.e.box@linux.intel.com>
>
> I'd probably just leave the authors bit out. It might have been useful
> in the 90's, but that's what git is for today.
>
> > + obj = buffer.pointer;
> > + /* Check if the number of elements in package is 5 */
> > + if (obj && obj->type == ACPI_TYPE_PACKAGE && obj->package.count ==
> > 5) {
> > + const union acpi_object *objs = obj->package.elements;
> > +
>
> The comment there is just not super useful. It might be useful to say
> *why* the number of elements needs to be 5.
>
> > +EXPORT_SYMBOL(intel_pmc_ipc);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Intel PMC IPC Mailbox accessor");
>
> Honestly, is this even worth being a module? How much code are we
> talking about here?

Yeah, this doesn't need to be a module either.

David

>
> > diff --git a/include/linux/platform_data/x86/intel_pmc_ipc.h
> > b/include/linux/platform_data/x86/intel_pmc_ipc.h
> > new file mode 100644
> > index 000000000000..d47b89f873fc
> > --- /dev/null
> > +++ b/include/linux/platform_data/x86/intel_pmc_ipc.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Intel Core SoC Power Management Controller Header File
> > + *
> > + * Copyright (c) 2023, Intel Corporation.
> > + * All Rights Reserved.
> ...
>
> This copyright is a _bit_ funky. It's worth at least saying in the cover
> letter that this patch has been sitting untouched for over a year, thus
> the old copyright.
>
> Or, if you've done actual work with it, I'd assume the copyright needs
> to get updated.
>
> > +struct pmc_ipc_cmd {
> > + u32 cmd;
> > + u32 sub_cmd;
> > + u32 size;
> > + u32 wbuf[4];
> > +};
> > +
> > +/**
> > + * intel_pmc_ipc() - PMC IPC Mailbox accessor
> > + * @ipc_cmd:  struct pmc_ipc_cmd prepared with input to send
>
> You probably don't need to restate the literal type of ipc_cmd.
>
> > + * @rbuf:     Allocated u32[4] array for returned IPC data
>
> The "Allocated" thing here threw me a bit. Does this mean it *must* be
> "allocated" as in it comes from kmalloc()? Or can it be on the stack? Or
> part of a static variable?
>
> > + * Return: 0 on success. Non-zero on mailbox error
> > + */
> > +int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd, u32 *rbuf);
>
> Also, if it can *only* be u32[4], then the best way to declare it is:
>
> struct pmc_ipc_rbuf {
> u32 buf[4];
> };
>
> and:
>
> int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd,
>   struct pmc_ipc_rbuf rbuf *rbuf);
>
> Then you don't need a comment saying that it must be a u32[4]. It's
> implied in the structure.