diff mbox series

[v5,04/19] firmware/psci: Add psci_early_test_conduit()

Message ID 20240819131924.372366-5-steven.price@arm.com (mailing list archive)
State New, archived
Headers show
Series arm64: Support for running as a guest in Arm CCA | expand

Commit Message

Steven Price Aug. 19, 2024, 1:19 p.m. UTC
From: Jean-Philippe Brucker <jean-philippe@linaro.org>

Add a function to test early if PSCI is present and what conduit it
uses. Because the PSCI conduit corresponds to the SMCCC one, this will
let the kernel know whether it can use SMC instructions to discuss with
the Realm Management Monitor (RMM), early enough to enable RAM and
serial access when running in a Realm.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Signed-off-by: Steven Price <steven.price@arm.com>
---
v4: New patch
---
 drivers/firmware/psci/psci.c | 25 +++++++++++++++++++++++++
 include/linux/psci.h         |  5 +++++
 2 files changed, 30 insertions(+)

Comments

Will Deacon Aug. 23, 2024, 1:29 p.m. UTC | #1
On Mon, Aug 19, 2024 at 02:19:09PM +0100, Steven Price wrote:
> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> 
> Add a function to test early if PSCI is present and what conduit it
> uses. Because the PSCI conduit corresponds to the SMCCC one, this will
> let the kernel know whether it can use SMC instructions to discuss with
> the Realm Management Monitor (RMM), early enough to enable RAM and
> serial access when running in a Realm.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
> v4: New patch
> ---
>  drivers/firmware/psci/psci.c | 25 +++++++++++++++++++++++++
>  include/linux/psci.h         |  5 +++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index 2328ca58bba6..2b308f97ef2c 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -13,6 +13,7 @@
>  #include <linux/errno.h>
>  #include <linux/linkage.h>
>  #include <linux/of.h>
> +#include <linux/of_fdt.h>
>  #include <linux/pm.h>
>  #include <linux/printk.h>
>  #include <linux/psci.h>
> @@ -769,6 +770,30 @@ int __init psci_dt_init(void)
>  	return ret;
>  }
>  
> +/*
> + * Test early if PSCI is supported, and if its conduit matches @conduit
> + */
> +bool __init psci_early_test_conduit(enum arm_smccc_conduit conduit)
> +{
> +	int len;
> +	int psci_node;
> +	const char *method;
> +	unsigned long dt_root;
> +
> +	/* DT hasn't been unflattened yet, we have to work with the flat blob */
> +	dt_root = of_get_flat_dt_root();
> +	psci_node = of_get_flat_dt_subnode_by_name(dt_root, "psci");
> +	if (psci_node <= 0)
> +		return false;
> +
> +	method = of_get_flat_dt_prop(psci_node, "method", &len);
> +	if (!method)
> +		return false;
> +
> +	return  (conduit == SMCCC_CONDUIT_SMC && strncmp(method, "smc", len) == 0) ||
> +		(conduit == SMCCC_CONDUIT_HVC && strncmp(method, "hvc", len) == 0);
> +}

This still looks incomplete to me as per my earlier comments:

https://lore.kernel.org/all/20240709104851.GE12978@willie-the-truck/

For the first implementation, can we punt the RIPAS_RAM to the bootloader
and drop support for earlycon? Even if we manage to shoe-horn enough code
into the early boot path, I think we'll regret it later on because there's
always something that wants to be first and it inevitably ends up being
a nightmare to maintain.

Will
Catalin Marinas Aug. 26, 2024, 10:03 a.m. UTC | #2
On Mon, Aug 19, 2024 at 02:19:09PM +0100, Steven Price wrote:
> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> 
> Add a function to test early if PSCI is present and what conduit it
> uses. Because the PSCI conduit corresponds to the SMCCC one, this will
> let the kernel know whether it can use SMC instructions to discuss with
> the Realm Management Monitor (RMM), early enough to enable RAM and
> serial access when running in a Realm.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Signed-off-by: Steven Price <steven.price@arm.com>

On the code itself:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

