| Message ID | Zv19Cc-oTOzv8wVO@earth.li (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] tpm: Workaround failed command reception on Infineon devices | expand |
On 10/2/24 1:04 PM, Jonathan McDowell wrote: > (I'm still in the process of testing this to confirm it fixes the > errata I've seen, but I wanted to send it out for comments to make sure > it's a reasonable approach.) > > Some Infineon devices have a issue where the status register will get > stuck with a quick REQUEST_USE / COMMAND_READY sequence. The work around Did you tell Infineon about it? Maybe they should have a look at their firmware. What are the TPMs in your fleet doing? I heared that some TPMs pre-create keys in the background once users requested a few. I would try to create a few primary keys with different combination of key flags set and in different hierarchies (and flush them) to use up these precreated keys and see whether that leads to any issues with the TIS responsiveness once presumably the device starts creating keys in the background... > is to retry the command submission. Add appropriate logic to do this in I would describe it as 'retrying to set the TPM_STS_COMMAND_READY flag on the TIS STS register' because you are not retring a (TPM) command submission, like resubmitting a TPM2_PCR_Extend for example. > the send path. > > Signed-off-by: Jonathan McDowell <noodles@meta.com> > --- > drivers/char/tpm/tpm_tis_core.c | 24 +++++++++++++++++++----- > drivers/char/tpm/tpm_tis_core.h | 1 + > include/linux/tpm.h | 1 + > 3 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index f6aa0dfadb93..940abd1a868e 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -432,16 +432,27 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count) > static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > - int rc, status, burstcnt; > + int rc, status, burstcnt, retry; > + bool status_fix = test_bit(TPM_TIS_STATUS_WORKAROUND, &priv->flags); This should probably be moved to the top. int retry = status_fix ? 3 : 1; > size_t count = 0; > bool itpm = test_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags); > > status = tpm_tis_status(chip); > if ((status & TPM_STS_COMMAND_READY) == 0) { > - tpm_tis_ready(chip); > - if (wait_for_tpm_stat > - (chip, TPM_STS_COMMAND_READY, chip->timeout_b, > - &priv->int_queue, false) < 0) { > + retry = status_fix ? 3 : 1; > + > + while (retry > 0) { > + tpm_tis_ready(chip); > + if (wait_for_tpm_stat > + (chip, TPM_STS_COMMAND_READY, chip->timeout_b, > + &priv->int_queue, false) >= 0) { > + break; > + } > + > + retry--; > + } > + > + if (retry == 0) { > rc = -ETIME; > goto out_err; > } > @@ -1147,6 +1158,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > priv->timeout_max = TIS_TIMEOUT_MAX_ATML; > } > > + if (priv->manufacturer_id == TPM_VID_IFX) > + set_bit(TPM_TIS_STATUS_WORKAROUND, &priv->flags); > + > if (is_bsw()) { > priv->ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR, > ILB_REMAP_SIZE); > diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h > index 13e99cf65efe..f888da57535d 100644 > --- a/drivers/char/tpm/tpm_tis_core.h > +++ b/drivers/char/tpm/tpm_tis_core.h > @@ -89,6 +89,7 @@ enum tpm_tis_flags { > TPM_TIS_INVALID_STATUS = 1, > TPM_TIS_DEFAULT_CANCELLATION = 2, > TPM_TIS_IRQ_TESTED = 3, > + TPM_TIS_STATUS_WORKAROUND = 4, > }; > > struct tpm_tis_data { > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > index 4ee9d13749ad..5f4998626a98 100644 > --- a/include/linux/tpm.h > +++ b/include/linux/tpm.h > @@ -272,6 +272,7 @@ enum tpm2_cc_attrs { > #define TPM_VID_WINBOND 0x1050 > #define TPM_VID_STM 0x104A > #define TPM_VID_ATML 0x1114 > +#define TPM_VID_IFX 0x15D1 > > enum tpm_chip_flags { > TPM_CHIP_FLAG_BOOTSTRAPPED = BIT(0),
On 10/3/24 5:59 PM, Stefan Berger wrote: > > > On 10/2/24 1:04 PM, Jonathan McDowell wrote: >> (I'm still in the process of testing this to confirm it fixes the >> errata I've seen, but I wanted to send it out for comments to make sure >> it's a reasonable approach.) >> >> Some Infineon devices have a issue where the status register will get >> stuck with a quick REQUEST_USE / COMMAND_READY sequence. The work around > > Did you tell Infineon about it? Maybe they should have a look at their > firmware. > > What are the TPMs in your fleet doing? I heared that some TPMs > pre-create keys in the background once users requested a few. I would > try to create a few primary keys with different combination of key flags Actually make this child keys of primary keys: > tsscreateprimary Handle 80000000 > while :; do time tsscreate -hp 80000000 -si -opem pubkey.pem ; cat pubkey.pem; done This should give a different key every time and maybe key creation time goes up at some point... > set and in different hierarchies (and flush them) to use up these > precreated keys and see whether that leads to any issues with the TIS > responsiveness once presumably the device starts creating keys in the > background... > >> is to retry the command submission. Add appropriate logic to do this in > > I would describe it as 'retrying to set the TPM_STS_COMMAND_READY flag > on the TIS STS register' because you are not retring a (TPM) command > submission, like resubmitting a TPM2_PCR_Extend for example. > > > >> the send path. >> >> Signed-off-by: Jonathan McDowell <noodles@meta.com> >> --- >> drivers/char/tpm/tpm_tis_core.c | 24 +++++++++++++++++++----- >> drivers/char/tpm/tpm_tis_core.h | 1 + >> include/linux/tpm.h | 1 + >> 3 files changed, 21 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm_tis_core.c >> b/drivers/char/tpm/tpm_tis_core.c >> index f6aa0dfadb93..940abd1a868e 100644 >> --- a/drivers/char/tpm/tpm_tis_core.c >> +++ b/drivers/char/tpm/tpm_tis_core.c >> @@ -432,16 +432,27 @@ static int tpm_tis_recv(struct tpm_chip *chip, >> u8 *buf, size_t count) >> static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, >> size_t len) >> { >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >> - int rc, status, burstcnt; >> + int rc, status, burstcnt, retry; >> + bool status_fix = test_bit(TPM_TIS_STATUS_WORKAROUND, &priv->flags); > > This should probably be moved to the top. > int retry = status_fix ? 3 : 1; > >> size_t count = 0; >> bool itpm = test_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags); >> status = tpm_tis_status(chip); >> if ((status & TPM_STS_COMMAND_READY) == 0) { >> - tpm_tis_ready(chip); >> - if (wait_for_tpm_stat >> - (chip, TPM_STS_COMMAND_READY, chip->timeout_b, >> - &priv->int_queue, false) < 0) { >> + retry = status_fix ? 3 : 1; >> + >> + while (retry > 0) { >> + tpm_tis_ready(chip); >> + if (wait_for_tpm_stat >> + (chip, TPM_STS_COMMAND_READY, chip->timeout_b, >> + &priv->int_queue, false) >= 0) { >> + break; >> + } >> + >> + retry--; >> + } >> + >> + if (retry == 0) { >> rc = -ETIME; >> goto out_err; >> } >> @@ -1147,6 +1158,9 @@ int tpm_tis_core_init(struct device *dev, struct >> tpm_tis_data *priv, int irq, >> priv->timeout_max = TIS_TIMEOUT_MAX_ATML; >> } >> + if (priv->manufacturer_id == TPM_VID_IFX) >> + set_bit(TPM_TIS_STATUS_WORKAROUND, &priv->flags); >> + >> if (is_bsw()) { >> priv->ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR, >> ILB_REMAP_SIZE); >> diff --git a/drivers/char/tpm/tpm_tis_core.h >> b/drivers/char/tpm/tpm_tis_core.h >> index 13e99cf65efe..f888da57535d 100644 >> --- a/drivers/char/tpm/tpm_tis_core.h >> +++ b/drivers/char/tpm/tpm_tis_core.h >> @@ -89,6 +89,7 @@ enum tpm_tis_flags { >> TPM_TIS_INVALID_STATUS = 1, >> TPM_TIS_DEFAULT_CANCELLATION = 2, >> TPM_TIS_IRQ_TESTED = 3, >> + TPM_TIS_STATUS_WORKAROUND = 4, >> }; >> struct tpm_tis_data { >> diff --git a/include/linux/tpm.h b/include/linux/tpm.h >> index 4ee9d13749ad..5f4998626a98 100644 >> --- a/include/linux/tpm.h >> +++ b/include/linux/tpm.h >> @@ -272,6 +272,7 @@ enum tpm2_cc_attrs { >> #define TPM_VID_WINBOND 0x1050 >> #define TPM_VID_STM 0x104A >> #define TPM_VID_ATML 0x1114 >> +#define TPM_VID_IFX 0x15D1 >> enum tpm_chip_flags { >> TPM_CHIP_FLAG_BOOTSTRAPPED = BIT(0), >
On Thu, Oct 03, 2024 at 06:25:14PM -0400, Stefan Berger wrote: > On 10/3/24 5:59 PM, Stefan Berger wrote: > > On 10/2/24 1:04 PM, Jonathan McDowell wrote: > > > (I'm still in the process of testing this to confirm it fixes the > > > errata I've seen, but I wanted to send it out for comments to make sure > > > it's a reasonable approach.) > > > > > > Some Infineon devices have a issue where the status register will get > > > stuck with a quick REQUEST_USE / COMMAND_READY sequence. The work around > > > > Did you tell Infineon about it? Maybe they should have a look at their > > firmware. I'm in contact with Infineon, and have their errata under NDA. > > What are the TPMs in your fleet doing? I heared that some TPMs > > pre-create keys in the background once users requested a few. I would > > try to create a few primary keys with different combination of key flags > > Actually make this child keys of primary keys: > > > tsscreateprimary > Handle 80000000 > > while :; do time tsscreate -hp 80000000 -si -opem pubkey.pem ; cat > pubkey.pem; done > > This should give a different key every time and maybe key creation time goes > up at some point... We're doing a TPM2_CC_CREATE, but as part of a "seal" operation for some data, rather than full asymmetric key creation. So I don't think this is likely to be the cause, but it's given me the idea to see if we are seeing a spike on a particular command - maybe there's something that's close to the edge on timings that is occasionally going over. Playing around with bpftrace seems to give useful data so I'll need to investigate if I can deploy that to do some monitoring. Our monitoring is running hourly and does the following TPM2 operations (including the in kernel context switching): TPM2_CC_SELF_TEST TPM2_CC_GET_CAPABILITY TPM2_CC_READ_PUBLIC TPM2_CC_READ_PUBLIC TPM2_CC_CREATE TPM2_CC_LOAD TPM2_CC_CONTEXT_SAVE TPM2_CC_FLUSH_CONTEXT TPM2_CC_CONTEXT_LOAD TPM2_CC_UNSEAL TPM2_CC_CONTEXT_SAVE TPM2_CC_FLUSH_CONTEXT TPM2_CC_CONTEXT_LOAD TPM2_CC_FLUSH_CONTEXT TPM2_CC_CONTEXT_SAVE TPM2_CC_READ_CLOCK TPM2_CC_GET_CAPABILITY TPM2_CC_GET_CAPABILITY TPM2_CC_GET_CAPABILITY TPM2_CC_GET_CAPABILITY TPM2_CC_GET_CAPABILITY TPM2_CC_GET_CAPABILITY TPM2_CC_READ_PUBLIC J.
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index f6aa0dfadb93..940abd1a868e 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -432,16 +432,27 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count) static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len) { struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); - int rc, status, burstcnt; + int rc, status, burstcnt, retry; + bool status_fix = test_bit(TPM_TIS_STATUS_WORKAROUND, &priv->flags); size_t count = 0; bool itpm = test_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags); status = tpm_tis_status(chip); if ((status & TPM_STS_COMMAND_READY) == 0) { - tpm_tis_ready(chip); - if (wait_for_tpm_stat - (chip, TPM_STS_COMMAND_READY, chip->timeout_b, - &priv->int_queue, false) < 0) { + retry = status_fix ? 3 : 1; + + while (retry > 0) { + tpm_tis_ready(chip); + if (wait_for_tpm_stat + (chip, TPM_STS_COMMAND_READY, chip->timeout_b, + &priv->int_queue, false) >= 0) { + break; + } + + retry--; + } + + if (retry == 0) { rc = -ETIME; goto out_err; } @@ -1147,6 +1158,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, priv->timeout_max = TIS_TIMEOUT_MAX_ATML; } + if (priv->manufacturer_id == TPM_VID_IFX) + set_bit(TPM_TIS_STATUS_WORKAROUND, &priv->flags); + if (is_bsw()) { priv->ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR, ILB_REMAP_SIZE); diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h index 13e99cf65efe..f888da57535d 100644 --- a/drivers/char/tpm/tpm_tis_core.h +++ b/drivers/char/tpm/tpm_tis_core.h @@ -89,6 +89,7 @@ enum tpm_tis_flags { TPM_TIS_INVALID_STATUS = 1, TPM_TIS_DEFAULT_CANCELLATION = 2, TPM_TIS_IRQ_TESTED = 3, + TPM_TIS_STATUS_WORKAROUND = 4, }; struct tpm_tis_data { diff --git a/include/linux/tpm.h b/include/linux/tpm.h index 4ee9d13749ad..5f4998626a98 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -272,6 +272,7 @@ enum tpm2_cc_attrs { #define TPM_VID_WINBOND 0x1050 #define TPM_VID_STM 0x104A #define TPM_VID_ATML 0x1114 +#define TPM_VID_IFX 0x15D1 enum tpm_chip_flags { TPM_CHIP_FLAG_BOOTSTRAPPED = BIT(0),
(I'm still in the process of testing this to confirm it fixes the errata I've seen, but I wanted to send it out for comments to make sure it's a reasonable approach.) Some Infineon devices have a issue where the status register will get stuck with a quick REQUEST_USE / COMMAND_READY sequence. The work around is to retry the command submission. Add appropriate logic to do this in the send path. Signed-off-by: Jonathan McDowell <noodles@meta.com> --- drivers/char/tpm/tpm_tis_core.c | 24 +++++++++++++++++++----- drivers/char/tpm/tpm_tis_core.h | 1 + include/linux/tpm.h | 1 + 3 files changed, 21 insertions(+), 5 deletions(-)