[PATCH v3 0/5] platform/x86/amd/pmf: Updates to AMD PMF driver

[PATCH v3 0/5] platform/x86/amd/pmf: Updates to AMD PMF driver

[Shyam Sundar S K]

Updates include:

- Add support for newer platforms
- Enhance the TA shared memory size to adapt to the updated TA
- Update the MAINTAINERS record, and
- Simplify certain code handling.


v3:
----
- Add a comment to answer "why" resource_size() is not used for getting the
  ACPI resource length.

v2:
----
- drop resource_size() usage
- use right format specifier

Shyam Sundar S K (5):
  platform/x86/amd/pmf: Add SMU metrics table support for 1Ah family 60h
    model
  platform/x86/amd/pmf: Use dev_err_probe() to simplify error handling
  MAINTAINERS: Change AMD PMF driver status to "Supported"
  platform/x86/amd/pmf: Switch to platform_get_resource() and
    devm_ioremap_resource()
  platform/x86/amd/pmf: Add PMF driver changes to make compatible with
    PMF-TA

 MAINTAINERS                           |  2 +-
 drivers/platform/x86/amd/pmf/Kconfig  |  1 +
 drivers/platform/x86/amd/pmf/acpi.c   | 46 +++++++++++----------------
 drivers/platform/x86/amd/pmf/core.c   |  9 +++---
 drivers/platform/x86/amd/pmf/pmf.h    |  8 +++--
 drivers/platform/x86/amd/pmf/spc.c    |  1 +
 drivers/platform/x86/amd/pmf/tee-if.c |  8 ++---
 7 files changed, 36 insertions(+), 39 deletions(-)

-- 
2.34.1

[PATCH v3 1/5] platform/x86/amd/pmf: Add SMU metrics table support for 1Ah family 60h model

[Shyam Sundar S K]

Add SMU metrics table support for 1Ah family 60h model. This information
will be used by the PMF driver to alter the system thermals.

Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/core.c | 1 +
 drivers/platform/x86/amd/pmf/spc.c  | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index d6af0ca036f1..347bb43a5f2b 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -261,6 +261,7 @@ int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev, bool alloc_buffer)
 			dev->mtable_size = sizeof(dev->m_table);
 			break;
 		case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
+		case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
 			dev->mtable_size = sizeof(dev->m_table_v2);
 			break;
 		default:
diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
index b5183969f9bf..06226eb0eab3 100644
--- a/drivers/platform/x86/amd/pmf/spc.c
+++ b/drivers/platform/x86/amd/pmf/spc.c
@@ -86,6 +86,7 @@ static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_ta
 					 ARRAY_SIZE(dev->m_table.avg_core_c0residency), in);
 		break;
 	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
+	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
 		memcpy(&dev->m_table_v2, dev->buf, dev->mtable_size);
 		in->ev_info.socket_power = dev->m_table_v2.apu_power + dev->m_table_v2.dgpu_power;
 		in->ev_info.skin_temperature = dev->m_table_v2.skin_temp;
-- 
2.34.1

Re: [PATCH v3 1/5] platform/x86/amd/pmf: Add SMU metrics table support for 1Ah family 60h model

[Mario Limonciello]

On 10/23/2024 01:32, Shyam Sundar S K wrote:
> Add SMU metrics table support for 1Ah family 60h model. This information
> will be used by the PMF driver to alter the system thermals.
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/platform/x86/amd/pmf/core.c | 1 +
>   drivers/platform/x86/amd/pmf/spc.c  | 1 +
>   2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index d6af0ca036f1..347bb43a5f2b 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -261,6 +261,7 @@ int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev, bool alloc_buffer)
>   			dev->mtable_size = sizeof(dev->m_table);
>   			break;
>   		case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
> +		case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
>   			dev->mtable_size = sizeof(dev->m_table_v2);
>   			break;
>   		default:
> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> index b5183969f9bf..06226eb0eab3 100644
> --- a/drivers/platform/x86/amd/pmf/spc.c
> +++ b/drivers/platform/x86/amd/pmf/spc.c
> @@ -86,6 +86,7 @@ static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_ta
>   					 ARRAY_SIZE(dev->m_table.avg_core_c0residency), in);
>   		break;
>   	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
> +	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
>   		memcpy(&dev->m_table_v2, dev->buf, dev->mtable_size);
>   		in->ev_info.socket_power = dev->m_table_v2.apu_power + dev->m_table_v2.dgpu_power;
>   		in->ev_info.skin_temperature = dev->m_table_v2.skin_temp;

Re: [PATCH v3 1/5] platform/x86/amd/pmf: Add SMU metrics table support for 1Ah family 60h model

[Hans de Goede]

Hi,

On 23-Oct-24 8:32 AM, Shyam Sundar S K wrote:
> Add SMU metrics table support for 1Ah family 60h model. This information
> will be used by the PMF driver to alter the system thermals.
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans



> ---
>  drivers/platform/x86/amd/pmf/core.c | 1 +
>  drivers/platform/x86/amd/pmf/spc.c  | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index d6af0ca036f1..347bb43a5f2b 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -261,6 +261,7 @@ int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev, bool alloc_buffer)
>  			dev->mtable_size = sizeof(dev->m_table);
>  			break;
>  		case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
> +		case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
>  			dev->mtable_size = sizeof(dev->m_table_v2);
>  			break;
>  		default:
> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> index b5183969f9bf..06226eb0eab3 100644
> --- a/drivers/platform/x86/amd/pmf/spc.c
> +++ b/drivers/platform/x86/amd/pmf/spc.c
> @@ -86,6 +86,7 @@ static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_ta
>  					 ARRAY_SIZE(dev->m_table.avg_core_c0residency), in);
>  		break;
>  	case PCI_DEVICE_ID_AMD_1AH_M20H_ROOT:
> +	case PCI_DEVICE_ID_AMD_1AH_M60H_ROOT:
>  		memcpy(&dev->m_table_v2, dev->buf, dev->mtable_size);
>  		in->ev_info.socket_power = dev->m_table_v2.apu_power + dev->m_table_v2.dgpu_power;
>  		in->ev_info.skin_temperature = dev->m_table_v2.skin_temp;

[PATCH v3 2/5] platform/x86/amd/pmf: Use dev_err_probe() to simplify error handling

[Shyam Sundar S K]