However, Will has a point and it would be good if we can avoid this
early setup as much as possible. If it's just the early console used for
debugging, maybe just pass the full IPA address on the command line and
allow those high addresses in fixmap. Not sure about the EFI map.
Steven Price Aug. 30, 2024, 3:54 p.m. UTC | #3
On 23/08/2024 14:29, Will Deacon wrote:
> On Mon, Aug 19, 2024 at 02:19:09PM +0100, Steven Price wrote:
>> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
>>
>> Add a function to test early if PSCI is present and what conduit it
>> uses. Because the PSCI conduit corresponds to the SMCCC one, this will
>> let the kernel know whether it can use SMC instructions to discuss with
>> the Realm Management Monitor (RMM), early enough to enable RAM and
>> serial access when running in a Realm.
>>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> v4: New patch
>> ---
>>  drivers/firmware/psci/psci.c | 25 +++++++++++++++++++++++++
>>  include/linux/psci.h         |  5 +++++
>>  2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
>> index 2328ca58bba6..2b308f97ef2c 100644
>> --- a/drivers/firmware/psci/psci.c
>> +++ b/drivers/firmware/psci/psci.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/errno.h>
>>  #include <linux/linkage.h>
>>  #include <linux/of.h>
>> +#include <linux/of_fdt.h>
>>  #include <linux/pm.h>
>>  #include <linux/printk.h>
>>  #include <linux/psci.h>
>> @@ -769,6 +770,30 @@ int __init psci_dt_init(void)
>>  	return ret;
>>  }
>>  
>> +/*
>> + * Test early if PSCI is supported, and if its conduit matches @conduit
>> + */
>> +bool __init psci_early_test_conduit(enum arm_smccc_conduit conduit)
>> +{
>> +	int len;
>> +	int psci_node;
>> +	const char *method;
>> +	unsigned long dt_root;
>> +
>> +	/* DT hasn't been unflattened yet, we have to work with the flat blob */
>> +	dt_root = of_get_flat_dt_root();
>> +	psci_node = of_get_flat_dt_subnode_by_name(dt_root, "psci");
>> +	if (psci_node <= 0)
>> +		return false;
>> +
>> +	method = of_get_flat_dt_prop(psci_node, "method", &len);
>> +	if (!method)
>> +		return false;
>> +
>> +	return  (conduit == SMCCC_CONDUIT_SMC && strncmp(method, "smc", len) == 0) ||
>> +		(conduit == SMCCC_CONDUIT_HVC && strncmp(method, "hvc", len) == 0);
>> +}
> 
> This still looks incomplete to me as per my earlier comments:
> 
> https://lore.kernel.org/all/20240709104851.GE12978@willie-the-truck/
> 
> For the first implementation, can we punt the RIPAS_RAM to the bootloader
> and drop support for earlycon? Even if we manage to shoe-horn enough code
> into the early boot path, I think we'll regret it later on because there's
> always something that wants to be first and it inevitably ends up being
> a nightmare to maintain.

Short-answer: yes, although it has drawbacks.

I've never been keen on the RIPAS_RAM requirement, the logic behind it
is that it makes it easier to have varying amounts of RAM given to the
guest without affecting the attestation. But it's a weak argument and
I'd personally prefer to punt the responsibility to a bootloader/VMM.

earlycon should be fairly easy to remove - and it doesn't have to
actually kill earlycon because we can pass in the address with the top
bit set - it just requires fixing up the VMM.

EFI is the main issue.

I'll have a go at coming up with a cut down series - at the very least
I'll see if I can rearrange to have the troublesome parts at the end so
they can be dropped if necessary.

Steve
Gavin Shan Sept. 9, 2024, 11:56 p.m. UTC | #4
On 8/19/24 11:19 PM, Steven Price wrote:
> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> 
> Add a function to test early if PSCI is present and what conduit it
> uses. Because the PSCI conduit corresponds to the SMCCC one, this will
> let the kernel know whether it can use SMC instructions to discuss with
> the Realm Management Monitor (RMM), early enough to enable RAM and
> serial access when running in a Realm.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
> v4: New patch
> ---
>   drivers/firmware/psci/psci.c | 25 +++++++++++++++++++++++++
>   include/linux/psci.h         |  5 +++++
>   2 files changed, 30 insertions(+)
> 

Question: Do we need same check for ACPI based system?

> diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> index 2328ca58bba6..2b308f97ef2c 100644
> --- a/drivers/firmware/psci/psci.c
> +++ b/drivers/firmware/psci/psci.c
> @@ -13,6 +13,7 @@
>   #include <linux/errno.h>
>   #include <linux/linkage.h>
>   #include <linux/of.h>
> +#include <linux/of_fdt.h>
>   #include <linux/pm.h>
>   #include <linux/printk.h>
>   #include <linux/psci.h>
> @@ -769,6 +770,30 @@ int __init psci_dt_init(void)
>   	return ret;
>   }
>   
> +/*
> + * Test early if PSCI is supported, and if its conduit matches @conduit
> + */

Nit: The comments needn't span into multiple lines.

