| Message ID | 20250206131859.2960543-4-yong.liang.choong@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Enable SGMII and 2500BASEX interface mode switching for Intel platforms | expand |
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? ... > 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? > + * 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? > 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.
On Thu, Feb 06, 2025 at 08:46:24AM -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? And i wounder what other interesting things can be accessed in this indirect manor, which the security policy would not otherwise allow. Somebody is going to have fun here. Or is already have fun here. Andrew
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.
On 20/2/2025 1:01 am, David E. Box wrote: > 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 > Hi David, Thank you for the confirmation. Let's work together to address the comments.
diff --git a/MAINTAINERS b/MAINTAINERS index d1086e53a317..7a0681f83449 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11862,8 +11862,10 @@ M: Rajneesh Bhardwaj <irenic.rajneesh@gmail.com> M: David E Box <david.e.box@intel.com> L: platform-driver-x86@vger.kernel.org S: Maintained +F: arch/x86/platform/intel/pmc_ipc.c F: Documentation/ABI/testing/sysfs-platform-intel-pmc F: drivers/platform/x86/intel/pmc/ +F: linux/platform_data/x86/intel_pmc_ipc.h INTEL PMIC GPIO DRIVERS M: Andy Shevchenko <andy@kernel.org> 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. + config IOSF_MBI tristate "Intel SoC IOSF Sideband support for SoC platforms" depends on PCI diff --git a/arch/x86/platform/intel/Makefile b/arch/x86/platform/intel/Makefile index dbee3b00f9d0..2f1805556d0c 100644 --- a/arch/x86/platform/intel/Makefile +++ b/arch/x86/platform/intel/Makefile @@ -1,2 +1,3 @@ # SPDX-License-Identifier: GPL-2.0-only obj-$(CONFIG_IOSF_MBI) += iosf_mbi.o +obj-$(CONFIG_INTEL_PMC_IPC) += pmc_ipc.o diff --git a/arch/x86/platform/intel/pmc_ipc.c b/arch/x86/platform/intel/pmc_ipc.c new file mode 100644 index 000000000000..a96234982710 --- /dev/null +++ b/arch/x86/platform/intel/pmc_ipc.c @@ -0,0 +1,75 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Intel Core SoC Power Management Controller IPC mailbox + * + * Copyright (c) 2023, Intel Corporation. + * All Rights Reserved. + * + * Authors: Choong Yong Liang <yong.liang.choong@linux.intel.com> + * David E. Box <david.e.box@linux.intel.com> + */ +#include <linux/module.h> +#include <linux/acpi.h> +#include <linux/platform_data/x86/intel_pmc_ipc.h> + +#define PMC_IPCS_PARAM_COUNT 7 + +int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd, u32 *rbuf) +{ + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; + union acpi_object params[PMC_IPCS_PARAM_COUNT] = { + {.type = ACPI_TYPE_INTEGER,}, + {.type = ACPI_TYPE_INTEGER,}, + {.type = ACPI_TYPE_INTEGER,}, + {.type = ACPI_TYPE_INTEGER,}, + {.type = ACPI_TYPE_INTEGER,}, + {.type = ACPI_TYPE_INTEGER,}, + {.type = ACPI_TYPE_INTEGER,}, + }; + struct acpi_object_list arg_list = { PMC_IPCS_PARAM_COUNT, params }; + union acpi_object *obj; + int status; + + if (!ipc_cmd || !rbuf) + return -EINVAL; + + /* + * 0: IPC Command + * 1: IPC Sub Command + * 2: Size + * 3-6: Write Buffer for offset + */ + params[0].integer.value = ipc_cmd->cmd; + params[1].integer.value = ipc_cmd->sub_cmd; + params[2].integer.value = ipc_cmd->size; + params[3].integer.value = ipc_cmd->wbuf[0]; + params[4].integer.value = ipc_cmd->wbuf[1]; + params[5].integer.value = ipc_cmd->wbuf[2]; + params[6].integer.value = ipc_cmd->wbuf[3]; + + status = acpi_evaluate_object(NULL, "\\IPCS", &arg_list, &buffer); + if (ACPI_FAILURE(status)) + return -ENODEV; + + 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; + + if ((u8)objs[0].integer.value != 0) + return -EINVAL; + + rbuf[0] = objs[1].integer.value; + rbuf[1] = objs[2].integer.value; + rbuf[2] = objs[3].integer.value; + rbuf[3] = objs[4].integer.value; + } else { + return -EINVAL; + } + + return 0; +} +EXPORT_SYMBOL(intel_pmc_ipc); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("Intel PMC IPC Mailbox accessor"); 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. + * + * Authors: Choong Yong Liang <yong.liang.choong@linux.intel.com> + * David E. Box <david.e.box@linux.intel.com> + */ +#ifndef INTEL_PMC_IPC_H +#define INTEL_PMC_IPC_H + +#define IPC_SOC_REGISTER_ACCESS 0xAA +#define IPC_SOC_SUB_CMD_READ 0x00 +#define IPC_SOC_SUB_CMD_WRITE 0x01 + +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 + * @rbuf: Allocated u32[4] array for returned IPC data + * + * Return: 0 on success. Non-zero on mailbox error + */ +int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd, u32 *rbuf); + +#endif /* INTEL_PMC_IPC_H */