To simplify error handling in the amd_pmf probe function and reduce code
size, dev_err() is replaced with dev_err_probe().

Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index 347bb43a5f2b..12c3e929fdd1 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -430,18 +430,18 @@ static int amd_pmf_probe(struct platform_device *pdev)
 
 	err = amd_smn_read(0, AMD_PMF_BASE_ADDR_LO, &val);
 	if (err) {
-		dev_err(dev->dev, "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_LO);
 		pci_dev_put(rdev);
-		return pcibios_err_to_errno(err);
+		return dev_err_probe(dev->dev, pcibios_err_to_errno(err),
+				     "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_LO);
 	}
 
 	base_addr_lo = val & AMD_PMF_BASE_ADDR_HI_MASK;
 
 	err = amd_smn_read(0, AMD_PMF_BASE_ADDR_HI, &val);
 	if (err) {
-		dev_err(dev->dev, "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_HI);
 		pci_dev_put(rdev);
-		return pcibios_err_to_errno(err);
+		return dev_err_probe(dev->dev, pcibios_err_to_errno(err),
+				     "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_HI);
 	}
 
 	base_addr_hi = val & AMD_PMF_BASE_ADDR_LO_MASK;
-- 
2.34.1

Re: [PATCH v3 2/5] platform/x86/amd/pmf: Use dev_err_probe() to simplify error handling

[Mario Limonciello]

On 10/23/2024 01:32, Shyam Sundar S K wrote:
> To simplify error handling in the amd_pmf probe function and reduce code
> size, dev_err() is replaced with dev_err_probe().
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/platform/x86/amd/pmf/core.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 347bb43a5f2b..12c3e929fdd1 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -430,18 +430,18 @@ static int amd_pmf_probe(struct platform_device *pdev)
>   
>   	err = amd_smn_read(0, AMD_PMF_BASE_ADDR_LO, &val);
>   	if (err) {
> -		dev_err(dev->dev, "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_LO);
>   		pci_dev_put(rdev);
> -		return pcibios_err_to_errno(err);
> +		return dev_err_probe(dev->dev, pcibios_err_to_errno(err),
> +				     "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_LO);
>   	}
>   
>   	base_addr_lo = val & AMD_PMF_BASE_ADDR_HI_MASK;
>   
>   	err = amd_smn_read(0, AMD_PMF_BASE_ADDR_HI, &val);
>   	if (err) {
> -		dev_err(dev->dev, "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_HI);
>   		pci_dev_put(rdev);
> -		return pcibios_err_to_errno(err);
> +		return dev_err_probe(dev->dev, pcibios_err_to_errno(err),
> +				     "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_HI);
>   	}
>   
>   	base_addr_hi = val & AMD_PMF_BASE_ADDR_LO_MASK;

[PATCH v3 3/5] MAINTAINERS: Change AMD PMF driver status to "Supported"

[Shyam Sundar S K]

The AMD PMF driver is actively being developed, so the MAINTAINERS record
should reflect "Supported" instead of "Maintained." Update the MAINTAINERS
database to reflect this change.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index e9659a5a7fb3..5748b251daac 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1124,7 +1124,7 @@ F:	drivers/platform/x86/amd/pmc/
 AMD PMF DRIVER
 M:	Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
 L:	platform-driver-x86@vger.kernel.org
-S:	Maintained
+S:	Supported
 F:	Documentation/ABI/testing/sysfs-amd-pmf
 F:	drivers/platform/x86/amd/pmf/
 
-- 
2.34.1

Re: [PATCH v3 3/5] MAINTAINERS: Change AMD PMF driver status to "Supported"

[Mario Limonciello]

On 10/23/2024 01:32, Shyam Sundar S K wrote:
> The AMD PMF driver is actively being developed, so the MAINTAINERS record
> should reflect "Supported" instead of "Maintained." Update the MAINTAINERS
> database to reflect this change.
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   MAINTAINERS | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9659a5a7fb3..5748b251daac 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1124,7 +1124,7 @@ F:	drivers/platform/x86/amd/pmc/
>   AMD PMF DRIVER
>   M:	Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>   L:	platform-driver-x86@vger.kernel.org
> -S:	Maintained
> +S:	Supported
>   F:	Documentation/ABI/testing/sysfs-amd-pmf
>   F:	drivers/platform/x86/amd/pmf/
>   

[PATCH v3 4/5] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

[Shyam Sundar S K]

Use platform_get_resource() to fetch the memory resource instead of
acpi_walk_resources() and devm_ioremap_resource() for mapping the
resources.

PS: We cannot use resource_size() here because it adds an extra byte to round
off the size. In the case of PMF ResourceTemplate(), this rounding is
already handled within the _CRS. Using resource_size() would increase the
resource size by 1, causing a mismatch with the length field and leading
to issues. Therefore, simply use end-start of the ACPI resource to obtain
the actual length.

Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/Kconfig  |  1 +
 drivers/platform/x86/amd/pmf/acpi.c   | 46 +++++++++++----------------
 drivers/platform/x86/amd/pmf/pmf.h    |  6 ++--
 drivers/platform/x86/amd/pmf/tee-if.c |  8 ++---
 4 files changed, 28 insertions(+), 33 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
index f4fa8bd8bda8..99d67cdbd91e 100644
--- a/drivers/platform/x86/amd/pmf/Kconfig
+++ b/drivers/platform/x86/amd/pmf/Kconfig
@@ -11,6 +11,7 @@ config AMD_PMF
 	select ACPI_PLATFORM_PROFILE
 	depends on TEE && AMDTEE
 	depends on AMD_SFH_HID
+	depends on HAS_IOMEM
 	help
 	  This driver provides support for the AMD Platform Management Framework.
 	  The goal is to enhance end user experience by making AMD PCs smarter,
diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
index d5b496433d69..62f984fe40c6 100644
--- a/drivers/platform/x86/amd/pmf/acpi.c
+++ b/drivers/platform/x86/amd/pmf/acpi.c
@@ -433,37 +433,29 @@ int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
 	return 0;
 }
 
-static acpi_status apmf_walk_resources(struct acpi_resource *res, void *data)
+int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
 {
-	struct amd_pmf_dev *dev = data;
+	struct platform_device *pdev = to_platform_device(pmf_dev->dev);
 
-	switch (res->type) {
-	case ACPI_RESOURCE_TYPE_ADDRESS64:
-		dev->policy_addr = res->data.address64.address.minimum;
-		dev->policy_sz = res->data.address64.address.address_length;
-		break;
-	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
-		dev->policy_addr = res->data.fixed_memory32.address;
-		dev->policy_sz = res->data.fixed_memory32.address_length;
-		break;
-	}
-
-	if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ || dev->policy_sz == 0) {
-		pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
-		return AE_ERROR;
+	pmf_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!pmf_dev->res) {
+		dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");
+		return -EINVAL;
 	}
 
-	return AE_OK;
-}
-
-int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
-{
-	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
-	acpi_status status;
-
-	status = acpi_walk_resources(ahandle, METHOD_NAME__CRS, apmf_walk_resources, pmf_dev);
-	if (ACPI_FAILURE(status)) {
-		dev_dbg(pmf_dev->dev, "acpi_walk_resources failed :%d\n", status);
+	pmf_dev->policy_addr = pmf_dev->res->start;
+	/*
+	 * We cannot use resource_size() here because it adds an extra byte to round off the size.
+	 * In the case of PMF ResourceTemplate(), this rounding is already handled within the _CRS.
+	 * Using resource_size() would increase the resource size by 1, causing a mismatch with the
+	 * length field and leading to issues. Therefore, simply use end-start of the ACPI resource
+	 * to obtain the actual length.
+	 */
+	pmf_dev->policy_sz = pmf_dev->res->end - pmf_dev->res->start;
+
+	if (!pmf_dev->policy_addr || pmf_dev->policy_sz > POLICY_BUF_MAX_SZ ||
+	    pmf_dev->policy_sz == 0) {
+		dev_err(pmf_dev->dev, "Incorrect policy params, possibly a SBIOS bug\n");
 		return -EINVAL;
 	}
 
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index 8ce8816da9c1..a79808fda1d8 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -13,6 +13,7 @@
 
 #include <linux/acpi.h>
 #include <linux/input.h>
+#include <linux/platform_device.h>
 #include <linux/platform_profile.h>
 
 #define POLICY_BUF_MAX_SZ		0x4b000
@@ -355,19 +356,20 @@ struct amd_pmf_dev {
 	/* Smart PC solution builder */
 	struct dentry *esbin;
 	unsigned char *policy_buf;
-	u32 policy_sz;
+	resource_size_t policy_sz;
 	struct tee_context *tee_ctx;
 	struct tee_shm *fw_shm_pool;
 	u32 session_id;
 	void *shbuf;
 	struct delayed_work pb_work;
 	struct pmf_action_table *prev_data;
-	u64 policy_addr;
+	resource_size_t policy_addr;
 	void __iomem *policy_base;
 	bool smart_pc_enabled;
 	u16 pmf_if_version;
 	struct input_dev *pmf_idev;
 	size_t mtable_size;
+	struct resource *res;
 };
 
 struct apmf_sps_prop_granular_v2 {
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 19c27b6e4666..555b8d6314e0 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -257,7 +257,7 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
 		return -ENODEV;
 	}
 
-	dev_dbg(dev->dev, "Policy Binary size: %u bytes\n", dev->policy_sz);
+	dev_dbg(dev->dev, "Policy Binary size: %llu bytes\n", dev->policy_sz);
 	memset(dev->shbuf, 0, dev->policy_sz);
 	ta_sm = dev->shbuf;
 	in = &ta_sm->pmf_input.init_table;
@@ -512,9 +512,9 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 	if (ret)
 		goto error;
 
-	dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr, dev->policy_sz);
-	if (!dev->policy_base) {
-		ret = -ENOMEM;
+	dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
+	if (IS_ERR(dev->policy_base)) {
+		ret = PTR_ERR(dev->policy_base);
 		goto error;
 	}
 
-- 
2.34.1

Re: [PATCH v3 4/5] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

[Mario Limonciello]

On 10/23/2024 01:32, Shyam Sundar S K wrote:
> Use platform_get_resource() to fetch the memory resource instead of
> acpi_walk_resources() and devm_ioremap_resource() for mapping the
> resources.
> 
> PS: We cannot use resource_size() here because it adds an extra byte to round
> off the size. In the case of PMF ResourceTemplate(), this rounding is
> already handled within the _CRS. Using resource_size() would increase the
> resource size by 1, causing a mismatch with the length field and leading
> to issues. Therefore, simply use end-start of the ACPI resource to obtain
> the actual length.
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>   drivers/platform/x86/amd/pmf/Kconfig  |  1 +
>   drivers/platform/x86/amd/pmf/acpi.c   | 46 +++++++++++----------------
>   drivers/platform/x86/amd/pmf/pmf.h    |  6 ++--
>   drivers/platform/x86/amd/pmf/tee-if.c |  8 ++---
>   4 files changed, 28 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
> index f4fa8bd8bda8..99d67cdbd91e 100644
> --- a/drivers/platform/x86/amd/pmf/Kconfig
> +++ b/drivers/platform/x86/amd/pmf/Kconfig
> @@ -11,6 +11,7 @@ config AMD_PMF
>   	select ACPI_PLATFORM_PROFILE
>   	depends on TEE && AMDTEE
>   	depends on AMD_SFH_HID
> +	depends on HAS_IOMEM
>   	help
>   	  This driver provides support for the AMD Platform Management Framework.
>   	  The goal is to enhance end user experience by making AMD PCs smarter,
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index d5b496433d69..62f984fe40c6 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -433,37 +433,29 @@ int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
>   	return 0;
>   }
>   
> -static acpi_status apmf_walk_resources(struct acpi_resource *res, void *data)
> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>   {
> -	struct amd_pmf_dev *dev = data;
> +	struct platform_device *pdev = to_platform_device(pmf_dev->dev);
>   
> -	switch (res->type) {
> -	case ACPI_RESOURCE_TYPE_ADDRESS64:
> -		dev->policy_addr = res->data.address64.address.minimum;
> -		dev->policy_sz = res->data.address64.address.address_length;
> -		break;
> -	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> -		dev->policy_addr = res->data.fixed_memory32.address;
> -		dev->policy_sz = res->data.fixed_memory32.address_length;
> -		break;
> -	}
> -
> -	if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ || dev->policy_sz == 0) {
> -		pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
> -		return AE_ERROR;
> +	pmf_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!pmf_dev->res) {
> +		dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");
> +		return -EINVAL;
>   	}
>   
> -	return AE_OK;
> -}
> -
> -int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
> -{
> -	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
> -	acpi_status status;
> -
> -	status = acpi_walk_resources(ahandle, METHOD_NAME__CRS, apmf_walk_resources, pmf_dev);
> -	if (ACPI_FAILURE(status)) {
> -		dev_dbg(pmf_dev->dev, "acpi_walk_resources failed :%d\n", status);
> +	pmf_dev->policy_addr = pmf_dev->res->start;
> +	/*
> +	 * We cannot use resource_size() here because it adds an extra byte to round off the size.
> +	 * In the case of PMF ResourceTemplate(), this rounding is already handled within the _CRS.
> +	 * Using resource_size() would increase the resource size by 1, causing a mismatch with the
> +	 * length field and leading to issues. Therefore, simply use end-start of the ACPI resource
> +	 * to obtain the actual length.
> +	 */
> +	pmf_dev->policy_sz = pmf_dev->res->end - pmf_dev->res->start;
> +
> +	if (!pmf_dev->policy_addr || pmf_dev->policy_sz > POLICY_BUF_MAX_SZ ||
> +	    pmf_dev->policy_sz == 0) {
> +		dev_err(pmf_dev->dev, "Incorrect policy params, possibly a SBIOS bug\n");

This upgrades the previous message from debug to error.

TL;DR I feel this error should stay as dev_dbg() if no function checks 
are present for Smart PC.

I don't think it's necessarily an error though.
Smart PC checks are a bit different than the other checks.  There isn't 
a check for a bit being set to indicate the function is supported.

So if the BIOS has the declaration for the region but it's not populated 
it might not have a Smart PC policy and this shouldn't be reported as a 
BIOS bug.

>   		return -EINVAL;
>   	}
>   
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 8ce8816da9c1..a79808fda1d8 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -13,6 +13,7 @@
>   
>   #include <linux/acpi.h>
>   #include <linux/input.h>
> +#include <linux/platform_device.h>
>   #include <linux/platform_profile.h>
>   
>   #define POLICY_BUF_MAX_SZ		0x4b000
> @@ -355,19 +356,20 @@ struct amd_pmf_dev {
>   	/* Smart PC solution builder */
>   	struct dentry *esbin;
>   	unsigned char *policy_buf;
> -	u32 policy_sz;
> +	resource_size_t policy_sz;
>   	struct tee_context *tee_ctx;
>   	struct tee_shm *fw_shm_pool;
>   	u32 session_id;
>   	void *shbuf;
>   	struct delayed_work pb_work;
>   	struct pmf_action_table *prev_data;
> -	u64 policy_addr;
> +	resource_size_t policy_addr;
>   	void __iomem *policy_base;
>   	bool smart_pc_enabled;
>   	u16 pmf_if_version;
>   	struct input_dev *pmf_idev;
>   	size_t mtable_size;
> +	struct resource *res;
>   };
>   
>   struct apmf_sps_prop_granular_v2 {
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 19c27b6e4666..555b8d6314e0 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -257,7 +257,7 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
>   		return -ENODEV;
>   	}
>   
> -	dev_dbg(dev->dev, "Policy Binary size: %u bytes\n", dev->policy_sz);
> +	dev_dbg(dev->dev, "Policy Binary size: %llu bytes\n", dev->policy_sz);
>   	memset(dev->shbuf, 0, dev->policy_sz);
>   	ta_sm = dev->shbuf;
>   	in = &ta_sm->pmf_input.init_table;
> @@ -512,9 +512,9 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>   	if (ret)
>   		goto error;
>   
> -	dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr, dev->policy_sz);
> -	if (!dev->policy_base) {
> -		ret = -ENOMEM;
> +	dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
> +	if (IS_ERR(dev->policy_base)) {
> +		ret = PTR_ERR(dev->policy_base);
>   		goto error;
>   	}
>   

Re: [PATCH v3 4/5] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

[Shyam Sundar S K]

On 10/23/2024 19:35, Mario Limonciello wrote:
> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>> Use platform_get_resource() to fetch the memory resource instead of
>> acpi_walk_resources() and devm_ioremap_resource() for mapping the
>> resources.
>>
>> PS: We cannot use resource_size() here because it adds an extra byte
>> to round
>> off the size. In the case of PMF ResourceTemplate(), this rounding is
>> already handled within the _CRS. Using resource_size() would
>> increase the
>> resource size by 1, causing a mismatch with the length field and
>> leading
>> to issues. Therefore, simply use end-start of the ACPI resource to
>> obtain
>> the actual length.
>>
>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>   drivers/platform/x86/amd/pmf/Kconfig  |  1 +
>>   drivers/platform/x86/amd/pmf/acpi.c   | 46
>> +++++++++++----------------
>>   drivers/platform/x86/amd/pmf/pmf.h    |  6 ++--
>>   drivers/platform/x86/amd/pmf/tee-if.c |  8 ++---
>>   4 files changed, 28 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/Kconfig
>> b/drivers/platform/x86/amd/pmf/Kconfig
>> index f4fa8bd8bda8..99d67cdbd91e 100644
>> --- a/drivers/platform/x86/amd/pmf/Kconfig
>> +++ b/drivers/platform/x86/amd/pmf/Kconfig
>> @@ -11,6 +11,7 @@ config AMD_PMF
>>       select ACPI_PLATFORM_PROFILE
>>       depends on TEE && AMDTEE
>>       depends on AMD_SFH_HID
>> +    depends on HAS_IOMEM
>>       help
>>         This driver provides support for the AMD Platform Management
>> Framework.
>>         The goal is to enhance end user experience by making AMD PCs
>> smarter,
>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c
>> b/drivers/platform/x86/amd/pmf/acpi.c
>> index d5b496433d69..62f984fe40c6 100644
>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>> @@ -433,37 +433,29 @@ int apmf_install_handler(struct amd_pmf_dev
>> *pmf_dev)
>>       return 0;
>>   }
>>   -static acpi_status apmf_walk_resources(struct acpi_resource *res,
>> void *data)
>> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>>   {
>> -    struct amd_pmf_dev *dev = data;
>> +    struct platform_device *pdev = to_platform_device(pmf_dev->dev);
>>   -    switch (res->type) {
>> -    case ACPI_RESOURCE_TYPE_ADDRESS64:
>> -        dev->policy_addr = res->data.address64.address.minimum;
>> -        dev->policy_sz = res->data.address64.address.address_length;
>> -        break;
>> -    case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
>> -        dev->policy_addr = res->data.fixed_memory32.address;
>> -        dev->policy_sz = res->data.fixed_memory32.address_length;
>> -        break;
>> -    }
>> -
>> -    if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ ||
>> dev->policy_sz == 0) {
>> -        pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
>> -        return AE_ERROR;
>> +    pmf_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +    if (!pmf_dev->res) {
>> +        dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");

here        ^^^^^^^

>> +        return -EINVAL;
>>       }
>>   -    return AE_OK;
>> -}
>> -
>> -int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>> -{
>> -    acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>> -    acpi_status status;
>> -
>> -    status = acpi_walk_resources(ahandle, METHOD_NAME__CRS,
>> apmf_walk_resources, pmf_dev);
>> -    if (ACPI_FAILURE(status)) {
>> -        dev_dbg(pmf_dev->dev, "acpi_walk_resources failed :%d\n",
>> status);
>> +    pmf_dev->policy_addr = pmf_dev->res->start;
>> +    /*
>> +     * We cannot use resource_size() here because it adds an extra
>> byte to round off the size.
>> +     * In the case of PMF ResourceTemplate(), this rounding is
>> already handled within the _CRS.
>> +     * Using resource_size() would increase the resource size by 1,
>> causing a mismatch with the
>> +     * length field and leading to issues. Therefore, simply use
>> end-start of the ACPI resource
>> +     * to obtain the actual length.
>> +     */
>> +    pmf_dev->policy_sz = pmf_dev->res->end - pmf_dev->res->start;
>> +
>> +    if (!pmf_dev->policy_addr || pmf_dev->policy_sz >
>> POLICY_BUF_MAX_SZ ||
>> +        pmf_dev->policy_sz == 0) {
>> +        dev_err(pmf_dev->dev, "Incorrect policy params, possibly a
>> SBIOS bug\n");
> 
> This upgrades the previous message from debug to error.

It is dev_err() even before this change.

> 
> TL;DR I feel this error should stay as dev_dbg() if no function checks
> are present for Smart PC.
> 
> I don't think it's necessarily an error though.
> Smart PC checks are a bit different than the other checks.  There
> isn't a check for a bit being set to indicate the function is supported.
> 
> So if the BIOS has the declaration for the region but it's not
> populated it might not have a Smart PC policy and this shouldn't be
> reported as a BIOS bug.

This should be included in the CPM package, and the BIOS team is
responsible for packaging a policy binary.

From a driver design standpoint, the absence of the policy binary
should be treated as an error, as there's no reason for the BIOS to
advertise the Smart PC bits without providing the policy binary.

Therefore, this should trigger a `dev_err()` and be considered a BIOS bug.

Thanks,
Shyam

> 
>>           return -EINVAL;
>>       }
>>   diff --git a/drivers/platform/x86/amd/pmf/pmf.h
>> b/drivers/platform/x86/amd/pmf/pmf.h
>> index 8ce8816da9c1..a79808fda1d8 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -13,6 +13,7 @@
>>     #include <linux/acpi.h>
>>   #include <linux/input.h>
>> +#include <linux/platform_device.h>
>>   #include <linux/platform_profile.h>
>>     #define POLICY_BUF_MAX_SZ        0x4b000
>> @@ -355,19 +356,20 @@ struct amd_pmf_dev {
>>       /* Smart PC solution builder */
>>       struct dentry *esbin;
>>       unsigned char *policy_buf;
>> -    u32 policy_sz;
>> +    resource_size_t policy_sz;
>>       struct tee_context *tee_ctx;
>>       struct tee_shm *fw_shm_pool;
>>       u32 session_id;
>>       void *shbuf;
>>       struct delayed_work pb_work;
>>       struct pmf_action_table *prev_data;
>> -    u64 policy_addr;
>> +    resource_size_t policy_addr;
>>       void __iomem *policy_base;
>>       bool smart_pc_enabled;
>>       u16 pmf_if_version;
>>       struct input_dev *pmf_idev;
>>       size_t mtable_size;
>> +    struct resource *res;
>>   };
>>     struct apmf_sps_prop_granular_v2 {
>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c
>> b/drivers/platform/x86/amd/pmf/tee-if.c
>> index 19c27b6e4666..555b8d6314e0 100644
>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>> @@ -257,7 +257,7 @@ static int amd_pmf_invoke_cmd_init(struct
>> amd_pmf_dev *dev)
>>           return -ENODEV;
>>       }
>>   -    dev_dbg(dev->dev, "Policy Binary size: %u bytes\n",
>> dev->policy_sz);
>> +    dev_dbg(dev->dev, "Policy Binary size: %llu bytes\n",
>> dev->policy_sz);
>>       memset(dev->shbuf, 0, dev->policy_sz);
>>       ta_sm = dev->shbuf;
>>       in = &ta_sm->pmf_input.init_table;
>> @@ -512,9 +512,9 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>       if (ret)
>>           goto error;
>>   -    dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr,
>> dev->policy_sz);
>> -    if (!dev->policy_base) {
>> -        ret = -ENOMEM;
>> +    dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
>> +    if (IS_ERR(dev->policy_base)) {
>> +        ret = PTR_ERR(dev->policy_base);
>>           goto error;
>>       }
>>   
> 

Re: [PATCH v3 4/5] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

[Mario Limonciello]

On 10/23/2024 09:37, Shyam Sundar S K wrote:
> 
> 
> On 10/23/2024 19:35, Mario Limonciello wrote:
>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>>> Use platform_get_resource() to fetch the memory resource instead of
>>> acpi_walk_resources() and devm_ioremap_resource() for mapping the
>>> resources.
>>>
>>> PS: We cannot use resource_size() here because it adds an extra byte
>>> to round
>>> off the size. In the case of PMF ResourceTemplate(), this rounding is
>>> already handled within the _CRS. Using resource_size() would
>>> increase the
>>> resource size by 1, causing a mismatch with the length field and
>>> leading
>>> to issues. Therefore, simply use end-start of the ACPI resource to
>>> obtain
>>> the actual length.
>>>
>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>> ---
>>>    drivers/platform/x86/amd/pmf/Kconfig  |  1 +
>>>    drivers/platform/x86/amd/pmf/acpi.c   | 46
>>> +++++++++++----------------
>>>    drivers/platform/x86/amd/pmf/pmf.h    |  6 ++--
>>>    drivers/platform/x86/amd/pmf/tee-if.c |  8 ++---
>>>    4 files changed, 28 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/amd/pmf/Kconfig
>>> b/drivers/platform/x86/amd/pmf/Kconfig
>>> index f4fa8bd8bda8..99d67cdbd91e 100644
>>> --- a/drivers/platform/x86/amd/pmf/Kconfig
>>> +++ b/drivers/platform/x86/amd/pmf/Kconfig
>>> @@ -11,6 +11,7 @@ config AMD_PMF
>>>        select ACPI_PLATFORM_PROFILE
>>>        depends on TEE && AMDTEE
>>>        depends on AMD_SFH_HID
>>> +    depends on HAS_IOMEM
>>>        help
>>>          This driver provides support for the AMD Platform Management
>>> Framework.
>>>          The goal is to enhance end user experience by making AMD PCs
>>> smarter,
>>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c
>>> b/drivers/platform/x86/amd/pmf/acpi.c
>>> index d5b496433d69..62f984fe40c6 100644
>>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>>> @@ -433,37 +433,29 @@ int apmf_install_handler(struct amd_pmf_dev
>>> *pmf_dev)
>>>        return 0;
>>>    }
>>>    -static acpi_status apmf_walk_resources(struct acpi_resource *res,
>>> void *data)
>>> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>>>    {
>>> -    struct amd_pmf_dev *dev = data;
>>> +    struct platform_device *pdev = to_platform_device(pmf_dev->dev);
>>>    -    switch (res->type) {
>>> -    case ACPI_RESOURCE_TYPE_ADDRESS64:
>>> -        dev->policy_addr = res->data.address64.address.minimum;
>>> -        dev->policy_sz = res->data.address64.address.address_length;
>>> -        break;
>>> -    case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
>>> -        dev->policy_addr = res->data.fixed_memory32.address;
>>> -        dev->policy_sz = res->data.fixed_memory32.address_length;
>>> -        break;
>>> -    }
>>> -
>>> -    if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ ||
>>> dev->policy_sz == 0) {
>>> -        pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
>>> -        return AE_ERROR;
>>> +    pmf_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +    if (!pmf_dev->res) {
>>> +        dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");
> 
> here        ^^^^^^^
> 
>>> +        return -EINVAL;
>>>        }
>>>    -    return AE_OK;
>>> -}
>>> -
>>> -int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>>> -{
>>> -    acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>>> -    acpi_status status;
>>> -
>>> -    status = acpi_walk_resources(ahandle, METHOD_NAME__CRS,
>>> apmf_walk_resources, pmf_dev);
>>> -    if (ACPI_FAILURE(status)) {
>>> -        dev_dbg(pmf_dev->dev, "acpi_walk_resources failed :%d\n",
>>> status);
>>> +    pmf_dev->policy_addr = pmf_dev->res->start;
>>> +    /*
>>> +     * We cannot use resource_size() here because it adds an extra
>>> byte to round off the size.
>>> +     * In the case of PMF ResourceTemplate(), this rounding is
>>> already handled within the _CRS.
>>> +     * Using resource_size() would increase the resource size by 1,
>>> causing a mismatch with the
>>> +     * length field and leading to issues. Therefore, simply use
>>> end-start of the ACPI resource
>>> +     * to obtain the actual length.
>>> +     */
>>> +    pmf_dev->policy_sz = pmf_dev->res->end - pmf_dev->res->start;
>>> +
>>> +    if (!pmf_dev->policy_addr || pmf_dev->policy_sz >
>>> POLICY_BUF_MAX_SZ ||
>>> +        pmf_dev->policy_sz == 0) {
>>> +        dev_err(pmf_dev->dev, "Incorrect policy params, possibly a
>>> SBIOS bug\n");
>>
>> This upgrades the previous message from debug to error.
> 
> It is dev_err() even before this change.
> 
>>
>> TL;DR I feel this error should stay as dev_dbg() if no function checks
>> are present for Smart PC.
>>
>> I don't think it's necessarily an error though.
>> Smart PC checks are a bit different than the other checks.  There
>> isn't a check for a bit being set to indicate the function is supported.
>>
>> So if the BIOS has the declaration for the region but it's not
>> populated it might not have a Smart PC policy and this shouldn't be
>> reported as a BIOS bug.
> 
> This should be included in the CPM package, and the BIOS team is
> responsible for packaging a policy binary.
> 
>  From a driver design standpoint, the absence of the policy binary
> should be treated as an error, as there's no reason for the BIOS to
> advertise the Smart PC bits without providing the policy binary.
> 
> Therefore, this should trigger a `dev_err()` and be considered a BIOS bug.
> 

OK I agree with this specific error, but I took a closer look at the bug 
associated with
03cea821b82cb ("platform/x86/amd: pmf: Decrease error message to debug")

As _CRS is patched out by BIOS I suspect that system will now start showing:

dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");

So how exactly is a platform designer supposed to not advertise smart PC 
bits?  It seems the only check is the presence of that resource.

> Thanks,
> Shyam
> 
>>
>>>            return -EINVAL;
>>>        }
>>>    diff --git a/drivers/platform/x86/amd/pmf/pmf.h
>>> b/drivers/platform/x86/amd/pmf/pmf.h
>>> index 8ce8816da9c1..a79808fda1d8 100644
>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>> @@ -13,6 +13,7 @@
>>>      #include <linux/acpi.h>
>>>    #include <linux/input.h>
>>> +#include <linux/platform_device.h>
>>>    #include <linux/platform_profile.h>
>>>      #define POLICY_BUF_MAX_SZ        0x4b000
>>> @@ -355,19 +356,20 @@ struct amd_pmf_dev {
>>>        /* Smart PC solution builder */
>>>        struct dentry *esbin;
>>>        unsigned char *policy_buf;
>>> -    u32 policy_sz;
>>> +    resource_size_t policy_sz;
>>>        struct tee_context *tee_ctx;
>>>        struct tee_shm *fw_shm_pool;
>>>        u32 session_id;
>>>        void *shbuf;
>>>        struct delayed_work pb_work;
>>>        struct pmf_action_table *prev_data;
>>> -    u64 policy_addr;
>>> +    resource_size_t policy_addr;
>>>        void __iomem *policy_base;
>>>        bool smart_pc_enabled;
>>>        u16 pmf_if_version;
>>>        struct input_dev *pmf_idev;
>>>        size_t mtable_size;
>>> +    struct resource *res;
>>>    };
>>>      struct apmf_sps_prop_granular_v2 {
>>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c
>>> b/drivers/platform/x86/amd/pmf/tee-if.c
>>> index 19c27b6e4666..555b8d6314e0 100644
>>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>>> @@ -257,7 +257,7 @@ static int amd_pmf_invoke_cmd_init(struct
>>> amd_pmf_dev *dev)
>>>            return -ENODEV;
>>>        }
>>>    -    dev_dbg(dev->dev, "Policy Binary size: %u bytes\n",
>>> dev->policy_sz);
>>> +    dev_dbg(dev->dev, "Policy Binary size: %llu bytes\n",
>>> dev->policy_sz);
>>>        memset(dev->shbuf, 0, dev->policy_sz);
>>>        ta_sm = dev->shbuf;
>>>        in = &ta_sm->pmf_input.init_table;
>>> @@ -512,9 +512,9 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>>        if (ret)
>>>            goto error;
>>>    -    dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr,
>>> dev->policy_sz);
>>> -    if (!dev->policy_base) {
>>> -        ret = -ENOMEM;
>>> +    dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
>>> +    if (IS_ERR(dev->policy_base)) {
>>> +        ret = PTR_ERR(dev->policy_base);
>>>            goto error;
>>>        }
>>>    
>>

Re: [PATCH v3 4/5] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

[Shyam Sundar S K]

On 10/23/2024 20:20, Mario Limonciello wrote:
> On 10/23/2024 09:37, Shyam Sundar S K wrote:
>>
>>
>> On 10/23/2024 19:35, Mario Limonciello wrote:
>>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>>>> Use platform_get_resource() to fetch the memory resource instead of
>>>> acpi_walk_resources() and devm_ioremap_resource() for mapping the
>>>> resources.
>>>>
>>>> PS: We cannot use resource_size() here because it adds an extra byte
>>>> to round
>>>> off the size. In the case of PMF ResourceTemplate(), this rounding is
>>>> already handled within the _CRS. Using resource_size() would
>>>> increase the
>>>> resource size by 1, causing a mismatch with the length field and
>>>> leading
>>>> to issues. Therefore, simply use end-start of the ACPI resource to
>>>> obtain
>>>> the actual length.
>>>>
>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>> ---
>>>>    drivers/platform/x86/amd/pmf/Kconfig  |  1 +
>>>>    drivers/platform/x86/amd/pmf/acpi.c   | 46
>>>> +++++++++++----------------
>>>>    drivers/platform/x86/amd/pmf/pmf.h    |  6 ++--
>>>>    drivers/platform/x86/amd/pmf/tee-if.c |  8 ++---
>>>>    4 files changed, 28 insertions(+), 33 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmf/Kconfig
>>>> b/drivers/platform/x86/amd/pmf/Kconfig
>>>> index f4fa8bd8bda8..99d67cdbd91e 100644
>>>> --- a/drivers/platform/x86/amd/pmf/Kconfig
>>>> +++ b/drivers/platform/x86/amd/pmf/Kconfig
>>>> @@ -11,6 +11,7 @@ config AMD_PMF
>>>>        select ACPI_PLATFORM_PROFILE
>>>>        depends on TEE && AMDTEE
>>>>        depends on AMD_SFH_HID
>>>> +    depends on HAS_IOMEM
>>>>        help
>>>>          This driver provides support for the AMD Platform Management
>>>> Framework.
>>>>          The goal is to enhance end user experience by making AMD PCs
>>>> smarter,
>>>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c
>>>> b/drivers/platform/x86/amd/pmf/acpi.c
>>>> index d5b496433d69..62f984fe40c6 100644
>>>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>>>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>>>> @@ -433,37 +433,29 @@ int apmf_install_handler(struct amd_pmf_dev
>>>> *pmf_dev)
>>>>        return 0;
>>>>    }
>>>>    -static acpi_status apmf_walk_resources(struct acpi_resource *res,
>>>> void *data)
>>>> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>>>>    {
>>>> -    struct amd_pmf_dev *dev = data;
>>>> +    struct platform_device *pdev = to_platform_device(pmf_dev->dev);
>>>>    -    switch (res->type) {
>>>> -    case ACPI_RESOURCE_TYPE_ADDRESS64:
>>>> -        dev->policy_addr = res->data.address64.address.minimum;
>>>> -        dev->policy_sz = res->data.address64.address.address_length;
>>>> -        break;
>>>> -    case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
>>>> -        dev->policy_addr = res->data.fixed_memory32.address;
>>>> -        dev->policy_sz = res->data.fixed_memory32.address_length;
>>>> -        break;
>>>> -    }
>>>> -
>>>> -    if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ ||
>>>> dev->policy_sz == 0) {
>>>> -        pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
>>>> -        return AE_ERROR;
>>>> +    pmf_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +    if (!pmf_dev->res) {
>>>> +        dev_err(pmf_dev->dev, "Failed to get I/O memory
>>>> resource\n");
>>
>> here        ^^^^^^^
>>
>>>> +        return -EINVAL;
>>>>        }
>>>>    -    return AE_OK;
>>>> -}
>>>> -
>>>> -int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>>>> -{
>>>> -    acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>>>> -    acpi_status status;
>>>> -
>>>> -    status = acpi_walk_resources(ahandle, METHOD_NAME__CRS,
>>>> apmf_walk_resources, pmf_dev);
>>>> -    if (ACPI_FAILURE(status)) {
>>>> -        dev_dbg(pmf_dev->dev, "acpi_walk_resources failed :%d\n",
>>>> status);
>>>> +    pmf_dev->policy_addr = pmf_dev->res->start;
>>>> +    /*
>>>> +     * We cannot use resource_size() here because it adds an extra
>>>> byte to round off the size.
>>>> +     * In the case of PMF ResourceTemplate(), this rounding is
>>>> already handled within the _CRS.
>>>> +     * Using resource_size() would increase the resource size by 1,
>>>> causing a mismatch with the
>>>> +     * length field and leading to issues. Therefore, simply use
>>>> end-start of the ACPI resource
>>>> +     * to obtain the actual length.
>>>> +     */
>>>> +    pmf_dev->policy_sz = pmf_dev->res->end - pmf_dev->res->start;
>>>> +
>>>> +    if (!pmf_dev->policy_addr || pmf_dev->policy_sz >
>>>> POLICY_BUF_MAX_SZ ||
>>>> +        pmf_dev->policy_sz == 0) {
>>>> +        dev_err(pmf_dev->dev, "Incorrect policy params, possibly a
>>>> SBIOS bug\n");
>>>
>>> This upgrades the previous message from debug to error.
>>
>> It is dev_err() even before this change.
>>
>>>
>>> TL;DR I feel this error should stay as dev_dbg() if no function checks
>>> are present for Smart PC.
>>>
>>> I don't think it's necessarily an error though.
>>> Smart PC checks are a bit different than the other checks.  There
>>> isn't a check for a bit being set to indicate the function is
>>> supported.
>>>
>>> So if the BIOS has the declaration for the region but it's not
>>> populated it might not have a Smart PC policy and this shouldn't be
>>> reported as a BIOS bug.
>>
>> This should be included in the CPM package, and the BIOS team is
>> responsible for packaging a policy binary.
>>
>>  From a driver design standpoint, the absence of the policy binary
>> should be treated as an error, as there's no reason for the BIOS to
>> advertise the Smart PC bits without providing the policy binary.
>>
>> Therefore, this should trigger a `dev_err()` and be considered a
>> BIOS bug.
>>
> 
> OK I agree with this specific error, but I took a closer look at the
> bug associated with
> 03cea821b82cb ("platform/x86/amd: pmf: Decrease error message to debug")

ah! but your comment was just inline to:

dev_err(pmf_dev->dev, "Incorrect policy params, possibly a SBIOS bug\n");

So, I was thinking you are saying to downgrade this to dev_dbg() and
hence the above clarification.

if the comment is for:
dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");

then Yes, I agree we should have dev_dbg() and I will respin a new
version.

Thanks,
Shyam


> 
> As _CRS is patched out by BIOS I suspect that system will now start
> showing:
> 
> dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");
> 
> So how exactly is a platform designer supposed to not advertise smart
> PC bits?  It seems the only check is the presence of that resource.
> 
>> Thanks,
>> Shyam
>>
>>>
>>>>            return -EINVAL;
>>>>        }
>>>>    diff --git a/drivers/platform/x86/amd/pmf/pmf.h
>>>> b/drivers/platform/x86/amd/pmf/pmf.h
>>>> index 8ce8816da9c1..a79808fda1d8 100644
>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>>> @@ -13,6 +13,7 @@
>>>>      #include <linux/acpi.h>
>>>>    #include <linux/input.h>
>>>> +#include <linux/platform_device.h>
>>>>    #include <linux/platform_profile.h>
>>>>      #define POLICY_BUF_MAX_SZ        0x4b000
>>>> @@ -355,19 +356,20 @@ struct amd_pmf_dev {
>>>>        /* Smart PC solution builder */
>>>>        struct dentry *esbin;
>>>>        unsigned char *policy_buf;
>>>> -    u32 policy_sz;
>>>> +    resource_size_t policy_sz;
>>>>        struct tee_context *tee_ctx;
>>>>        struct tee_shm *fw_shm_pool;
>>>>        u32 session_id;
>>>>        void *shbuf;
>>>>        struct delayed_work pb_work;
>>>>        struct pmf_action_table *prev_data;
>>>> -    u64 policy_addr;
>>>> +    resource_size_t policy_addr;
>>>>        void __iomem *policy_base;
>>>>        bool smart_pc_enabled;
>>>>        u16 pmf_if_version;
>>>>        struct input_dev *pmf_idev;
>>>>        size_t mtable_size;
>>>> +    struct resource *res;
>>>>    };
>>>>      struct apmf_sps_prop_granular_v2 {
>>>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c
>>>> b/drivers/platform/x86/amd/pmf/tee-if.c
>>>> index 19c27b6e4666..555b8d6314e0 100644
>>>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>>>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>>>> @@ -257,7 +257,7 @@ static int amd_pmf_invoke_cmd_init(struct
>>>> amd_pmf_dev *dev)
>>>>            return -ENODEV;
>>>>        }
>>>>    -    dev_dbg(dev->dev, "Policy Binary size: %u bytes\n",
>>>> dev->policy_sz);
>>>> +    dev_dbg(dev->dev, "Policy Binary size: %llu bytes\n",
>>>> dev->policy_sz);
>>>>        memset(dev->shbuf, 0, dev->policy_sz);
>>>>        ta_sm = dev->shbuf;
>>>>        in = &ta_sm->pmf_input.init_table;
>>>> @@ -512,9 +512,9 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev
>>>> *dev)
>>>>        if (ret)
>>>>            goto error;
>>>>    -    dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr,
>>>> dev->policy_sz);
>>>> -    if (!dev->policy_base) {
>>>> -        ret = -ENOMEM;
>>>> +    dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
>>>> +    if (IS_ERR(dev->policy_base)) {
>>>> +        ret = PTR_ERR(dev->policy_base);
>>>>            goto error;
>>>>        }
>>>>    
>>>
> 

Re: [PATCH v3 4/5] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

[Mario Limonciello]

On 10/23/2024 10:14, Shyam Sundar S K wrote:
> 
> 
> On 10/23/2024 20:20, Mario Limonciello wrote:
>> On 10/23/2024 09:37, Shyam Sundar S K wrote:
>>>
>>>
>>> On 10/23/2024 19:35, Mario Limonciello wrote:
>>>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>>>>> Use platform_get_resource() to fetch the memory resource instead of
>>>>> acpi_walk_resources() and devm_ioremap_resource() for mapping the
>>>>> resources.
>>>>>
>>>>> PS: We cannot use resource_size() here because it adds an extra byte
>>>>> to round
>>>>> off the size. In the case of PMF ResourceTemplate(), this rounding is
>>>>> already handled within the _CRS. Using resource_size() would
>>>>> increase the
>>>>> resource size by 1, causing a mismatch with the length field and
>>>>> leading
>>>>> to issues. Therefore, simply use end-start of the ACPI resource to
>>>>> obtain
>>>>> the actual length.
>>>>>
>>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>> ---
>>>>>     drivers/platform/x86/amd/pmf/Kconfig  |  1 +
>>>>>     drivers/platform/x86/amd/pmf/acpi.c   | 46
>>>>> +++++++++++----------------
>>>>>     drivers/platform/x86/amd/pmf/pmf.h    |  6 ++--
>>>>>     drivers/platform/x86/amd/pmf/tee-if.c |  8 ++---
>>>>>     4 files changed, 28 insertions(+), 33 deletions(-)
>>>>>
>>>>> diff --git a/drivers/platform/x86/amd/pmf/Kconfig
>>>>> b/drivers/platform/x86/amd/pmf/Kconfig
>>>>> index f4fa8bd8bda8..99d67cdbd91e 100644
>>>>> --- a/drivers/platform/x86/amd/pmf/Kconfig
>>>>> +++ b/drivers/platform/x86/amd/pmf/Kconfig
>>>>> @@ -11,6 +11,7 @@ config AMD_PMF
>>>>>         select ACPI_PLATFORM_PROFILE
>>>>>         depends on TEE && AMDTEE
>>>>>         depends on AMD_SFH_HID
>>>>> +    depends on HAS_IOMEM
>>>>>         help
>>>>>           This driver provides support for the AMD Platform Management
>>>>> Framework.
>>>>>           The goal is to enhance end user experience by making AMD PCs
>>>>> smarter,
>>>>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c
>>>>> b/drivers/platform/x86/amd/pmf/acpi.c
>>>>> index d5b496433d69..62f984fe40c6 100644
>>>>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>>>>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>>>>> @@ -433,37 +433,29 @@ int apmf_install_handler(struct amd_pmf_dev
>>>>> *pmf_dev)
>>>>>         return 0;
>>>>>     }
>>>>>     -static acpi_status apmf_walk_resources(struct acpi_resource *res,
>>>>> void *data)
>>>>> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>>>>>     {
>>>>> -    struct amd_pmf_dev *dev = data;
>>>>> +    struct platform_device *pdev = to_platform_device(pmf_dev->dev);
>>>>>     -    switch (res->type) {
>>>>> -    case ACPI_RESOURCE_TYPE_ADDRESS64:
>>>>> -        dev->policy_addr = res->data.address64.address.minimum;
>>>>> -        dev->policy_sz = res->data.address64.address.address_length;
>>>>> -        break;
>>>>> -    case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
>>>>> -        dev->policy_addr = res->data.fixed_memory32.address;
>>>>> -        dev->policy_sz = res->data.fixed_memory32.address_length;
>>>>> -        break;
>>>>> -    }
>>>>> -
>>>>> -    if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ ||
>>>>> dev->policy_sz == 0) {
>>>>> -        pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
>>>>> -        return AE_ERROR;
>>>>> +    pmf_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> +    if (!pmf_dev->res) {
>>>>> +        dev_err(pmf_dev->dev, "Failed to get I/O memory
>>>>> resource\n");
>>>
>>> here        ^^^^^^^
>>>
>>>>> +        return -EINVAL;
>>>>>         }
>>>>>     -    return AE_OK;
>>>>> -}
>>>>> -
>>>>> -int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>>>>> -{
>>>>> -    acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>>>>> -    acpi_status status;
>>>>> -
>>>>> -    status = acpi_walk_resources(ahandle, METHOD_NAME__CRS,
>>>>> apmf_walk_resources, pmf_dev);
>>>>> -    if (ACPI_FAILURE(status)) {
>>>>> -        dev_dbg(pmf_dev->dev, "acpi_walk_resources failed :%d\n",
>>>>> status);
>>>>> +    pmf_dev->policy_addr = pmf_dev->res->start;
>>>>> +    /*
>>>>> +     * We cannot use resource_size() here because it adds an extra
>>>>> byte to round off the size.
>>>>> +     * In the case of PMF ResourceTemplate(), this rounding is
>>>>> already handled within the _CRS.
>>>>> +     * Using resource_size() would increase the resource size by 1,
>>>>> causing a mismatch with the
>>>>> +     * length field and leading to issues. Therefore, simply use
>>>>> end-start of the ACPI resource
>>>>> +     * to obtain the actual length.
>>>>> +     */
>>>>> +    pmf_dev->policy_sz = pmf_dev->res->end - pmf_dev->res->start;
>>>>> +
>>>>> +    if (!pmf_dev->policy_addr || pmf_dev->policy_sz >
>>>>> POLICY_BUF_MAX_SZ ||
>>>>> +        pmf_dev->policy_sz == 0) {
>>>>> +        dev_err(pmf_dev->dev, "Incorrect policy params, possibly a
>>>>> SBIOS bug\n");
>>>>
>>>> This upgrades the previous message from debug to error.
>>>
>>> It is dev_err() even before this change.
>>>
>>>>
>>>> TL;DR I feel this error should stay as dev_dbg() if no function checks
>>>> are present for Smart PC.
>>>>
>>>> I don't think it's necessarily an error though.
>>>> Smart PC checks are a bit different than the other checks.  There
>>>> isn't a check for a bit being set to indicate the function is
>>>> supported.
>>>>
>>>> So if the BIOS has the declaration for the region but it's not
>>>> populated it might not have a Smart PC policy and this shouldn't be
>>>> reported as a BIOS bug.
>>>
>>> This should be included in the CPM package, and the BIOS team is
>>> responsible for packaging a policy binary.
>>>
>>>   From a driver design standpoint, the absence of the policy binary
>>> should be treated as an error, as there's no reason for the BIOS to
>>> advertise the Smart PC bits without providing the policy binary.
>>>
>>> Therefore, this should trigger a `dev_err()` and be considered a
>>> BIOS bug.
>>>
>>
>> OK I agree with this specific error, but I took a closer look at the
>> bug associated with
>> 03cea821b82cb ("platform/x86/amd: pmf: Decrease error message to debug")
> 
> ah! but your comment was just inline to:
> 
> dev_err(pmf_dev->dev, "Incorrect policy params, possibly a SBIOS bug\n");
> 
> So, I was thinking you are saying to downgrade this to dev_dbg() and
> hence the above clarification.
> 
> if the comment is for:
> dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");
> 
> then Yes, I agree we should have dev_dbg() and I will respin a new
> version.
> 

It was originally for that line, but you corrected me.

It looks that we reached the conclusion on the right line that should be 
fixed.

Thanks!

[PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Shyam Sundar S K]

The PMF driver will allocate shared buffer memory using the
tee_shm_alloc_kernel_buf(). This allocated memory is located in the
secure world and is used for communication with the PMF-TA.

The latest PMF-TA version introduces new structures with OEM debug
information and additional policy input conditions for evaluating the
policy binary. Consequently, the shared memory size must be increased to
ensure compatibility between the PMF driver and the updated PMF-TA.

Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/pmf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index a79808fda1d8..18f12aad46a9 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -106,7 +106,7 @@ struct cookie_header {
 #define PMF_TA_IF_VERSION_MAJOR				1
 #define TA_PMF_ACTION_MAX					32
 #define TA_PMF_UNDO_MAX						8
-#define TA_OUTPUT_RESERVED_MEM				906
+#define TA_OUTPUT_RESERVED_MEM				922
 #define MAX_OPERATION_PARAMS					4
 
 #define PMF_IF_V1		1
-- 
2.34.1

Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Mario Limonciello]

On 10/23/2024 01:32, Shyam Sundar S K wrote:
> The PMF driver will allocate shared buffer memory using the
> tee_shm_alloc_kernel_buf(). This allocated memory is located in the
> secure world and is used for communication with the PMF-TA.
> 
> The latest PMF-TA version introduces new structures with OEM debug
> information and additional policy input conditions for evaluating the
> policy binary. Consequently, the shared memory size must be increased to
> ensure compatibility between the PMF driver and the updated PMF-TA.
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

How does this present to a user?  From what you describe it seems to me 
like this means a new TA will fail on older kernel in some way.
Some ideas:

1) Should there be header version check on the TA and dynamically 
allocate the structure size based on the version of the F/W?

2) Or is there a command to the TA that can query the expected output size?

3) Or should the new TA filename be versioned, and the driver has a 
fallback policy?

Whatever the outcome is; I think it's best that if possible this change 
goes back to stable to try to minimize regressions to users as distros 
update linux-firmware.  For example Fedora updates this monthly, but 
also tracks stable kernels.

> ---
>   drivers/platform/x86/amd/pmf/pmf.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index a79808fda1d8..18f12aad46a9 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -106,7 +106,7 @@ struct cookie_header {
>   #define PMF_TA_IF_VERSION_MAJOR				1
>   #define TA_PMF_ACTION_MAX					32
>   #define TA_PMF_UNDO_MAX						8
> -#define TA_OUTPUT_RESERVED_MEM				906
> +#define TA_OUTPUT_RESERVED_MEM				922
>   #define MAX_OPERATION_PARAMS					4
>   
>   #define PMF_IF_V1		1

Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Shyam Sundar S K]

On 10/23/2024 19:41, Mario Limonciello wrote:
> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>> The PMF driver will allocate shared buffer memory using the
>> tee_shm_alloc_kernel_buf(). This allocated memory is located in the
>> secure world and is used for communication with the PMF-TA.
>>
>> The latest PMF-TA version introduces new structures with OEM debug
>> information and additional policy input conditions for evaluating the
>> policy binary. Consequently, the shared memory size must be
>> increased to
>> ensure compatibility between the PMF driver and the updated PMF-TA.
>>
>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> 
> How does this present to a user?  From what you describe it seems to
> me like this means a new TA will fail on older kernel in some way.

Newer TA will not fail on older systems. This change is just about the
increase in TA reserved memory that is presented as "shared memory",
as TA needs the additional memory for its own debug data structures.

From user standpoint, always be on latest FW, irrespective of the
platform. At this point in time, I don't see a need for FW versioning
name (in the future, if there is a need for having a limited support
to older platforms, we can carve out a logic to do versioning stuff).

> Some ideas:
> 
> 1) Should there be header version check on the TA and dynamically
> allocate the structure size based on the version of the F/W?
> 

This can be done, when the TA versioning upgrade happens, like from
1.3 to 1.4, apart from that there is no header stuff association.

> 2) Or is there a command to the TA that can query the expected output
> size?
> 

No, this is just the initial shared memory that the driver allocates
to pass the inputs and the commands to TA.

> 3) Or should the new TA filename be versioned, and the driver has a
> fallback policy?
> 
> Whatever the outcome is; I think it's best that if possible this
> change goes back to stable to try to minimize regressions to users as
> distros update linux-firmware.  For example Fedora updates this
> monthly, but also tracks stable kernels.
> 

Advisory to distros should be to pick the latest PMF TA (note that, I
have not still submitted to new TA FW).

Thanks,
Shyam

>> ---
>>   drivers/platform/x86/amd/pmf/pmf.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h
>> b/drivers/platform/x86/amd/pmf/pmf.h
>> index a79808fda1d8..18f12aad46a9 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -106,7 +106,7 @@ struct cookie_header {
>>   #define PMF_TA_IF_VERSION_MAJOR                1
>>   #define TA_PMF_ACTION_MAX                    32
>>   #define TA_PMF_UNDO_MAX                        8
>> -#define TA_OUTPUT_RESERVED_MEM                906
>> +#define TA_OUTPUT_RESERVED_MEM                922
>>   #define MAX_OPERATION_PARAMS                    4
>>     #define PMF_IF_V1        1
> 

Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Mario Limonciello]

On 10/23/2024 09:29, Shyam Sundar S K wrote:
> 
> 
> On 10/23/2024 19:41, Mario Limonciello wrote:
>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>>> The PMF driver will allocate shared buffer memory using the
>>> tee_shm_alloc_kernel_buf(). This allocated memory is located in the
>>> secure world and is used for communication with the PMF-TA.
>>>
>>> The latest PMF-TA version introduces new structures with OEM debug
>>> information and additional policy input conditions for evaluating the
>>> policy binary. Consequently, the shared memory size must be
>>> increased to
>>> ensure compatibility between the PMF driver and the updated PMF-TA.
>>>
>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>
>> How does this present to a user?  From what you describe it seems to
>> me like this means a new TA will fail on older kernel in some way.
> 
> Newer TA will not fail on older systems. This change is just about the
> increase in TA reserved memory that is presented as "shared memory",
> as TA needs the additional memory for its own debug data structures.

Thx for comments. But so if you use new TA with older kernel driver, 
what will happen?  Can TA do a buffer overrun because the presented 
shared memory was too small?

> 
>  From user standpoint, always be on latest FW, irrespective of the
> platform. At this point in time, I don't see a need for FW versioning
> name (in the future, if there is a need for having a limited support
> to older platforms, we can carve out a logic to do versioning stuff).

I wish we could enforce this, but In the Linux world there is an 
expectation that these two trains don't need to arrive at station at the 
same time.

> 
>> Some ideas:
>>
>> 1) Should there be header version check on the TA and dynamically
>> allocate the structure size based on the version of the F/W?
>>
> 
> This can be done, when the TA versioning upgrade happens, like from
> 1.3 to 1.4, apart from that there is no header stuff association.
> 
>> 2) Or is there a command to the TA that can query the expected output
>> size?
>>
> 
> No, this is just the initial shared memory that the driver allocates
> to pass the inputs and the commands to TA.
> 
>> 3) Or should the new TA filename be versioned, and the driver has a
>> fallback policy?
>>
>> Whatever the outcome is; I think it's best that if possible this
>> change goes back to stable to try to minimize regressions to users as
>> distros update linux-firmware.  For example Fedora updates this
>> monthly, but also tracks stable kernels.
>>
> 
> Advisory to distros should be to pick the latest PMF TA (note that, I
> have not still submitted to new TA FW).

Yeah we can advise distros to pick it up when upstreamed as long as 
there isn't tight dependency on this patch being present.

> 
> Thanks,
> Shyam
> 
>>> ---
>>>    drivers/platform/x86/amd/pmf/pmf.h | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h
>>> b/drivers/platform/x86/amd/pmf/pmf.h
>>> index a79808fda1d8..18f12aad46a9 100644
>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>> @@ -106,7 +106,7 @@ struct cookie_header {
>>>    #define PMF_TA_IF_VERSION_MAJOR                1
>>>    #define TA_PMF_ACTION_MAX                    32
>>>    #define TA_PMF_UNDO_MAX                        8
>>> -#define TA_OUTPUT_RESERVED_MEM                906
>>> +#define TA_OUTPUT_RESERVED_MEM                922
>>>    #define MAX_OPERATION_PARAMS                    4
>>>      #define PMF_IF_V1        1
>>

Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Shyam Sundar S K]

On 10/23/2024 20:04, Mario Limonciello wrote:
> On 10/23/2024 09:29, Shyam Sundar S K wrote:
>>
>>
>> On 10/23/2024 19:41, Mario Limonciello wrote:
>>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>>>> The PMF driver will allocate shared buffer memory using the
>>>> tee_shm_alloc_kernel_buf(). This allocated memory is located in the
>>>> secure world and is used for communication with the PMF-TA.
>>>>
>>>> The latest PMF-TA version introduces new structures with OEM debug
>>>> information and additional policy input conditions for evaluating the
>>>> policy binary. Consequently, the shared memory size must be
>>>> increased to
>>>> ensure compatibility between the PMF driver and the updated PMF-TA.
>>>>
>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>
>>> How does this present to a user?  From what you describe it seems to
>>> me like this means a new TA will fail on older kernel in some way.
>>
>> Newer TA will not fail on older systems. This change is just about the
>> increase in TA reserved memory that is presented as "shared memory",
>> as TA needs the additional memory for its own debug data structures.
> 
> Thx for comments. But so if you use new TA with older kernel driver,
> what will happen?  Can TA do a buffer overrun because the presented
> shared memory was too small?
> 

New TA will fail on older kernel and hence this change will be
required for new TA to work.

>>
>>  From user standpoint, always be on latest FW, irrespective of the
>> platform. At this point in time, I don't see a need for FW versioning
>> name (in the future, if there is a need for having a limited support
>> to older platforms, we can carve out a logic to do versioning stuff).
> 
> I wish we could enforce this, but In the Linux world there is an
> expectation that these two trains don't need to arrive at station at
> the same time.
> 
>>
>>> Some ideas:
>>>
>>> 1) Should there be header version check on the TA and dynamically
>>> allocate the structure size based on the version of the F/W?
>>>
>>
>> This can be done, when the TA versioning upgrade happens, like from
>> 1.3 to 1.4, apart from that there is no header stuff association.
>>
>>> 2) Or is there a command to the TA that can query the expected output
>>> size?
>>>
>>
>> No, this is just the initial shared memory that the driver allocates
>> to pass the inputs and the commands to TA.
>>
>>> 3) Or should the new TA filename be versioned, and the driver has a
>>> fallback policy?
>>>
>>> Whatever the outcome is; I think it's best that if possible this
>>> change goes back to stable to try to minimize regressions to users as
>>> distros update linux-firmware.  For example Fedora updates this
>>> monthly, but also tracks stable kernels.
>>>
>>
>> Advisory to distros should be to pick the latest PMF TA (note that, I
>> have not still submitted to new TA FW).
> 
> Yeah we can advise distros to pick it up when upstreamed as long as
> there isn't tight dependency on this patch being present.
> 

That is the reason I am waiting for this change to land. Once that is
done, I will submit the new TA, you can send out a advisory to upgrade
the kernel or this change has to be back-ported to stable/oem kernels
for their enablement.

Makes sense?

Thanks,
Shyam

>>
>> Thanks,
>> Shyam
>>
>>>> ---
>>>>    drivers/platform/x86/amd/pmf/pmf.h | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h
>>>> b/drivers/platform/x86/amd/pmf/pmf.h
>>>> index a79808fda1d8..18f12aad46a9 100644
>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>>> @@ -106,7 +106,7 @@ struct cookie_header {
>>>>    #define PMF_TA_IF_VERSION_MAJOR                1
>>>>    #define TA_PMF_ACTION_MAX                    32
>>>>    #define TA_PMF_UNDO_MAX                        8
>>>> -#define TA_OUTPUT_RESERVED_MEM                906
>>>> +#define TA_OUTPUT_RESERVED_MEM                922
>>>>    #define MAX_OPERATION_PARAMS                    4
>>>>      #define PMF_IF_V1        1
>>>
> 

Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Mario Limonciello]

On 10/23/2024 10:32, Shyam Sundar S K wrote:
> 
> 
> On 10/23/2024 20:04, Mario Limonciello wrote:
>> On 10/23/2024 09:29, Shyam Sundar S K wrote:
>>>
>>>
>>> On 10/23/2024 19:41, Mario Limonciello wrote:
>>>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>>>>> The PMF driver will allocate shared buffer memory using the
>>>>> tee_shm_alloc_kernel_buf(). This allocated memory is located in the
>>>>> secure world and is used for communication with the PMF-TA.
>>>>>
>>>>> The latest PMF-TA version introduces new structures with OEM debug
>>>>> information and additional policy input conditions for evaluating the
>>>>> policy binary. Consequently, the shared memory size must be
>>>>> increased to
>>>>> ensure compatibility between the PMF driver and the updated PMF-TA.
>>>>>
>>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>
>>>> How does this present to a user?  From what you describe it seems to
>>>> me like this means a new TA will fail on older kernel in some way.
>>>
>>> Newer TA will not fail on older systems. This change is just about the
>>> increase in TA reserved memory that is presented as "shared memory",
>>> as TA needs the additional memory for its own debug data structures.
>>
>> Thx for comments. But so if you use new TA with older kernel driver,
>> what will happen?  Can TA do a buffer overrun because the presented
>> shared memory was too small?
>>
> 
> New TA will fail on older kernel and hence this change will be
> required for new TA to work.

OK, that's what I was worried about.

> 
>>>
>>>   From user standpoint, always be on latest FW, irrespective of the
>>> platform. At this point in time, I don't see a need for FW versioning
>>> name (in the future, if there is a need for having a limited support
>>> to older platforms, we can carve out a logic to do versioning stuff).
>>
>> I wish we could enforce this, but In the Linux world there is an
>> expectation that these two trains don't need to arrive at station at
>> the same time.
>>
>>>
>>>> Some ideas:
>>>>
>>>> 1) Should there be header version check on the TA and dynamically
>>>> allocate the structure size based on the version of the F/W?
>>>>
>>>
>>> This can be done, when the TA versioning upgrade happens, like from
>>> 1.3 to 1.4, apart from that there is no header stuff association.
>>>
>>>> 2) Or is there a command to the TA that can query the expected output
>>>> size?
>>>>
>>>
>>> No, this is just the initial shared memory that the driver allocates
>>> to pass the inputs and the commands to TA.
>>>
>>>> 3) Or should the new TA filename be versioned, and the driver has a
>>>> fallback policy?
>>>>
>>>> Whatever the outcome is; I think it's best that if possible this
>>>> change goes back to stable to try to minimize regressions to users as
>>>> distros update linux-firmware.  For example Fedora updates this
>>>> monthly, but also tracks stable kernels.
>>>>
>>>
>>> Advisory to distros should be to pick the latest PMF TA (note that, I
>>> have not still submitted to new TA FW).
>>
>> Yeah we can advise distros to pick it up when upstreamed as long as
>> there isn't tight dependency on this patch being present.
>>
> 
> That is the reason I am waiting for this change to land. Once that is
> done, I will submit the new TA, you can send out a advisory to upgrade
> the kernel or this change has to be back-ported to stable/oem kernels
> for their enablement.
> 
> Makes sense?
> 

I think we need Hans' and Ilpo's comments here to decide what to do.

I will say that when we had this happen in amdgpu for a breaking reason 
there was a new firmware binary filename created/upstreamed for the 
breaking version (IIRC foo.bin -> foo_1.bin) and amdgpu had to have 
fallback code so it could be compatible with either binary.

* If user on older kernel took newer linux-firmware package they used 
older binary.
* If user on newer kernel took older linux-firmware package they used 
older binary.
* If user on newer kernel took newer linux-firmware package they used 
newer binary.

If the decision is this goes in "as is" it definitely needs to go back 
to stable kernels.

Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Shyam Sundar S K]

On 10/23/2024 21:10, Mario Limonciello wrote:
> On 10/23/2024 10:32, Shyam Sundar S K wrote:
>>
>>
>> On 10/23/2024 20:04, Mario Limonciello wrote:
>>> On 10/23/2024 09:29, Shyam Sundar S K wrote:
>>>>
>>>>
>>>> On 10/23/2024 19:41, Mario Limonciello wrote:
>>>>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>>>>>> The PMF driver will allocate shared buffer memory using the
>>>>>> tee_shm_alloc_kernel_buf(). This allocated memory is located in the
>>>>>> secure world and is used for communication with the PMF-TA.
>>>>>>
>>>>>> The latest PMF-TA version introduces new structures with OEM debug
>>>>>> information and additional policy input conditions for
>>>>>> evaluating the
>>>>>> policy binary. Consequently, the shared memory size must be
>>>>>> increased to
>>>>>> ensure compatibility between the PMF driver and the updated PMF-TA.
>>>>>>
>>>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>>
>>>>> How does this present to a user?  From what you describe it seems to
>>>>> me like this means a new TA will fail on older kernel in some way.
>>>>
>>>> Newer TA will not fail on older systems. This change is just about
>>>> the
>>>> increase in TA reserved memory that is presented as "shared memory",
>>>> as TA needs the additional memory for its own debug data structures.
>>>
>>> Thx for comments. But so if you use new TA with older kernel driver,
>>> what will happen?  Can TA do a buffer overrun because the presented
>>> shared memory was too small?
>>>
>>
>> New TA will fail on older kernel and hence this change will be
>> required for new TA to work.
> 
> OK, that's what I was worried about.
> 
>>
>>>>
>>>>   From user standpoint, always be on latest FW, irrespective of the
>>>> platform. At this point in time, I don't see a need for FW versioning
>>>> name (in the future, if there is a need for having a limited support
>>>> to older platforms, we can carve out a logic to do versioning stuff).
>>>
>>> I wish we could enforce this, but In the Linux world there is an
>>> expectation that these two trains don't need to arrive at station at
>>> the same time.
>>>
>>>>
>>>>> Some ideas:
>>>>>
>>>>> 1) Should there be header version check on the TA and dynamically
>>>>> allocate the structure size based on the version of the F/W?
>>>>>
>>>>
>>>> This can be done, when the TA versioning upgrade happens, like from
>>>> 1.3 to 1.4, apart from that there is no header stuff association.
>>>>
>>>>> 2) Or is there a command to the TA that can query the expected
>>>>> output
>>>>> size?
>>>>>
>>>>
>>>> No, this is just the initial shared memory that the driver allocates
>>>> to pass the inputs and the commands to TA.
>>>>
>>>>> 3) Or should the new TA filename be versioned, and the driver has a
>>>>> fallback policy?
>>>>>
>>>>> Whatever the outcome is; I think it's best that if possible this
>>>>> change goes back to stable to try to minimize regressions to
>>>>> users as
>>>>> distros update linux-firmware.  For example Fedora updates this
>>>>> monthly, but also tracks stable kernels.
>>>>>
>>>>
>>>> Advisory to distros should be to pick the latest PMF TA (note that, I
>>>> have not still submitted to new TA FW).
>>>
>>> Yeah we can advise distros to pick it up when upstreamed as long as
>>> there isn't tight dependency on this patch being present.
>>>
>>
>> That is the reason I am waiting for this change to land. Once that is
>> done, I will submit the new TA, you can send out a advisory to upgrade
>> the kernel or this change has to be back-ported to stable/oem kernels
>> for their enablement.
>>
>> Makes sense?
>>
> 
> I think we need Hans' and Ilpo's comments here to decide what to do.
> 

