diff mbox series

[RFC,v3,1/2] mmc: core: Add vendor hook to control reprogram keys to Crypto Engine

Message ID 20241006135530.17363-2-quic_spuppala@quicinc.com (mailing list archive)
State New
Headers show
Series Avoid reprogram all keys to Inline Crypto Engine for MMC runtime suspend resume | expand

Commit Message

Seshu Madhavi Puppala Oct. 6, 2024, 1:55 p.m. UTC 
Add mmc_host_ops hook avoid_reprogram_allkeys to control
reprogramming keys to Inline Crypto Engine by vendor as some
vendors might not require this feature.

Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
---
 drivers/mmc/core/crypto.c | 8 +++++---
 drivers/mmc/host/sdhci.c  | 6 ++++++
 include/linux/mmc/host.h  | 7 +++++++
 3 files changed, 18 insertions(+), 3 deletions(-)

Comments

Ulf Hansson Oct. 8, 2024, 2 p.m. UTC | #1 
On Sun, 6 Oct 2024 at 15:55, Seshu Madhavi Puppala
<quic_spuppala@quicinc.com> wrote:
>
> Add mmc_host_ops hook avoid_reprogram_allkeys to control
> reprogramming keys to Inline Crypto Engine by vendor as some
> vendors might not require this feature.
>
> Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
> Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> ---
>  drivers/mmc/core/crypto.c | 8 +++++---
>  drivers/mmc/host/sdhci.c  | 6 ++++++
>  include/linux/mmc/host.h  | 7 +++++++
>  3 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
> index fec4fbf16a5b..4168f7d135ff 100644
> --- a/drivers/mmc/core/crypto.c
> +++ b/drivers/mmc/core/crypto.c
> @@ -14,9 +14,11 @@
>
>  void mmc_crypto_set_initial_state(struct mmc_host *host)
>  {
> -       /* Reset might clear all keys, so reprogram all the keys. */
> -       if (host->caps2 & MMC_CAP2_CRYPTO)
> -               blk_crypto_reprogram_all_keys(&host->crypto_profile);
> +       if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
> +               /* Reset might clear all keys, so reprogram all the keys. */
> +               if (host->caps2 & MMC_CAP2_CRYPTO)
> +                       blk_crypto_reprogram_all_keys(&host->crypto_profile);

Don't you even need to call this once, during the first initialization
of the card?

> +       }
>  }
>

[...]

Kind regards
Uffe
Seshu Madhavi Puppala Oct. 19, 2024, 4:55 a.m. UTC | #2 
On 10/8/2024 7:30 PM, Ulf Hansson wrote:
> On Sun, 6 Oct 2024 at 15:55, Seshu Madhavi Puppala
> <quic_spuppala@quicinc.com> wrote:
>>
>> Add mmc_host_ops hook avoid_reprogram_allkeys to control
>> reprogramming keys to Inline Crypto Engine by vendor as some
>> vendors might not require this feature.
>>
>> Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
>> Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>> ---
>>   drivers/mmc/core/crypto.c | 8 +++++---
>>   drivers/mmc/host/sdhci.c  | 6 ++++++
>>   include/linux/mmc/host.h  | 7 +++++++
>>   3 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
>> index fec4fbf16a5b..4168f7d135ff 100644
>> --- a/drivers/mmc/core/crypto.c
>> +++ b/drivers/mmc/core/crypto.c
>> @@ -14,9 +14,11 @@
>>
>>   void mmc_crypto_set_initial_state(struct mmc_host *host)
>>   {
>> -       /* Reset might clear all keys, so reprogram all the keys. */
>> -       if (host->caps2 & MMC_CAP2_CRYPTO)
>> -               blk_crypto_reprogram_all_keys(&host->crypto_profile);
>> +       if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
>> +               /* Reset might clear all keys, so reprogram all the keys. */
>> +               if (host->caps2 & MMC_CAP2_CRYPTO)
>> +                       blk_crypto_reprogram_all_keys(&host->crypto_profile);
> 
> Don't you even need to call this once, during the first initialization
> of the card?

The first card initialization is done during the boot up for qcom socs 
and the kernel keyring contains no keys immediately after bootup.After 
the initialization of the card, the block i/o operations to encrypted 
folders will automatically trigger the corresponding program key calls 
to the crypto engine since the kernel keyring does not contain the 
required encryption key. So, it is not necessary to explicitly reprogram 
all keys for qcom socs.
> 
>> +       }
>>   }
>>
> 
> [...]
> 
> Kind regards
> Uffe
Ulf Hansson Oct. 22, 2024, 12:27 p.m. UTC | #3 
+ Eric, Abel