> +bool __init psci_early_test_conduit(enum arm_smccc_conduit conduit)
> +{
> +	int len;
> +	int psci_node;
> +	const char *method;
> +	unsigned long dt_root;
> +
> +	/* DT hasn't been unflattened yet, we have to work with the flat blob */
> +	dt_root = of_get_flat_dt_root();
> +	psci_node = of_get_flat_dt_subnode_by_name(dt_root, "psci");
> +	if (psci_node <= 0)
> +		return false;
> +

Nit: Strictly speaking, "psci_node == 0" isn't an error. So the check would be
"if (psci_node < 0)" if I'm correct.

> +	method = of_get_flat_dt_prop(psci_node, "method", &len);
> +	if (!method)
> +		return false;
> +
> +	return  (conduit == SMCCC_CONDUIT_SMC && strncmp(method, "smc", len) == 0) ||
> +		(conduit == SMCCC_CONDUIT_HVC && strncmp(method, "hvc", len) == 0);
> +}
> +
>   #ifdef CONFIG_ACPI
>   /*
>    * We use PSCI 0.2+ when ACPI is deployed on ARM64 and it's
> diff --git a/include/linux/psci.h b/include/linux/psci.h
> index 4ca0060a3fc4..a1fc1703ba20 100644
> --- a/include/linux/psci.h
> +++ b/include/linux/psci.h
> @@ -45,8 +45,13 @@ struct psci_0_1_function_ids get_psci_0_1_function_ids(void);
>   
>   #if defined(CONFIG_ARM_PSCI_FW)
>   int __init psci_dt_init(void);
> +bool __init psci_early_test_conduit(enum arm_smccc_conduit conduit);
>   #else
>   static inline int psci_dt_init(void) { return 0; }
> +static inline bool psci_early_test_conduit(enum arm_smccc_conduit conduit)
> +{
> +	return false;
> +}
>   #endif
>   
>   #if defined(CONFIG_ARM_PSCI_FW) && defined(CONFIG_ACPI)

Thanks,
Gavin
Suzuki K Poulose Sept. 13, 2024, 1:52 p.m. UTC | #5
On 26/08/2024 11:03, Catalin Marinas wrote:
> On Mon, Aug 19, 2024 at 02:19:09PM +0100, Steven Price wrote:
>> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
>>
>> Add a function to test early if PSCI is present and what conduit it
>> uses. Because the PSCI conduit corresponds to the SMCCC one, this will
>> let the kernel know whether it can use SMC instructions to discuss with
>> the Realm Management Monitor (RMM), early enough to enable RAM and
>> serial access when running in a Realm.
>>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>> Signed-off-by: Steven Price <steven.price@arm.com>
> 
> On the code itself:
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> 
> However, Will has a point and it would be good if we can avoid this
> early setup as much as possible. If it's just the early console used for
> debugging, maybe just pass the full IPA address on the command line and
> allow those high addresses in fixmap. Not sure about the EFI map.
> 

We could delay the RSI init until we have probed the PSCI conduit.
This could be done from setup_arch(), after the psci_{dt,acpi}_init().
This is safe, as the EFI maps are only created later, as an 
early_initcall().

Kind regards
Suzuki
diff mbox series

Patch

diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
index 2328ca58bba6..2b308f97ef2c 100644
--- a/drivers/firmware/psci/psci.c
+++ b/drivers/firmware/psci/psci.c
@@ -13,6 +13,7 @@ 
 #include <linux/errno.h>
 #include <linux/linkage.h>
 #include <linux/of.h>
+#include <linux/of_fdt.h>
 #include <linux/pm.h>
 #include <linux/printk.h>
 #include <linux/psci.h>
@@ -769,6 +770,30 @@  int __init psci_dt_init(void)
 	return ret;
 }
 
+/*
+ * Test early if PSCI is supported, and if its conduit matches @conduit
+ */
+bool __init psci_early_test_conduit(enum arm_smccc_conduit conduit)
+{
+	int len;
+	int psci_node;
+	const char *method;
+	unsigned long dt_root;
+
+	/* DT hasn't been unflattened yet, we have to work with the flat blob */
+	dt_root = of_get_flat_dt_root();
+	psci_node = of_get_flat_dt_subnode_by_name(dt_root, "psci");
+	if (psci_node <= 0)
+		return false;
+
+	method = of_get_flat_dt_prop(psci_node, "method", &len);
+	if (!method)
+		return false;
+
+	return  (conduit == SMCCC_CONDUIT_SMC && strncmp(method, "smc", len) == 0) ||
+		(conduit == SMCCC_CONDUIT_HVC && strncmp(method, "hvc", len) == 0);
+}
+
 #ifdef CONFIG_ACPI
 /*
  * We use PSCI 0.2+ when ACPI is deployed on ARM64 and it's
diff --git a/include/linux/psci.h b/include/linux/psci.h
index 4ca0060a3fc4..a1fc1703ba20 100644
--- a/include/linux/psci.h
+++ b/include/linux/psci.h
@@ -45,8 +45,13 @@  struct psci_0_1_function_ids get_psci_0_1_function_ids(void);
 
 #if defined(CONFIG_ARM_PSCI_FW)
 int __init psci_dt_init(void);
+bool __init psci_early_test_conduit(enum arm_smccc_conduit conduit);
 #else
 static inline int psci_dt_init(void) { return 0; }
+static inline bool psci_early_test_conduit(enum arm_smccc_conduit conduit)
+{
+	return false;
+}
 #endif
 
 #if defined(CONFIG_ARM_PSCI_FW) && defined(CONFIG_ACPI)