Sure.

> I will say that when we had this happen in amdgpu for a breaking
> reason there was a new firmware binary filename created/upstreamed for
> the breaking version (IIRC foo.bin -> foo_1.bin) and amdgpu had to
> have fallback code so it could be compatible with either binary.
> 

True. In case of amdgpu, the FW loading is part of the amdgpu driver.
But in case of PMF, the PMF TA gets picked from the AMD TEE driver
through the TEE commands.

So, there is no need for FW versioning logic in PMF driver.


> * If user on older kernel took newer linux-firmware package they used
> older binary.
> * If user on newer kernel took older linux-firmware package they used
> older binary.
> * If user on newer kernel took newer linux-firmware package they used
> newer binary.
> 
> If the decision is this goes in "as is" it definitely needs to go back
> to stable kernels.
> 

IMHO, let's not put too many fallback mechanisms. The philosophy
should be use latest driver and latest FW that avoids a lot of
confusion and yeah for that to happen this change has to go to stable.

Thanks,
Shyam

Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Mario Limonciello]

On 10/23/2024 10:52, Shyam Sundar S K wrote:
> 
> 
> On 10/23/2024 21:10, Mario Limonciello wrote:
>> On 10/23/2024 10:32, Shyam Sundar S K wrote:
>>>
>>>
>>> On 10/23/2024 20:04, Mario Limonciello wrote:
>>>> On 10/23/2024 09:29, Shyam Sundar S K wrote:
>>>>>
>>>>>
>>>>> On 10/23/2024 19:41, Mario Limonciello wrote:
>>>>>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>>>>>>> The PMF driver will allocate shared buffer memory using the
>>>>>>> tee_shm_alloc_kernel_buf(). This allocated memory is located in the
>>>>>>> secure world and is used for communication with the PMF-TA.
>>>>>>>
>>>>>>> The latest PMF-TA version introduces new structures with OEM debug
>>>>>>> information and additional policy input conditions for
>>>>>>> evaluating the
>>>>>>> policy binary. Consequently, the shared memory size must be
>>>>>>> increased to
>>>>>>> ensure compatibility between the PMF driver and the updated PMF-TA.
>>>>>>>
>>>>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>>>
>>>>>> How does this present to a user?  From what you describe it seems to
>>>>>> me like this means a new TA will fail on older kernel in some way.
>>>>>
>>>>> Newer TA will not fail on older systems. This change is just about
>>>>> the
>>>>> increase in TA reserved memory that is presented as "shared memory",
>>>>> as TA needs the additional memory for its own debug data structures.
>>>>
>>>> Thx for comments. But so if you use new TA with older kernel driver,
>>>> what will happen?  Can TA do a buffer overrun because the presented
>>>> shared memory was too small?
>>>>
>>>
>>> New TA will fail on older kernel and hence this change will be
>>> required for new TA to work.
>>
>> OK, that's what I was worried about.
>>
>>>
>>>>>
>>>>>    From user standpoint, always be on latest FW, irrespective of the
>>>>> platform. At this point in time, I don't see a need for FW versioning
>>>>> name (in the future, if there is a need for having a limited support
>>>>> to older platforms, we can carve out a logic to do versioning stuff).
>>>>
>>>> I wish we could enforce this, but In the Linux world there is an
>>>> expectation that these two trains don't need to arrive at station at
>>>> the same time.
>>>>
>>>>>
>>>>>> Some ideas:
>>>>>>
>>>>>> 1) Should there be header version check on the TA and dynamically
>>>>>> allocate the structure size based on the version of the F/W?
>>>>>>
>>>>>
>>>>> This can be done, when the TA versioning upgrade happens, like from
>>>>> 1.3 to 1.4, apart from that there is no header stuff association.
>>>>>
>>>>>> 2) Or is there a command to the TA that can query the expected
>>>>>> output
>>>>>> size?
>>>>>>
>>>>>
>>>>> No, this is just the initial shared memory that the driver allocates
>>>>> to pass the inputs and the commands to TA.
>>>>>
>>>>>> 3) Or should the new TA filename be versioned, and the driver has a
>>>>>> fallback policy?
>>>>>>
>>>>>> Whatever the outcome is; I think it's best that if possible this
>>>>>> change goes back to stable to try to minimize regressions to
>>>>>> users as
>>>>>> distros update linux-firmware.  For example Fedora updates this
>>>>>> monthly, but also tracks stable kernels.
>>>>>>
>>>>>
>>>>> Advisory to distros should be to pick the latest PMF TA (note that, I
>>>>> have not still submitted to new TA FW).
>>>>
>>>> Yeah we can advise distros to pick it up when upstreamed as long as
>>>> there isn't tight dependency on this patch being present.
>>>>
>>>
>>> That is the reason I am waiting for this change to land. Once that is
>>> done, I will submit the new TA, you can send out a advisory to upgrade
>>> the kernel or this change has to be back-ported to stable/oem kernels
>>> for their enablement.
>>>
>>> Makes sense?
>>>
>>
>> I think we need Hans' and Ilpo's comments here to decide what to do.
>>
> 
> Sure.
> 
>> I will say that when we had this happen in amdgpu for a breaking
>> reason there was a new firmware binary filename created/upstreamed for
>> the breaking version (IIRC foo.bin -> foo_1.bin) and amdgpu had to
>> have fallback code so it could be compatible with either binary.
>>
> 
> True. In case of amdgpu, the FW loading is part of the amdgpu driver.
> But in case of PMF, the PMF TA gets picked from the AMD TEE driver
> through the TEE commands.
> 
> So, there is no need for FW versioning logic in PMF driver.
> 