On Sat, 19 Oct 2024 at 06:55, Seshu Madhavi Puppala
<quic_spuppala@quicinc.com> wrote:
>
>
>
> On 10/8/2024 7:30 PM, Ulf Hansson wrote:
> > On Sun, 6 Oct 2024 at 15:55, Seshu Madhavi Puppala
> > <quic_spuppala@quicinc.com> wrote:
> >>
> >> Add mmc_host_ops hook avoid_reprogram_allkeys to control
> >> reprogramming keys to Inline Crypto Engine by vendor as some
> >> vendors might not require this feature.
> >>
> >> Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
> >> Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> >> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> >> ---
> >>   drivers/mmc/core/crypto.c | 8 +++++---
> >>   drivers/mmc/host/sdhci.c  | 6 ++++++
> >>   include/linux/mmc/host.h  | 7 +++++++
> >>   3 files changed, 18 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
> >> index fec4fbf16a5b..4168f7d135ff 100644
> >> --- a/drivers/mmc/core/crypto.c
> >> +++ b/drivers/mmc/core/crypto.c
> >> @@ -14,9 +14,11 @@
> >>
> >>   void mmc_crypto_set_initial_state(struct mmc_host *host)
> >>   {
> >> -       /* Reset might clear all keys, so reprogram all the keys. */
> >> -       if (host->caps2 & MMC_CAP2_CRYPTO)
> >> -               blk_crypto_reprogram_all_keys(&host->crypto_profile);
> >> +       if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
> >> +               /* Reset might clear all keys, so reprogram all the keys. */
> >> +               if (host->caps2 & MMC_CAP2_CRYPTO)
> >> +                       blk_crypto_reprogram_all_keys(&host->crypto_profile);
> >
> > Don't you even need to call this once, during the first initialization
> > of the card?
>
> The first card initialization is done during the boot up for qcom socs
> and the kernel keyring contains no keys immediately after bootup.After
> the initialization of the card, the block i/o operations to encrypted
> folders will automatically trigger the corresponding program key calls
> to the crypto engine since the kernel keyring does not contain the
> required encryption key. So, it is not necessary to explicitly reprogram
> all keys for qcom socs.

Okay, I see. I have looped in Abel and Eric who worked on this
feature, just to confirm that this makes sense for them too.

I assume the reason why you want to avoid the re-programming is that
it adds latency when re-initializing the card, right? In that case, do
you have some numbers of what we save by doing this?

Kind regards
Uffe
Adrian Hunter Oct. 23, 2024, 8:59 a.m. UTC | #4 
On 6/10/24 16:55, Seshu Madhavi Puppala wrote:
> Add mmc_host_ops hook avoid_reprogram_allkeys to control
> reprogramming keys to Inline Crypto Engine by vendor as some
> vendors might not require this feature.
> 
> Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
> Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> ---
>  drivers/mmc/core/crypto.c | 8 +++++---
>  drivers/mmc/host/sdhci.c  | 6 ++++++
>  include/linux/mmc/host.h  | 7 +++++++
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
> index fec4fbf16a5b..4168f7d135ff 100644
> --- a/drivers/mmc/core/crypto.c
> +++ b/drivers/mmc/core/crypto.c
> @@ -14,9 +14,11 @@
>  
>  void mmc_crypto_set_initial_state(struct mmc_host *host)
>  {
> -	/* Reset might clear all keys, so reprogram all the keys. */
> -	if (host->caps2 & MMC_CAP2_CRYPTO)
> -		blk_crypto_reprogram_all_keys(&host->crypto_profile);
> +	if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
> +		/* Reset might clear all keys, so reprogram all the keys. */
> +		if (host->caps2 & MMC_CAP2_CRYPTO)
> +			blk_crypto_reprogram_all_keys(&host->crypto_profile);
> +	}

Probably nicer to put MMC_CAP2_CRYPTO check first, but also the logic
needs a tweak:

	/* Reset might clear all keys, so reprogram all the keys. */
	if (host->caps2 & MMC_CAP2_CRYPTO &&
	    (!host->ops->avoid_reprogram_allkeys ||
	     !host->ops->avoid_reprogram_allkeys()))
		blk_crypto_reprogram_all_keys(&host->crypto_profile);

>  }
>  
>  void mmc_crypto_setup_queue(struct request_queue *q, struct mmc_host *host)
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index fbf7a91bed35..cd663899c025 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2704,6 +2704,11 @@ int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>  }
>  EXPORT_SYMBOL_GPL(sdhci_start_signal_voltage_switch);
>  
> +static bool sdhci_avoid_reprogram_allkeys(void)
> +{
> +	return false;
> +}
> +
>  static int sdhci_card_busy(struct mmc_host *mmc)
>  {
>  	struct sdhci_host *host = mmc_priv(mmc);
> @@ -3066,6 +3071,7 @@ static const struct mmc_host_ops sdhci_ops = {
>  	.execute_tuning			= sdhci_execute_tuning,
>  	.card_event			= sdhci_card_event,
>  	.card_busy	= sdhci_card_busy,
> +	.avoid_reprogram_allkeys	= sdhci_avoid_reprogram_allkeys,

There isn't any need for this

>  };
>  
>  /*****************************************************************************\
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 88c6a76042ee..c4109d17f177 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -218,6 +218,13 @@ struct mmc_host_ops {
>  
>  	/* Initialize an SD express card, mandatory for MMC_CAP2_SD_EXP. */
>  	int	(*init_sd_express)(struct mmc_host *host, struct mmc_ios *ios);
> +
> +	/*
> +	 * Optional callback to support controllers that dont require to
> +	 * reprogram all crypto keys on card suspend/resume.
> +	 */
> +	bool	(*avoid_reprogram_allkeys)(void);
> +
>  };
>  
>  struct mmc_cqe_ops {
Eric Biggers Oct. 23, 2024, 9:28 p.m. UTC | #5 
On Sun, Oct 06, 2024 at 07:25:29PM +0530, Seshu Madhavi Puppala wrote:
> Add mmc_host_ops hook avoid_reprogram_allkeys to control
> reprogramming keys to Inline Crypto Engine by vendor as some
> vendors might not require this feature.
> 
> Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
> Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> ---
>  drivers/mmc/core/crypto.c | 8 +++++---
>  drivers/mmc/host/sdhci.c  | 6 ++++++
>  include/linux/mmc/host.h  | 7 +++++++
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
> index fec4fbf16a5b..4168f7d135ff 100644
> --- a/drivers/mmc/core/crypto.c
> +++ b/drivers/mmc/core/crypto.c
> @@ -14,9 +14,11 @@
>  
>  void mmc_crypto_set_initial_state(struct mmc_host *host)
>  {
> -	/* Reset might clear all keys, so reprogram all the keys. */
> -	if (host->caps2 & MMC_CAP2_CRYPTO)
> -		blk_crypto_reprogram_all_keys(&host->crypto_profile);
> +	if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
> +		/* Reset might clear all keys, so reprogram all the keys. */
> +		if (host->caps2 & MMC_CAP2_CRYPTO)
> +			blk_crypto_reprogram_all_keys(&host->crypto_profile);
> +	}

This should be a simple flag, not an indirect function call which is
inefficient.

It could be a bit in mmc_host_ops, though based on the existing code maybe a new
bit in MMC_CAP2_* would be more appropriate.

- Eric
Ulf Hansson Oct. 24, 2024, 10:47 p.m. UTC | #6 
On Wed, 23 Oct 2024 at 23:28, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Sun, Oct 06, 2024 at 07:25:29PM +0530, Seshu Madhavi Puppala wrote:
> > Add mmc_host_ops hook avoid_reprogram_allkeys to control
> > reprogramming keys to Inline Crypto Engine by vendor as some
> > vendors might not require this feature.
> >
> > Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
> > Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> > Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> > ---
> >  drivers/mmc/core/crypto.c | 8 +++++---
> >  drivers/mmc/host/sdhci.c  | 6 ++++++
> >  include/linux/mmc/host.h  | 7 +++++++
> >  3 files changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
> > index fec4fbf16a5b..4168f7d135ff 100644
> > --- a/drivers/mmc/core/crypto.c
> > +++ b/drivers/mmc/core/crypto.c
> > @@ -14,9 +14,11 @@
> >
> >  void mmc_crypto_set_initial_state(struct mmc_host *host)
> >  {
> > -     /* Reset might clear all keys, so reprogram all the keys. */
> > -     if (host->caps2 & MMC_CAP2_CRYPTO)
> > -             blk_crypto_reprogram_all_keys(&host->crypto_profile);
> > +     if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
> > +             /* Reset might clear all keys, so reprogram all the keys. */
> > +             if (host->caps2 & MMC_CAP2_CRYPTO)
> > +                     blk_crypto_reprogram_all_keys(&host->crypto_profile);
> > +     }
>
> This should be a simple flag, not an indirect function call which is
> inefficient.
>
> It could be a bit in mmc_host_ops, though based on the existing code maybe a new
> bit in MMC_CAP2_* would be more appropriate.

Thanks for commenting Eric. From a principle point of view, it sounds
like this makes sense to you too.

When it comes to the implementation details, I agree with the above.
Perhaps MMC_CAP2_CRYPTO_NO_PROG, or something along those lines would
be better. Unless the callback is intended to dynamically allow the
decision to be changed, but it doesn't look like that in subsequent
patches.

Kind regards
Uffe
Seshu Madhavi Puppala Nov. 21, 2024, 5:16 a.m. UTC | #7 
On 10/24/2024 2:58 AM, Eric Biggers wrote:
> On Sun, Oct 06, 2024 at 07:25:29PM +0530, Seshu Madhavi Puppala wrote:
>> Add mmc_host_ops hook avoid_reprogram_allkeys to control
>> reprogramming keys to Inline Crypto Engine by vendor as some
>> vendors might not require this feature.
>>
>> Signed-off-by: Seshu Madhavi Puppala <quic_spuppala@quicinc.com>
>> Co-developed-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>> ---
>>   drivers/mmc/core/crypto.c | 8 +++++---
>>   drivers/mmc/host/sdhci.c  | 6 ++++++
>>   include/linux/mmc/host.h  | 7 +++++++
>>   3 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
>> index fec4fbf16a5b..4168f7d135ff 100644
>> --- a/drivers/mmc/core/crypto.c
>> +++ b/drivers/mmc/core/crypto.c
>> @@ -14,9 +14,11 @@
>>   
>>   void mmc_crypto_set_initial_state(struct mmc_host *host)
>>   {
>> -	/* Reset might clear all keys, so reprogram all the keys. */
>> -	if (host->caps2 & MMC_CAP2_CRYPTO)
>> -		blk_crypto_reprogram_all_keys(&host->crypto_profile);
>> +	if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
>> +		/* Reset might clear all keys, so reprogram all the keys. */
>> +		if (host->caps2 & MMC_CAP2_CRYPTO)
>> +			blk_crypto_reprogram_all_keys(&host->crypto_profile);
>> +	}
> 
> This should be a simple flag, not an indirect function call which is
> inefficient.
> 
> It could be a bit in mmc_host_ops, though based on the existing code maybe a new
> bit in MMC_CAP2_* would be more appropriate.

I will update this in v2 patchset by adding the flag as part of host->caps2

Thanks,
Seshu
> 
> - Eric
diff mbox series

Patch

diff --git a/drivers/mmc/core/crypto.c b/drivers/mmc/core/crypto.c
index fec4fbf16a5b..4168f7d135ff 100644
--- a/drivers/mmc/core/crypto.c
+++ b/drivers/mmc/core/crypto.c
@@ -14,9 +14,11 @@ 
 
 void mmc_crypto_set_initial_state(struct mmc_host *host)
 {
-	/* Reset might clear all keys, so reprogram all the keys. */
-	if (host->caps2 & MMC_CAP2_CRYPTO)
-		blk_crypto_reprogram_all_keys(&host->crypto_profile);
+	if (host->ops->avoid_reprogram_allkeys && !host->ops->avoid_reprogram_allkeys()) {
+		/* Reset might clear all keys, so reprogram all the keys. */
+		if (host->caps2 & MMC_CAP2_CRYPTO)
+			blk_crypto_reprogram_all_keys(&host->crypto_profile);
+	}
 }
 
 void mmc_crypto_setup_queue(struct request_queue *q, struct mmc_host *host)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index fbf7a91bed35..cd663899c025 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2704,6 +2704,11 @@  int sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
 }
 EXPORT_SYMBOL_GPL(sdhci_start_signal_voltage_switch);
 
+static bool sdhci_avoid_reprogram_allkeys(void)
+{
+	return false;
+}
+
 static int sdhci_card_busy(struct mmc_host *mmc)
 {
 	struct sdhci_host *host = mmc_priv(mmc);
@@ -3066,6 +3071,7 @@  static const struct mmc_host_ops sdhci_ops = {
 	.execute_tuning			= sdhci_execute_tuning,
 	.card_event			= sdhci_card_event,
 	.card_busy	= sdhci_card_busy,
+	.avoid_reprogram_allkeys	= sdhci_avoid_reprogram_allkeys,
 };
 
 /*****************************************************************************\
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 88c6a76042ee..c4109d17f177 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -218,6 +218,13 @@  struct mmc_host_ops {
 
 	/* Initialize an SD express card, mandatory for MMC_CAP2_SD_EXP. */
 	int	(*init_sd_express)(struct mmc_host *host, struct mmc_ios *ios);
+
+	/*
+	 * Optional callback to support controllers that dont require to
+	 * reprogram all crypto keys on card suspend/resume.
+	 */
+	bool	(*avoid_reprogram_allkeys)(void);
+
 };
 
 struct mmc_cqe_ops {