That's a very good point, and this is a lot of complexity then.

> 
>> * If user on older kernel took newer linux-firmware package they used
>> older binary.
>> * If user on newer kernel took older linux-firmware package they used
>> older binary.
>> * If user on newer kernel took newer linux-firmware package they used
>> newer binary.
>>
>> If the decision is this goes in "as is" it definitely needs to go back
>> to stable kernels.
>>
> 
> IMHO, let's not put too many fallback mechanisms. The philosophy
> should be use latest driver and latest FW that avoids a lot of
> confusion and yeah for that to happen this change has to go to stable.
> 
> Thanks,
> Shyam

Of course Hans and Ilpo make the final call, but I think from our 
discussions here it would be ideal that patch 1 and patch 5 from this 
series go into 6.12 and have stable tags, the rest would be 6.13 material.

Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Ilpo Järvinen]

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

Hi Hens,

There a question / item needing your input below.

On Wed, 23 Oct 2024, Mario Limonciello wrote:
> On 10/23/2024 10:52, Shyam Sundar S K wrote:
> > On 10/23/2024 21:10, Mario Limonciello wrote:
> > > On 10/23/2024 10:32, Shyam Sundar S K wrote:
> > > > On 10/23/2024 20:04, Mario Limonciello wrote:
> > > > > On 10/23/2024 09:29, Shyam Sundar S K wrote:
> > > > > > On 10/23/2024 19:41, Mario Limonciello wrote:
> > > > > > > On 10/23/2024 01:32, Shyam Sundar S K wrote:
> > > > > > > > The PMF driver will allocate shared buffer memory using the
> > > > > > > > tee_shm_alloc_kernel_buf(). This allocated memory is located in
> > > > > > > > the
> > > > > > > > secure world and is used for communication with the PMF-TA.
> > > > > > > > 
> > > > > > > > The latest PMF-TA version introduces new structures with OEM
> > > > > > > > debug
> > > > > > > > information and additional policy input conditions for
> > > > > > > > evaluating the
> > > > > > > > policy binary. Consequently, the shared memory size must be
> > > > > > > > increased to
> > > > > > > > ensure compatibility between the PMF driver and the updated
> > > > > > > > PMF-TA.
> > > > > > > > 
> > > > > > > > Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> > > > > > > > Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> > > > > > > > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> > > > > > > 
> > > > > > > How does this present to a user?  From what you describe it seems
> > > > > > > to
> > > > > > > me like this means a new TA will fail on older kernel in some way.
> > > > > > 
> > > > > > Newer TA will not fail on older systems. This change is just about
> > > > > > the
> > > > > > increase in TA reserved memory that is presented as "shared memory",
> > > > > > as TA needs the additional memory for its own debug data structures.
> > > > > 
> > > > > Thx for comments. But so if you use new TA with older kernel driver,
> > > > > what will happen?  Can TA do a buffer overrun because the presented
> > > > > shared memory was too small?
> > > > > 
> > > > 
> > > > New TA will fail on older kernel and hence this change will be
> > > > required for new TA to work.
> > > 
> > > OK, that's what I was worried about.
> > > 
> > > > 
> > > > > > 
> > > > > >    From user standpoint, always be on latest FW, irrespective of the
> > > > > > platform. At this point in time, I don't see a need for FW
> > > > > > versioning
> > > > > > name (in the future, if there is a need for having a limited support
> > > > > > to older platforms, we can carve out a logic to do versioning
> > > > > > stuff).
> > > > > 
> > > > > I wish we could enforce this, but In the Linux world there is an
> > > > > expectation that these two trains don't need to arrive at station at
> > > > > the same time.
> > > > > 
> > > > > > 
> > > > > > > Some ideas:
> > > > > > > 
> > > > > > > 1) Should there be header version check on the TA and dynamically
> > > > > > > allocate the structure size based on the version of the F/W?
> > > > > > > 
> > > > > > 
> > > > > > This can be done, when the TA versioning upgrade happens, like from
> > > > > > 1.3 to 1.4, apart from that there is no header stuff association.
> > > > > > 
> > > > > > > 2) Or is there a command to the TA that can query the expected
> > > > > > > output
> > > > > > > size?
> > > > > > > 
> > > > > > 
> > > > > > No, this is just the initial shared memory that the driver allocates
> > > > > > to pass the inputs and the commands to TA.
> > > > > > 
> > > > > > > 3) Or should the new TA filename be versioned, and the driver has
> > > > > > > a
> > > > > > > fallback policy?
> > > > > > > 
> > > > > > > Whatever the outcome is; I think it's best that if possible this
> > > > > > > change goes back to stable to try to minimize regressions to
> > > > > > > users as
> > > > > > > distros update linux-firmware.  For example Fedora updates this
> > > > > > > monthly, but also tracks stable kernels.
> > > > > > > 
> > > > > > 
> > > > > > Advisory to distros should be to pick the latest PMF TA (note that,
> > > > > > I
> > > > > > have not still submitted to new TA FW).
> > > > > 
> > > > > Yeah we can advise distros to pick it up when upstreamed as long as
> > > > > there isn't tight dependency on this patch being present.
> > > > > 
> > > > 
> > > > That is the reason I am waiting for this change to land. Once that is
> > > > done, I will submit the new TA, you can send out a advisory to upgrade
> > > > the kernel or this change has to be back-ported to stable/oem kernels
> > > > for their enablement.
> > > > 
> > > > Makes sense?
> > > > 
> > > 
> > > I think we need Hans' and Ilpo's comments here to decide what to do.
> > > 
> > 
> > Sure.
> > 
> > > I will say that when we had this happen in amdgpu for a breaking
> > > reason there was a new firmware binary filename created/upstreamed for
> > > the breaking version (IIRC foo.bin -> foo_1.bin) and amdgpu had to
> > > have fallback code so it could be compatible with either binary.
> > > 
> > 
> > True. In case of amdgpu, the FW loading is part of the amdgpu driver.
> > But in case of PMF, the PMF TA gets picked from the AMD TEE driver
> > through the TEE commands.
> > 
> > So, there is no need for FW versioning logic in PMF driver.
> > 
> 
> That's a very good point, and this is a lot of complexity then.
> 
> > 
> > > * If user on older kernel took newer linux-firmware package they used
> > > older binary.
> > > * If user on newer kernel took older linux-firmware package they used
> > > older binary.
> > > * If user on newer kernel took newer linux-firmware package they used
> > > newer binary.
> > > 
> > > If the decision is this goes in "as is" it definitely needs to go back
> > > to stable kernels.
> > > 
> > 
> > IMHO, let's not put too many fallback mechanisms. The philosophy
> > should be use latest driver and latest FW that avoids a lot of
> > confusion and yeah for that to happen this change has to go to stable.
> > 
> > Thanks,
> > Shyam
> 
> Of course Hans and Ilpo make the final call, but I think from our discussions
> here it would be ideal that patch 1 and patch 5 from this series go into 6.12
> and have stable tags, the rest would be 6.13 material.

Distros and SW component management challenges are more in the domain of 
Hans' expertise so I'd prefer to hear his opinion on this.

Personally I feel though that the commit message is not entirely honest 
on all the impact as is. The wordings are sounding quite innocent while if 
I infer the above right, an incorrect combination will cause a 
non-gracious failure.

-- 
 i.

Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Hans de Goede]

Hi,

On 29-Oct-24 3:07 PM, Ilpo Järvinen wrote:
> Hi Hens,
> 
> There a question / item needing your input below.
> 
> On Wed, 23 Oct 2024, Mario Limonciello wrote:
>> On 10/23/2024 10:52, Shyam Sundar S K wrote:
>>> On 10/23/2024 21:10, Mario Limonciello wrote:
>>>> On 10/23/2024 10:32, Shyam Sundar S K wrote:
>>>>> On 10/23/2024 20:04, Mario Limonciello wrote:
>>>>>> On 10/23/2024 09:29, Shyam Sundar S K wrote:
>>>>>>> On 10/23/2024 19:41, Mario Limonciello wrote:
>>>>>>>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>>>>>>>>> The PMF driver will allocate shared buffer memory using the
>>>>>>>>> tee_shm_alloc_kernel_buf(). This allocated memory is located in
>>>>>>>>> the
>>>>>>>>> secure world and is used for communication with the PMF-TA.
>>>>>>>>>
>>>>>>>>> The latest PMF-TA version introduces new structures with OEM
>>>>>>>>> debug
>>>>>>>>> information and additional policy input conditions for
>>>>>>>>> evaluating the
>>>>>>>>> policy binary. Consequently, the shared memory size must be
>>>>>>>>> increased to
>>>>>>>>> ensure compatibility between the PMF driver and the updated
>>>>>>>>> PMF-TA.
>>>>>>>>>
>>>>>>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>>>>>
>>>>>>>> How does this present to a user?  From what you describe it seems
>>>>>>>> to
>>>>>>>> me like this means a new TA will fail on older kernel in some way.
>>>>>>>
>>>>>>> Newer TA will not fail on older systems. This change is just about
>>>>>>> the
>>>>>>> increase in TA reserved memory that is presented as "shared memory",
>>>>>>> as TA needs the additional memory for its own debug data structures.
>>>>>>
>>>>>> Thx for comments. But so if you use new TA with older kernel driver,
>>>>>> what will happen?  Can TA do a buffer overrun because the presented
>>>>>> shared memory was too small?
>>>>>>
>>>>>
>>>>> New TA will fail on older kernel and hence this change will be
>>>>> required for new TA to work.
>>>>
>>>> OK, that's what I was worried about.
>>>>
>>>>>
>>>>>>>
>>>>>>>    From user standpoint, always be on latest FW, irrespective of the
>>>>>>> platform. At this point in time, I don't see a need for FW
>>>>>>> versioning
>>>>>>> name (in the future, if there is a need for having a limited support
>>>>>>> to older platforms, we can carve out a logic to do versioning
>>>>>>> stuff).
>>>>>>
>>>>>> I wish we could enforce this, but In the Linux world there is an
>>>>>> expectation that these two trains don't need to arrive at station at
>>>>>> the same time.
>>>>>>
>>>>>>>
>>>>>>>> Some ideas:
>>>>>>>>
>>>>>>>> 1) Should there be header version check on the TA and dynamically
>>>>>>>> allocate the structure size based on the version of the F/W?
>>>>>>>>
>>>>>>>
>>>>>>> This can be done, when the TA versioning upgrade happens, like from
>>>>>>> 1.3 to 1.4, apart from that there is no header stuff association.
>>>>>>>
>>>>>>>> 2) Or is there a command to the TA that can query the expected
>>>>>>>> output
>>>>>>>> size?
>>>>>>>>
>>>>>>>
>>>>>>> No, this is just the initial shared memory that the driver allocates
>>>>>>> to pass the inputs and the commands to TA.
>>>>>>>
>>>>>>>> 3) Or should the new TA filename be versioned, and the driver has
>>>>>>>> a
>>>>>>>> fallback policy?
>>>>>>>>
>>>>>>>> Whatever the outcome is; I think it's best that if possible this
>>>>>>>> change goes back to stable to try to minimize regressions to
>>>>>>>> users as
>>>>>>>> distros update linux-firmware.  For example Fedora updates this
>>>>>>>> monthly, but also tracks stable kernels.
>>>>>>>>
>>>>>>>
>>>>>>> Advisory to distros should be to pick the latest PMF TA (note that,
>>>>>>> I
>>>>>>> have not still submitted to new TA FW).
>>>>>>
>>>>>> Yeah we can advise distros to pick it up when upstreamed as long as
>>>>>> there isn't tight dependency on this patch being present.
>>>>>>
>>>>>
>>>>> That is the reason I am waiting for this change to land. Once that is
>>>>> done, I will submit the new TA, you can send out a advisory to upgrade
>>>>> the kernel or this change has to be back-ported to stable/oem kernels
>>>>> for their enablement.
>>>>>
>>>>> Makes sense?
>>>>>
>>>>
>>>> I think we need Hans' and Ilpo's comments here to decide what to do.
>>>>
>>>
>>> Sure.
>>>
>>>> I will say that when we had this happen in amdgpu for a breaking
>>>> reason there was a new firmware binary filename created/upstreamed for
>>>> the breaking version (IIRC foo.bin -> foo_1.bin) and amdgpu had to
>>>> have fallback code so it could be compatible with either binary.
>>>>
>>>
>>> True. In case of amdgpu, the FW loading is part of the amdgpu driver.
>>> But in case of PMF, the PMF TA gets picked from the AMD TEE driver
>>> through the TEE commands.
>>>
>>> So, there is no need for FW versioning logic in PMF driver.
>>>
>>
>> That's a very good point, and this is a lot of complexity then.
>>
>>>
>>>> * If user on older kernel took newer linux-firmware package they used
>>>> older binary.
>>>> * If user on newer kernel took older linux-firmware package they used
>>>> older binary.
>>>> * If user on newer kernel took newer linux-firmware package they used
>>>> newer binary.
>>>>
>>>> If the decision is this goes in "as is" it definitely needs to go back
>>>> to stable kernels.
>>>>
>>>
>>> IMHO, let's not put too many fallback mechanisms. The philosophy
>>> should be use latest driver and latest FW that avoids a lot of
>>> confusion and yeah for that to happen this change has to go to stable.
>>>
>>> Thanks,
>>> Shyam
>>
>> Of course Hans and Ilpo make the final call, but I think from our discussions
>> here it would be ideal that patch 1 and patch 5 from this series go into 6.12
>> and have stable tags, the rest would be 6.13 material.
> 
> Distros and SW component management challenges are more in the domain of 
> Hans' expertise so I'd prefer to hear his opinion on this.
> 
> Personally I feel though that the commit message is not entirely honest 
> on all the impact as is. The wordings are sounding quite innocent while if 
> I infer the above right, an incorrect combination will cause a 
> non-gracious failure.

There are basically 4 possible scenarios and to me it
is only clear from this thread what will happen in 3 of
the 4 scenarios :

1. Old TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> works
2. New TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> broken
3. Old TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> ???
4. New TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> works

If the answer to 3 is: "works" then I agree that this patch
should be submitted to Linus as a fix with Cc: stable ASAP
and then once that has hit most stable series it should be
ok to upgrade the fw in linux-firmware

Note this is still not ideal but IMHO it would be ok.

But if the answer is "broken" then we will really need to
find some way to unbreak this, which could be as simple
as querying the fw-version and basing the size on this,
but having a kernel change which will regress things for
users who do not have the old firmware yet is simply
not acceptable.

Regards,

Hans

Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Shyam Sundar S K]

Hi,

On 10/30/2024 19:30, Hans de Goede wrote:
> Hi,
> 
> On 29-Oct-24 3:07 PM, Ilpo Järvinen wrote:
>> Hi Hens,
>>
>> There a question / item needing your input below.
>>
>> On Wed, 23 Oct 2024, Mario Limonciello wrote:
>>> On 10/23/2024 10:52, Shyam Sundar S K wrote:
>>>> On 10/23/2024 21:10, Mario Limonciello wrote:
>>>>> On 10/23/2024 10:32, Shyam Sundar S K wrote:
>>>>>> On 10/23/2024 20:04, Mario Limonciello wrote:
>>>>>>> On 10/23/2024 09:29, Shyam Sundar S K wrote:
>>>>>>>> On 10/23/2024 19:41, Mario Limonciello wrote:
>>>>>>>>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>>>>>>>>>> The PMF driver will allocate shared buffer memory using the
>>>>>>>>>> tee_shm_alloc_kernel_buf(). This allocated memory is located in
>>>>>>>>>> the
>>>>>>>>>> secure world and is used for communication with the PMF-TA.
>>>>>>>>>>
>>>>>>>>>> The latest PMF-TA version introduces new structures with OEM
>>>>>>>>>> debug
>>>>>>>>>> information and additional policy input conditions for
>>>>>>>>>> evaluating the
>>>>>>>>>> policy binary. Consequently, the shared memory size must be
>>>>>>>>>> increased to
>>>>>>>>>> ensure compatibility between the PMF driver and the updated
>>>>>>>>>> PMF-TA.
>>>>>>>>>>
>>>>>>>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>>>>>>
>>>>>>>>> How does this present to a user?  From what you describe it seems
>>>>>>>>> to
>>>>>>>>> me like this means a new TA will fail on older kernel in some way.
>>>>>>>>
>>>>>>>> Newer TA will not fail on older systems. This change is just about
>>>>>>>> the
>>>>>>>> increase in TA reserved memory that is presented as "shared memory",
>>>>>>>> as TA needs the additional memory for its own debug data structures.
>>>>>>>
>>>>>>> Thx for comments. But so if you use new TA with older kernel driver,
>>>>>>> what will happen?  Can TA do a buffer overrun because the presented
>>>>>>> shared memory was too small?
>>>>>>>
>>>>>>
>>>>>> New TA will fail on older kernel and hence this change will be
>>>>>> required for new TA to work.
>>>>>
>>>>> OK, that's what I was worried about.
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>>    From user standpoint, always be on latest FW, irrespective of the
>>>>>>>> platform. At this point in time, I don't see a need for FW
>>>>>>>> versioning
>>>>>>>> name (in the future, if there is a need for having a limited support
>>>>>>>> to older platforms, we can carve out a logic to do versioning
>>>>>>>> stuff).
>>>>>>>
>>>>>>> I wish we could enforce this, but In the Linux world there is an
>>>>>>> expectation that these two trains don't need to arrive at station at
>>>>>>> the same time.
>>>>>>>
>>>>>>>>
>>>>>>>>> Some ideas:
>>>>>>>>>
>>>>>>>>> 1) Should there be header version check on the TA and dynamically
>>>>>>>>> allocate the structure size based on the version of the F/W?
>>>>>>>>>
>>>>>>>>
>>>>>>>> This can be done, when the TA versioning upgrade happens, like from
>>>>>>>> 1.3 to 1.4, apart from that there is no header stuff association.
>>>>>>>>
>>>>>>>>> 2) Or is there a command to the TA that can query the expected
>>>>>>>>> output
>>>>>>>>> size?
>>>>>>>>>
>>>>>>>>
>>>>>>>> No, this is just the initial shared memory that the driver allocates
>>>>>>>> to pass the inputs and the commands to TA.
>>>>>>>>
>>>>>>>>> 3) Or should the new TA filename be versioned, and the driver has
>>>>>>>>> a
>>>>>>>>> fallback policy?
>>>>>>>>>
>>>>>>>>> Whatever the outcome is; I think it's best that if possible this
>>>>>>>>> change goes back to stable to try to minimize regressions to
>>>>>>>>> users as
>>>>>>>>> distros update linux-firmware.  For example Fedora updates this
>>>>>>>>> monthly, but also tracks stable kernels.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Advisory to distros should be to pick the latest PMF TA (note that,
>>>>>>>> I
>>>>>>>> have not still submitted to new TA FW).
>>>>>>>
>>>>>>> Yeah we can advise distros to pick it up when upstreamed as long as
>>>>>>> there isn't tight dependency on this patch being present.
>>>>>>>
>>>>>>
>>>>>> That is the reason I am waiting for this change to land. Once that is
>>>>>> done, I will submit the new TA, you can send out a advisory to upgrade
>>>>>> the kernel or this change has to be back-ported to stable/oem kernels
>>>>>> for their enablement.
>>>>>>
>>>>>> Makes sense?
>>>>>>
>>>>>
>>>>> I think we need Hans' and Ilpo's comments here to decide what to do.
>>>>>
>>>>
>>>> Sure.
>>>>
>>>>> I will say that when we had this happen in amdgpu for a breaking
>>>>> reason there was a new firmware binary filename created/upstreamed for
>>>>> the breaking version (IIRC foo.bin -> foo_1.bin) and amdgpu had to
>>>>> have fallback code so it could be compatible with either binary.
>>>>>
>>>>
>>>> True. In case of amdgpu, the FW loading is part of the amdgpu driver.
>>>> But in case of PMF, the PMF TA gets picked from the AMD TEE driver
>>>> through the TEE commands.
>>>>
>>>> So, there is no need for FW versioning logic in PMF driver.
>>>>
>>>
>>> That's a very good point, and this is a lot of complexity then.
>>>
>>>>
>>>>> * If user on older kernel took newer linux-firmware package they used
>>>>> older binary.
>>>>> * If user on newer kernel took older linux-firmware package they used
>>>>> older binary.
>>>>> * If user on newer kernel took newer linux-firmware package they used
>>>>> newer binary.
>>>>>
>>>>> If the decision is this goes in "as is" it definitely needs to go back
>>>>> to stable kernels.
>>>>>
>>>>
>>>> IMHO, let's not put too many fallback mechanisms. The philosophy
>>>> should be use latest driver and latest FW that avoids a lot of
>>>> confusion and yeah for that to happen this change has to go to stable.
>>>>
>>>> Thanks,
>>>> Shyam
>>>
>>> Of course Hans and Ilpo make the final call, but I think from our discussions
>>> here it would be ideal that patch 1 and patch 5 from this series go into 6.12
>>> and have stable tags, the rest would be 6.13 material.
>>
>> Distros and SW component management challenges are more in the domain of 
>> Hans' expertise so I'd prefer to hear his opinion on this.
>>
>> Personally I feel though that the commit message is not entirely honest 
>> on all the impact as is. The wordings are sounding quite innocent while if 
>> I infer the above right, an incorrect combination will cause a 
>> non-gracious failure.
> 
> There are basically 4 possible scenarios and to me it
> is only clear from this thread what will happen in 3 of
> the 4 scenarios :
> 
> 1. Old TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> works
> 2. New TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> broken
> 3. Old TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> ???
> 4. New TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> works
> 
> If the answer to 3 is: "works" then I agree that this patch
> should be submitted to Linus as a fix with Cc: stable ASAP
> and then once that has hit most stable series it should be
> ok to upgrade the fw in linux-firmware
> 

Short answer, "yes" it does not work for "3." and you can consider it
a broken.

> Note this is still not ideal but IMHO it would be ok.
> 
> But if the answer is "broken" then we will really need to
> find some way to unbreak this, which could be as simple
> as querying the fw-version and basing the size on this,
> but having a kernel change which will regress things for
> users who do not have the old firmware yet is simply
> not acceptable.
> 

I am not sure if there is a firmware versioning interface that the ASP
(AMD Security Processor) returns back the kernel/driver.

The code path in this case is:

AMD PMF driver -> AMD TEE driver -> AMD CCP driver -> ASP TEE -> ASP
TA -> ASP HW.

So, I uncertain which module has this information and where exactly
the code of fw versioning has to reside. It will take a while for me
to dig this in.

Meanwhile, shall I drop this patch and resend the series (by
addressing the dev_dbg change Mario commented) so that this atleast
becomes a 6.13 material?


Thanks,
Shyam

> Regards,
> 
> Hans
> 
> 
> 
> 

Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Mario Limonciello]

On 10/30/2024 11:03, Shyam Sundar S K wrote:
> Hi,
> 
> On 10/30/2024 19:30, Hans de Goede wrote:
>> Hi,
>>
>> On 29-Oct-24 3:07 PM, Ilpo Järvinen wrote:
>>> Hi Hens,
>>>
>>> There a question / item needing your input below.
>>>
>>> On Wed, 23 Oct 2024, Mario Limonciello wrote:
>>>> On 10/23/2024 10:52, Shyam Sundar S K wrote:
>>>>> On 10/23/2024 21:10, Mario Limonciello wrote:
>>>>>> On 10/23/2024 10:32, Shyam Sundar S K wrote:
>>>>>>> On 10/23/2024 20:04, Mario Limonciello wrote:
>>>>>>>> On 10/23/2024 09:29, Shyam Sundar S K wrote:
>>>>>>>>> On 10/23/2024 19:41, Mario Limonciello wrote:
>>>>>>>>>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>>>>>>>>>>> The PMF driver will allocate shared buffer memory using the
>>>>>>>>>>> tee_shm_alloc_kernel_buf(). This allocated memory is located in
>>>>>>>>>>> the
>>>>>>>>>>> secure world and is used for communication with the PMF-TA.
>>>>>>>>>>>
>>>>>>>>>>> The latest PMF-TA version introduces new structures with OEM
>>>>>>>>>>> debug
>>>>>>>>>>> information and additional policy input conditions for
>>>>>>>>>>> evaluating the
>>>>>>>>>>> policy binary. Consequently, the shared memory size must be
>>>>>>>>>>> increased to
>>>>>>>>>>> ensure compatibility between the PMF driver and the updated
>>>>>>>>>>> PMF-TA.
>>>>>>>>>>>
>>>>>>>>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>>>>>>>
>>>>>>>>>> How does this present to a user?  From what you describe it seems
>>>>>>>>>> to
>>>>>>>>>> me like this means a new TA will fail on older kernel in some way.
>>>>>>>>>
>>>>>>>>> Newer TA will not fail on older systems. This change is just about
>>>>>>>>> the
>>>>>>>>> increase in TA reserved memory that is presented as "shared memory",
>>>>>>>>> as TA needs the additional memory for its own debug data structures.
>>>>>>>>
>>>>>>>> Thx for comments. But so if you use new TA with older kernel driver,
>>>>>>>> what will happen?  Can TA do a buffer overrun because the presented
>>>>>>>> shared memory was too small?
>>>>>>>>
>>>>>>>
>>>>>>> New TA will fail on older kernel and hence this change will be
>>>>>>> required for new TA to work.
>>>>>>
>>>>>> OK, that's what I was worried about.
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>     From user standpoint, always be on latest FW, irrespective of the
>>>>>>>>> platform. At this point in time, I don't see a need for FW
>>>>>>>>> versioning
>>>>>>>>> name (in the future, if there is a need for having a limited support
>>>>>>>>> to older platforms, we can carve out a logic to do versioning
>>>>>>>>> stuff).
>>>>>>>>
>>>>>>>> I wish we could enforce this, but In the Linux world there is an
>>>>>>>> expectation that these two trains don't need to arrive at station at
>>>>>>>> the same time.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Some ideas:
>>>>>>>>>>
>>>>>>>>>> 1) Should there be header version check on the TA and dynamically
>>>>>>>>>> allocate the structure size based on the version of the F/W?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This can be done, when the TA versioning upgrade happens, like from
>>>>>>>>> 1.3 to 1.4, apart from that there is no header stuff association.
>>>>>>>>>
>>>>>>>>>> 2) Or is there a command to the TA that can query the expected
>>>>>>>>>> output
>>>>>>>>>> size?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> No, this is just the initial shared memory that the driver allocates
>>>>>>>>> to pass the inputs and the commands to TA.
>>>>>>>>>
>>>>>>>>>> 3) Or should the new TA filename be versioned, and the driver has
>>>>>>>>>> a
>>>>>>>>>> fallback policy?
>>>>>>>>>>
>>>>>>>>>> Whatever the outcome is; I think it's best that if possible this
>>>>>>>>>> change goes back to stable to try to minimize regressions to
>>>>>>>>>> users as
>>>>>>>>>> distros update linux-firmware.  For example Fedora updates this
>>>>>>>>>> monthly, but also tracks stable kernels.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Advisory to distros should be to pick the latest PMF TA (note that,
>>>>>>>>> I
>>>>>>>>> have not still submitted to new TA FW).
>>>>>>>>
>>>>>>>> Yeah we can advise distros to pick it up when upstreamed as long as
>>>>>>>> there isn't tight dependency on this patch being present.
>>>>>>>>
>>>>>>>
>>>>>>> That is the reason I am waiting for this change to land. Once that is
>>>>>>> done, I will submit the new TA, you can send out a advisory to upgrade
>>>>>>> the kernel or this change has to be back-ported to stable/oem kernels
>>>>>>> for their enablement.
>>>>>>>
>>>>>>> Makes sense?
>>>>>>>
>>>>>>
>>>>>> I think we need Hans' and Ilpo's comments here to decide what to do.
>>>>>>
>>>>>
>>>>> Sure.
>>>>>
>>>>>> I will say that when we had this happen in amdgpu for a breaking
>>>>>> reason there was a new firmware binary filename created/upstreamed for
>>>>>> the breaking version (IIRC foo.bin -> foo_1.bin) and amdgpu had to
>>>>>> have fallback code so it could be compatible with either binary.
>>>>>>
>>>>>
>>>>> True. In case of amdgpu, the FW loading is part of the amdgpu driver.
>>>>> But in case of PMF, the PMF TA gets picked from the AMD TEE driver
>>>>> through the TEE commands.
>>>>>
>>>>> So, there is no need for FW versioning logic in PMF driver.
>>>>>
>>>>
>>>> That's a very good point, and this is a lot of complexity then.
>>>>
>>>>>
>>>>>> * If user on older kernel took newer linux-firmware package they used
>>>>>> older binary.
>>>>>> * If user on newer kernel took older linux-firmware package they used
>>>>>> older binary.
>>>>>> * If user on newer kernel took newer linux-firmware package they used
>>>>>> newer binary.
>>>>>>
>>>>>> If the decision is this goes in "as is" it definitely needs to go back
>>>>>> to stable kernels.
>>>>>>
>>>>>
>>>>> IMHO, let's not put too many fallback mechanisms. The philosophy
>>>>> should be use latest driver and latest FW that avoids a lot of
>>>>> confusion and yeah for that to happen this change has to go to stable.
>>>>>
>>>>> Thanks,
>>>>> Shyam
>>>>
>>>> Of course Hans and Ilpo make the final call, but I think from our discussions
>>>> here it would be ideal that patch 1 and patch 5 from this series go into 6.12
>>>> and have stable tags, the rest would be 6.13 material.
>>>
>>> Distros and SW component management challenges are more in the domain of
>>> Hans' expertise so I'd prefer to hear his opinion on this.
>>>
>>> Personally I feel though that the commit message is not entirely honest
>>> on all the impact as is. The wordings are sounding quite innocent while if
>>> I infer the above right, an incorrect combination will cause a
>>> non-gracious failure.
>>
>> There are basically 4 possible scenarios and to me it
>> is only clear from this thread what will happen in 3 of
>> the 4 scenarios :
>>
>> 1. Old TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> works
>> 2. New TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> broken
>> 3. Old TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> ???
>> 4. New TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> works
>>
>> If the answer to 3 is: "works" then I agree that this patch
>> should be submitted to Linus as a fix with Cc: stable ASAP
>> and then once that has hit most stable series it should be
>> ok to upgrade the fw in linux-firmware
>>
> 
> Short answer, "yes" it does not work for "3." and you can consider it
> a broken.
> 
>> Note this is still not ideal but IMHO it would be ok.
>>
>> But if the answer is "broken" then we will really need to
>> find some way to unbreak this, which could be as simple
>> as querying the fw-version and basing the size on this,
>> but having a kernel change which will regress things for
>> users who do not have the old firmware yet is simply
>> not acceptable.
>>
> 
> I am not sure if there is a firmware versioning interface that the ASP
> (AMD Security Processor) returns back the kernel/driver.
  > The code path in this case is:
> 
> AMD PMF driver -> AMD TEE driver -> AMD CCP driver -> ASP TEE -> ASP
> TA -> ASP HW.
> 
> So, I uncertain which module has this information and where exactly
> the code of fw versioning has to reside. It will take a while for me
> to dig this in.

As a solution to this, can amd-pmf explicitly do it's own 
request_firmware() call to load the firmware binary and determine the 
size to use in the array and then discard the loaded binary?

This would let the TEE module still do it's own load later like normal 
without having to plumb this information across subsystems.

> 
> Meanwhile, shall I drop this patch and resend the series (by
> addressing the dev_dbg change Mario commented) so that this atleast
> becomes a 6.13 material?
 > >
> Thanks,
> Shyam
> 
>> Regards,
>>
>> Hans
>>
>>
>>
>>

Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Shyam Sundar S K]

On 10/30/2024 21:38, Mario Limonciello wrote:
> On 10/30/2024 11:03, Shyam Sundar S K wrote:
>> Hi,
>>
>> On 10/30/2024 19:30, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 29-Oct-24 3:07 PM, Ilpo Järvinen wrote:
>>>> Hi Hens,
>>>>
>>>> There a question / item needing your input below.
>>>>
>>>> On Wed, 23 Oct 2024, Mario Limonciello wrote:
>>>>> On 10/23/2024 10:52, Shyam Sundar S K wrote:
>>>>>> On 10/23/2024 21:10, Mario Limonciello wrote:
>>>>>>> On 10/23/2024 10:32, Shyam Sundar S K wrote:
>>>>>>>> On 10/23/2024 20:04, Mario Limonciello wrote:
>>>>>>>>> On 10/23/2024 09:29, Shyam Sundar S K wrote:
>>>>>>>>>> On 10/23/2024 19:41, Mario Limonciello wrote:
>>>>>>>>>>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>>>>>>>>>>>> The PMF driver will allocate shared buffer memory using the
>>>>>>>>>>>> tee_shm_alloc_kernel_buf(). This allocated memory is
>>>>>>>>>>>> located in
>>>>>>>>>>>> the
>>>>>>>>>>>> secure world and is used for communication with the PMF-TA.
>>>>>>>>>>>>
>>>>>>>>>>>> The latest PMF-TA version introduces new structures with OEM
>>>>>>>>>>>> debug
>>>>>>>>>>>> information and additional policy input conditions for
>>>>>>>>>>>> evaluating the
>>>>>>>>>>>> policy binary. Consequently, the shared memory size must be
>>>>>>>>>>>> increased to
>>>>>>>>>>>> ensure compatibility between the PMF driver and the updated
>>>>>>>>>>>> PMF-TA.
>>>>>>>>>>>>
>>>>>>>>>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>>>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>>>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>>>>>>>>
>>>>>>>>>>> How does this present to a user?  From what you describe it
>>>>>>>>>>> seems
>>>>>>>>>>> to
>>>>>>>>>>> me like this means a new TA will fail on older kernel in
>>>>>>>>>>> some way.
>>>>>>>>>>
>>>>>>>>>> Newer TA will not fail on older systems. This change is just
>>>>>>>>>> about
>>>>>>>>>> the
>>>>>>>>>> increase in TA reserved memory that is presented as "shared
>>>>>>>>>> memory",
>>>>>>>>>> as TA needs the additional memory for its own debug data
>>>>>>>>>> structures.
>>>>>>>>>
>>>>>>>>> Thx for comments. But so if you use new TA with older kernel
>>>>>>>>> driver,
>>>>>>>>> what will happen?  Can TA do a buffer overrun because the
>>>>>>>>> presented
>>>>>>>>> shared memory was too small?
>>>>>>>>>
>>>>>>>>
>>>>>>>> New TA will fail on older kernel and hence this change will be
>>>>>>>> required for new TA to work.
>>>>>>>
>>>>>>> OK, that's what I was worried about.
>>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>     From user standpoint, always be on latest FW,
>>>>>>>>>> irrespective of the
>>>>>>>>>> platform. At this point in time, I don't see a need for FW
>>>>>>>>>> versioning
>>>>>>>>>> name (in the future, if there is a need for having a limited
>>>>>>>>>> support
>>>>>>>>>> to older platforms, we can carve out a logic to do versioning
>>>>>>>>>> stuff).
>>>>>>>>>
>>>>>>>>> I wish we could enforce this, but In the Linux world there is an
>>>>>>>>> expectation that these two trains don't need to arrive at
>>>>>>>>> station at
>>>>>>>>> the same time.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Some ideas:
>>>>>>>>>>>
>>>>>>>>>>> 1) Should there be header version check on the TA and
>>>>>>>>>>> dynamically
>>>>>>>>>>> allocate the structure size based on the version of the F/W?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This can be done, when the TA versioning upgrade happens,
>>>>>>>>>> like from
>>>>>>>>>> 1.3 to 1.4, apart from that there is no header stuff
>>>>>>>>>> association.
>>>>>>>>>>
>>>>>>>>>>> 2) Or is there a command to the TA that can query the expected
>>>>>>>>>>> output
>>>>>>>>>>> size?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> No, this is just the initial shared memory that the driver
>>>>>>>>>> allocates
>>>>>>>>>> to pass the inputs and the commands to TA.
>>>>>>>>>>
>>>>>>>>>>> 3) Or should the new TA filename be versioned, and the
>>>>>>>>>>> driver has
>>>>>>>>>>> a
>>>>>>>>>>> fallback policy?
>>>>>>>>>>>
>>>>>>>>>>> Whatever the outcome is; I think it's best that if possible
>>>>>>>>>>> this
>>>>>>>>>>> change goes back to stable to try to minimize regressions to
>>>>>>>>>>> users as
>>>>>>>>>>> distros update linux-firmware.  For example Fedora updates
>>>>>>>>>>> this
>>>>>>>>>>> monthly, but also tracks stable kernels.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Advisory to distros should be to pick the latest PMF TA
>>>>>>>>>> (note that,
>>>>>>>>>> I
>>>>>>>>>> have not still submitted to new TA FW).
>>>>>>>>>
>>>>>>>>> Yeah we can advise distros to pick it up when upstreamed as
>>>>>>>>> long as
>>>>>>>>> there isn't tight dependency on this patch being present.
>>>>>>>>>
>>>>>>>>
>>>>>>>> That is the reason I am waiting for this change to land. Once
>>>>>>>> that is
>>>>>>>> done, I will submit the new TA, you can send out a advisory to
>>>>>>>> upgrade
>>>>>>>> the kernel or this change has to be back-ported to stable/oem
>>>>>>>> kernels
>>>>>>>> for their enablement.
>>>>>>>>
>>>>>>>> Makes sense?
>>>>>>>>
>>>>>>>
>>>>>>> I think we need Hans' and Ilpo's comments here to decide what
>>>>>>> to do.
>>>>>>>
>>>>>>
>>>>>> Sure.
>>>>>>
>>>>>>> I will say that when we had this happen in amdgpu for a breaking
>>>>>>> reason there was a new firmware binary filename
>>>>>>> created/upstreamed for
>>>>>>> the breaking version (IIRC foo.bin -> foo_1.bin) and amdgpu had to
>>>>>>> have fallback code so it could be compatible with either binary.
>>>>>>>
>>>>>>
>>>>>> True. In case of amdgpu, the FW loading is part of the amdgpu
>>>>>> driver.
>>>>>> But in case of PMF, the PMF TA gets picked from the AMD TEE driver
>>>>>> through the TEE commands.
>>>>>>
>>>>>> So, there is no need for FW versioning logic in PMF driver.
>>>>>>
>>>>>
>>>>> That's a very good point, and this is a lot of complexity then.
>>>>>
>>>>>>
>>>>>>> * If user on older kernel took newer linux-firmware package
>>>>>>> they used
>>>>>>> older binary.
>>>>>>> * If user on newer kernel took older linux-firmware package
>>>>>>> they used
>>>>>>> older binary.
>>>>>>> * If user on newer kernel took newer linux-firmware package
>>>>>>> they used
>>>>>>> newer binary.
>>>>>>>
>>>>>>> If the decision is this goes in "as is" it definitely needs to
>>>>>>> go back
>>>>>>> to stable kernels.
>>>>>>>
>>>>>>
>>>>>> IMHO, let's not put too many fallback mechanisms. The philosophy
>>>>>> should be use latest driver and latest FW that avoids a lot of
>>>>>> confusion and yeah for that to happen this change has to go to
>>>>>> stable.
>>>>>>
>>>>>> Thanks,
>>>>>> Shyam
>>>>>
>>>>> Of course Hans and Ilpo make the final call, but I think from our
>>>>> discussions
>>>>> here it would be ideal that patch 1 and patch 5 from this series
>>>>> go into 6.12
>>>>> and have stable tags, the rest would be 6.13 material.
>>>>
>>>> Distros and SW component management challenges are more in the
>>>> domain of
>>>> Hans' expertise so I'd prefer to hear his opinion on this.
>>>>
>>>> Personally I feel though that the commit message is not entirely
>>>> honest
>>>> on all the impact as is. The wordings are sounding quite innocent
>>>> while if
>>>> I infer the above right, an incorrect combination will cause a
>>>> non-gracious failure.
>>>
>>> There are basically 4 possible scenarios and to me it
>>> is only clear from this thread what will happen in 3 of
>>> the 4 scenarios :
>>>
>>> 1. Old TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> works
>>> 2. New TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> broken
>>> 3. Old TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> ???
>>> 4. New TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> works
>>>
>>> If the answer to 3 is: "works" then I agree that this patch
>>> should be submitted to Linus as a fix with Cc: stable ASAP
>>> and then once that has hit most stable series it should be
>>> ok to upgrade the fw in linux-firmware
>>>
>>
>> Short answer, "yes" it does not work for "3." and you can consider it
>> a broken.
>>
>>> Note this is still not ideal but IMHO it would be ok.
>>>
>>> But if the answer is "broken" then we will really need to
>>> find some way to unbreak this, which could be as simple
>>> as querying the fw-version and basing the size on this,
>>> but having a kernel change which will regress things for
>>> users who do not have the old firmware yet is simply
>>> not acceptable.
>>>
>>
>> I am not sure if there is a firmware versioning interface that the ASP
>> (AMD Security Processor) returns back the kernel/driver.
>  > The code path in this case is:
>>
>> AMD PMF driver -> AMD TEE driver -> AMD CCP driver -> ASP TEE -> ASP
>> TA -> ASP HW.
>>
>> So, I uncertain which module has this information and where exactly
>> the code of fw versioning has to reside. It will take a while for me
>> to dig this in.
> 
> As a solution to this, can amd-pmf explicitly do it's own
> request_firmware() call to load the firmware binary and determine the
> size to use in the array and then discard the loaded binary?
> 
> This would let the TEE module still do it's own load later like normal
> without having to plumb this information across subsystems.
> 

TEE driver feeds in a lot of metadata and the structure information
for PSP headers and I don't think just having a request_firmware()
will help.

Sidebar, TEE driver has a lot of plumbing that can be used decrypt the
policy binaries to debug issues related to TA load failures and policy
binary issues (basically the descriptors)

So we might end up in replicating a majority of TEE code into PMF
driver, which might not be a good design choice.

Let me first talk to internal folks to see if we can solve it by not
making complex changes.

Thanks,
Shyam


>>
>> Meanwhile, shall I drop this patch and resend the series (by
>> addressing the dev_dbg change Mario commented) so that this atleast
>> becomes a 6.13 material?
>> >
>> Thanks,
>> Shyam
>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>
>>>
> 

Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Mario Limonciello]

On 10/30/2024 11:23, Shyam Sundar S K wrote:
> 
> 
> On 10/30/2024 21:38, Mario Limonciello wrote:
>> On 10/30/2024 11:03, Shyam Sundar S K wrote:
>>> Hi,
>>>
>>> On 10/30/2024 19:30, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 29-Oct-24 3:07 PM, Ilpo Järvinen wrote:
>>>>> Hi Hens,
>>>>>
>>>>> There a question / item needing your input below.
>>>>>
>>>>> On Wed, 23 Oct 2024, Mario Limonciello wrote:
>>>>>> On 10/23/2024 10:52, Shyam Sundar S K wrote:
>>>>>>> On 10/23/2024 21:10, Mario Limonciello wrote:
>>>>>>>> On 10/23/2024 10:32, Shyam Sundar S K wrote:
>>>>>>>>> On 10/23/2024 20:04, Mario Limonciello wrote:
>>>>>>>>>> On 10/23/2024 09:29, Shyam Sundar S K wrote:
>>>>>>>>>>> On 10/23/2024 19:41, Mario Limonciello wrote:
>>>>>>>>>>>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>>>>>>>>>>>>> The PMF driver will allocate shared buffer memory using the
>>>>>>>>>>>>> tee_shm_alloc_kernel_buf(). This allocated memory is
>>>>>>>>>>>>> located in
>>>>>>>>>>>>> the
>>>>>>>>>>>>> secure world and is used for communication with the PMF-TA.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The latest PMF-TA version introduces new structures with OEM
>>>>>>>>>>>>> debug
>>>>>>>>>>>>> information and additional policy input conditions for
>>>>>>>>>>>>> evaluating the
>>>>>>>>>>>>> policy binary. Consequently, the shared memory size must be
>>>>>>>>>>>>> increased to
>>>>>>>>>>>>> ensure compatibility between the PMF driver and the updated
>>>>>>>>>>>>> PMF-TA.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>>>>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>>>>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>>>>>>>>>
>>>>>>>>>>>> How does this present to a user?  From what you describe it
>>>>>>>>>>>> seems
>>>>>>>>>>>> to
>>>>>>>>>>>> me like this means a new TA will fail on older kernel in
>>>>>>>>>>>> some way.
>>>>>>>>>>>
>>>>>>>>>>> Newer TA will not fail on older systems. This change is just
>>>>>>>>>>> about
>>>>>>>>>>> the
>>>>>>>>>>> increase in TA reserved memory that is presented as "shared
>>>>>>>>>>> memory",
>>>>>>>>>>> as TA needs the additional memory for its own debug data
>>>>>>>>>>> structures.
>>>>>>>>>>
>>>>>>>>>> Thx for comments. But so if you use new TA with older kernel
>>>>>>>>>> driver,
>>>>>>>>>> what will happen?  Can TA do a buffer overrun because the
>>>>>>>>>> presented
>>>>>>>>>> shared memory was too small?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> New TA will fail on older kernel and hence this change will be
>>>>>>>>> required for new TA to work.
>>>>>>>>
>>>>>>>> OK, that's what I was worried about.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>      From user standpoint, always be on latest FW,
>>>>>>>>>>> irrespective of the
>>>>>>>>>>> platform. At this point in time, I don't see a need for FW
>>>>>>>>>>> versioning
>>>>>>>>>>> name (in the future, if there is a need for having a limited
>>>>>>>>>>> support
>>>>>>>>>>> to older platforms, we can carve out a logic to do versioning
>>>>>>>>>>> stuff).
>>>>>>>>>>
>>>>>>>>>> I wish we could enforce this, but In the Linux world there is an
>>>>>>>>>> expectation that these two trains don't need to arrive at
>>>>>>>>>> station at
>>>>>>>>>> the same time.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> Some ideas:
>>>>>>>>>>>>
>>>>>>>>>>>> 1) Should there be header version check on the TA and
>>>>>>>>>>>> dynamically
>>>>>>>>>>>> allocate the structure size based on the version of the F/W?
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> This can be done, when the TA versioning upgrade happens,
>>>>>>>>>>> like from
>>>>>>>>>>> 1.3 to 1.4, apart from that there is no header stuff
>>>>>>>>>>> association.
>>>>>>>>>>>
>>>>>>>>>>>> 2) Or is there a command to the TA that can query the expected
>>>>>>>>>>>> output
>>>>>>>>>>>> size?
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> No, this is just the initial shared memory that the driver
>>>>>>>>>>> allocates
>>>>>>>>>>> to pass the inputs and the commands to TA.
>>>>>>>>>>>
>>>>>>>>>>>> 3) Or should the new TA filename be versioned, and the
>>>>>>>>>>>> driver has
>>>>>>>>>>>> a
>>>>>>>>>>>> fallback policy?
>>>>>>>>>>>>
>>>>>>>>>>>> Whatever the outcome is; I think it's best that if possible
>>>>>>>>>>>> this
>>>>>>>>>>>> change goes back to stable to try to minimize regressions to
>>>>>>>>>>>> users as
>>>>>>>>>>>> distros update linux-firmware.  For example Fedora updates
>>>>>>>>>>>> this
>>>>>>>>>>>> monthly, but also tracks stable kernels.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Advisory to distros should be to pick the latest PMF TA
>>>>>>>>>>> (note that,
>>>>>>>>>>> I
>>>>>>>>>>> have not still submitted to new TA FW).
>>>>>>>>>>
>>>>>>>>>> Yeah we can advise distros to pick it up when upstreamed as
>>>>>>>>>> long as
>>>>>>>>>> there isn't tight dependency on this patch being present.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> That is the reason I am waiting for this change to land. Once
>>>>>>>>> that is
>>>>>>>>> done, I will submit the new TA, you can send out a advisory to
>>>>>>>>> upgrade
>>>>>>>>> the kernel or this change has to be back-ported to stable/oem
>>>>>>>>> kernels
>>>>>>>>> for their enablement.
>>>>>>>>>
>>>>>>>>> Makes sense?
>>>>>>>>>
>>>>>>>>
>>>>>>>> I think we need Hans' and Ilpo's comments here to decide what
>>>>>>>> to do.
>>>>>>>>
>>>>>>>
>>>>>>> Sure.
>>>>>>>
>>>>>>>> I will say that when we had this happen in amdgpu for a breaking
>>>>>>>> reason there was a new firmware binary filename
>>>>>>>> created/upstreamed for
>>>>>>>> the breaking version (IIRC foo.bin -> foo_1.bin) and amdgpu had to
>>>>>>>> have fallback code so it could be compatible with either binary.
>>>>>>>>
>>>>>>>
>>>>>>> True. In case of amdgpu, the FW loading is part of the amdgpu
>>>>>>> driver.
>>>>>>> But in case of PMF, the PMF TA gets picked from the AMD TEE driver
>>>>>>> through the TEE commands.
>>>>>>>
>>>>>>> So, there is no need for FW versioning logic in PMF driver.
>>>>>>>
>>>>>>
>>>>>> That's a very good point, and this is a lot of complexity then.
>>>>>>
>>>>>>>
>>>>>>>> * If user on older kernel took newer linux-firmware package
>>>>>>>> they used
>>>>>>>> older binary.
>>>>>>>> * If user on newer kernel took older linux-firmware package
>>>>>>>> they used
>>>>>>>> older binary.
>>>>>>>> * If user on newer kernel took newer linux-firmware package
>>>>>>>> they used
>>>>>>>> newer binary.
>>>>>>>>
>>>>>>>> If the decision is this goes in "as is" it definitely needs to
>>>>>>>> go back
>>>>>>>> to stable kernels.
>>>>>>>>
>>>>>>>
>>>>>>> IMHO, let's not put too many fallback mechanisms. The philosophy
>>>>>>> should be use latest driver and latest FW that avoids a lot of
>>>>>>> confusion and yeah for that to happen this change has to go to
>>>>>>> stable.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Shyam
>>>>>>
>>>>>> Of course Hans and Ilpo make the final call, but I think from our
>>>>>> discussions
>>>>>> here it would be ideal that patch 1 and patch 5 from this series
>>>>>> go into 6.12
>>>>>> and have stable tags, the rest would be 6.13 material.
>>>>>
>>>>> Distros and SW component management challenges are more in the
>>>>> domain of
>>>>> Hans' expertise so I'd prefer to hear his opinion on this.
>>>>>
>>>>> Personally I feel though that the commit message is not entirely
>>>>> honest
>>>>> on all the impact as is. The wordings are sounding quite innocent
>>>>> while if
>>>>> I infer the above right, an incorrect combination will cause a
>>>>> non-gracious failure.
>>>>
>>>> There are basically 4 possible scenarios and to me it
>>>> is only clear from this thread what will happen in 3 of
>>>> the 4 scenarios :
>>>>
>>>> 1. Old TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> works
>>>> 2. New TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> broken
>>>> 3. Old TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> ???
>>>> 4. New TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> works
>>>>
>>>> If the answer to 3 is: "works" then I agree that this patch
>>>> should be submitted to Linus as a fix with Cc: stable ASAP
>>>> and then once that has hit most stable series it should be
>>>> ok to upgrade the fw in linux-firmware
>>>>
>>>
>>> Short answer, "yes" it does not work for "3." and you can consider it
>>> a broken.
>>>
>>>> Note this is still not ideal but IMHO it would be ok.
>>>>
>>>> But if the answer is "broken" then we will really need to
>>>> find some way to unbreak this, which could be as simple
>>>> as querying the fw-version and basing the size on this,
>>>> but having a kernel change which will regress things for
>>>> users who do not have the old firmware yet is simply
>>>> not acceptable.
>>>>
>>>
>>> I am not sure if there is a firmware versioning interface that the ASP
>>> (AMD Security Processor) returns back the kernel/driver.
>>   > The code path in this case is:
>>>
>>> AMD PMF driver -> AMD TEE driver -> AMD CCP driver -> ASP TEE -> ASP
>>> TA -> ASP HW.
>>>
>>> So, I uncertain which module has this information and where exactly
>>> the code of fw versioning has to reside. It will take a while for me
>>> to dig this in.
>>
>> As a solution to this, can amd-pmf explicitly do it's own
>> request_firmware() call to load the firmware binary and determine the
>> size to use in the array and then discard the loaded binary?
>>
>> This would let the TEE module still do it's own load later like normal
>> without having to plumb this information across subsystems.
>>
> 
> TEE driver feeds in a lot of metadata and the structure information
> for PSP headers and I don't think just having a request_firmware()
> will help.
> 
> Sidebar, TEE driver has a lot of plumbing that can be used decrypt the
> policy binaries to debug issues related to TA load failures and policy
> binary issues (basically the descriptors)
> 
> So we might end up in replicating a majority of TEE code into PMF
> driver, which might not be a good design choice.
> 
> Let me first talk to internal folks to see if we can solve it by not
> making complex changes.
> 

OK, I think for the purpose of 6.13 then this series minus the last 
patch probably makes sense.

I also think the first patch should ideally come in 6.12 if Hans is OK 
with that.  As 6.12 is probably going to be the next LTS we'll want 
hardware supported as widely as possible within stable rules.

> Thanks,
> Shyam
> 
> 
>>>
>>> Meanwhile, shall I drop this patch and resend the series (by
>>> addressing the dev_dbg change Mario commented) so that this atleast
>>> becomes a 6.13 material?
>>>>
>>> Thanks,
>>> Shyam
>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>
>>>>
>>>>
>>

Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Hans de Goede]

Hi,

On 30-Oct-24 5:38 PM, Mario Limonciello wrote:
> On 10/30/2024 11:23, Shyam Sundar S K wrote:
>>
>>
>> On 10/30/2024 21:38, Mario Limonciello wrote:
>>> On 10/30/2024 11:03, Shyam Sundar S K wrote:
>>>> Hi,
>>>>
>>>> On 10/30/2024 19:30, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 29-Oct-24 3:07 PM, Ilpo Järvinen wrote:
>>>>>> Hi Hens,
>>>>>>
>>>>>> There a question / item needing your input below.
>>>>>>
>>>>>> On Wed, 23 Oct 2024, Mario Limonciello wrote:
>>>>>>> On 10/23/2024 10:52, Shyam Sundar S K wrote:
>>>>>>>> On 10/23/2024 21:10, Mario Limonciello wrote:
>>>>>>>>> On 10/23/2024 10:32, Shyam Sundar S K wrote:
>>>>>>>>>> On 10/23/2024 20:04, Mario Limonciello wrote:
>>>>>>>>>>> On 10/23/2024 09:29, Shyam Sundar S K wrote:
>>>>>>>>>>>> On 10/23/2024 19:41, Mario Limonciello wrote:
>>>>>>>>>>>>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
>>>>>>>>>>>>>> The PMF driver will allocate shared buffer memory using the
>>>>>>>>>>>>>> tee_shm_alloc_kernel_buf(). This allocated memory is
>>>>>>>>>>>>>> located in
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>> secure world and is used for communication with the PMF-TA.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The latest PMF-TA version introduces new structures with OEM
>>>>>>>>>>>>>> debug
>>>>>>>>>>>>>> information and additional policy input conditions for
>>>>>>>>>>>>>> evaluating the
>>>>>>>>>>>>>> policy binary. Consequently, the shared memory size must be
>>>>>>>>>>>>>> increased to
>>>>>>>>>>>>>> ensure compatibility between the PMF driver and the updated
>>>>>>>>>>>>>> PMF-TA.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>>>>>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>>>>>>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>> How does this present to a user?  From what you describe it
>>>>>>>>>>>>> seems
>>>>>>>>>>>>> to
>>>>>>>>>>>>> me like this means a new TA will fail on older kernel in
>>>>>>>>>>>>> some way.
>>>>>>>>>>>>
>>>>>>>>>>>> Newer TA will not fail on older systems. This change is just
>>>>>>>>>>>> about
>>>>>>>>>>>> the
>>>>>>>>>>>> increase in TA reserved memory that is presented as "shared
>>>>>>>>>>>> memory",
>>>>>>>>>>>> as TA needs the additional memory for its own debug data
>>>>>>>>>>>> structures.
>>>>>>>>>>>
>>>>>>>>>>> Thx for comments. But so if you use new TA with older kernel
>>>>>>>>>>> driver,
>>>>>>>>>>> what will happen?  Can TA do a buffer overrun because the
>>>>>>>>>>> presented
>>>>>>>>>>> shared memory was too small?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> New TA will fail on older kernel and hence this change will be
>>>>>>>>>> required for new TA to work.
>>>>>>>>>
>>>>>>>>> OK, that's what I was worried about.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>      From user standpoint, always be on latest FW,
>>>>>>>>>>>> irrespective of the
>>>>>>>>>>>> platform. At this point in time, I don't see a need for FW
>>>>>>>>>>>> versioning
>>>>>>>>>>>> name (in the future, if there is a need for having a limited
>>>>>>>>>>>> support
>>>>>>>>>>>> to older platforms, we can carve out a logic to do versioning
>>>>>>>>>>>> stuff).
>>>>>>>>>>>
>>>>>>>>>>> I wish we could enforce this, but In the Linux world there is an
>>>>>>>>>>> expectation that these two trains don't need to arrive at
>>>>>>>>>>> station at
>>>>>>>>>>> the same time.
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> Some ideas:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1) Should there be header version check on the TA and
>>>>>>>>>>>>> dynamically
>>>>>>>>>>>>> allocate the structure size based on the version of the F/W?
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> This can be done, when the TA versioning upgrade happens,
>>>>>>>>>>>> like from
>>>>>>>>>>>> 1.3 to 1.4, apart from that there is no header stuff
>>>>>>>>>>>> association.
>>>>>>>>>>>>
>>>>>>>>>>>>> 2) Or is there a command to the TA that can query the expected
>>>>>>>>>>>>> output
>>>>>>>>>>>>> size?
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> No, this is just the initial shared memory that the driver
>>>>>>>>>>>> allocates
>>>>>>>>>>>> to pass the inputs and the commands to TA.
>>>>>>>>>>>>
>>>>>>>>>>>>> 3) Or should the new TA filename be versioned, and the
>>>>>>>>>>>>> driver has
>>>>>>>>>>>>> a
>>>>>>>>>>>>> fallback policy?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Whatever the outcome is; I think it's best that if possible
>>>>>>>>>>>>> this
>>>>>>>>>>>>> change goes back to stable to try to minimize regressions to
>>>>>>>>>>>>> users as
>>>>>>>>>>>>> distros update linux-firmware.  For example Fedora updates
>>>>>>>>>>>>> this
>>>>>>>>>>>>> monthly, but also tracks stable kernels.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Advisory to distros should be to pick the latest PMF TA
>>>>>>>>>>>> (note that,
>>>>>>>>>>>> I
>>>>>>>>>>>> have not still submitted to new TA FW).
>>>>>>>>>>>
>>>>>>>>>>> Yeah we can advise distros to pick it up when upstreamed as
>>>>>>>>>>> long as
>>>>>>>>>>> there isn't tight dependency on this patch being present.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> That is the reason I am waiting for this change to land. Once
>>>>>>>>>> that is
>>>>>>>>>> done, I will submit the new TA, you can send out a advisory to
>>>>>>>>>> upgrade
>>>>>>>>>> the kernel or this change has to be back-ported to stable/oem
>>>>>>>>>> kernels
>>>>>>>>>> for their enablement.
>>>>>>>>>>
>>>>>>>>>> Makes sense?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I think we need Hans' and Ilpo's comments here to decide what
>>>>>>>>> to do.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Sure.
>>>>>>>>
>>>>>>>>> I will say that when we had this happen in amdgpu for a breaking
>>>>>>>>> reason there was a new firmware binary filename
>>>>>>>>> created/upstreamed for
>>>>>>>>> the breaking version (IIRC foo.bin -> foo_1.bin) and amdgpu had to
>>>>>>>>> have fallback code so it could be compatible with either binary.
>>>>>>>>>
>>>>>>>>
>>>>>>>> True. In case of amdgpu, the FW loading is part of the amdgpu
>>>>>>>> driver.
>>>>>>>> But in case of PMF, the PMF TA gets picked from the AMD TEE driver
>>>>>>>> through the TEE commands.
>>>>>>>>
>>>>>>>> So, there is no need for FW versioning logic in PMF driver.
>>>>>>>>
>>>>>>>
>>>>>>> That's a very good point, and this is a lot of complexity then.
>>>>>>>
>>>>>>>>
>>>>>>>>> * If user on older kernel took newer linux-firmware package
>>>>>>>>> they used
>>>>>>>>> older binary.
>>>>>>>>> * If user on newer kernel took older linux-firmware package
>>>>>>>>> they used
>>>>>>>>> older binary.
>>>>>>>>> * If user on newer kernel took newer linux-firmware package
>>>>>>>>> they used
>>>>>>>>> newer binary.
>>>>>>>>>
>>>>>>>>> If the decision is this goes in "as is" it definitely needs to
>>>>>>>>> go back
>>>>>>>>> to stable kernels.
>>>>>>>>>
>>>>>>>>
>>>>>>>> IMHO, let's not put too many fallback mechanisms. The philosophy
>>>>>>>> should be use latest driver and latest FW that avoids a lot of
>>>>>>>> confusion and yeah for that to happen this change has to go to
>>>>>>>> stable.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Shyam
>>>>>>>
>>>>>>> Of course Hans and Ilpo make the final call, but I think from our
>>>>>>> discussions
>>>>>>> here it would be ideal that patch 1 and patch 5 from this series
>>>>>>> go into 6.12
>>>>>>> and have stable tags, the rest would be 6.13 material.
>>>>>>
>>>>>> Distros and SW component management challenges are more in the
>>>>>> domain of
>>>>>> Hans' expertise so I'd prefer to hear his opinion on this.
>>>>>>
>>>>>> Personally I feel though that the commit message is not entirely
>>>>>> honest
>>>>>> on all the impact as is. The wordings are sounding quite innocent
>>>>>> while if
>>>>>> I infer the above right, an incorrect combination will cause a
>>>>>> non-gracious failure.
>>>>>
>>>>> There are basically 4 possible scenarios and to me it
>>>>> is only clear from this thread what will happen in 3 of
>>>>> the 4 scenarios :
>>>>>
>>>>> 1. Old TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> works
>>>>> 2. New TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> broken
>>>>> 3. Old TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> ???
>>>>> 4. New TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> works
>>>>>
>>>>> If the answer to 3 is: "works" then I agree that this patch
>>>>> should be submitted to Linus as a fix with Cc: stable ASAP
>>>>> and then once that has hit most stable series it should be
>>>>> ok to upgrade the fw in linux-firmware
>>>>>
>>>>
>>>> Short answer, "yes" it does not work for "3." and you can consider it
>>>> a broken.
>>>>
>>>>> Note this is still not ideal but IMHO it would be ok.
>>>>>
>>>>> But if the answer is "broken" then we will really need to
>>>>> find some way to unbreak this, which could be as simple
>>>>> as querying the fw-version and basing the size on this,
>>>>> but having a kernel change which will regress things for
>>>>> users who do not have the old firmware yet is simply
>>>>> not acceptable.
>>>>>
>>>>
>>>> I am not sure if there is a firmware versioning interface that the ASP
>>>> (AMD Security Processor) returns back the kernel/driver.
>>>   > The code path in this case is:
>>>>
>>>> AMD PMF driver -> AMD TEE driver -> AMD CCP driver -> ASP TEE -> ASP
>>>> TA -> ASP HW.
>>>>
>>>> So, I uncertain which module has this information and where exactly
>>>> the code of fw versioning has to reside. It will take a while for me
>>>> to dig this in.
>>>
>>> As a solution to this, can amd-pmf explicitly do it's own
>>> request_firmware() call to load the firmware binary and determine the
>>> size to use in the array and then discard the loaded binary?
>>>
>>> This would let the TEE module still do it's own load later like normal
>>> without having to plumb this information across subsystems.
>>>
>>
>> TEE driver feeds in a lot of metadata and the structure information
>> for PSP headers and I don't think just having a request_firmware()
>> will help.
>>
>> Sidebar, TEE driver has a lot of plumbing that can be used decrypt the
>> policy binaries to debug issues related to TA load failures and policy
>> binary issues (basically the descriptors)
>>
>> So we might end up in replicating a majority of TEE code into PMF
>> driver, which might not be a good design choice.
>>
>> Let me first talk to internal folks to see if we can solve it by not
>> making complex changes.
>>
> 
> OK, I think for the purpose of 6.13 then this series minus the last patch probably makes sense.
> 
> I also think the first patch should ideally come in 6.12 if Hans is OK with that.  As 6.12 is probably going to be the next LTS we'll want hardware supported as widely as possible within stable rules.

Yes I have already tagged the first patch in patchwork for picking it up
for 6.12 .

Regards,

Hans

Re: [PATCH v3 5/5] platform/x86/amd/pmf: Add PMF driver changes to make compatible with PMF-TA

[Ilpo Järvinen]

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

On Wed, 30 Oct 2024, Shyam Sundar S K wrote:
> On 10/30/2024 19:30, Hans de Goede wrote:
> > On 29-Oct-24 3:07 PM, Ilpo Järvinen wrote:
> >> Hi Hens,
> >>
> >> There a question / item needing your input below.
> >>
> >> On Wed, 23 Oct 2024, Mario Limonciello wrote:
> >>> On 10/23/2024 10:52, Shyam Sundar S K wrote:
> >>>> On 10/23/2024 21:10, Mario Limonciello wrote:
> >>>>> On 10/23/2024 10:32, Shyam Sundar S K wrote:
> >>>>>> On 10/23/2024 20:04, Mario Limonciello wrote:
> >>>>>>> On 10/23/2024 09:29, Shyam Sundar S K wrote:
> >>>>>>>> On 10/23/2024 19:41, Mario Limonciello wrote:
> >>>>>>>>> On 10/23/2024 01:32, Shyam Sundar S K wrote:
> >>>>>>>>>> The PMF driver will allocate shared buffer memory using the
> >>>>>>>>>> tee_shm_alloc_kernel_buf(). This allocated memory is located in
> >>>>>>>>>> the
> >>>>>>>>>> secure world and is used for communication with the PMF-TA.
> >>>>>>>>>>
> >>>>>>>>>> The latest PMF-TA version introduces new structures with OEM
> >>>>>>>>>> debug
> >>>>>>>>>> information and additional policy input conditions for
> >>>>>>>>>> evaluating the
> >>>>>>>>>> policy binary. Consequently, the shared memory size must be
> >>>>>>>>>> increased to
> >>>>>>>>>> ensure compatibility between the PMF driver and the updated
> >>>>>>>>>> PMF-TA.
> >>>>>>>>>>
> >>>>>>>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> >>>>>>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> >>>>>>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >>>>>>>>>
> >>>>>>>>> How does this present to a user?  From what you describe it seems
> >>>>>>>>> to
> >>>>>>>>> me like this means a new TA will fail on older kernel in some way.
> >>>>>>>>
> >>>>>>>> Newer TA will not fail on older systems. This change is just about
> >>>>>>>> the
> >>>>>>>> increase in TA reserved memory that is presented as "shared memory",
> >>>>>>>> as TA needs the additional memory for its own debug data structures.
> >>>>>>>
> >>>>>>> Thx for comments. But so if you use new TA with older kernel driver,
> >>>>>>> what will happen?  Can TA do a buffer overrun because the presented
> >>>>>>> shared memory was too small?
> >>>>>>>
> >>>>>>
> >>>>>> New TA will fail on older kernel and hence this change will be
> >>>>>> required for new TA to work.
> >>>>>
> >>>>> OK, that's what I was worried about.
> >>>>>
> >>>>>>
> >>>>>>>>
> >>>>>>>>    From user standpoint, always be on latest FW, irrespective of the
> >>>>>>>> platform. At this point in time, I don't see a need for FW
> >>>>>>>> versioning
> >>>>>>>> name (in the future, if there is a need for having a limited support
> >>>>>>>> to older platforms, we can carve out a logic to do versioning
> >>>>>>>> stuff).
> >>>>>>>
> >>>>>>> I wish we could enforce this, but In the Linux world there is an
> >>>>>>> expectation that these two trains don't need to arrive at station at
> >>>>>>> the same time.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>> Some ideas:
> >>>>>>>>>
> >>>>>>>>> 1) Should there be header version check on the TA and dynamically
> >>>>>>>>> allocate the structure size based on the version of the F/W?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> This can be done, when the TA versioning upgrade happens, like from
> >>>>>>>> 1.3 to 1.4, apart from that there is no header stuff association.
> >>>>>>>>
> >>>>>>>>> 2) Or is there a command to the TA that can query the expected
> >>>>>>>>> output
> >>>>>>>>> size?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> No, this is just the initial shared memory that the driver allocates
> >>>>>>>> to pass the inputs and the commands to TA.
> >>>>>>>>
> >>>>>>>>> 3) Or should the new TA filename be versioned, and the driver has
> >>>>>>>>> a
> >>>>>>>>> fallback policy?
> >>>>>>>>>
> >>>>>>>>> Whatever the outcome is; I think it's best that if possible this
> >>>>>>>>> change goes back to stable to try to minimize regressions to
> >>>>>>>>> users as
> >>>>>>>>> distros update linux-firmware.  For example Fedora updates this
> >>>>>>>>> monthly, but also tracks stable kernels.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Advisory to distros should be to pick the latest PMF TA (note that,
> >>>>>>>> I
> >>>>>>>> have not still submitted to new TA FW).
> >>>>>>>
> >>>>>>> Yeah we can advise distros to pick it up when upstreamed as long as
> >>>>>>> there isn't tight dependency on this patch being present.
> >>>>>>>
> >>>>>>
> >>>>>> That is the reason I am waiting for this change to land. Once that is
> >>>>>> done, I will submit the new TA, you can send out a advisory to upgrade
> >>>>>> the kernel or this change has to be back-ported to stable/oem kernels
> >>>>>> for their enablement.
> >>>>>>
> >>>>>> Makes sense?
> >>>>>>
> >>>>>
> >>>>> I think we need Hans' and Ilpo's comments here to decide what to do.
> >>>>>
> >>>>
> >>>> Sure.
> >>>>
> >>>>> I will say that when we had this happen in amdgpu for a breaking
> >>>>> reason there was a new firmware binary filename created/upstreamed for
> >>>>> the breaking version (IIRC foo.bin -> foo_1.bin) and amdgpu had to
> >>>>> have fallback code so it could be compatible with either binary.
> >>>>>
> >>>>
> >>>> True. In case of amdgpu, the FW loading is part of the amdgpu driver.
> >>>> But in case of PMF, the PMF TA gets picked from the AMD TEE driver
> >>>> through the TEE commands.
> >>>>
> >>>> So, there is no need for FW versioning logic in PMF driver.
> >>>>
> >>>
> >>> That's a very good point, and this is a lot of complexity then.
> >>>
> >>>>
> >>>>> * If user on older kernel took newer linux-firmware package they used
> >>>>> older binary.
> >>>>> * If user on newer kernel took older linux-firmware package they used
> >>>>> older binary.
> >>>>> * If user on newer kernel took newer linux-firmware package they used
> >>>>> newer binary.
> >>>>>
> >>>>> If the decision is this goes in "as is" it definitely needs to go back
> >>>>> to stable kernels.
> >>>>>
> >>>>
> >>>> IMHO, let's not put too many fallback mechanisms. The philosophy
> >>>> should be use latest driver and latest FW that avoids a lot of
> >>>> confusion and yeah for that to happen this change has to go to stable.
> >>>>
> >>>> Thanks,
> >>>> Shyam
> >>>
> >>> Of course Hans and Ilpo make the final call, but I think from our discussions
> >>> here it would be ideal that patch 1 and patch 5 from this series go into 6.12
> >>> and have stable tags, the rest would be 6.13 material.
> >>
> >> Distros and SW component management challenges are more in the domain of 
> >> Hans' expertise so I'd prefer to hear his opinion on this.
> >>
> >> Personally I feel though that the commit message is not entirely honest 
> >> on all the impact as is. The wordings are sounding quite innocent while if 
> >> I infer the above right, an incorrect combination will cause a 
> >> non-gracious failure.
> > 
> > There are basically 4 possible scenarios and to me it
> > is only clear from this thread what will happen in 3 of
> > the 4 scenarios :
> > 
> > 1. Old TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> works
> > 2. New TA fw, Old kernel (TA_OUTPUT_RESERVED_MEM=906) -> broken
> > 3. Old TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> ???
> > 4. New TA fw, new kernel (TA_OUTPUT_RESERVED_MEM=922) -> works
> > 
> > If the answer to 3 is: "works" then I agree that this patch
> > should be submitted to Linus as a fix with Cc: stable ASAP
> > and then once that has hit most stable series it should be
> > ok to upgrade the fw in linux-firmware
> > 
> 
> Short answer, "yes" it does not work for "3." and you can consider it
> a broken.
> 
> > Note this is still not ideal but IMHO it would be ok.
> > 
> > But if the answer is "broken" then we will really need to
> > find some way to unbreak this, which could be as simple
> > as querying the fw-version and basing the size on this,
> > but having a kernel change which will regress things for
> > users who do not have the old firmware yet is simply
> > not acceptable.
> > 
> 
> I am not sure if there is a firmware versioning interface that the ASP
> (AMD Security Processor) returns back the kernel/driver.
> 
> The code path in this case is:
> 
> AMD PMF driver -> AMD TEE driver -> AMD CCP driver -> ASP TEE -> ASP
> TA -> ASP HW.
> 
> So, I uncertain which module has this information and where exactly
> the code of fw versioning has to reside. It will take a while for me
> to dig this in.
> 
> Meanwhile, shall I drop this patch and resend the series (by
> addressing the dev_dbg change Mario commented) so that this atleast
> becomes a 6.13 material?

Yes, please do send a new version which has the comment addressed (I think 
Hans can pick the first patch out of this version so you don't need to 
include that one).

Also, please remember add a lore Link: to this thread into the patch 5/5 
when you send it later in some form.

-- 
 i.