Re: [PATCH] iommu/amd: Add sanity check for interrupt remapping table length macros
Suravee Suthikulpanit @ 2020-12-10 09:24 MST:
> Currently, macros related to the interrupt remapping table length are
> defined separately. This has resulted in an oversight in which one of
> the macros were missed when changing the length. To prevent this,
> redefine the macros to add built-in sanity check.
>
> Also, rename macros to use the name of the DTE[IntTabLen] field as
> specified in the AMD IOMMU specification. There is no functional change.
>
> Suggested-by: Linus Torvalds
> Reviewed-by: Tom Lendacky
> Signed-off-by: Suravee Suthikulpanit
> Cc: Will Deacon
> Cc: Jerry Snitselaar
> Cc: Joerg Roedel
> ---
> drivers/iommu/amd/amd_iommu_types.h | 19 ++-
> drivers/iommu/amd/init.c| 6 +++---
> drivers/iommu/amd/iommu.c | 2 +-
> 3 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iommu/amd/amd_iommu_types.h
> b/drivers/iommu/amd/amd_iommu_types.h
> index 494b42a31b7a..899ce62df3f0 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -255,11 +255,19 @@
> /* Bit value definition for dte irq remapping fields*/
> #define DTE_IRQ_PHYS_ADDR_MASK (((1ULL << 45)-1) << 6)
> #define DTE_IRQ_REMAP_INTCTL_MASK(0x3ULL << 60)
> -#define DTE_IRQ_TABLE_LEN_MASK (0xfULL << 1)
> #define DTE_IRQ_REMAP_INTCTL(2ULL << 60)
> -#define DTE_IRQ_TABLE_LEN (9ULL << 1)
> #define DTE_IRQ_REMAP_ENABLE1ULL
>
> +/*
> + * AMD IOMMU hardware only support 512 IRTEs despite
> + * the architectural limitation of 2048 entries.
> + */
> +#define DTE_INTTAB_ALIGNMENT128
> +#define DTE_INTTABLEN_VALUE 9ULL
> +#define DTE_INTTABLEN (DTE_INTTABLEN_VALUE << 1)
> +#define DTE_INTTABLEN_MASK (0xfULL << 1)
> +#define MAX_IRQS_PER_TABLE (1 << DTE_INTTABLEN_VALUE)
> +
> #define PAGE_MODE_NONE0x00
> #define PAGE_MODE_1_LEVEL 0x01
> #define PAGE_MODE_2_LEVEL 0x02
> @@ -409,13 +417,6 @@ extern bool amd_iommu_np_cache;
> /* Only true if all IOMMUs support device IOTLBs */
> extern bool amd_iommu_iotlb_sup;
>
> -/*
> - * AMD IOMMU hardware only support 512 IRTEs despite
> - * the architectural limitation of 2048 entries.
> - */
> -#define MAX_IRQS_PER_TABLE 512
> -#define IRQ_TABLE_ALIGNMENT 128
> -
> struct irq_remap_table {
> raw_spinlock_t lock;
> unsigned min_index;
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 23a790f8f550..6bec8913d064 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -989,10 +989,10 @@ static bool copy_device_table(void)
>
> irq_v = old_devtb[devid].data[2] & DTE_IRQ_REMAP_ENABLE;
> int_ctl = old_devtb[devid].data[2] & DTE_IRQ_REMAP_INTCTL_MASK;
> - int_tab_len = old_devtb[devid].data[2] & DTE_IRQ_TABLE_LEN_MASK;
> + int_tab_len = old_devtb[devid].data[2] & DTE_INTTABLEN_MASK;
> if (irq_v && (int_ctl || int_tab_len)) {
> if ((int_ctl != DTE_IRQ_REMAP_INTCTL) ||
> - (int_tab_len != DTE_IRQ_TABLE_LEN)) {
> + (int_tab_len != DTE_INTTABLEN)) {
> pr_err("Wrong old irq remapping flag: %#x\n",
> devid);
> return false;
> }
> @@ -2674,7 +2674,7 @@ static int __init early_amd_iommu_init(void)
> remap_cache_sz = MAX_IRQS_PER_TABLE * (sizeof(u64) * 2);
> amd_iommu_irq_cache = kmem_cache_create("irq_remap_cache",
> remap_cache_sz,
> - IRQ_TABLE_ALIGNMENT,
> + DTE_INTTAB_ALIGNMENT,
> 0, NULL);
> if (!amd_iommu_irq_cache)
> goto out;
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index b9cf59443843..f7abf16d1e3a 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -3191,7 +3191,7 @@ static void set_dte_irq_entry(u16 devid, struct
> irq_remap_table *table)
> dte &= ~DTE_IRQ_PHYS_ADDR_MASK;
> dte |= iommu_virt_to_phys(table->table);
> dte |= DTE_IRQ_REMAP_INTCTL;
> - dte |= DTE_IRQ_TABLE_LEN;
> + dte |= DTE_INTTABLEN;
> dte |= DTE_IRQ_REMAP_ENABLE;
>
> amd_iommu_dev_table[devid].data[2] = dte;
Reviewed-by: Jerry Snitselaar
Re: [GIT PULL] IOMMU fix for 5.10 (-final)
On Wed, Dec 9, 2020 at 12:18 PM Linus Torvalds wrote: > > On Wed, Dec 9, 2020 at 11:12 AM Jerry Snitselaar wrote: > > > > Since the field in the device table entry format expects it to be n > > where there are 2^n entries in the table I guess it should be: > > > > #define DTE_IRQ_TABLE_LEN 9 > > #define MAX_IRQS_PER_TABLE (1 << DTE_IRQ_TABLE_LEN) > > No, that "DTE_IRQ_TABLE_LEN" is not the size shift - it's the size > shift value in that DTE field, which is shifted up by 1. > > That's why the current code does that > >#define DTE_IRQ_TABLE_LEN (9ULL << 1) > > there.. > > Which was why I suggested that new #define that is the *actual* shift > value, and then the DTE thing and the MAX_IRQS_PER_TABLE values would > depend on that. > >Linus > Yes, when I read it my head was translating it as setting them both to 512 and then I forgot that it gets shifted over 1. Which considering I was the once who noticed the original problem of it still being 8 was a nice brain fart. This should be fixed like you suggest.
Re: [GIT PULL] IOMMU fix for 5.10 (-final)
On Wed, Dec 9, 2020 at 12:12 PM Jerry Snitselaar wrote: > > > Will Deacon @ 2020-12-09 11:50 MST: > > > On Wed, Dec 09, 2020 at 10:07:46AM -0800, Linus Torvalds wrote: > >> On Wed, Dec 9, 2020 at 6:12 AM Will Deacon wrote: > >> > > >> > Please pull this one-liner AMD IOMMU fix for 5.10. It's actually a fix > >> > for a fix, where the size of the interrupt remapping table was increased > >> > but a related constant for the size of the interrupt table was forgotten. > >> > >> Pulled. > > > > Thanks. > > > >> However, why didn't this then add some sanity checking for the two > >> different #defines to be in sync? > >> > >> IOW, something like > >> > >>#define AMD_IOMMU_IRQ_TABLE_SHIFT 9 > >> > >>#define MAX_IRQS_PER_TABLE (1 << AMD_IOMMU_IRQ_TABLE_SHIFT) > >>#define DTE_IRQ_TABLE_LEN ((u64)AMD_IOMMU_IRQ_TABLE_SHIFT << 1) > > Since the field in the device table entry format expects it to be n > where there are 2^n entries in the table I guess it should be: > > #define DTE_IRQ_TABLE_LEN 9 > #define MAX_IRQS_PER_TABLE (1 << DTE_IRQ_TABLE_LEN) > No, ignore that. I'm being stupid. > >> > >> or whatever. Hmm? > > > > This looks like a worthwhile change to me, but I don't have any hardware > > so I've been very reluctant to make even "obvious" driver changes here. > > > > Suravee -- please can you post a patch implementing the above? > > > >> That way this won't happen again, but perhaps equally importantly the > >> linkage will be more clear, and there won't be those random constants. > >> > >> Naming above is probably garbage - I assume there's some actual > >> architectural name for that irq table length field in the DTE? > > > > The one in the spec is even better: "IntTabLen". > > > > Will > > ___ > > iommu mailing list > > io...@lists.linux-foundation.org > > https://lists.linuxfoundation.org/mailman/listinfo/iommu >
Re: [GIT PULL] IOMMU fix for 5.10 (-final)
Will Deacon @ 2020-12-09 11:50 MST: > On Wed, Dec 09, 2020 at 10:07:46AM -0800, Linus Torvalds wrote: >> On Wed, Dec 9, 2020 at 6:12 AM Will Deacon wrote: >> > >> > Please pull this one-liner AMD IOMMU fix for 5.10. It's actually a fix >> > for a fix, where the size of the interrupt remapping table was increased >> > but a related constant for the size of the interrupt table was forgotten. >> >> Pulled. > > Thanks. > >> However, why didn't this then add some sanity checking for the two >> different #defines to be in sync? >> >> IOW, something like >> >>#define AMD_IOMMU_IRQ_TABLE_SHIFT 9 >> >>#define MAX_IRQS_PER_TABLE (1 << AMD_IOMMU_IRQ_TABLE_SHIFT) >>#define DTE_IRQ_TABLE_LEN ((u64)AMD_IOMMU_IRQ_TABLE_SHIFT << 1) Since the field in the device table entry format expects it to be n where there are 2^n entries in the table I guess it should be: #define DTE_IRQ_TABLE_LEN 9 #define MAX_IRQS_PER_TABLE (1 << DTE_IRQ_TABLE_LEN) >> >> or whatever. Hmm? > > This looks like a worthwhile change to me, but I don't have any hardware > so I've been very reluctant to make even "obvious" driver changes here. > > Suravee -- please can you post a patch implementing the above? > >> That way this won't happen again, but perhaps equally importantly the >> linkage will be more clear, and there won't be those random constants. >> >> Naming above is probably garbage - I assume there's some actual >> architectural name for that irq table length field in the DTE? > > The one in the spec is even better: "IntTabLen". > > Will > ___ > iommu mailing list > io...@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/amd: Set DTE[IntTabLen] to represent 512 IRTEs
Suravee Suthikulpanit @ 2020-12-07 02:19 MST:
> According to the AMD IOMMU spec, the commit 73db2fc595f3
> ("iommu/amd: Increase interrupt remapping table limit to 512 entries")
> also requires the interrupt table length (IntTabLen) to be set to 9
> (power of 2) in the device table mapping entry (DTE).
>
> Fixes: 73db2fc595f3 ("iommu/amd: Increase interrupt remapping table limit to
> 512 entries")
> Reported-by: Jerry Snitselaar
> Signed-off-by: Suravee Suthikulpanit
> ---
> drivers/iommu/amd/amd_iommu_types.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/amd/amd_iommu_types.h
> b/drivers/iommu/amd/amd_iommu_types.h
> index 89647700bab2..494b42a31b7a 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -257,7 +257,7 @@
> #define DTE_IRQ_REMAP_INTCTL_MASK(0x3ULL << 60)
> #define DTE_IRQ_TABLE_LEN_MASK (0xfULL << 1)
> #define DTE_IRQ_REMAP_INTCTL(2ULL << 60)
> -#define DTE_IRQ_TABLE_LEN (8ULL << 1)
> +#define DTE_IRQ_TABLE_LEN (9ULL << 1)
> #define DTE_IRQ_REMAP_ENABLE1ULL
>
> #define PAGE_MODE_NONE0x00
Reviewed-by: Jerry Snitselaar
Re: [PATCH v3 2/4] drm/i915/pmu: Use kstat_irqs to get interrupt count
Thomas Gleixner @ 2020-12-06 09:38 MST:
> On Fri, Dec 04 2020 at 18:43, Jerry Snitselaar wrote:
>
>> Now that kstat_irqs is exported, get rid of count_interrupts in
>> i915_pmu.c
>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>> @@ -423,22 +423,6 @@ static enum hrtimer_restart i915_sample(struct hrtimer
>> *hrtimer)
>> return HRTIMER_RESTART;
>> }
>>
>> -static u64 count_interrupts(struct drm_i915_private *i915)
>> -{
>> -/* open-coded kstat_irqs() */
>> -struct irq_desc *desc = irq_to_desc(i915->drm.pdev->irq);
>> -u64 sum = 0;
>> -int cpu;
>> -
>> -if (!desc || !desc->kstat_irqs)
>> -return 0;
>> -
>> -for_each_possible_cpu(cpu)
>> -sum += *per_cpu_ptr(desc->kstat_irqs, cpu);
>> -
>> -return sum;
>> -}
>
> May I ask why this has been merged in the first place?
>
> Nothing in a driver has ever to fiddle with the internals of an irq
> descriptor. We have functions for properly accessing them. Just because
> C allows to fiddle with everything is not a justification. If the
> required function is not exported then adding the export with a proper
> explanation is not asked too much.
>
> Also this lacks protection or at least a comment why this can be called
> safely and is not subject to a concurrent removal of the irq descriptor.
> The same problem exists when calling kstat_irqs(). It's even documented
> at the top of the function.
>
> Thanks,
>
> tglx
I don't know the history behind this bit. I stumbled across it in cscope
when looking for places using kstat_irqs.
Re: [PATCH v3 1/4] irq: export kstat_irqs
Thomas Gleixner @ 2020-12-06 10:54 MST: > Jerry, > > On Fri, Dec 04 2020 at 18:43, Jerry Snitselaar wrote: > > The proper prefix is 'genirq:' git log kernel/irq/irqdesc.c would have > told you. > >> To try and detect potential interrupt storms that >> have been occurring with tpm_tis devices it was suggested >> to use kstat_irqs() to get the number of interrupts. >> Since tpm_tis can be built as a module it needs kstat_irqs >> exported. > > I'm not really enthused about exporting this without making it at least > safe. Using it from an interrupt handler is obviously safe vs. concurrent > removal, but the next driver writer who thinks this is cool is going to > get it wrong for sure. > > Though I still have to figure out what the advantage of invoking a > function which needs to do a radix tree lookup over a device local > counter is just to keep track of this. > > I'll reply on the TPM part of this as well. > > Thanks, > > tglx I can rework it to use a device local counter.
[PATCH v3 0/4] tpm_tis: Detect interrupt storms
This patchset is an attempt to try and catch tpm_tis devices that have interrupt storm issues, disable the interrupt, and use polling. In 2016 the tpm_tis interrupt code was accidently disabled, and polling was just being used. When we initially tried to enable interrupts again there were some reports of systems being hit with interrupt storms. It turned out that the ThinkPad T490s had misconfigured a gpio pin being used for the interrupt. The problem is more widespread though, with interrupt storms also being seen on other platforms and different TPM vendors. With the L490 the system hangs at tpm_tis initialization even with the detection code, so change the earlier detection code that used dmi to look for the T490s to instead look for the L490 and disable interrupts. Since kstat_irqs needs to be exported to allow building of tpm_tis as a module, I've included a patch to change the i915_pmu code to use kstat_irqs where before it was using its own version. If this isn't desired it can be dropped. I've been testing this on top of James' proposed patchset which re-enables interrupts for tpm_tis. With the patchsets applied it detects the problem on the T490s and on the Ice Lake development system where I found the issue. I have Lenovo verifying that the dmi detection code will now detect the L490 and avoid the hang it experiences. I'm also working on getting access to an L490 to see if I can figure out what the underlying issue is. Changes from v2: - Export kstat_irqs to allow building tpm_tis as a module. - Change i915_pmu.c to use kstat_irqs instead of it's own version count_interrupts. - Change include from linux/kernel_stat.h to linux/irq.h. - Change dmi checking code to now look for L490 instead of T490s. Changes from v1: - drop tpm_tis specific workqueue and use just system_w. Jerry Snitselaar (4): irq: export kstat_irqs drm/i915/pmu: Use kstat_irqs to get interrupt count tpm_tis: Disable interrupts if interrupt storm detected tpm_tis: Disable Interrupts on the ThinkPad L490 Cc: Thomas Gleixner Cc: Jani Nikula Cc: Rodrigo Vivi Cc: David Airlie Cc: Daniel Vetter Cc: intel-...@lists.freedesktop.org Cc: dri-de...@lists.freedesktop.org Cc: Jarkko Sakkinen Cc: Jason Gunthorpe Cc: Peter Huewe Cc: James Bottomley Cc: Matthew Garrett Cc: Hans de Goede Cc: linux-integr...@vger.kernel.org drivers/char/tpm/tpm_tis.c | 4 ++-- drivers/char/tpm/tpm_tis_core.c | 27 +++ drivers/char/tpm/tpm_tis_core.h | 2 ++ drivers/gpu/drm/i915/i915_pmu.c | 18 +- include/linux/irqdesc.h | 1 + kernel/irq/irqdesc.c| 1 + 6 files changed, 34 insertions(+), 19 deletions(-) -- 2.27.0
[PATCH v3 3/4] tpm_tis: Disable interrupts if interrupt storm detected
When enabling the interrupt code for the tpm_tis driver we have
noticed some systems have a bios issue causing an interrupt storm to
occur. The issue isn't limited to a single tpm or system manufacturer
so keeping a denylist of systems with the issue isn't optimal. Instead
try to detect the problem occurring, disable interrupts, and revert to
polling when it happens.
Cc: Jarkko Sakkinen
Cc: Jason Gunthorpe
Cc: Peter Huewe
Cc: James Bottomley
Cc: Matthew Garrett
Cc: Hans de Goede
Signed-off-by: Jerry Snitselaar
---
v3: - Change include from linux/kernel_stat.h to linux/irq.h
v2: - drop tpm_tis specific workqueue and use just system_w
drivers/char/tpm/tpm_tis_core.c | 27 +++
drivers/char/tpm/tpm_tis_core.h | 2 ++
2 files changed, 29 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 92c51c6cfd1b..d817ff5664d1 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -24,6 +24,8 @@
#include
#include
#include
+#include
+#include
#include "tpm.h"
#include "tpm_tis_core.h"
@@ -715,9 +717,23 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
{
struct tpm_chip *chip = dev_id;
struct tpm_tis_data *priv = dev_get_drvdata(>dev);
+ static bool check_storm = true;
+ static unsigned int check_start;
u32 interrupt;
int i, rc;
+ if (unlikely(check_storm)) {
+ if (!check_start) {
+ check_start = jiffies_to_msecs(jiffies);
+ } else if ((kstat_irqs(priv->irq) > 1000) &&
+ (jiffies_to_msecs(jiffies) - check_start < 500)) {
+ check_storm = false;
+ schedule_work(>storm_work);
+ } else if (jiffies_to_msecs(jiffies) - check_start >= 500) {
+ check_storm = false;
+ }
+ }
+
rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), );
if (rc < 0)
return IRQ_NONE;
@@ -943,6 +959,14 @@ static const struct tpm_class_ops tpm_tis = {
.clk_enable = tpm_tis_clkrun_enable,
};
+static void tpm_tis_storm_work(struct work_struct *work)
+{
+ struct tpm_tis_data *priv = container_of(work, struct tpm_tis_data,
storm_work);
+
+ disable_interrupts(priv->chip);
+ dev_warn(>chip->dev, "Interrupt storm detected, using
polling.\n");
+}
+
int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
const struct tpm_tis_phy_ops *phy_ops,
acpi_handle acpi_dev_handle)
@@ -959,6 +983,9 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
if (IS_ERR(chip))
return PTR_ERR(chip);
+ priv->chip = chip;
+ INIT_WORK(>storm_work, tpm_tis_storm_work);
+
#ifdef CONFIG_ACPI
chip->acpi_dev_handle = acpi_dev_handle;
#endif
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 9b2d32a59f67..973297ee2e16 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -95,6 +95,8 @@ struct tpm_tis_data {
u16 clkrun_enabled;
wait_queue_head_t int_queue;
wait_queue_head_t read_queue;
+ struct work_struct storm_work;
+ struct tpm_chip *chip;
const struct tpm_tis_phy_ops *phy_ops;
unsigned short rng_quality;
};
--
2.27.0
[PATCH v3 2/4] drm/i915/pmu: Use kstat_irqs to get interrupt count
Now that kstat_irqs is exported, get rid of count_interrupts in
i915_pmu.c
Cc: Thomas Gleixner
Cc: Jani Nikula
Cc: Rodrigo Vivi
Cc: David Airlie
Cc: Daniel Vetter
Cc: intel-...@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Cc: Jarkko Sakkinen
Cc: Jason Gunthorpe
Cc: Peter Huewe
Cc: James Bottomley
Cc: Matthew Garrett
Cc: Hans de Goede
Signed-off-by: Jerry Snitselaar
---
drivers/gpu/drm/i915/i915_pmu.c | 18 +-
1 file changed, 1 insertion(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 69c0fa20eba1..a3e63f03da8c 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -423,22 +423,6 @@ static enum hrtimer_restart i915_sample(struct hrtimer
*hrtimer)
return HRTIMER_RESTART;
}
-static u64 count_interrupts(struct drm_i915_private *i915)
-{
- /* open-coded kstat_irqs() */
- struct irq_desc *desc = irq_to_desc(i915->drm.pdev->irq);
- u64 sum = 0;
- int cpu;
-
- if (!desc || !desc->kstat_irqs)
- return 0;
-
- for_each_possible_cpu(cpu)
- sum += *per_cpu_ptr(desc->kstat_irqs, cpu);
-
- return sum;
-}
-
static void i915_pmu_event_destroy(struct perf_event *event)
{
struct drm_i915_private *i915 =
@@ -581,7 +565,7 @@ static u64 __i915_pmu_event_read(struct perf_event *event)
USEC_PER_SEC /* to MHz */);
break;
case I915_PMU_INTERRUPTS:
- val = count_interrupts(i915);
+ val = kstat_irqs(i915->drm.pdev->irq);
break;
case I915_PMU_RC6_RESIDENCY:
val = get_rc6(>gt);
--
2.27.0
[PATCH v3 1/4] irq: export kstat_irqs
To try and detect potential interrupt storms that have been occurring with tpm_tis devices it was suggested to use kstat_irqs() to get the number of interrupts. Since tpm_tis can be built as a module it needs kstat_irqs exported. Reported-by: kernel test robot Cc: Thomas Gleixner Cc: Jarkko Sakkinen Cc: Jason Gunthorpe Cc: Peter Huewe Cc: James Bottomley Cc: Matthew Garrett Cc: Hans de Goede Signed-off-by: Jerry Snitselaar --- include/linux/irqdesc.h | 1 + kernel/irq/irqdesc.c| 1 + 2 files changed, 2 insertions(+) diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h index 5745491303e0..fff88c1f1ac6 100644 --- a/include/linux/irqdesc.h +++ b/include/linux/irqdesc.h @@ -153,6 +153,7 @@ static inline void generic_handle_irq_desc(struct irq_desc *desc) } int generic_handle_irq(unsigned int irq); +unsigned int kstat_irqs(unsigned int irq); #ifdef CONFIG_HANDLE_DOMAIN_IRQ /* diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c index 1a7723604399..12398ef1796b 100644 --- a/kernel/irq/irqdesc.c +++ b/kernel/irq/irqdesc.c @@ -1000,6 +1000,7 @@ unsigned int kstat_irqs(unsigned int irq) sum += *per_cpu_ptr(desc->kstat_irqs, cpu); return sum; } +EXPORT_SYMBOL_GPL(kstat_irqs); /** * kstat_irqs_usr - Get the statistics for an interrupt -- 2.27.0
[PATCH v3 4/4] tpm_tis: Disable Interrupts on the ThinkPad L490
The interrupt storm detection code detects the issue on the ThinkPad
T490s, but the L490 still hangs at initialization. So swap out the
T490s for the L490 in the dmi check.
Cc: Jarkko Sakkinen
Cc: Jason Gunthorpe
Cc: Peter Huewe
Cc: James Bottomley
Cc: Matthew Garrett
Cc: Hans de Goede
Signed-off-by: Jerry Snitselaar
---
drivers/char/tpm/tpm_tis.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 4ed6e660273a..7322e0986a83 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -77,10 +77,10 @@ static int tpm_tis_disable_irq(const struct dmi_system_id
*d)
static const struct dmi_system_id tpm_tis_dmi_table[] = {
{
.callback = tpm_tis_disable_irq,
- .ident = "ThinkPad T490s",
+ .ident = "ThinkPad L490",
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
- DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T490s"),
+ DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad L490"),
},
},
{}
--
2.27.0
Re: [PATCH v2] tpm_tis: Disable interrupts if interrupt storm detected
James Bottomley @ 2020-12-03 14:05 MST: > On Thu, 2020-12-03 at 13:14 -0700, Jerry Snitselaar wrote: >> Jerry Snitselaar @ 2020-12-02 23:11 MST: > [...] >> > The interrupt storm detection code works on the T490s. I'm not sure >> > what is going on with the L490. I will see if I can get access to >> > one. >> > >> > Jerry >> >> Lenovo verified that the L490 hangs. > > Just to confirm, that's this system: > > https://www.lenovo.com/us/en/laptops/thinkpad/thinkpad-l/ThinkPad-L490/p/22TP2TBL490 > > We could ask if lenovo will give us one, but if not we could pull a > Jens. [the backstory is that when Jens was doing queueing in the block > layer, there were lots of SATA devices that didn't work quite right but > you couldn't tell unless you actually tried them out. Getting > manufacturers to send samples is rather arduous, so he took to ordering > them online, testing them out, and then returning them for a full > refund within the allowed window] > > It looks like Lenovo has a nice christmas returns policy: > > https://www.lenovo.com/us/en/shopping-faq/#returns > > James Yes, that is the one. I'm seeing if we have any located somewhere, or if Lenovo will loan me one. I think for the time being the patch that disabled interrupts for the T490s could be changed to it for the L490 instead. I'll post a v3 of my current patchset. It would probably make sense for it to go in with your patches when they land.
Re: [PATCH v2] tpm_tis: Disable interrupts if interrupt storm detected
Jerry Snitselaar @ 2020-12-02 23:11 MST:
> Jerry Snitselaar @ 2020-12-02 17:02 MST:
>
>> Jarkko Sakkinen @ 2020-12-02 09:49 MST:
>>
>>> On Tue, Dec 01, 2020 at 12:59:23PM -0700, Jerry Snitselaar wrote:
>>>>
>>>> Jerry Snitselaar @ 2020-11-30 20:26 MST:
>>>>
>>>> > Jerry Snitselaar @ 2020-11-30 19:58 MST:
>>>> >
>>>> >> When enabling the interrupt code for the tpm_tis driver we have
>>>> >> noticed some systems have a bios issue causing an interrupt storm to
>>>> >> occur. The issue isn't limited to a single tpm or system manufacturer
>>>> >> so keeping a denylist of systems with the issue isn't optimal. Instead
>>>> >> try to detect the problem occurring, disable interrupts, and revert to
>>>> >> polling when it happens.
>>>> >>
>>>> >> Cc: Jarkko Sakkinen
>>>> >> Cc: Jason Gunthorpe
>>>> >> Cc: Peter Huewe
>>>> >> Cc: James Bottomley
>>>> >> Cc: Matthew Garrett
>>>> >> Cc: Hans de Goede
>>>> >> Signed-off-by: Jerry Snitselaar
>>>> >> ---
>>>> >> v2: drop tpm_tis specific workqueue and use just system_wq
>>>> >>
>>>> >> drivers/char/tpm/tpm_tis_core.c | 27 +++
>>>> >> drivers/char/tpm/tpm_tis_core.h | 2 ++
>>>> >> 2 files changed, 29 insertions(+)
>>>> >>
>>>> >> diff --git a/drivers/char/tpm/tpm_tis_core.c
>>>> >> b/drivers/char/tpm/tpm_tis_core.c
>>>> >> index 23b60583928b..72cc8a5a152c 100644
>>>> >> --- a/drivers/char/tpm/tpm_tis_core.c
>>>> >> +++ b/drivers/char/tpm/tpm_tis_core.c
>>>> >> @@ -24,6 +24,8 @@
>>>> >> #include
>>>> >> #include
>>>> >> #include
>>>> >> +#include
>>>> >> +#include
>>>> >> #include "tpm.h"
>>>> >> #include "tpm_tis_core.h"
>>>> >>
>>>> >> @@ -745,9 +747,23 @@ static irqreturn_t tis_int_handler(int dummy, void
>>>> >> *dev_id)
>>>> >> {
>>>> >> struct tpm_chip *chip = dev_id;
>>>> >> struct tpm_tis_data *priv = dev_get_drvdata(>dev);
>>>> >> + static bool check_storm = true;
>>>> >> + static unsigned int check_start;
>>>> >> u32 interrupt;
>>>> >> int i, rc;
>>>> >>
>>>> >> + if (unlikely(check_storm)) {
>>>> >> + if (!check_start) {
>>>> >> + check_start = jiffies_to_msecs(jiffies);
>>>> >> + } else if ((kstat_irqs(priv->irq) > 1000) &&
>>>> >> + (jiffies_to_msecs(jiffies) - check_start <
>>>> >> 500)) {
>>>> >> + check_storm = false;
>>>> >> + schedule_work(>storm_work);
>>>> >> + } else if (jiffies_to_msecs(jiffies) - check_start >=
>>>> >> 500) {
>>>> >> + check_storm = false;
>>>> >> + }
>>>> >> + }
>>>> >> +
>>>> >> rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality),
>>>> >> );
>>>> >> if (rc < 0)
>>>> >> return IRQ_NONE;
>>>> >> @@ -987,6 +1003,14 @@ static const struct tpm_class_ops tpm_tis = {
>>>> >> .clk_enable = tpm_tis_clkrun_enable,
>>>> >> };
>>>> >>
>>>> >> +static void tpm_tis_storm_work(struct work_struct *work)
>>>> >> +{
>>>> >> + struct tpm_tis_data *priv = container_of(work, struct
>>>> >> tpm_tis_data, storm_work);
>>>> >> +
>>>> >> + disable_interrupts(priv->chip);
>>>> >> + dev_warn(>chip->dev, "Interrupt storm detected, using
>>>> >> polling.\n");
>>>> >> +}
>>>> >> +
>>>> >> int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv,
&g
Re: [PATCH v2] tpm_tis: Disable interrupts if interrupt storm detected
Jerry Snitselaar @ 2020-12-02 17:02 MST:
> Jarkko Sakkinen @ 2020-12-02 09:49 MST:
>
>> On Tue, Dec 01, 2020 at 12:59:23PM -0700, Jerry Snitselaar wrote:
>>>
>>> Jerry Snitselaar @ 2020-11-30 20:26 MST:
>>>
>>> > Jerry Snitselaar @ 2020-11-30 19:58 MST:
>>> >
>>> >> When enabling the interrupt code for the tpm_tis driver we have
>>> >> noticed some systems have a bios issue causing an interrupt storm to
>>> >> occur. The issue isn't limited to a single tpm or system manufacturer
>>> >> so keeping a denylist of systems with the issue isn't optimal. Instead
>>> >> try to detect the problem occurring, disable interrupts, and revert to
>>> >> polling when it happens.
>>> >>
>>> >> Cc: Jarkko Sakkinen
>>> >> Cc: Jason Gunthorpe
>>> >> Cc: Peter Huewe
>>> >> Cc: James Bottomley
>>> >> Cc: Matthew Garrett
>>> >> Cc: Hans de Goede
>>> >> Signed-off-by: Jerry Snitselaar
>>> >> ---
>>> >> v2: drop tpm_tis specific workqueue and use just system_wq
>>> >>
>>> >> drivers/char/tpm/tpm_tis_core.c | 27 +++
>>> >> drivers/char/tpm/tpm_tis_core.h | 2 ++
>>> >> 2 files changed, 29 insertions(+)
>>> >>
>>> >> diff --git a/drivers/char/tpm/tpm_tis_core.c
>>> >> b/drivers/char/tpm/tpm_tis_core.c
>>> >> index 23b60583928b..72cc8a5a152c 100644
>>> >> --- a/drivers/char/tpm/tpm_tis_core.c
>>> >> +++ b/drivers/char/tpm/tpm_tis_core.c
>>> >> @@ -24,6 +24,8 @@
>>> >> #include
>>> >> #include
>>> >> #include
>>> >> +#include
>>> >> +#include
>>> >> #include "tpm.h"
>>> >> #include "tpm_tis_core.h"
>>> >>
>>> >> @@ -745,9 +747,23 @@ static irqreturn_t tis_int_handler(int dummy, void
>>> >> *dev_id)
>>> >> {
>>> >> struct tpm_chip *chip = dev_id;
>>> >> struct tpm_tis_data *priv = dev_get_drvdata(>dev);
>>> >> +static bool check_storm = true;
>>> >> +static unsigned int check_start;
>>> >> u32 interrupt;
>>> >> int i, rc;
>>> >>
>>> >> +if (unlikely(check_storm)) {
>>> >> +if (!check_start) {
>>> >> +check_start = jiffies_to_msecs(jiffies);
>>> >> +} else if ((kstat_irqs(priv->irq) > 1000) &&
>>> >> + (jiffies_to_msecs(jiffies) - check_start <
>>> >> 500)) {
>>> >> +check_storm = false;
>>> >> +schedule_work(>storm_work);
>>> >> +} else if (jiffies_to_msecs(jiffies) - check_start >=
>>> >> 500) {
>>> >> +check_storm = false;
>>> >> +}
>>> >> +}
>>> >> +
>>> >> rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality),
>>> >> );
>>> >> if (rc < 0)
>>> >> return IRQ_NONE;
>>> >> @@ -987,6 +1003,14 @@ static const struct tpm_class_ops tpm_tis = {
>>> >> .clk_enable = tpm_tis_clkrun_enable,
>>> >> };
>>> >>
>>> >> +static void tpm_tis_storm_work(struct work_struct *work)
>>> >> +{
>>> >> +struct tpm_tis_data *priv = container_of(work, struct
>>> >> tpm_tis_data, storm_work);
>>> >> +
>>> >> +disable_interrupts(priv->chip);
>>> >> +dev_warn(>chip->dev, "Interrupt storm detected, using
>>> >> polling.\n");
>>> >> +}
>>> >> +
>>> >> int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv,
>>> >> int irq,
>>> >>const struct tpm_tis_phy_ops *phy_ops,
>>> >>acpi_handle acpi_dev_handle)
>>> >> @@ -1003,6 +1027,9 @@ int tpm_tis_core_init(struct device *dev, struct
>>> >> tpm_tis_data *priv, int irq,
>>> >> if (IS_ERR(chip))
>>
Re: [PATCH v2] tpm_tis: Disable interrupts if interrupt storm detected
Jarkko Sakkinen @ 2020-12-02 09:49 MST:
> On Tue, Dec 01, 2020 at 12:59:23PM -0700, Jerry Snitselaar wrote:
>>
>> Jerry Snitselaar @ 2020-11-30 20:26 MST:
>>
>> > Jerry Snitselaar @ 2020-11-30 19:58 MST:
>> >
>> >> When enabling the interrupt code for the tpm_tis driver we have
>> >> noticed some systems have a bios issue causing an interrupt storm to
>> >> occur. The issue isn't limited to a single tpm or system manufacturer
>> >> so keeping a denylist of systems with the issue isn't optimal. Instead
>> >> try to detect the problem occurring, disable interrupts, and revert to
>> >> polling when it happens.
>> >>
>> >> Cc: Jarkko Sakkinen
>> >> Cc: Jason Gunthorpe
>> >> Cc: Peter Huewe
>> >> Cc: James Bottomley
>> >> Cc: Matthew Garrett
>> >> Cc: Hans de Goede
>> >> Signed-off-by: Jerry Snitselaar
>> >> ---
>> >> v2: drop tpm_tis specific workqueue and use just system_wq
>> >>
>> >> drivers/char/tpm/tpm_tis_core.c | 27 +++
>> >> drivers/char/tpm/tpm_tis_core.h | 2 ++
>> >> 2 files changed, 29 insertions(+)
>> >>
>> >> diff --git a/drivers/char/tpm/tpm_tis_core.c
>> >> b/drivers/char/tpm/tpm_tis_core.c
>> >> index 23b60583928b..72cc8a5a152c 100644
>> >> --- a/drivers/char/tpm/tpm_tis_core.c
>> >> +++ b/drivers/char/tpm/tpm_tis_core.c
>> >> @@ -24,6 +24,8 @@
>> >> #include
>> >> #include
>> >> #include
>> >> +#include
>> >> +#include
>> >> #include "tpm.h"
>> >> #include "tpm_tis_core.h"
>> >>
>> >> @@ -745,9 +747,23 @@ static irqreturn_t tis_int_handler(int dummy, void
>> >> *dev_id)
>> >> {
>> >> struct tpm_chip *chip = dev_id;
>> >> struct tpm_tis_data *priv = dev_get_drvdata(>dev);
>> >> + static bool check_storm = true;
>> >> + static unsigned int check_start;
>> >> u32 interrupt;
>> >> int i, rc;
>> >>
>> >> + if (unlikely(check_storm)) {
>> >> + if (!check_start) {
>> >> + check_start = jiffies_to_msecs(jiffies);
>> >> + } else if ((kstat_irqs(priv->irq) > 1000) &&
>> >> +(jiffies_to_msecs(jiffies) - check_start < 500)) {
>> >> + check_storm = false;
>> >> + schedule_work(>storm_work);
>> >> + } else if (jiffies_to_msecs(jiffies) - check_start >= 500) {
>> >> + check_storm = false;
>> >> + }
>> >> + }
>> >> +
>> >> rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), );
>> >> if (rc < 0)
>> >> return IRQ_NONE;
>> >> @@ -987,6 +1003,14 @@ static const struct tpm_class_ops tpm_tis = {
>> >> .clk_enable = tpm_tis_clkrun_enable,
>> >> };
>> >>
>> >> +static void tpm_tis_storm_work(struct work_struct *work)
>> >> +{
>> >> + struct tpm_tis_data *priv = container_of(work, struct tpm_tis_data,
>> >> storm_work);
>> >> +
>> >> + disable_interrupts(priv->chip);
>> >> + dev_warn(>chip->dev, "Interrupt storm detected, using
>> >> polling.\n");
>> >> +}
>> >> +
>> >> int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int
>> >> irq,
>> >> const struct tpm_tis_phy_ops *phy_ops,
>> >> acpi_handle acpi_dev_handle)
>> >> @@ -1003,6 +1027,9 @@ int tpm_tis_core_init(struct device *dev, struct
>> >> tpm_tis_data *priv, int irq,
>> >> if (IS_ERR(chip))
>> >> return PTR_ERR(chip);
>> >>
>> >> + priv->chip = chip;
>> >> + INIT_WORK(>storm_work, tpm_tis_storm_work);
>> >> +
>> >> #ifdef CONFIG_ACPI
>> >> chip->acpi_dev_handle = acpi_dev_handle;
>> >> #endif
>> >> diff --git a/drivers/char/tpm/tpm_tis_core.h
>> >> b/drivers/char/tpm/tpm_tis_core.h
>> >> index edeb5dc61c95..5630f294dc0c 100644
>> >> --- a/drivers/char/tpm/tpm_tis_core.h
>> >> +++ b/drivers/char/tpm/tpm_tis_core.h
>> >> @@ -95,6 +95,8 @@ struct tpm_tis
Re: [PATCH] iommu/amd: Increase interrupt remapping table limit to 512 entries
Suravee Suthikulpanit @ 2020-10-14 19:50 MST:
> Certain device drivers allocate IO queues on a per-cpu basis.
> On AMD EPYC platform, which can support up-to 256 cpu threads,
> this can exceed the current MAX_IRQ_PER_TABLE limit of 256,
> and result in the error message:
>
> AMD-Vi: Failed to allocate IRTE
>
> This has been observed with certain NVME devices.
>
> AMD IOMMU hardware can actually support upto 512 interrupt
> remapping table entries. Therefore, update the driver to
> match the hardware limit.
>
> Please note that this also increases the size of interrupt remapping
> table to 8KB per device when using the 128-bit IRTE format.
>
> Signed-off-by: Suravee Suthikulpanit
> ---
> drivers/iommu/amd/amd_iommu_types.h | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/amd/amd_iommu_types.h
> b/drivers/iommu/amd/amd_iommu_types.h
> index 30a5d412255a..427484c45589 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -406,7 +406,11 @@ extern bool amd_iommu_np_cache;
> /* Only true if all IOMMUs support device IOTLBs */
> extern bool amd_iommu_iotlb_sup;
>
> -#define MAX_IRQS_PER_TABLE 256
> +/*
> + * AMD IOMMU hardware only support 512 IRTEs despite
> + * the architectural limitation of 2048 entries.
> + */
> +#define MAX_IRQS_PER_TABLE 512
> #define IRQ_TABLE_ALIGNMENT 128
>
> struct irq_remap_table {
With this change should DTE_IRQ_TABLE_LEN be changed to 9? IIUC the spec
correctly leaving it at 8 is saying the table is 256 entries long.
Regards,
Jerry
Re: [PATCH v2] tpm_tis: Disable interrupts if interrupt storm detected
Jerry Snitselaar @ 2020-11-30 20:26 MST:
> Jerry Snitselaar @ 2020-11-30 19:58 MST:
>
>> When enabling the interrupt code for the tpm_tis driver we have
>> noticed some systems have a bios issue causing an interrupt storm to
>> occur. The issue isn't limited to a single tpm or system manufacturer
>> so keeping a denylist of systems with the issue isn't optimal. Instead
>> try to detect the problem occurring, disable interrupts, and revert to
>> polling when it happens.
>>
>> Cc: Jarkko Sakkinen
>> Cc: Jason Gunthorpe
>> Cc: Peter Huewe
>> Cc: James Bottomley
>> Cc: Matthew Garrett
>> Cc: Hans de Goede
>> Signed-off-by: Jerry Snitselaar
>> ---
>> v2: drop tpm_tis specific workqueue and use just system_wq
>>
>> drivers/char/tpm/tpm_tis_core.c | 27 +++
>> drivers/char/tpm/tpm_tis_core.h | 2 ++
>> 2 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c
>> b/drivers/char/tpm/tpm_tis_core.c
>> index 23b60583928b..72cc8a5a152c 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -24,6 +24,8 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>> #include "tpm.h"
>> #include "tpm_tis_core.h"
>>
>> @@ -745,9 +747,23 @@ static irqreturn_t tis_int_handler(int dummy, void
>> *dev_id)
>> {
>> struct tpm_chip *chip = dev_id;
>> struct tpm_tis_data *priv = dev_get_drvdata(>dev);
>> +static bool check_storm = true;
>> +static unsigned int check_start;
>> u32 interrupt;
>> int i, rc;
>>
>> +if (unlikely(check_storm)) {
>> +if (!check_start) {
>> +check_start = jiffies_to_msecs(jiffies);
>> +} else if ((kstat_irqs(priv->irq) > 1000) &&
>> + (jiffies_to_msecs(jiffies) - check_start < 500)) {
>> +check_storm = false;
>> +schedule_work(>storm_work);
>> +} else if (jiffies_to_msecs(jiffies) - check_start >= 500) {
>> +check_storm = false;
>> +}
>> +}
>> +
>> rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), );
>> if (rc < 0)
>> return IRQ_NONE;
>> @@ -987,6 +1003,14 @@ static const struct tpm_class_ops tpm_tis = {
>> .clk_enable = tpm_tis_clkrun_enable,
>> };
>>
>> +static void tpm_tis_storm_work(struct work_struct *work)
>> +{
>> +struct tpm_tis_data *priv = container_of(work, struct tpm_tis_data,
>> storm_work);
>> +
>> +disable_interrupts(priv->chip);
>> +dev_warn(>chip->dev, "Interrupt storm detected, using
>> polling.\n");
>> +}
>> +
>> int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int
>> irq,
>>const struct tpm_tis_phy_ops *phy_ops,
>>acpi_handle acpi_dev_handle)
>> @@ -1003,6 +1027,9 @@ int tpm_tis_core_init(struct device *dev, struct
>> tpm_tis_data *priv, int irq,
>> if (IS_ERR(chip))
>> return PTR_ERR(chip);
>>
>> +priv->chip = chip;
>> +INIT_WORK(>storm_work, tpm_tis_storm_work);
>> +
>> #ifdef CONFIG_ACPI
>> chip->acpi_dev_handle = acpi_dev_handle;
>> #endif
>> diff --git a/drivers/char/tpm/tpm_tis_core.h
>> b/drivers/char/tpm/tpm_tis_core.h
>> index edeb5dc61c95..5630f294dc0c 100644
>> --- a/drivers/char/tpm/tpm_tis_core.h
>> +++ b/drivers/char/tpm/tpm_tis_core.h
>> @@ -95,6 +95,8 @@ struct tpm_tis_data {
>> u16 clkrun_enabled;
>> wait_queue_head_t int_queue;
>> wait_queue_head_t read_queue;
>> +struct work_struct storm_work;
>> +struct tpm_chip *chip;
>> const struct tpm_tis_phy_ops *phy_ops;
>> unsigned short rng_quality;
>> };
>
> I've tested this with the Intel platform that has an Infineon chip that
> I found the other week. It works, but isn't the complete fix. With this
> on top of James' patchset I sometimes see the message "Lost Interrupt
> waiting for TPM stat", so I guess there needs to be a check in
> wait_for_tpm_stat and request_locality to see if interrupts were
> disabled when the wait_event_interruptible_timeout call times out.
As kernel test robot pointed out. kstat_irqs isn't visible when tpm_tis
builds as a module. It looks like it is only called by kstat_irq_usrs,
and that is only by the fs/proc code. I have a patch to export it, but
the i915 driver open codes their own version instead of using it. Is
there any reason not to export it?
Re: [PATCH v2] tpm_tis: Disable interrupts if interrupt storm detected
Jerry Snitselaar @ 2020-11-30 19:58 MST:
> When enabling the interrupt code for the tpm_tis driver we have
> noticed some systems have a bios issue causing an interrupt storm to
> occur. The issue isn't limited to a single tpm or system manufacturer
> so keeping a denylist of systems with the issue isn't optimal. Instead
> try to detect the problem occurring, disable interrupts, and revert to
> polling when it happens.
>
> Cc: Jarkko Sakkinen
> Cc: Jason Gunthorpe
> Cc: Peter Huewe
> Cc: James Bottomley
> Cc: Matthew Garrett
> Cc: Hans de Goede
> Signed-off-by: Jerry Snitselaar
> ---
> v2: drop tpm_tis specific workqueue and use just system_wq
>
> drivers/char/tpm/tpm_tis_core.c | 27 +++
> drivers/char/tpm/tpm_tis_core.h | 2 ++
> 2 files changed, 29 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 23b60583928b..72cc8a5a152c 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -24,6 +24,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include "tpm.h"
> #include "tpm_tis_core.h"
>
> @@ -745,9 +747,23 @@ static irqreturn_t tis_int_handler(int dummy, void
> *dev_id)
> {
> struct tpm_chip *chip = dev_id;
> struct tpm_tis_data *priv = dev_get_drvdata(>dev);
> + static bool check_storm = true;
> + static unsigned int check_start;
> u32 interrupt;
> int i, rc;
>
> + if (unlikely(check_storm)) {
> + if (!check_start) {
> + check_start = jiffies_to_msecs(jiffies);
> + } else if ((kstat_irqs(priv->irq) > 1000) &&
> +(jiffies_to_msecs(jiffies) - check_start < 500)) {
> + check_storm = false;
> + schedule_work(>storm_work);
> + } else if (jiffies_to_msecs(jiffies) - check_start >= 500) {
> + check_storm = false;
> + }
> + }
> +
> rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), );
> if (rc < 0)
> return IRQ_NONE;
> @@ -987,6 +1003,14 @@ static const struct tpm_class_ops tpm_tis = {
> .clk_enable = tpm_tis_clkrun_enable,
> };
>
> +static void tpm_tis_storm_work(struct work_struct *work)
> +{
> + struct tpm_tis_data *priv = container_of(work, struct tpm_tis_data,
> storm_work);
> +
> + disable_interrupts(priv->chip);
> + dev_warn(>chip->dev, "Interrupt storm detected, using
> polling.\n");
> +}
> +
> int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> const struct tpm_tis_phy_ops *phy_ops,
> acpi_handle acpi_dev_handle)
> @@ -1003,6 +1027,9 @@ int tpm_tis_core_init(struct device *dev, struct
> tpm_tis_data *priv, int irq,
> if (IS_ERR(chip))
> return PTR_ERR(chip);
>
> + priv->chip = chip;
> + INIT_WORK(>storm_work, tpm_tis_storm_work);
> +
> #ifdef CONFIG_ACPI
> chip->acpi_dev_handle = acpi_dev_handle;
> #endif
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index edeb5dc61c95..5630f294dc0c 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -95,6 +95,8 @@ struct tpm_tis_data {
> u16 clkrun_enabled;
> wait_queue_head_t int_queue;
> wait_queue_head_t read_queue;
> + struct work_struct storm_work;
> + struct tpm_chip *chip;
> const struct tpm_tis_phy_ops *phy_ops;
> unsigned short rng_quality;
> };
I've tested this with the Intel platform that has an Infineon chip that
I found the other week. It works, but isn't the complete fix. With this
on top of James' patchset I sometimes see the message "Lost Interrupt
waiting for TPM stat", so I guess there needs to be a check in
wait_for_tpm_stat and request_locality to see if interrupts were
disabled when the wait_event_interruptible_timeout call times out.
[PATCH v2] tpm_tis: Disable interrupts if interrupt storm detected
When enabling the interrupt code for the tpm_tis driver we have
noticed some systems have a bios issue causing an interrupt storm to
occur. The issue isn't limited to a single tpm or system manufacturer
so keeping a denylist of systems with the issue isn't optimal. Instead
try to detect the problem occurring, disable interrupts, and revert to
polling when it happens.
Cc: Jarkko Sakkinen
Cc: Jason Gunthorpe
Cc: Peter Huewe
Cc: James Bottomley
Cc: Matthew Garrett
Cc: Hans de Goede
Signed-off-by: Jerry Snitselaar
---
v2: drop tpm_tis specific workqueue and use just system_wq
drivers/char/tpm/tpm_tis_core.c | 27 +++
drivers/char/tpm/tpm_tis_core.h | 2 ++
2 files changed, 29 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 23b60583928b..72cc8a5a152c 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -24,6 +24,8 @@
#include
#include
#include
+#include
+#include
#include "tpm.h"
#include "tpm_tis_core.h"
@@ -745,9 +747,23 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
{
struct tpm_chip *chip = dev_id;
struct tpm_tis_data *priv = dev_get_drvdata(>dev);
+ static bool check_storm = true;
+ static unsigned int check_start;
u32 interrupt;
int i, rc;
+ if (unlikely(check_storm)) {
+ if (!check_start) {
+ check_start = jiffies_to_msecs(jiffies);
+ } else if ((kstat_irqs(priv->irq) > 1000) &&
+ (jiffies_to_msecs(jiffies) - check_start < 500)) {
+ check_storm = false;
+ schedule_work(>storm_work);
+ } else if (jiffies_to_msecs(jiffies) - check_start >= 500) {
+ check_storm = false;
+ }
+ }
+
rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), );
if (rc < 0)
return IRQ_NONE;
@@ -987,6 +1003,14 @@ static const struct tpm_class_ops tpm_tis = {
.clk_enable = tpm_tis_clkrun_enable,
};
+static void tpm_tis_storm_work(struct work_struct *work)
+{
+ struct tpm_tis_data *priv = container_of(work, struct tpm_tis_data,
storm_work);
+
+ disable_interrupts(priv->chip);
+ dev_warn(>chip->dev, "Interrupt storm detected, using
polling.\n");
+}
+
int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
const struct tpm_tis_phy_ops *phy_ops,
acpi_handle acpi_dev_handle)
@@ -1003,6 +1027,9 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
if (IS_ERR(chip))
return PTR_ERR(chip);
+ priv->chip = chip;
+ INIT_WORK(>storm_work, tpm_tis_storm_work);
+
#ifdef CONFIG_ACPI
chip->acpi_dev_handle = acpi_dev_handle;
#endif
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index edeb5dc61c95..5630f294dc0c 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -95,6 +95,8 @@ struct tpm_tis_data {
u16 clkrun_enabled;
wait_queue_head_t int_queue;
wait_queue_head_t read_queue;
+ struct work_struct storm_work;
+ struct tpm_chip *chip;
const struct tpm_tis_phy_ops *phy_ops;
unsigned short rng_quality;
};
--
2.27.0
[PATCH] tpm_tis: Disable interrupts if interrupt storm detected
When enabling the interrupt code for the tpm_tis driver we have
noticed some systems have a bios issue causing an interrupt storm to
occur. The issue isn't limited to a single tpm or system vendor
so keeping a denylist of systems with the issue isn't optimal. Instead
try to detect the problem occurring, disable interrupts, and revert to
polling when it happens.
Cc: Jarkko Sakkinen
Cc: Jason Gunthorpe
Cc: Peter Huewe
Cc: James Bottomley
Cc: Matthew Garrett
Cc: Hans de Goede
Signed-off-by: Jerry Snitselaar
---
drivers/char/tpm/tpm_tis_core.c | 34 +
drivers/char/tpm/tpm_tis_core.h | 2 ++
2 files changed, 36 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 92c51c6cfd1b..19115a628f25 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -24,6 +24,8 @@
#include
#include
#include
+#include
+#include
#include "tpm.h"
#include "tpm_tis_core.h"
@@ -711,13 +713,30 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip,
u8 status)
}
}
+static struct workqueue_struct *tpm_tis_wq;
+static DEFINE_MUTEX(tpm_tis_wq_lock);
+
static irqreturn_t tis_int_handler(int dummy, void *dev_id)
{
struct tpm_chip *chip = dev_id;
struct tpm_tis_data *priv = dev_get_drvdata(>dev);
+ static bool check_storm = true;
+ static unsigned int check_start;
u32 interrupt;
int i, rc;
+ if (unlikely(check_storm)) {
+ if (!check_start) {
+ check_start = jiffies_to_msecs(jiffies);
+ } else if ((kstat_irqs(priv->irq) > 1000) &&
+ (jiffies_to_msecs(jiffies) - check_start < 500)) {
+ check_storm = false;
+ queue_work(tpm_tis_wq, >storm_work);
+ } else if (jiffies_to_msecs(jiffies) - check_start >= 500) {
+ check_storm = false;
+ }
+ }
+
rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), );
if (rc < 0)
return IRQ_NONE;
@@ -943,6 +962,14 @@ static const struct tpm_class_ops tpm_tis = {
.clk_enable = tpm_tis_clkrun_enable,
};
+static void tpm_tis_storm_work(struct work_struct *work)
+{
+ struct tpm_tis_data *priv = container_of(work, struct tpm_tis_data,
storm_work);
+
+ disable_interrupts(priv->chip);
+ dev_warn(>chip->dev, "Interrupt storm detected, using
polling.\n");
+}
+
int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
const struct tpm_tis_phy_ops *phy_ops,
acpi_handle acpi_dev_handle)
@@ -959,6 +986,13 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
if (IS_ERR(chip))
return PTR_ERR(chip);
+ priv->chip = chip;
+ tpm_tis_wq = alloc_workqueue("tpm_tis_wq", WQ_MEM_RECLAIM, 0);
+ if (!tpm_tis_wq)
+ return -ENOMEM;
+
+ INIT_WORK(>storm_work, tpm_tis_storm_work);
+
#ifdef CONFIG_ACPI
chip->acpi_dev_handle = acpi_dev_handle;
#endif
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 9b2d32a59f67..973297ee2e16 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -95,6 +95,8 @@ struct tpm_tis_data {
u16 clkrun_enabled;
wait_queue_head_t int_queue;
wait_queue_head_t read_queue;
+ struct work_struct storm_work;
+ struct tpm_chip *chip;
const struct tpm_tis_phy_ops *phy_ops;
unsigned short rng_quality;
};
--
2.27.0
Re: Question about domain_init (v5.3-v5.7)
Jerry Snitselaar @ 2020-11-30 10:50 MST: > Lu Baolu @ 2020-11-26 19:12 MST: > >> Hi Jerry, >> >> On 11/27/20 5:35 AM, Jerry Snitselaar wrote: >>> Lu Baolu @ 2020-11-26 04:01 MST: >>> >>>> Hi Jerry, >>>> >>>> On 2020/11/26 4:27, Jerry Snitselaar wrote: >>>>> Is there a reason we check the requested guest address width against >>>>> the >>>>> iommu's mgaw, instead of the agaw that we already know for the iommu? >>>>> I've run into a case with a new system where the mgaw reported is 57, >>>>> but if they set PAE to 46 instead of 52 in the bios, then sagaw reports >>>>> the highest supported agaw is 48 and the domain_init code fails here. In >>>> >>>> Isn't this a platform bug? If it's too late to fix it in the BIOS, you >>>> maybe have to add a platform specific quirk to set mgaw to the highest >>>> supported agaw? >>>> >>>> Best regards, >>>> baolu >>> Is there somewhere you can point me to that discusses how they >>> should be >>> setting the mgaw? I misunderstood when I previously asked you about >>> whether the mgaw could be a value that was greater than any of sagaw. >>> If it is a bios issue, then they should fix it there. >> >> MGAW indicates the max gpa width supported by 2nd translation. The VT-d >> spec requires that this value must be at least equal to the host >> physical addressibility. According to this, BIOS is good, right? >> >> For this failure case, domain_init() just wants to find a suitable agaw >> for the private domain. I think it makes sense to check against >> iommu->agaw instead of cap_mgaw. >> >> Best regards, >> baolu >> > > From this bit in the spec about MGAW: > > Guest addressability for a given DMA request is limited to the > minimum of the value reported through this field and the adjusted > guest address width of the corresponding page-table structure. > (Adjusted guest address widths supported by hardware are reported > through the SAGAW field). > > That does suggest it should be adjusted down to the sagaw value in this case, > yes? > Just want to make sure I'm understanding it correctly. Or I guess that is really talking about if you had an mgaw lower than the the sagaw, the dma request would be limited to that lower mgaw value. > >>> >>>> >>>>> other places like prepare_domain_attach_device, the dmar domain agaw >>>>> gets adjusted down to the iommu agaw. The agaw of the iommu gets >>>>> determined based off what is reported for sagaw. I'm wondering if it >>>>> can't instead do: >>>>> --- >>>>>drivers/iommu/intel-iommu.c | 4 ++-- >>>>>1 file changed, 2 insertions(+), 2 deletions(-) >>>>> diff --git a/drivers/iommu/intel-iommu.c >>>>> b/drivers/iommu/intel-iommu.c >>>>> index 6ca5c92ef2e5..a8e41ec36d9e 100644 >>>>> --- a/drivers/iommu/intel-iommu.c >>>>> +++ b/drivers/iommu/intel-iommu.c >>>>> @@ -1862,8 +1862,8 @@ static int domain_init(struct dmar_domain *domain, >>>>> struct intel_iommu *iommu, >>>>> domain_reserve_special_ranges(domain); >>>>> /* calculate AGAW */ >>>>> - if (guest_width > cap_mgaw(iommu->cap)) >>>>> - guest_width = cap_mgaw(iommu->cap); >>>>> + if (guest_width > agaw_to_width(iommu->agaw)) >>>>> + guest_width = agaw_to_width(iommu->agaw); >>>>> domain->gaw = guest_width; >>>>> adjust_width = guestwidth_to_adjustwidth(guest_width); >>>>> agaw = width_to_agaw(adjust_width); >>>>> -- >>>>> 2.27.0 >>>>> >>>>> Thoughts? With the former code the ehci device for the ilo fails when >>>>> trying to get a private domain. >>>>> Thanks, >>>>> Jerry >>>>> >>>
Re: Question about domain_init (v5.3-v5.7)
Lu Baolu @ 2020-11-26 19:12 MST: > Hi Jerry, > > On 11/27/20 5:35 AM, Jerry Snitselaar wrote: >> Lu Baolu @ 2020-11-26 04:01 MST: >> >>> Hi Jerry, >>> >>> On 2020/11/26 4:27, Jerry Snitselaar wrote: >>>> Is there a reason we check the requested guest address width against >>>> the >>>> iommu's mgaw, instead of the agaw that we already know for the iommu? >>>> I've run into a case with a new system where the mgaw reported is 57, >>>> but if they set PAE to 46 instead of 52 in the bios, then sagaw reports >>>> the highest supported agaw is 48 and the domain_init code fails here. In >>> >>> Isn't this a platform bug? If it's too late to fix it in the BIOS, you >>> maybe have to add a platform specific quirk to set mgaw to the highest >>> supported agaw? >>> >>> Best regards, >>> baolu >> Is there somewhere you can point me to that discusses how they >> should be >> setting the mgaw? I misunderstood when I previously asked you about >> whether the mgaw could be a value that was greater than any of sagaw. >> If it is a bios issue, then they should fix it there. > > MGAW indicates the max gpa width supported by 2nd translation. The VT-d > spec requires that this value must be at least equal to the host > physical addressibility. According to this, BIOS is good, right? > > For this failure case, domain_init() just wants to find a suitable agaw > for the private domain. I think it makes sense to check against > iommu->agaw instead of cap_mgaw. > > Best regards, > baolu > >From this bit in the spec about MGAW: Guest addressability for a given DMA request is limited to the minimum of the value reported through this field and the adjusted guest address width of the corresponding page-table structure. (Adjusted guest address widths supported by hardware are reported through the SAGAW field). That does suggest it should be adjusted down to the sagaw value in this case, yes? Just want to make sure I'm understanding it correctly. >> >>> >>>> other places like prepare_domain_attach_device, the dmar domain agaw >>>> gets adjusted down to the iommu agaw. The agaw of the iommu gets >>>> determined based off what is reported for sagaw. I'm wondering if it >>>> can't instead do: >>>> --- >>>>drivers/iommu/intel-iommu.c | 4 ++-- >>>>1 file changed, 2 insertions(+), 2 deletions(-) >>>> diff --git a/drivers/iommu/intel-iommu.c >>>> b/drivers/iommu/intel-iommu.c >>>> index 6ca5c92ef2e5..a8e41ec36d9e 100644 >>>> --- a/drivers/iommu/intel-iommu.c >>>> +++ b/drivers/iommu/intel-iommu.c >>>> @@ -1862,8 +1862,8 @@ static int domain_init(struct dmar_domain *domain, >>>> struct intel_iommu *iommu, >>>>domain_reserve_special_ranges(domain); >>>>/* calculate AGAW */ >>>> - if (guest_width > cap_mgaw(iommu->cap)) >>>> - guest_width = cap_mgaw(iommu->cap); >>>> + if (guest_width > agaw_to_width(iommu->agaw)) >>>> + guest_width = agaw_to_width(iommu->agaw); >>>>domain->gaw = guest_width; >>>>adjust_width = guestwidth_to_adjustwidth(guest_width); >>>>agaw = width_to_agaw(adjust_width); >>>> -- >>>> 2.27.0 >>>> >>>> Thoughts? With the former code the ehci device for the ilo fails when >>>> trying to get a private domain. >>>> Thanks, >>>> Jerry >>>> >>
Re: Question about domain_init (v5.3-v5.7)
Lu Baolu @ 2020-11-26 19:12 MST: > Hi Jerry, > > On 11/27/20 5:35 AM, Jerry Snitselaar wrote: >> Lu Baolu @ 2020-11-26 04:01 MST: >> >>> Hi Jerry, >>> >>> On 2020/11/26 4:27, Jerry Snitselaar wrote: >>>> Is there a reason we check the requested guest address width against >>>> the >>>> iommu's mgaw, instead of the agaw that we already know for the iommu? >>>> I've run into a case with a new system where the mgaw reported is 57, >>>> but if they set PAE to 46 instead of 52 in the bios, then sagaw reports >>>> the highest supported agaw is 48 and the domain_init code fails here. In >>> >>> Isn't this a platform bug? If it's too late to fix it in the BIOS, you >>> maybe have to add a platform specific quirk to set mgaw to the highest >>> supported agaw? >>> >>> Best regards, >>> baolu >> Is there somewhere you can point me to that discusses how they >> should be >> setting the mgaw? I misunderstood when I previously asked you about >> whether the mgaw could be a value that was greater than any of sagaw. >> If it is a bios issue, then they should fix it there. > > MGAW indicates the max gpa width supported by 2nd translation. The VT-d > spec requires that this value must be at least equal to the host > physical addressibility. According to this, BIOS is good, right? > Yes, the host address width is 46. MGAW reports 57 (56+1), and highest sagaw bit is for 48. > For this failure case, domain_init() just wants to find a suitable agaw > for the private domain. I think it makes sense to check against > iommu->agaw instead of cap_mgaw. > > Best regards, > baolu > >> >>> >>>> other places like prepare_domain_attach_device, the dmar domain agaw >>>> gets adjusted down to the iommu agaw. The agaw of the iommu gets >>>> determined based off what is reported for sagaw. I'm wondering if it >>>> can't instead do: >>>> --- >>>>drivers/iommu/intel-iommu.c | 4 ++-- >>>>1 file changed, 2 insertions(+), 2 deletions(-) >>>> diff --git a/drivers/iommu/intel-iommu.c >>>> b/drivers/iommu/intel-iommu.c >>>> index 6ca5c92ef2e5..a8e41ec36d9e 100644 >>>> --- a/drivers/iommu/intel-iommu.c >>>> +++ b/drivers/iommu/intel-iommu.c >>>> @@ -1862,8 +1862,8 @@ static int domain_init(struct dmar_domain *domain, >>>> struct intel_iommu *iommu, >>>>domain_reserve_special_ranges(domain); >>>>/* calculate AGAW */ >>>> - if (guest_width > cap_mgaw(iommu->cap)) >>>> - guest_width = cap_mgaw(iommu->cap); >>>> + if (guest_width > agaw_to_width(iommu->agaw)) >>>> + guest_width = agaw_to_width(iommu->agaw); >>>>domain->gaw = guest_width; >>>>adjust_width = guestwidth_to_adjustwidth(guest_width); >>>>agaw = width_to_agaw(adjust_width); >>>> -- >>>> 2.27.0 >>>> >>>> Thoughts? With the former code the ehci device for the ilo fails when >>>> trying to get a private domain. >>>> Thanks, >>>> Jerry >>>> >>
Re: Question about domain_init (v5.3-v5.7)
Lu Baolu @ 2020-11-26 04:01 MST: > Hi Jerry, > > On 2020/11/26 4:27, Jerry Snitselaar wrote: >> Is there a reason we check the requested guest address width against >> the >> iommu's mgaw, instead of the agaw that we already know for the iommu? >> I've run into a case with a new system where the mgaw reported is 57, >> but if they set PAE to 46 instead of 52 in the bios, then sagaw reports >> the highest supported agaw is 48 and the domain_init code fails here. In > > Isn't this a platform bug? If it's too late to fix it in the BIOS, you > maybe have to add a platform specific quirk to set mgaw to the highest > supported agaw? > > Best regards, > baolu Is there somewhere you can point me to that discusses how they should be setting the mgaw? I misunderstood when I previously asked you about whether the mgaw could be a value that was greater than any of sagaw. If it is a bios issue, then they should fix it there. > >> other places like prepare_domain_attach_device, the dmar domain agaw >> gets adjusted down to the iommu agaw. The agaw of the iommu gets >> determined based off what is reported for sagaw. I'm wondering if it >> can't instead do: >> --- >> drivers/iommu/intel-iommu.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> diff --git a/drivers/iommu/intel-iommu.c >> b/drivers/iommu/intel-iommu.c >> index 6ca5c92ef2e5..a8e41ec36d9e 100644 >> --- a/drivers/iommu/intel-iommu.c >> +++ b/drivers/iommu/intel-iommu.c >> @@ -1862,8 +1862,8 @@ static int domain_init(struct dmar_domain *domain, >> struct intel_iommu *iommu, >> domain_reserve_special_ranges(domain); >> /* calculate AGAW */ >> -if (guest_width > cap_mgaw(iommu->cap)) >> -guest_width = cap_mgaw(iommu->cap); >> +if (guest_width > agaw_to_width(iommu->agaw)) >> +guest_width = agaw_to_width(iommu->agaw); >> domain->gaw = guest_width; >> adjust_width = guestwidth_to_adjustwidth(guest_width); >> agaw = width_to_agaw(adjust_width); >> -- >> 2.27.0 >> >> Thoughts? With the former code the ehci device for the ilo fails when >> trying to get a private domain. >> Thanks, >> Jerry >>
Question about domain_init (v5.3-v5.7)
Is there a reason we check the requested guest address width against the iommu's mgaw, instead of the agaw that we already know for the iommu? I've run into a case with a new system where the mgaw reported is 57, but if they set PAE to 46 instead of 52 in the bios, then sagaw reports the highest supported agaw is 48 and the domain_init code fails here. In other places like prepare_domain_attach_device, the dmar domain agaw gets adjusted down to the iommu agaw. The agaw of the iommu gets determined based off what is reported for sagaw. I'm wondering if it can't instead do: --- drivers/iommu/intel-iommu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c index 6ca5c92ef2e5..a8e41ec36d9e 100644 --- a/drivers/iommu/intel-iommu.c +++ b/drivers/iommu/intel-iommu.c @@ -1862,8 +1862,8 @@ static int domain_init(struct dmar_domain *domain, struct intel_iommu *iommu, domain_reserve_special_ranges(domain); /* calculate AGAW */ - if (guest_width > cap_mgaw(iommu->cap)) - guest_width = cap_mgaw(iommu->cap); + if (guest_width > agaw_to_width(iommu->agaw)) + guest_width = agaw_to_width(iommu->agaw); domain->gaw = guest_width; adjust_width = guestwidth_to_adjustwidth(guest_width); agaw = width_to_agaw(adjust_width); -- 2.27.0 Thoughts? With the former code the ehci device for the ilo fails when trying to get a private domain. Thanks, Jerry
Re: [PATCH] tpm_tis: Disable interrupts on ThinkPad T490s
Jarkko Sakkinen @ 2020-11-23 20:26 MST:
> On Wed, Nov 18, 2020 at 11:36:20PM -0700, Jerry Snitselaar wrote:
>>
>> Matthew Garrett @ 2020-10-15 15:39 MST:
>>
>> > On Thu, Oct 15, 2020 at 2:44 PM Jerry Snitselaar
>> > wrote:
>> >>
>> >> There is a misconfiguration in the bios of the gpio pin used for the
>> >> interrupt in the T490s. When interrupts are enabled in the tpm_tis
>> >> driver code this results in an interrupt storm. This was initially
>> >> reported when we attempted to enable the interrupt code in the tpm_tis
>> >> driver, which previously wasn't setting a flag to enable it. Due to
>> >> the reports of the interrupt storm that code was reverted and we went back
>> >> to polling instead of using interrupts. Now that we know the T490s problem
>> >> is a firmware issue, add code to check if the system is a T490s and
>> >> disable interrupts if that is the case. This will allow us to enable
>> >> interrupts for everyone else. If the user has a fixed bios they can
>> >> force the enabling of interrupts with tpm_tis.interrupts=1 on the
>> >> kernel command line.
>> >
>> > I think an implication of this is that systems haven't been
>> > well-tested with interrupts enabled. In general when we've found a
>> > firmware issue in one place it ends up happening elsewhere as well, so
>> > it wouldn't surprise me if there are other machines that will also be
>> > unhappy with interrupts enabled. Would it be possible to automatically
>> > detect this case (eg, if we get more than a certain number of
>> > interrupts in a certain timeframe immediately after enabling the
>> > interrupt) and automatically fall back to polling in that case? It
>> > would also mean that users with fixed firmware wouldn't need to pass a
>> > parameter.
>>
>> I believe Matthew is correct here. I found another system today
>> with completely different vendor for both the system and the tpm chip.
>> In addition another Lenovo model, the L490, has the issue.
>>
>> This initial attempt at a solution like Matthew suggested works on
>> the system I found today, but I imagine it is all sorts of wrong.
>> In the 2 systems where I've seen it, there are about 10 interrupts
>> in around 1.5 seconds, and then the irq code shuts down the interrupt
>> because they aren't being handled.
>>
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c
>> b/drivers/char/tpm/tpm_tis_core.c
>> index 49ae09ac604f..478e9d02a3fa 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -27,6 +27,11 @@
>> #include "tpm.h"
>> #include "tpm_tis_core.h"
>>
>> +static unsigned int time_start = 0;
>> +static bool storm_check = true;
>> +static bool storm_killed = false;
>> +static u32 irqs_fired = 0;
>
> Maybe kstat_irqs() would be a better idea than ad hoc stats.
>
Thanks, yes that would be better.
>> +
>> static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value);
>>
>> static void tpm_tis_enable_interrupt(struct tpm_chip *chip, u8 mask)
>> @@ -464,25 +469,31 @@ static int tpm_tis_send_data(struct tpm_chip *chip,
>> const u8 *buf, size_t len)
>> return rc;
>> }
>>
>> -static void disable_interrupts(struct tpm_chip *chip)
>> +static void __disable_interrupts(struct tpm_chip *chip)
>> {
>> struct tpm_tis_data *priv = dev_get_drvdata(>dev);
>> u32 intmask;
>> int rc;
>>
>> - if (priv->irq == 0)
>> - return;
>> -
>> rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), );
>> if (rc < 0)
>> intmask = 0;
>>
>> intmask &= ~TPM_GLOBAL_INT_ENABLE;
>> rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
>> + chip->flags &= ~TPM_CHIP_FLAG_IRQ;
>> +}
>> +
>> +static void disable_interrupts(struct tpm_chip *chip)
>> +{
>> + struct tpm_tis_data *priv = dev_get_drvdata(>dev);
>>
>> + if (priv->irq == 0)
>> + return;
>> +
>> + __disable_interrupts(chip);
>> devm_free_irq(chip->dev.parent, priv->irq, chip);
>> priv->irq = 0;
>> - chip->flags &= ~TPM_CHIP_FLAG_IRQ;
>> }
>>
>> /*
>> @@ -528,6 +539,12 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf,
>&
Re: [PATCH] tpm_tis: Disable interrupts on ThinkPad T490s
Hans de Goede @ 2020-11-19 07:42 MST: > Hi, > > On 11/19/20 7:36 AM, Jerry Snitselaar wrote: >> >> Matthew Garrett @ 2020-10-15 15:39 MST: >> >>> On Thu, Oct 15, 2020 at 2:44 PM Jerry Snitselaar >>> wrote: >>>> >>>> There is a misconfiguration in the bios of the gpio pin used for the >>>> interrupt in the T490s. When interrupts are enabled in the tpm_tis >>>> driver code this results in an interrupt storm. This was initially >>>> reported when we attempted to enable the interrupt code in the tpm_tis >>>> driver, which previously wasn't setting a flag to enable it. Due to >>>> the reports of the interrupt storm that code was reverted and we went back >>>> to polling instead of using interrupts. Now that we know the T490s problem >>>> is a firmware issue, add code to check if the system is a T490s and >>>> disable interrupts if that is the case. This will allow us to enable >>>> interrupts for everyone else. If the user has a fixed bios they can >>>> force the enabling of interrupts with tpm_tis.interrupts=1 on the >>>> kernel command line. >>> >>> I think an implication of this is that systems haven't been >>> well-tested with interrupts enabled. In general when we've found a >>> firmware issue in one place it ends up happening elsewhere as well, so >>> it wouldn't surprise me if there are other machines that will also be >>> unhappy with interrupts enabled. Would it be possible to automatically >>> detect this case (eg, if we get more than a certain number of >>> interrupts in a certain timeframe immediately after enabling the >>> interrupt) and automatically fall back to polling in that case? It >>> would also mean that users with fixed firmware wouldn't need to pass a >>> parameter. >> >> I believe Matthew is correct here. I found another system today >> with completely different vendor for both the system and the tpm chip. >> In addition another Lenovo model, the L490, has the issue. >> >> This initial attempt at a solution like Matthew suggested works on >> the system I found today, but I imagine it is all sorts of wrong. >> In the 2 systems where I've seen it, there are about 10 interrupts >> in around 1.5 seconds, and then the irq code shuts down the interrupt >> because they aren't being handled. > > Is that with your patch? The IRQ should be silenced as soon as > devm_free_irq(chip->dev.parent, priv->irq, chip); is called. > No that is just with James' patchset that enables interrupts for tpm_tis. It looks like the irq is firing, but the tpm's int_status register is clear, so the handler immediately returns IRQ_NONE. After that happens 10 times the core irq code shuts down the irq, but it isn't released so I could still see the stats in /proc/interrupts. With my attempt below on top of that patchset it releases the irq. I had to stick the check prior to it checking the int_status register because it is cleared and the handler returns, and I couldn't do the devm_free_irq directly in tis_int_handler, so I tried sticking it in tpm_tis_send where the other odd irq testing code was already located. I'm not sure if there is another place that would work better for calling the devm_free_irq. > Depending on if we can get your storm-detection to work or not, > we might also choose to just never try to use the IRQ (at least on > x86 systems). AFAIK the TPM is never used for high-throughput stuff > so the polling overhead should not be a big deal (and I'm getting the feeling > that Windows always polls). > I was wondering about Windows as well. In addition to the Lenovo systems which the T490s had Nuvoton tpm, the system I found yesterday was a development system we have from a partner with an Infineon tpm. Dan Williams has seen it internally at Intel as well on some system. > Regards, > > Hans > > > >> diff --git a/drivers/char/tpm/tpm_tis_core.c >> b/drivers/char/tpm/tpm_tis_core.c >> index 49ae09ac604f..478e9d02a3fa 100644 >> --- a/drivers/char/tpm/tpm_tis_core.c >> +++ b/drivers/char/tpm/tpm_tis_core.c >> @@ -27,6 +27,11 @@ >> #include "tpm.h" >> #include "tpm_tis_core.h" >> >> +static unsigned int time_start = 0; >> +static bool storm_check = true; >> +static bool storm_killed = false; >> +static u32 irqs_fired = 0; >> + >> static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value); >> >> static void tpm_tis_enable_interrupt(struct tpm_chip *chip, u8 mask) >> @@ -464,25 +469,31 @@ static int tpm_tis_send_data(struct tpm_chip
Re: [PATCH] tpm_tis: Disable interrupts on ThinkPad T490s
Matthew Garrett @ 2020-10-15 15:39 MST:
> On Thu, Oct 15, 2020 at 2:44 PM Jerry Snitselaar wrote:
>>
>> There is a misconfiguration in the bios of the gpio pin used for the
>> interrupt in the T490s. When interrupts are enabled in the tpm_tis
>> driver code this results in an interrupt storm. This was initially
>> reported when we attempted to enable the interrupt code in the tpm_tis
>> driver, which previously wasn't setting a flag to enable it. Due to
>> the reports of the interrupt storm that code was reverted and we went back
>> to polling instead of using interrupts. Now that we know the T490s problem
>> is a firmware issue, add code to check if the system is a T490s and
>> disable interrupts if that is the case. This will allow us to enable
>> interrupts for everyone else. If the user has a fixed bios they can
>> force the enabling of interrupts with tpm_tis.interrupts=1 on the
>> kernel command line.
>
> I think an implication of this is that systems haven't been
> well-tested with interrupts enabled. In general when we've found a
> firmware issue in one place it ends up happening elsewhere as well, so
> it wouldn't surprise me if there are other machines that will also be
> unhappy with interrupts enabled. Would it be possible to automatically
> detect this case (eg, if we get more than a certain number of
> interrupts in a certain timeframe immediately after enabling the
> interrupt) and automatically fall back to polling in that case? It
> would also mean that users with fixed firmware wouldn't need to pass a
> parameter.
I believe Matthew is correct here. I found another system today
with completely different vendor for both the system and the tpm chip.
In addition another Lenovo model, the L490, has the issue.
This initial attempt at a solution like Matthew suggested works on
the system I found today, but I imagine it is all sorts of wrong.
In the 2 systems where I've seen it, there are about 10 interrupts
in around 1.5 seconds, and then the irq code shuts down the interrupt
because they aren't being handled.
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 49ae09ac604f..478e9d02a3fa 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -27,6 +27,11 @@
#include "tpm.h"
#include "tpm_tis_core.h"
+static unsigned int time_start = 0;
+static bool storm_check = true;
+static bool storm_killed = false;
+static u32 irqs_fired = 0;
+
static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value);
static void tpm_tis_enable_interrupt(struct tpm_chip *chip, u8 mask)
@@ -464,25 +469,31 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const
u8 *buf, size_t len)
return rc;
}
-static void disable_interrupts(struct tpm_chip *chip)
+static void __disable_interrupts(struct tpm_chip *chip)
{
struct tpm_tis_data *priv = dev_get_drvdata(>dev);
u32 intmask;
int rc;
- if (priv->irq == 0)
- return;
-
rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), );
if (rc < 0)
intmask = 0;
intmask &= ~TPM_GLOBAL_INT_ENABLE;
rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
+ chip->flags &= ~TPM_CHIP_FLAG_IRQ;
+}
+
+static void disable_interrupts(struct tpm_chip *chip)
+{
+ struct tpm_tis_data *priv = dev_get_drvdata(>dev);
+ if (priv->irq == 0)
+ return;
+
+ __disable_interrupts(chip);
devm_free_irq(chip->dev.parent, priv->irq, chip);
priv->irq = 0;
- chip->flags &= ~TPM_CHIP_FLAG_IRQ;
}
/*
@@ -528,6 +539,12 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf,
size_t len)
int rc, irq;
struct tpm_tis_data *priv = dev_get_drvdata(>dev);
+ if (unlikely(storm_killed)) {
+ devm_free_irq(chip->dev.parent, priv->irq, chip);
+ priv->irq = 0;
+ storm_killed = false;
+ }
+
if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
return tpm_tis_send_main(chip, buf, len);
@@ -748,6 +765,21 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id)
u32 interrupt;
int i, rc;
+ if (storm_check) {
+ irqs_fired++;
+
+ if (!time_start) {
+ time_start = jiffies_to_msecs(jiffies);
+ } else if ((irqs_fired > 1000) && (jiffies_to_msecs(jiffies) -
jiffies < 500)) {
+ __disable_interrupts(chip);
+ storm_check = false;
+ storm_killed = true;
+ return IRQ_HANDLED;
+ } else if ((jiffies_to_msecs(jiffies) - time_start > 500) &&
(irqs_fired < 1000)) {
+ storm_check = false;
+ }
+ }
+
rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), );
if (rc < 0)
return IRQ_NONE;
Re: [PATCH] tpm_tis: Disable interrupts on ThinkPad T490s
James should this get tacked on the end of your patchset? Regards, Jerry
[PATCH] tpm_tis: Disable interrupts on ThinkPad T490s
There is a misconfiguration in the bios of the gpio pin used for the
interrupt in the T490s. When interrupts are enabled in the tpm_tis
driver code this results in an interrupt storm. This was initially
reported when we attempted to enable the interrupt code in the tpm_tis
driver, which previously wasn't setting a flag to enable it. Due to
the reports of the interrupt storm that code was reverted and we went back
to polling instead of using interrupts. Now that we know the T490s problem
is a firmware issue, add code to check if the system is a T490s and
disable interrupts if that is the case. This will allow us to enable
interrupts for everyone else. If the user has a fixed bios they can
force the enabling of interrupts with tpm_tis.interrupts=1 on the
kernel command line.
Cc: jar...@kernel.org
Cc: Peter Huewe
Cc: Jason Gunthorpe
Cc: Hans de Goede
Reviewed-by: James Bottomley
Signed-off-by: Jerry Snitselaar
---
drivers/char/tpm/tpm_tis.c | 29 +++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 0b214963539d..4ed6e660273a 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "tpm.h"
#include "tpm_tis_core.h"
@@ -49,8 +50,8 @@ static inline struct tpm_tis_tcg_phy
*to_tpm_tis_tcg_phy(struct tpm_tis_data *da
return container_of(data, struct tpm_tis_tcg_phy, priv);
}
-static bool interrupts = true;
-module_param(interrupts, bool, 0444);
+static int interrupts = -1;
+module_param(interrupts, int, 0444);
MODULE_PARM_DESC(interrupts, "Enable interrupts");
static bool itpm;
@@ -63,6 +64,28 @@ module_param(force, bool, 0444);
MODULE_PARM_DESC(force, "Force device probe rather than using ACPI entry");
#endif
+static int tpm_tis_disable_irq(const struct dmi_system_id *d)
+{
+ if (interrupts == -1) {
+ pr_notice("tpm_tis: %s detected: disabling interrupts.\n",
d->ident);
+ interrupts = 0;
+ }
+
+ return 0;
+}
+
+static const struct dmi_system_id tpm_tis_dmi_table[] = {
+ {
+ .callback = tpm_tis_disable_irq,
+ .ident = "ThinkPad T490s",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T490s"),
+ },
+ },
+ {}
+};
+
#if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
static int has_hid(struct acpi_device *dev, const char *hid)
{
@@ -192,6 +215,8 @@ static int tpm_tis_init(struct device *dev, struct tpm_info
*tpm_info)
int irq = -1;
int rc;
+ dmi_check_system(tpm_tis_dmi_table);
+
rc = check_acpi_tpm2(dev);
if (rc)
return rc;
--
2.28.0
kdump boot failing with IVRS checksum failure
Hello Joerg,
We are seeing a kdump kernel boot failure in test on an HP DL325 Gen10
and it was tracked down to 387caf0b759a ("iommu/amd: Treat per-device
exclusion ranges as r/w unity-mapped regions"). Reproduced on 5.9-rc5
and goes away with revert of the commit. There is a follow on commit
that depends on this that was reverted as well 2ca6b6dc8512 ("iommu/amd:
Remove unused variable"). I'm working on getting system access and want
to see what the IVRS table looks like, but thought I'd give you heads
up.
Regards,
Jerry
Re: [PATCH v2 0/2] iommu: Move AMD and Intel Kconfig + Makefile bits into their directories
Jerry Snitselaar @ 2020-06-30 13:06 MST: > This patchset imeplements the suggestion from Linus to move the > Kconfig and Makefile bits for AMD and Intel into their respective > directories. > > v2: Rebase against v5.8-rc3. Dropped ---help--- changes from Kconfig as that > was > dealt with in systemwide cleanup. > > Jerry Snitselaar (2): > iommu/vt-d: Move Kconfig and Makefile bits down into intel directory > iommu/amd: Move Kconfig and Makefile bits down into amd directory > > > ___ > iommu mailing list > io...@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu Hi Joerg, Looks like I forgot to cc you on this cover letter for v2. Does this work for you now? Regards, Jerry
Re: [PATCH v9 2/2] tpm: Add support for event log pointer found in TPM2 ACPI table
Jarkko Sakkinen @ 2020-07-06 16:09 MST: > On Mon, Jul 06, 2020 at 02:19:53PM -0400, Stefan Berger wrote: >> From: Stefan Berger >> >> In case a TPM2 is attached, search for a TPM2 ACPI table when trying >> to get the event log from ACPI. If one is found, use it to get the >> start and length of the log area. This allows non-UEFI systems, such >> as SeaBIOS, to pass an event log when using a TPM2. >> >> Signed-off-by: Stefan Berger > > Do you think that QEMU with TPM 1.2 emulator turned on would be a viable > way to test this? > > I'm anyway more worried about breaking existing TPM 1.2 functionality > and that requires only QEMU without extras. > > /Jarkko The 1.2 bits should be functionally the same as before, right?
Re: [PATCH v9 2/2] tpm: Add support for event log pointer found in TPM2 ACPI table
Stefan Berger @ 2020-07-06 11:19 MST:
> From: Stefan Berger
>
> In case a TPM2 is attached, search for a TPM2 ACPI table when trying
> to get the event log from ACPI. If one is found, use it to get the
> start and length of the log area. This allows non-UEFI systems, such
> as SeaBIOS, to pass an event log when using a TPM2.
>
> Signed-off-by: Stefan Berger
Reviewed-by: Jerry Snitselaar
> ---
> drivers/char/tpm/eventlog/acpi.c | 63 +---
> 1 file changed, 42 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/char/tpm/eventlog/acpi.c
> b/drivers/char/tpm/eventlog/acpi.c
> index 63ada5e53f13..3633ed70f48f 100644
> --- a/drivers/char/tpm/eventlog/acpi.c
> +++ b/drivers/char/tpm/eventlog/acpi.c
> @@ -49,9 +49,9 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
> void __iomem *virt;
> u64 len, start;
> struct tpm_bios_log *log;
> -
> - if (chip->flags & TPM_CHIP_FLAG_TPM2)
> - return -ENODEV;
> + struct acpi_table_tpm2 *tbl;
> + struct acpi_tpm2_phy *tpm2_phy;
> + int format;
>
> log = >log;
>
> @@ -61,23 +61,44 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
> if (!chip->acpi_dev_handle)
> return -ENODEV;
>
> - /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
> - status = acpi_get_table(ACPI_SIG_TCPA, 1,
> - (struct acpi_table_header **));
> -
> - if (ACPI_FAILURE(status))
> - return -ENODEV;
> -
> - switch(buff->platform_class) {
> - case BIOS_SERVER:
> - len = buff->server.log_max_len;
> - start = buff->server.log_start_addr;
> - break;
> - case BIOS_CLIENT:
> - default:
> - len = buff->client.log_max_len;
> - start = buff->client.log_start_addr;
> - break;
> + if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> + status = acpi_get_table("TPM2", 1,
> + (struct acpi_table_header **));
> + if (ACPI_FAILURE(status))
> + return -ENODEV;
> +
> + if (tbl->header.length <
> + sizeof(*tbl) + sizeof(struct acpi_tpm2_phy))
> + return -ENODEV;
> +
> + tpm2_phy = (void *)tbl + sizeof(*tbl);
> + len = tpm2_phy->log_area_minimum_length;
> +
> + start = tpm2_phy->log_area_start_address;
> + if (!start || !len)
> + return -ENODEV;
> +
> + format = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
> + } else {
> + /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
> + status = acpi_get_table(ACPI_SIG_TCPA, 1,
> + (struct acpi_table_header **));
> + if (ACPI_FAILURE(status))
> + return -ENODEV;
> +
> + switch (buff->platform_class) {
> + case BIOS_SERVER:
> + len = buff->server.log_max_len;
> + start = buff->server.log_start_addr;
> + break;
> + case BIOS_CLIENT:
> + default:
> + len = buff->client.log_max_len;
> + start = buff->client.log_start_addr;
> + break;
> + }
> +
> + format = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> }
> if (!len) {
> dev_warn(>dev, "%s: TCPA log area empty\n", __func__);
> @@ -98,7 +119,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
> memcpy_fromio(log->bios_event_log, virt, len);
>
> acpi_os_unmap_iomem(virt, len);
> - return EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> + return format;
>
> err:
> kfree(log->bios_event_log);
Re: [PATCH v9 1/2] acpi: Extend TPM2 ACPI table with missing log fields
Stefan Berger @ 2020-07-06 11:19 MST:
> From: Stefan Berger
>
> Recent extensions of the TPM2 ACPI table added 3 more fields
> including 12 bytes of start method specific parameters and Log Area
> Minimum Length (u32) and Log Area Start Address (u64). So, we define
> a new structure acpi_tpm2_phy that holds these optional new fields.
> The new fields allow non-UEFI systems to access the TPM2's log.
>
> The specification that has the new fields is the following:
> TCG ACPI Specification
> Family "1.2" and "2.0"
> Version 1.2, Revision 8
>
> https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
>
> Signed-off-by: Stefan Berger
> Cc: linux-a...@vger.kernel.org
> Acked-by: Rafael J. Wysocki
> Reviewed-by: Jarkko Sakkinen
Reviewed-by: Jerry Snitselaar
> ---
> include/acpi/actbl3.h | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/include/acpi/actbl3.h b/include/acpi/actbl3.h
> index b0b163b9efc6..bdcac69fa6bd 100644
> --- a/include/acpi/actbl3.h
> +++ b/include/acpi/actbl3.h
> @@ -415,6 +415,13 @@ struct acpi_table_tpm2 {
> /* Platform-specific data follows */
> };
>
> +/* Optional trailer for revision 4 holding platform-specific data */
> +struct acpi_tpm2_phy {
> + u8 start_method_specific[12];
> + u32 log_area_minimum_length;
> + u64 log_area_start_address;
> +};
> +
> /* Values for start_method above */
>
> #define ACPI_TPM2_NOT_ALLOWED 0
Re: [PATCH] Revert commit e918e570415c ("tpm_tis: Remove the HID IFX0102")
Jarkko Sakkinen @ 2020-07-06 13:53 MST:
> Removing IFX0102 from tpm_tis was not a right move because both tpm_tis
> and tpm_infineon use the same device ID. Revert the commit and add a
> remark about a bug caused by commit 93e1b7d42e1e ("[PATCH] tpm: add HID
> module parameter").
>
> Fixes: e918e570415c ("tpm_tis: Remove the HID IFX0102")
> Reported-by: Peter Huewe
> Signed-off-by: Jarkko Sakkinen
Reviewed-by: Jerry Snitselaar
> ---
> drivers/char/tpm/tpm_tis.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index c58ea10fc92f..0b214963539d 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -235,9 +235,17 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
> return tpm_tis_init(_dev->dev, _info);
> }
>
> +/*
> + * There is a known bug caused by 93e1b7d42e1e ("[PATCH] tpm: add HID module
> + * parameter"). This commit added IFX0102 device ID, which is also used by
> + * tpm_infineon but ignored to add quirks to probe which driver ought to be
> + * used.
> + */
> +
> static struct pnp_device_id tpm_pnp_tbl[] = {
> {"PNP0C31", 0}, /* TPM */
> {"ATM1200", 0}, /* Atmel */
> + {"IFX0102", 0}, /* Infineon */
> {"BCM0101", 0}, /* Broadcom */
> {"BCM0102", 0}, /* Broadcom */
> {"NSC1200", 0}, /* National */
Re: [PATCH] tpm: Define TPM2_SPACE_BUFFER_SIZE to replace the use of PAGE_SIZE
On Fri Jul 03 20, Jarkko Sakkinen wrote:
The size of the buffers for storing context's and sessions can vary from
arch to arch as PAGE_SIZE can be anything between 4 kB and 256 kB (the
maximum for PPC64). Define a fixed buffer size set to 16 kB. This should be
enough for most use with three handles (that is how many we allow at the
moment). Parametrize the buffer size while doing this, so that it is easier
to revisit this later on if required.
Reported-by: Stefan Berger
Cc: sta...@vger.kernel.org
Fixes: 745b361e989a ("tpm: infrastructure for TPM spaces")
Signed-off-by: Jarkko Sakkinen
Reviewed-by: Jerry Snitselaar
---
v2: Also use the new buffer size for chip->work_buffer (reported by stefanb)
drivers/char/tpm/tpm-chip.c | 9 ++---
drivers/char/tpm/tpm.h| 5 -
drivers/char/tpm/tpm2-space.c | 26 --
drivers/char/tpm/tpmrm-dev.c | 2 +-
include/linux/tpm.h | 5 +++--
5 files changed, 26 insertions(+), 21 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 8c77e88012e9..ddaeceb7e109 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -386,13 +386,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
chip->cdev.owner = THIS_MODULE;
chip->cdevs.owner = THIS_MODULE;
- chip->work_space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
- if (!chip->work_space.context_buf) {
- rc = -ENOMEM;
- goto out;
- }
- chip->work_space.session_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
- if (!chip->work_space.session_buf) {
+ rc = tpm2_init_space(>work_space, TPM2_SPACE_BUFFER_SIZE);
+ if (rc) {
rc = -ENOMEM;
goto out;
}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 0fbcede241ea..947d1db0a5cc 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -59,6 +59,9 @@ enum tpm_addr {
#define TPM_TAG_RQU_COMMAND 193
+/* TPM2 specific constants. */
+#define TPM2_SPACE_BUFFER_SIZE 16384 /* 16 kB */
+
struct stclear_flags_t {
__be16 tag;
u8 deactivated;
@@ -228,7 +231,7 @@ unsigned long tpm2_calc_ordinal_duration(struct tpm_chip
*chip, u32 ordinal);
int tpm2_probe(struct tpm_chip *chip);
int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip);
int tpm2_find_cc(struct tpm_chip *chip, u32 cc);
-int tpm2_init_space(struct tpm_space *space);
+int tpm2_init_space(struct tpm_space *space, unsigned int buf_size);
void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space);
void tpm2_flush_space(struct tpm_chip *chip);
int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u8 *cmd,
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 982d341d8837..784b8b3cb903 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -38,18 +38,21 @@ static void tpm2_flush_sessions(struct tpm_chip *chip,
struct tpm_space *space)
}
}
-int tpm2_init_space(struct tpm_space *space)
+int tpm2_init_space(struct tpm_space *space, unsigned int buf_size)
{
- space->context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+ space->context_buf = kzalloc(buf_size, GFP_KERNEL);
if (!space->context_buf)
return -ENOMEM;
- space->session_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+ space->session_buf = kzalloc(buf_size, GFP_KERNEL);
if (space->session_buf == NULL) {
kfree(space->context_buf);
+ /* Prevent caller getting a dangling pointer. */
+ space->context_buf = NULL;
return -ENOMEM;
}
+ space->buf_size = buf_size;
return 0;
}
@@ -311,8 +314,10 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct
tpm_space *space, u8 *cmd,
sizeof(space->context_tbl));
memcpy(>work_space.session_tbl, >session_tbl,
sizeof(space->session_tbl));
- memcpy(chip->work_space.context_buf, space->context_buf, PAGE_SIZE);
- memcpy(chip->work_space.session_buf, space->session_buf, PAGE_SIZE);
+ memcpy(chip->work_space.context_buf, space->context_buf,
+ space->buf_size);
+ memcpy(chip->work_space.session_buf, space->session_buf,
+ space->buf_size);
rc = tpm2_load_space(chip);
if (rc) {
@@ -492,7 +497,7 @@ static int tpm2_save_space(struct tpm_chip *chip)
continue;
rc = tpm2_save_context(chip, space->context_tbl[i],
- space->context_buf, PAGE_SIZE,
+ space->context_buf, space->buf_size,
);
if (rc == -ENOENT) {
space->context_tbl[i] = 0;
@@ -509,9 +514,8 @@ static int tpm2_s
[PATCH v2 1/2] iommu/vt-d: Move Kconfig and Makefile bits down into intel directory
Move Intel Kconfig and Makefile bits down into intel directory with the rest of the Intel specific files. Cc: Joerg Roedel Cc: Lu Baolu Signed-off-by: Jerry Snitselaar --- drivers/iommu/Kconfig| 86 +--- drivers/iommu/Makefile | 8 +--- drivers/iommu/intel/Kconfig | 86 drivers/iommu/intel/Makefile | 7 +++ 4 files changed, 96 insertions(+), 91 deletions(-) create mode 100644 drivers/iommu/intel/Kconfig create mode 100644 drivers/iommu/intel/Makefile diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index 6dc49ed8377a..281cd6bd0fe0 100644 --- a/drivers/iommu/Kconfig +++ b/drivers/iommu/Kconfig @@ -176,91 +176,7 @@ config AMD_IOMMU_DEBUGFS This option is -NOT- intended for production environments, and should not generally be enabled. -# Intel IOMMU support -config DMAR_TABLE - bool - -config INTEL_IOMMU - bool "Support for Intel IOMMU using DMA Remapping Devices" - depends on PCI_MSI && ACPI && (X86 || IA64) - select IOMMU_API - select IOMMU_IOVA - select NEED_DMA_MAP_STATE - select DMAR_TABLE - select SWIOTLB - select IOASID - help - DMA remapping (DMAR) devices support enables independent address - translations for Direct Memory Access (DMA) from devices. - These DMA remapping devices are reported via ACPI tables - and include PCI device scope covered by these DMA - remapping devices. - -config INTEL_IOMMU_DEBUGFS - bool "Export Intel IOMMU internals in Debugfs" - depends on INTEL_IOMMU && IOMMU_DEBUGFS - help - !!!WARNING!!! - - DO NOT ENABLE THIS OPTION UNLESS YOU REALLY KNOW WHAT YOU ARE DOING!!! - - Expose Intel IOMMU internals in Debugfs. - - This option is -NOT- intended for production environments, and should - only be enabled for debugging Intel IOMMU. - -config INTEL_IOMMU_SVM - bool "Support for Shared Virtual Memory with Intel IOMMU" - depends on INTEL_IOMMU && X86_64 - select PCI_PASID - select PCI_PRI - select MMU_NOTIFIER - select IOASID - help - Shared Virtual Memory (SVM) provides a facility for devices - to access DMA resources through process address space by - means of a Process Address Space ID (PASID). - -config INTEL_IOMMU_DEFAULT_ON - def_bool y - prompt "Enable Intel DMA Remapping Devices by default" - depends on INTEL_IOMMU - help - Selecting this option will enable a DMAR device at boot time if - one is found. If this option is not selected, DMAR support can - be enabled by passing intel_iommu=on to the kernel. - -config INTEL_IOMMU_BROKEN_GFX_WA - bool "Workaround broken graphics drivers (going away soon)" - depends on INTEL_IOMMU && BROKEN && X86 - help - Current Graphics drivers tend to use physical address - for DMA and avoid using DMA APIs. Setting this config - option permits the IOMMU driver to set a unity map for - all the OS-visible memory. Hence the driver can continue - to use physical addresses for DMA, at least until this - option is removed in the 2.6.32 kernel. - -config INTEL_IOMMU_FLOPPY_WA - def_bool y - depends on INTEL_IOMMU && X86 - help - Floppy disk drivers are known to bypass DMA API calls - thereby failing to work when IOMMU is enabled. This - workaround will setup a 1:1 mapping for the first - 16MiB to make floppy (an ISA device) work. - -config INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON - bool "Enable Intel IOMMU scalable mode by default" - depends on INTEL_IOMMU - help - Selecting this option will enable by default the scalable mode if - hardware presents the capability. The scalable mode is defined in - VT-d 3.0. The scalable mode capability could be checked by reading - /sys/devices/virtual/iommu/dmar*/intel-iommu/ecap. If this option - is not selected, scalable mode support could also be enabled by - passing intel_iommu=sm_on to the kernel. If not sure, please use - the default value. +source "drivers/iommu/intel/Kconfig" config IRQ_REMAP bool "Support for Interrupt Remapping" diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile index 342190196dfb..71dd2f382e78 100644 --- a/drivers/iommu/Makefile +++ b/drivers/iommu/Makefile @@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 +obj-y += intel/ obj-$(CONFIG_IOMMU_API) += iommu.o obj-$(CONFIG_IOMMU_API) += iommu-traces.o obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o @@ -17,13 +18,8 @@ obj-$(CONFIG_AMD_IOMMU_V2) += amd/iommu_v2.o obj-$(CONFIG_ARM_SMMU) += arm_smmu.o arm_smmu-objs
[PATCH v2 0/2] iommu: Move AMD and Intel Kconfig + Makefile bits into their directories
This patchset imeplements the suggestion from Linus to move the Kconfig and Makefile bits for AMD and Intel into their respective directories. v2: Rebase against v5.8-rc3. Dropped ---help--- changes from Kconfig as that was dealt with in systemwide cleanup. Jerry Snitselaar (2): iommu/vt-d: Move Kconfig and Makefile bits down into intel directory iommu/amd: Move Kconfig and Makefile bits down into amd directory
[PATCH v2 2/2] iommu/amd: Move Kconfig and Makefile bits down into amd directory
Move AMD Kconfig and Makefile bits down into the amd directory with the rest of the AMD specific files. Cc: Joerg Roedel Cc: Suravee Suthikulpanit Signed-off-by: Jerry Snitselaar --- drivers/iommu/Kconfig | 45 +- drivers/iommu/Makefile | 5 + drivers/iommu/amd/Kconfig | 44 + drivers/iommu/amd/Makefile | 4 4 files changed, 50 insertions(+), 48 deletions(-) create mode 100644 drivers/iommu/amd/Kconfig create mode 100644 drivers/iommu/amd/Makefile diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index 281cd6bd0fe0..24000e7ed0fa 100644 --- a/drivers/iommu/Kconfig +++ b/drivers/iommu/Kconfig @@ -132,50 +132,7 @@ config IOMMU_PGTABLES_L2 def_bool y depends on MSM_IOMMU && MMU && SMP && CPU_DCACHE_DISABLE=n -# AMD IOMMU support -config AMD_IOMMU - bool "AMD IOMMU support" - select SWIOTLB - select PCI_MSI - select PCI_ATS - select PCI_PRI - select PCI_PASID - select IOMMU_API - select IOMMU_IOVA - select IOMMU_DMA - depends on X86_64 && PCI && ACPI - help - With this option you can enable support for AMD IOMMU hardware in - your system. An IOMMU is a hardware component which provides - remapping of DMA memory accesses from devices. With an AMD IOMMU you - can isolate the DMA memory of different devices and protect the - system from misbehaving device drivers or hardware. - - You can find out if your system has an AMD IOMMU if you look into - your BIOS for an option to enable it or if you have an IVRS ACPI - table. - -config AMD_IOMMU_V2 - tristate "AMD IOMMU Version 2 driver" - depends on AMD_IOMMU - select MMU_NOTIFIER - help - This option enables support for the AMD IOMMUv2 features of the IOMMU - hardware. Select this option if you want to use devices that support - the PCI PRI and PASID interface. - -config AMD_IOMMU_DEBUGFS - bool "Enable AMD IOMMU internals in DebugFS" - depends on AMD_IOMMU && IOMMU_DEBUGFS - help - !!!WARNING!!! !!!WARNING!!! !!!WARNING!!! !!!WARNING!!! - - DO NOT ENABLE THIS OPTION UNLESS YOU REALLY, -REALLY- KNOW WHAT YOU ARE DOING!!! - Exposes AMD IOMMU device internals in DebugFS. - - This option is -NOT- intended for production environments, and should - not generally be enabled. - +source "drivers/iommu/amd/Kconfig" source "drivers/iommu/intel/Kconfig" config IRQ_REMAP diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile index 71dd2f382e78..f356bc12b1c7 100644 --- a/drivers/iommu/Makefile +++ b/drivers/iommu/Makefile @@ -1,5 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 -obj-y += intel/ +obj-y += amd/ intel/ obj-$(CONFIG_IOMMU_API) += iommu.o obj-$(CONFIG_IOMMU_API) += iommu-traces.o obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o @@ -12,9 +12,6 @@ obj-$(CONFIG_IOASID) += ioasid.o obj-$(CONFIG_IOMMU_IOVA) += iova.o obj-$(CONFIG_OF_IOMMU) += of_iommu.o obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o -obj-$(CONFIG_AMD_IOMMU) += amd/iommu.o amd/init.o amd/quirks.o -obj-$(CONFIG_AMD_IOMMU_DEBUGFS) += amd/debugfs.o -obj-$(CONFIG_AMD_IOMMU_V2) += amd/iommu_v2.o obj-$(CONFIG_ARM_SMMU) += arm_smmu.o arm_smmu-objs += arm-smmu.o arm-smmu-impl.o arm-smmu-qcom.o obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o diff --git a/drivers/iommu/amd/Kconfig b/drivers/iommu/amd/Kconfig new file mode 100644 index ..1f061d91e0b8 --- /dev/null +++ b/drivers/iommu/amd/Kconfig @@ -0,0 +1,44 @@ +# SPDX-License-Identifier: GPL-2.0-only +# AMD IOMMU support +config AMD_IOMMU + bool "AMD IOMMU support" + select SWIOTLB + select PCI_MSI + select PCI_ATS + select PCI_PRI + select PCI_PASID + select IOMMU_API + select IOMMU_IOVA + select IOMMU_DMA + depends on X86_64 && PCI && ACPI + help + With this option you can enable support for AMD IOMMU hardware in + your system. An IOMMU is a hardware component which provides + remapping of DMA memory accesses from devices. With an AMD IOMMU you + can isolate the DMA memory of different devices and protect the + system from misbehaving device drivers or hardware. + + You can find out if your system has an AMD IOMMU if you look into + your BIOS for an option to enable it or if you have an IVRS ACPI + table. + +config AMD_IOMMU_V2 + tristate "AMD IOMMU Version 2 driver" + depends on AMD_IOMMU + select MMU_NOTIFIER + help + This option enables support for the AMD IOMMUv2 features of the IOMMU + hardware. Select this option if you want to use devices that support + the PCI PRI and PASID int
Re: [PATCH v2] tpm_tis: Remove the HID IFX0102
On Fri Jun 26 20, Jarkko Sakkinen wrote:
On Thu, Jun 25, 2020 at 02:19:23PM -0700, Jerry Snitselaar wrote:
On Fri Jun 26 20, Jarkko Sakkinen wrote:
> On Wed, Jun 24, 2020 at 11:21:50PM -0700, Jerry Snitselaar wrote:
> > On Thu Jun 25 20, Jarkko Sakkinen wrote:
> > > Acer C720 running Linux v5.3 reports this in klog:
> > >
> > > tpm_tis: 1.2 TPM (device-id 0xB, rev-id 16)
> > > tpm tpm0: tpm_try_transmit: send(): error -5
> > > tpm tpm0: A TPM error (-5) occurred attempting to determine the timeouts
> > > tpm_tis tpm_tis: Could not get TPM timeouts and durations
> > > tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16)
> > > tpm tpm0: tpm_try_transmit: send(): error -5
> > > tpm tpm0: A TPM error (-5) occurred attempting to determine the timeouts
> > > tpm_tis 00:08: Could not get TPM timeouts and durations
> > > ima: No TPM chip found, activating TPM-bypass!
> > > tpm_inf_pnp 00:08: Found TPM with ID IFX0102
> > >
> > > % git --no-pager grep IFX0102 drivers/char/tpm
> > > drivers/char/tpm/tpm_infineon.c: {"IFX0102", 0},
> > > drivers/char/tpm/tpm_tis.c: {"IFX0102", 0}, /*
Infineon */
> > >
> > > Obviously IFX0102 was added to the HID table for the TCG TIS driver by
> > > mistake.
> > >
> > > Fixes: 93e1b7d42e1e ("[PATCH] tpm: add HID module parameter")
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=203877
> > > Cc: Kylene Jo Hall
> > > Reported-by: Ferry Toth:
> > > Signed-off-by: Jarkko Sakkinen
> >
> > Reviewed-by: Jerry Snitselaar
>
> Bugzilla has an example of similar behavior with v4.15. I'll apply this
> asap.
>
> /Jarkko
>
Any idea what happened to git.infradead.org? It was offline the other day,
and at the moment not all repos from before seem to be there.
Now the kernel tree is back online.
/Jarkko
Hi Jarkko, I still see your linux-tpmdd repository as not being online:
git remote show tpmdd
fatal: remote error: access denied or repository not exported:
/users/jjs/linux-tpmdd.git
Regards,
Jerry
Re: [PATCH 00/13] iommu: Remove usage of dev->archdata.iommu
On Thu Jun 25 20, Joerg Roedel wrote: From: Joerg Roedel Hi, here is a patch-set to remove the usage of dev->archdata.iommu from the IOMMU code in the kernel and replace its uses by the iommu per-device private data field. The changes also remove the field entirely from the architectures which no longer need it. On PowerPC the field is called dev->archdata.iommu_domain and was only used by the PAMU IOMMU driver. It gets removed as well. The patches have been runtime tested on Intel VT-d and compile tested with allyesconfig for: * x86 (32 and 64 bit) * arm and arm64 * ia64 (only drivers/ because build failed for me in arch/ia64) * PPC64 Besides that the changes also survived my IOMMU tree compile tests. Please review. Regards, Joerg Joerg Roedel (13): iommu/exynos: Use dev_iommu_priv_get/set() iommu/vt-d: Use dev_iommu_priv_get/set() iommu/msm: Use dev_iommu_priv_get/set() iommu/omap: Use dev_iommu_priv_get/set() iommu/rockchip: Use dev_iommu_priv_get/set() iommu/tegra: Use dev_iommu_priv_get/set() iommu/pamu: Use dev_iommu_priv_get/set() iommu/mediatek: Do no use dev->archdata.iommu x86: Remove dev->archdata.iommu pointer ia64: Remove dev->archdata.iommu pointer arm: Remove dev->archdata.iommu pointer arm64: Remove dev->archdata.iommu pointer powerpc/dma: Remove dev->archdata.iommu_domain arch/arm/include/asm/device.h | 3 --- arch/arm64/include/asm/device.h | 3 --- arch/ia64/include/asm/device.h| 3 --- arch/powerpc/include/asm/device.h | 3 --- arch/x86/include/asm/device.h | 3 --- .../gpu/drm/i915/selftests/mock_gem_device.c | 10 -- drivers/iommu/exynos-iommu.c | 20 +-- drivers/iommu/fsl_pamu_domain.c | 8 drivers/iommu/intel/iommu.c | 18 - drivers/iommu/msm_iommu.c | 4 ++-- drivers/iommu/mtk_iommu.h | 2 ++ drivers/iommu/mtk_iommu_v1.c | 10 -- drivers/iommu/omap-iommu.c| 20 +-- drivers/iommu/rockchip-iommu.c| 8 drivers/iommu/tegra-gart.c| 8 drivers/iommu/tegra-smmu.c| 8 .../media/platform/s5p-mfc/s5p_mfc_iommu.h| 4 +++- 17 files changed, 64 insertions(+), 71 deletions(-) -- 2.27.0 Reviewed-by: Jerry Snitselaar
Re: [PATCH v2] tpm: tpm2-space: Resize session and context buffers dynamically
On Thu Jun 25 20, Jarkko Sakkinen wrote:
Re-allocate context and session buffers when needed. Scale them in page
increments so that the reallocation is only seldomly required, and thus
causes minimal stress to the system. Add a static maximum limit of four
pages for buffer sizes.
Cc: James Bottomley
Suggested-by: Stefan Berger
Signed-off-by: Jarkko Sakkinen
---
Tested only for compilation.
v2: TPM2_SPACE_DEFAULT_BUFFER_SIZE
drivers/char/tpm/tpm2-space.c | 87 ---
include/linux/tpm.h | 6 ++-
2 files changed, 64 insertions(+), 29 deletions(-)
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 982d341d8837..b8ece01d6afb 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -15,6 +15,9 @@
#include
#include "tpm.h"
+#define TPM2_SPACE_DEFAULT_BUFFER_SIZE PAGE_SIZE
+#define TPM2_SPACE_MAX_BUFFER_SIZE (4 * PAGE_SIZE)
+
enum tpm2_handle_types {
TPM2_HT_HMAC_SESSION= 0x0200,
TPM2_HT_POLICY_SESSION = 0x0300,
@@ -40,16 +43,21 @@ static void tpm2_flush_sessions(struct tpm_chip *chip,
struct tpm_space *space)
int tpm2_init_space(struct tpm_space *space)
{
- space->context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+ space->context_buf = kzalloc(TPM2_SPACE_DEFAULT_BUFFER_SIZE,
+GFP_KERNEL);
if (!space->context_buf)
return -ENOMEM;
- space->session_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+ space->session_buf = kzalloc(TPM2_SPACE_DEFAULT_BUFFER_SIZE,
+GFP_KERNEL);
if (space->session_buf == NULL) {
kfree(space->context_buf);
+ space->context_buf = NULL;
return -ENOMEM;
}
+ space->context_size = TPM2_SPACE_DEFAULT_BUFFER_SIZE;
+ space->session_size = TPM2_SPACE_DEFAULT_BUFFER_SIZE;
return 0;
}
@@ -116,11 +124,13 @@ static int tpm2_load_context(struct tpm_chip *chip, u8
*buf,
return 0;
}
-static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
-unsigned int buf_size, unsigned int *offset)
+static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 **buf,
+unsigned int *buf_size, unsigned int *offset)
{
- struct tpm_buf tbuf;
+ unsigned int new_buf_size;
unsigned int body_size;
+ struct tpm_buf tbuf;
+ void *new_buf;
int rc;
rc = tpm_buf_init(, TPM2_ST_NO_SESSIONS, TPM2_CC_CONTEXT_SAVE);
@@ -131,31 +141,48 @@ static int tpm2_save_context(struct tpm_chip *chip, u32
handle, u8 *buf,
rc = tpm_transmit_cmd(chip, , 0, NULL);
if (rc < 0) {
- dev_warn(>dev, "%s: failed with a system error %d\n",
-__func__, rc);
- tpm_buf_destroy();
- return -EFAULT;
+ rc = -EFAULT;
+ goto err;
} else if (tpm2_rc_value(rc) == TPM2_RC_REFERENCE_H0) {
- tpm_buf_destroy();
- return -ENOENT;
+ rc = -ENOENT;
+ goto out;
} else if (rc) {
- dev_warn(>dev, "%s: failed with a TPM error 0x%04X\n",
-__func__, rc);
- tpm_buf_destroy();
- return -EFAULT;
+ rc = -EFAULT;
+ goto err;
}
Would it be worthwhile to still output something here since it is changing
the value of rc returned from tpm_transmit_cmd()? Wondering if it would
be useful for debugging to know what the returned error was. Other than
that question looks good to me pending what is decided on using PAGE_SIZE.
Regards,
Jerry
body_size = tpm_buf_length() - TPM_HEADER_SIZE;
- if ((*offset + body_size) > buf_size) {
- dev_warn(>dev, "%s: out of backing storage\n", __func__);
- tpm_buf_destroy();
- return -ENOMEM;
+ /* We grow the buffer in page increments. */
+ new_buf_size = PFN_UP(*offset + body_size);
+
+ if (new_buf_size > TPM2_SPACE_MAX_BUFFER_SIZE) {
+ rc = -ENOMEM;
+ goto err;
}
- memcpy([*offset], [TPM_HEADER_SIZE], body_size);
+ if (new_buf_size > *buf_size) {
+ new_buf = krealloc(*buf, new_buf_size, GFP_KERNEL);
+ if (!new_buf) {
+ rc = -ENOMEM;
+ goto err;
+ }
+
+ *buf = new_buf;
+ *buf_size = new_buf_size;
+ }
+
+ memcpy(*buf + *offset, [TPM_HEADER_SIZE], body_size);
*offset += body_size;
+
+out:
tpm_buf_destroy();
- return 0;
+ return rc;
+
+err:
+ dev_warn(>dev, "%s: ret=%d\n", __func__, rc);
+
+ tpm_buf_destroy();
+ return rc;
}
void tpm2_flush_space(struct tpm_chip *chip)
@@ -311,8 +338,10 @@ int tpm2_prepare_space(struct tpm_chip *chip,
Re: [PATCH v2] tpm_tis: Remove the HID IFX0102
On Fri Jun 26 20, Jarkko Sakkinen wrote:
On Wed, Jun 24, 2020 at 11:21:50PM -0700, Jerry Snitselaar wrote:
On Thu Jun 25 20, Jarkko Sakkinen wrote:
> Acer C720 running Linux v5.3 reports this in klog:
>
> tpm_tis: 1.2 TPM (device-id 0xB, rev-id 16)
> tpm tpm0: tpm_try_transmit: send(): error -5
> tpm tpm0: A TPM error (-5) occurred attempting to determine the timeouts
> tpm_tis tpm_tis: Could not get TPM timeouts and durations
> tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16)
> tpm tpm0: tpm_try_transmit: send(): error -5
> tpm tpm0: A TPM error (-5) occurred attempting to determine the timeouts
> tpm_tis 00:08: Could not get TPM timeouts and durations
> ima: No TPM chip found, activating TPM-bypass!
> tpm_inf_pnp 00:08: Found TPM with ID IFX0102
>
> % git --no-pager grep IFX0102 drivers/char/tpm
> drivers/char/tpm/tpm_infineon.c: {"IFX0102", 0},
> drivers/char/tpm/tpm_tis.c:{"IFX0102", 0}, /* Infineon
*/
>
> Obviously IFX0102 was added to the HID table for the TCG TIS driver by
> mistake.
>
> Fixes: 93e1b7d42e1e ("[PATCH] tpm: add HID module parameter")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=203877
> Cc: Kylene Jo Hall
> Reported-by: Ferry Toth:
> Signed-off-by: Jarkko Sakkinen
Reviewed-by: Jerry Snitselaar
Bugzilla has an example of similar behavior with v4.15. I'll apply this
asap.
/Jarkko
Any idea what happened to git.infradead.org? It was offline the other day,
and at the moment not all repos from before seem to be there.
Re: [PATCH v2] tpm_tis: Remove the HID IFX0102
On Thu Jun 25 20, Jarkko Sakkinen wrote:
Acer C720 running Linux v5.3 reports this in klog:
tpm_tis: 1.2 TPM (device-id 0xB, rev-id 16)
tpm tpm0: tpm_try_transmit: send(): error -5
tpm tpm0: A TPM error (-5) occurred attempting to determine the timeouts
tpm_tis tpm_tis: Could not get TPM timeouts and durations
tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16)
tpm tpm0: tpm_try_transmit: send(): error -5
tpm tpm0: A TPM error (-5) occurred attempting to determine the timeouts
tpm_tis 00:08: Could not get TPM timeouts and durations
ima: No TPM chip found, activating TPM-bypass!
tpm_inf_pnp 00:08: Found TPM with ID IFX0102
% git --no-pager grep IFX0102 drivers/char/tpm
drivers/char/tpm/tpm_infineon.c:{"IFX0102", 0},
drivers/char/tpm/tpm_tis.c: {"IFX0102", 0}, /* Infineon */
Obviously IFX0102 was added to the HID table for the TCG TIS driver by
mistake.
Fixes: 93e1b7d42e1e ("[PATCH] tpm: add HID module parameter")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=203877
Cc: Kylene Jo Hall
Reported-by: Ferry Toth:
Signed-off-by: Jarkko Sakkinen
Reviewed-by: Jerry Snitselaar
Re: [PATCHv2] tpm: ibmvtpm: Wait for ready buffer before probing for TPM2 attributes
On Fri Jun 19 20, David Gibson wrote:
The tpm2_get_cc_attrs_tbl() call will result in TPM commands being issued,
which will need the use of the internal command/response buffer. But,
we're issuing this *before* we've waited to make sure that buffer is
allocated.
This can result in intermittent failures to probe if the hypervisor / TPM
implementation doesn't respond quickly enough. I find it fails almost
every time with an 8 vcpu guest under KVM with software emulated TPM.
To fix it, just move the tpm2_get_cc_attrs_tlb() call after the
existing code to wait for initialization, which will ensure the buffer
is allocated.
Fixes: 18b3670d79ae9 ("tpm: ibmvtpm: Add support for TPM2")
Signed-off-by: David Gibson
---
Reviewed-by: Jerry Snitselaar
Changes from v1:
* Fixed a formatting error in the commit message
* Added some more detail to the commit message
drivers/char/tpm/tpm_ibmvtpm.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 09fe45246b8cc..994385bf37c0c 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -683,13 +683,6 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
if (rc)
goto init_irq_cleanup;
- if (!strcmp(id->compat, "IBM,vtpm20")) {
- chip->flags |= TPM_CHIP_FLAG_TPM2;
- rc = tpm2_get_cc_attrs_tbl(chip);
- if (rc)
- goto init_irq_cleanup;
- }
-
if (!wait_event_timeout(ibmvtpm->crq_queue.wq,
ibmvtpm->rtce_buf != NULL,
HZ)) {
@@ -697,6 +690,13 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
goto init_irq_cleanup;
}
+ if (!strcmp(id->compat, "IBM,vtpm20")) {
+ chip->flags |= TPM_CHIP_FLAG_TPM2;
+ rc = tpm2_get_cc_attrs_tbl(chip);
+ if (rc)
+ goto init_irq_cleanup;
+ }
+
return tpm_chip_register(chip);
init_irq_cleanup:
do {
--
2.26.2
Re: [PATCH 1/1] iommu/vt-d: Fix misuse of iommu_domain_identity_map()
On Fri Jun 19 20, Lu Baolu wrote:
The iommu_domain_identity_map() helper takes start/end PFN as arguments.
Fix a misuse case where the start and end addresses are passed.
Fixes: e70b081c6f376 ("iommu/vt-d: Remove IOVA handling code from the non-dma_ops
path")
Cc: Tom Murphy
Reported-by: Alex Williamson
Signed-off-by: Lu Baolu
Reviewed-by: Jerry Snitselaar
[PATCH 0/2] iommu: Move AMD and Intel Kconfig + Makefile bits into their directories
This patchset imeplements the suggestion from Linus to move the Kconfig and Makefile bits for AMD and Intel into their respective directories. It also cleans up a couple Kconfig entries to use the newer help attribute instead of ---help--- (complaint from checkpatch). Jerry Snitselaar (2): iommu/vt-d: Move Kconfig and Makefile bits down into intel directory iommu/amd: Move Kconfig and Makefile bits down into amd directory
[PATCH 2/2] iommu/amd: Move Kconfig and Makefile bits down into amd directory
Move AMD Kconfig and Makefile bits down into the amd directory with the rest of the AMD specific files. Cc: Joerg Roedel Cc: Suravee Suthikulpanit Signed-off-by: Jerry Snitselaar --- drivers/iommu/Kconfig | 45 +- drivers/iommu/Makefile | 5 + drivers/iommu/amd/Kconfig | 44 + drivers/iommu/amd/Makefile | 4 4 files changed, 50 insertions(+), 48 deletions(-) create mode 100644 drivers/iommu/amd/Kconfig create mode 100644 drivers/iommu/amd/Makefile diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index b12d4ec124f6..78a8be0053b3 100644 --- a/drivers/iommu/Kconfig +++ b/drivers/iommu/Kconfig @@ -132,50 +132,7 @@ config IOMMU_PGTABLES_L2 def_bool y depends on MSM_IOMMU && MMU && SMP && CPU_DCACHE_DISABLE=n -# AMD IOMMU support -config AMD_IOMMU - bool "AMD IOMMU support" - select SWIOTLB - select PCI_MSI - select PCI_ATS - select PCI_PRI - select PCI_PASID - select IOMMU_API - select IOMMU_IOVA - select IOMMU_DMA - depends on X86_64 && PCI && ACPI - ---help--- - With this option you can enable support for AMD IOMMU hardware in - your system. An IOMMU is a hardware component which provides - remapping of DMA memory accesses from devices. With an AMD IOMMU you - can isolate the DMA memory of different devices and protect the - system from misbehaving device drivers or hardware. - - You can find out if your system has an AMD IOMMU if you look into - your BIOS for an option to enable it or if you have an IVRS ACPI - table. - -config AMD_IOMMU_V2 - tristate "AMD IOMMU Version 2 driver" - depends on AMD_IOMMU - select MMU_NOTIFIER - ---help--- - This option enables support for the AMD IOMMUv2 features of the IOMMU - hardware. Select this option if you want to use devices that support - the PCI PRI and PASID interface. - -config AMD_IOMMU_DEBUGFS - bool "Enable AMD IOMMU internals in DebugFS" - depends on AMD_IOMMU && IOMMU_DEBUGFS - ---help--- - !!!WARNING!!! !!!WARNING!!! !!!WARNING!!! !!!WARNING!!! - - DO NOT ENABLE THIS OPTION UNLESS YOU REALLY, -REALLY- KNOW WHAT YOU ARE DOING!!! - Exposes AMD IOMMU device internals in DebugFS. - - This option is -NOT- intended for production environments, and should - not generally be enabled. - +source "drivers/iommu/amd/Kconfig" source "drivers/iommu/intel/Kconfig" config IRQ_REMAP diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile index 71dd2f382e78..f356bc12b1c7 100644 --- a/drivers/iommu/Makefile +++ b/drivers/iommu/Makefile @@ -1,5 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 -obj-y += intel/ +obj-y += amd/ intel/ obj-$(CONFIG_IOMMU_API) += iommu.o obj-$(CONFIG_IOMMU_API) += iommu-traces.o obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o @@ -12,9 +12,6 @@ obj-$(CONFIG_IOASID) += ioasid.o obj-$(CONFIG_IOMMU_IOVA) += iova.o obj-$(CONFIG_OF_IOMMU) += of_iommu.o obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o -obj-$(CONFIG_AMD_IOMMU) += amd/iommu.o amd/init.o amd/quirks.o -obj-$(CONFIG_AMD_IOMMU_DEBUGFS) += amd/debugfs.o -obj-$(CONFIG_AMD_IOMMU_V2) += amd/iommu_v2.o obj-$(CONFIG_ARM_SMMU) += arm_smmu.o arm_smmu-objs += arm-smmu.o arm-smmu-impl.o arm-smmu-qcom.o obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o diff --git a/drivers/iommu/amd/Kconfig b/drivers/iommu/amd/Kconfig new file mode 100644 index ..1f061d91e0b8 --- /dev/null +++ b/drivers/iommu/amd/Kconfig @@ -0,0 +1,44 @@ +# SPDX-License-Identifier: GPL-2.0-only +# AMD IOMMU support +config AMD_IOMMU + bool "AMD IOMMU support" + select SWIOTLB + select PCI_MSI + select PCI_ATS + select PCI_PRI + select PCI_PASID + select IOMMU_API + select IOMMU_IOVA + select IOMMU_DMA + depends on X86_64 && PCI && ACPI + help + With this option you can enable support for AMD IOMMU hardware in + your system. An IOMMU is a hardware component which provides + remapping of DMA memory accesses from devices. With an AMD IOMMU you + can isolate the DMA memory of different devices and protect the + system from misbehaving device drivers or hardware. + + You can find out if your system has an AMD IOMMU if you look into + your BIOS for an option to enable it or if you have an IVRS ACPI + table. + +config AMD_IOMMU_V2 + tristate "AMD IOMMU Version 2 driver" + depends on AMD_IOMMU + select MMU_NOTIFIER + help + This option enables support for the AMD IOMMUv2 features of the IOMMU + hardware. Select this option if you want to use devices that support + the PCI
[PATCH 1/2] iommu/vt-d: Move Kconfig and Makefile bits down into intel directory
Move Intel Kconfig and Makefile bits down into intel directory with the rest of the Intel specific files. Cc: Joerg Roedel Cc: Lu Baolu Signed-off-by: Jerry Snitselaar --- drivers/iommu/Kconfig| 86 +--- drivers/iommu/Makefile | 8 +--- drivers/iommu/intel/Kconfig | 86 drivers/iommu/intel/Makefile | 7 +++ 4 files changed, 96 insertions(+), 91 deletions(-) create mode 100644 drivers/iommu/intel/Kconfig create mode 100644 drivers/iommu/intel/Makefile diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index aca76383f201..b12d4ec124f6 100644 --- a/drivers/iommu/Kconfig +++ b/drivers/iommu/Kconfig @@ -176,91 +176,7 @@ config AMD_IOMMU_DEBUGFS This option is -NOT- intended for production environments, and should not generally be enabled. -# Intel IOMMU support -config DMAR_TABLE - bool - -config INTEL_IOMMU - bool "Support for Intel IOMMU using DMA Remapping Devices" - depends on PCI_MSI && ACPI && (X86 || IA64) - select IOMMU_API - select IOMMU_IOVA - select NEED_DMA_MAP_STATE - select DMAR_TABLE - select SWIOTLB - select IOASID - help - DMA remapping (DMAR) devices support enables independent address - translations for Direct Memory Access (DMA) from devices. - These DMA remapping devices are reported via ACPI tables - and include PCI device scope covered by these DMA - remapping devices. - -config INTEL_IOMMU_DEBUGFS - bool "Export Intel IOMMU internals in Debugfs" - depends on INTEL_IOMMU && IOMMU_DEBUGFS - help - !!!WARNING!!! - - DO NOT ENABLE THIS OPTION UNLESS YOU REALLY KNOW WHAT YOU ARE DOING!!! - - Expose Intel IOMMU internals in Debugfs. - - This option is -NOT- intended for production environments, and should - only be enabled for debugging Intel IOMMU. - -config INTEL_IOMMU_SVM - bool "Support for Shared Virtual Memory with Intel IOMMU" - depends on INTEL_IOMMU && X86 - select PCI_PASID - select PCI_PRI - select MMU_NOTIFIER - select IOASID - help - Shared Virtual Memory (SVM) provides a facility for devices - to access DMA resources through process address space by - means of a Process Address Space ID (PASID). - -config INTEL_IOMMU_DEFAULT_ON - def_bool y - prompt "Enable Intel DMA Remapping Devices by default" - depends on INTEL_IOMMU - help - Selecting this option will enable a DMAR device at boot time if - one is found. If this option is not selected, DMAR support can - be enabled by passing intel_iommu=on to the kernel. - -config INTEL_IOMMU_BROKEN_GFX_WA - bool "Workaround broken graphics drivers (going away soon)" - depends on INTEL_IOMMU && BROKEN && X86 - ---help--- - Current Graphics drivers tend to use physical address - for DMA and avoid using DMA APIs. Setting this config - option permits the IOMMU driver to set a unity map for - all the OS-visible memory. Hence the driver can continue - to use physical addresses for DMA, at least until this - option is removed in the 2.6.32 kernel. - -config INTEL_IOMMU_FLOPPY_WA - def_bool y - depends on INTEL_IOMMU && X86 - ---help--- - Floppy disk drivers are known to bypass DMA API calls - thereby failing to work when IOMMU is enabled. This - workaround will setup a 1:1 mapping for the first - 16MiB to make floppy (an ISA device) work. - -config INTEL_IOMMU_SCALABLE_MODE_DEFAULT_ON - bool "Enable Intel IOMMU scalable mode by default" - depends on INTEL_IOMMU - help - Selecting this option will enable by default the scalable mode if - hardware presents the capability. The scalable mode is defined in - VT-d 3.0. The scalable mode capability could be checked by reading - /sys/devices/virtual/iommu/dmar*/intel-iommu/ecap. If this option - is not selected, scalable mode support could also be enabled by - passing intel_iommu=sm_on to the kernel. If not sure, please use - the default value. +source "drivers/iommu/intel/Kconfig" config IRQ_REMAP bool "Support for Interrupt Remapping" diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile index 342190196dfb..71dd2f382e78 100644 --- a/drivers/iommu/Makefile +++ b/drivers/iommu/Makefile @@ -1,4 +1,5 @@ # SPDX-License-Identifier: GPL-2.0 +obj-y += intel/ obj-$(CONFIG_IOMMU_API) += iommu.o obj-$(CONFIG_IOMMU_API) += iommu-traces.o obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o @@ -17,13 +18,8 @@ obj-$(CONFIG_AMD_IOMMU_V2) += amd/iommu_v2.o obj-$(CONFIG_ARM_SMMU) += arm_smmu.o ar
Re: [PATCH] ima: Remove __init annotation from ima_pcrread()
On Sun Jun 07 20, Roberto Sassu wrote:
Commit 6cc7c266e5b4 ("ima: Call ima_calc_boot_aggregate() in
ima_eventdigest_init()") added a call to ima_calc_boot_aggregate() so that
the digest can be recalculated for the boot_aggregate measurement entry if
the 'd' template field has been requested. For the 'd' field, only SHA1 and
MD5 digests are accepted.
Given that ima_eventdigest_init() does not have the __init annotation, all
functions called should not have it. This patch removes __init from
ima_pcrread().
Cc: sta...@vger.kernel.org
Fixes: 6cc7c266e5b4 ("ima: Call ima_calc_boot_aggregate() in
ima_eventdigest_init()")
Reported-by: Linus Torvalds
Signed-off-by: Roberto Sassu
Reviewed-by: Jerry Snitselaar
---
security/integrity/ima/ima_crypto.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_crypto.c
b/security/integrity/ima/ima_crypto.c
index ba5cc3264240..220b14920c37 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -786,7 +786,7 @@ int ima_calc_buffer_hash(const void *buf, loff_t len,
return calc_buffer_shash(buf, len, hash);
}
-static void __init ima_pcrread(u32 idx, struct tpm_digest *d)
+static void ima_pcrread(u32 idx, struct tpm_digest *d)
{
if (!ima_tpm_chip)
return;
--
2.17.1
[PATCH] iommu: add include/uapi/linux/iommu.h to MAINTAINERS file
When include/uapi/linux/iommu.h was created it was never added to the file list in MAINTAINERS. Cc: Joerg Roedel Signed-off-by: Jerry Snitselaar --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index e1897ed32930..061648b6e393 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8954,6 +8954,7 @@ F:drivers/iommu/ F: include/linux/iommu.h F: include/linux/iova.h F: include/linux/of_iommu.h +F: include/uapi/linux/iommu.h IO_URING M: Jens Axboe -- 2.24.0
[PATCH] iommu: Don't attach deferred device in iommu_group_do_dma_attach
Attaching a deferred device should be delayed until dma api is called.
Cc: io...@lists.linux-foundation.org
Suggested-by: Joerg Roedel
Signed-off-by: Jerry Snitselaar
---
If you already have thrown a patch together, then ignore this. Also
feel free to swap out the signed-off-by with your's since
this is more your patch than mine. You can put a reviewed-by
and tested-by instead for me.
drivers/iommu/iommu.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b5ea203f6c68..d43120eb1dc5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1680,8 +1680,12 @@ static void probe_alloc_default_domain(struct bus_type
*bus,
static int iommu_group_do_dma_attach(struct device *dev, void *data)
{
struct iommu_domain *domain = data;
+ int ret = 0;
- return __iommu_attach_device(domain, dev);
+ if (!iommu_is_attach_deferred(domain, dev))
+ ret = __iommu_attach_device(domain, dev);
+
+ return ret;
}
static int __iommu_group_dma_attach(struct iommu_group *group)
--
2.24.0
Re: [PATCH 0/2] iommu: Move Intel and AMD drivers into their own subdirectory
On Thu Jun 04 20, Lu Baolu wrote:
Hi Joerg,
On 6/2/20 5:26 PM, Joerg Roedel wrote:
Hi,
two small patches to move the Intel and AMD IOMMU drivers into their own
subdirectory under drivers/iommu/ to make the file structure a bit less
cluttered.
Does the MAINTAINERS file need to update?
Best regards,
baolu
Yes, that should be updated to point at the new directories. Good catch.
Regards,
Joerg
Joerg Roedel (2):
iommu/amd: Move AMD IOMMU driver into subdirectory
iommu/vt-d: Move Intel IOMMU driver into subdirectory
drivers/iommu/Makefile | 18 +-
drivers/iommu/{ => amd}/amd_iommu.h| 0
drivers/iommu/{ => amd}/amd_iommu_types.h | 0
.../{amd_iommu_debugfs.c => amd/debugfs.c} | 0
drivers/iommu/{amd_iommu_init.c => amd/init.c} | 2 +-
drivers/iommu/{amd_iommu.c => amd/iommu.c} | 2 +-
.../iommu/{amd_iommu_v2.c => amd/iommu_v2.c} | 0
.../iommu/{amd_iommu_quirks.c => amd/quirks.c} | 0
.../{intel-iommu-debugfs.c => intel/debugfs.c} | 0
drivers/iommu/{ => intel}/dmar.c | 2 +-
drivers/iommu/{ => intel}/intel-pasid.h| 0
drivers/iommu/{intel-iommu.c => intel/iommu.c} | 2 +-
.../irq_remapping.c} | 2 +-
drivers/iommu/{intel-pasid.c => intel/pasid.c} | 0
drivers/iommu/{intel-svm.c => intel/svm.c} | 0
drivers/iommu/{intel-trace.c => intel/trace.c} | 0
16 files changed, 14 insertions(+), 14 deletions(-)
rename drivers/iommu/{ => amd}/amd_iommu.h (100%)
rename drivers/iommu/{ => amd}/amd_iommu_types.h (100%)
rename drivers/iommu/{amd_iommu_debugfs.c => amd/debugfs.c} (100%)
rename drivers/iommu/{amd_iommu_init.c => amd/init.c} (99%)
rename drivers/iommu/{amd_iommu.c => amd/iommu.c} (99%)
rename drivers/iommu/{amd_iommu_v2.c => amd/iommu_v2.c} (100%)
rename drivers/iommu/{amd_iommu_quirks.c => amd/quirks.c} (100%)
rename drivers/iommu/{intel-iommu-debugfs.c => intel/debugfs.c} (100%)
rename drivers/iommu/{ => intel}/dmar.c (99%)
rename drivers/iommu/{ => intel}/intel-pasid.h (100%)
rename drivers/iommu/{intel-iommu.c => intel/iommu.c} (99%)
rename drivers/iommu/{intel_irq_remapping.c => intel/irq_remapping.c} (99%)
rename drivers/iommu/{intel-pasid.c => intel/pasid.c} (100%)
rename drivers/iommu/{intel-svm.c => intel/svm.c} (100%)
rename drivers/iommu/{intel-trace.c => intel/trace.c} (100%)
Re: [PATCH 0/2] iommu: Move Intel and AMD drivers into their own subdirectory
On Tue Jun 02 20, Joerg Roedel wrote:
Hi,
two small patches to move the Intel and AMD IOMMU drivers into their own
subdirectory under drivers/iommu/ to make the file structure a bit less
cluttered.
Regards,
Joerg
Joerg Roedel (2):
iommu/amd: Move AMD IOMMU driver into subdirectory
iommu/vt-d: Move Intel IOMMU driver into subdirectory
drivers/iommu/Makefile | 18 +-
drivers/iommu/{ => amd}/amd_iommu.h| 0
drivers/iommu/{ => amd}/amd_iommu_types.h | 0
.../{amd_iommu_debugfs.c => amd/debugfs.c} | 0
drivers/iommu/{amd_iommu_init.c => amd/init.c} | 2 +-
drivers/iommu/{amd_iommu.c => amd/iommu.c} | 2 +-
.../iommu/{amd_iommu_v2.c => amd/iommu_v2.c} | 0
.../iommu/{amd_iommu_quirks.c => amd/quirks.c} | 0
.../{intel-iommu-debugfs.c => intel/debugfs.c} | 0
drivers/iommu/{ => intel}/dmar.c | 2 +-
drivers/iommu/{ => intel}/intel-pasid.h| 0
drivers/iommu/{intel-iommu.c => intel/iommu.c} | 2 +-
.../irq_remapping.c} | 2 +-
drivers/iommu/{intel-pasid.c => intel/pasid.c} | 0
drivers/iommu/{intel-svm.c => intel/svm.c} | 0
drivers/iommu/{intel-trace.c => intel/trace.c} | 0
16 files changed, 14 insertions(+), 14 deletions(-)
rename drivers/iommu/{ => amd}/amd_iommu.h (100%)
rename drivers/iommu/{ => amd}/amd_iommu_types.h (100%)
rename drivers/iommu/{amd_iommu_debugfs.c => amd/debugfs.c} (100%)
rename drivers/iommu/{amd_iommu_init.c => amd/init.c} (99%)
rename drivers/iommu/{amd_iommu.c => amd/iommu.c} (99%)
rename drivers/iommu/{amd_iommu_v2.c => amd/iommu_v2.c} (100%)
rename drivers/iommu/{amd_iommu_quirks.c => amd/quirks.c} (100%)
rename drivers/iommu/{intel-iommu-debugfs.c => intel/debugfs.c} (100%)
rename drivers/iommu/{ => intel}/dmar.c (99%)
rename drivers/iommu/{ => intel}/intel-pasid.h (100%)
rename drivers/iommu/{intel-iommu.c => intel/iommu.c} (99%)
rename drivers/iommu/{intel_irq_remapping.c => intel/irq_remapping.c} (99%)
rename drivers/iommu/{intel-pasid.c => intel/pasid.c} (100%)
rename drivers/iommu/{intel-svm.c => intel/svm.c} (100%)
rename drivers/iommu/{intel-trace.c => intel/trace.c} (100%)
--
2.17.1
Reviewed-by: Jerry Snitselaar
Re: [PATCH v2 00/33] iommu: Move iommu_group setup to IOMMU core code
On Tue Jun 02 20, Jerry Snitselaar wrote:
On Tue Jun 02 20, Joerg Roedel wrote:
Hi Jerry,
On Mon, Jun 01, 2020 at 05:02:36PM -0700, Jerry Snitselaar wrote:
Yeah, that will solve the panic.
If you still see the kdump faults, can you please try with the attached
diff? I was not able to reproduce them in my setup.
Regards,
Joerg
I have another hp proliant server now, and reproduced. I will have the
patch below tested shortly. Minor change, I switched group->domain to
domain since group isn't an argument, and *data being passed in comes
from group->domain anyways.
Looks like it solves problem for both the epyc system, and the hp proliant
server,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b5ea203f6c68..5a6d509f72b6 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1680,8 +1680,12 @@ static void probe_alloc_default_domain(struct bus_type
*bus,
static int iommu_group_do_dma_attach(struct device *dev, void *data)
{
struct iommu_domain *domain = data;
+ int ret = 0;
- return __iommu_attach_device(domain, dev);
+ if (!iommu_is_attach_deferred(group->domain, dev))
+ ret = __iommu_attach_device(group->domain, dev);
+
+ return ret;
}
static int __iommu_group_dma_attach(struct iommu_group *group)
___
iommu mailing list
io...@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 00/33] iommu: Move iommu_group setup to IOMMU core code
On Tue Jun 02 20, Joerg Roedel wrote:
Hi Jerry,
On Mon, Jun 01, 2020 at 05:02:36PM -0700, Jerry Snitselaar wrote:
Yeah, that will solve the panic.
If you still see the kdump faults, can you please try with the attached
diff? I was not able to reproduce them in my setup.
Regards,
Joerg
I have another hp proliant server now, and reproduced. I will have the
patch below tested shortly. Minor change, I switched group->domain to
domain since group isn't an argument, and *data being passed in comes
from group->domain anyways.
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b5ea203f6c68..5a6d509f72b6 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1680,8 +1680,12 @@ static void probe_alloc_default_domain(struct bus_type
*bus,
static int iommu_group_do_dma_attach(struct device *dev, void *data)
{
struct iommu_domain *domain = data;
+ int ret = 0;
- return __iommu_attach_device(domain, dev);
+ if (!iommu_is_attach_deferred(group->domain, dev))
+ ret = __iommu_attach_device(group->domain, dev);
+
+ return ret;
}
static int __iommu_group_dma_attach(struct iommu_group *group)
___
iommu mailing list
io...@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 00/33] iommu: Move iommu_group setup to IOMMU core code
On Tue Jun 02 20, Lu Baolu wrote:
Hi Jerry,
On 6/1/20 6:42 PM, Jerry Snitselaar wrote:
Hi Joerg,
With this patchset, I have an epyc system where if I boot with
iommu=nopt and force a dump I will see some io page faults for a nic
on the system. The vmcore is harvested and the system reboots. I
haven't reproduced it on other systems yet, but without the patchset I
don't see the io page faults during the kdump.
Regards,
Jerry
I just hit an issue on a separate intel based system (kdump iommu=nopt),
where it panics in during intel_iommu_attach_device, in is_aux_domain,
due to device_domain_info being DEFER_DEVICE_DOMAIN_INFO. That doesn't
get set to a valid address until the domain_add_dev_info call.
Is it as simple as the following?
I guess you won't hit this issue if you use iommu/next branch of Joerg's
tree. We've changed to use a generic helper to retrieve the valid per
device iommu data or NULL (if there's no).
Best regards,
baolu
Yeah, that will solve the panic.
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 29d3940847d3..f1bbeed46a4c 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5053,8 +5053,8 @@ is_aux_domain(struct device *dev, struct
iommu_domain *domain)
 {
   struct device_domain_info *info = dev->archdata.iommu;
-Â Â return info && info->auxd_enabled &&
-Â Â domain->type == IOMMU_DOMAIN_UNMANAGED;
+Â Â return info && info != DEFER_DEVICE_DOMAIN_INFO &&
+Â Â info->auxd_enabled && domain->type ==
IOMMU_DOMAIN_UNMANAGED;
 }
 static void auxiliary_link_device(struct dmar_domain *domain,
Regards,
Jerry
___
iommu mailing list
io...@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 00/33] iommu: Move iommu_group setup to IOMMU core code
On Mon Jun 01 20, Jerry Snitselaar wrote:
On Fri May 29 20, Jerry Snitselaar wrote:
On Tue Apr 14 20, Joerg Roedel wrote:
Hi,
here is the second version of this patch-set. The first version with
some more introductory text can be found here:
https://lore.kernel.org/lkml/20200407183742.4344-1-j...@8bytes.org/
Changes v1->v2:
* Rebased to v5.7-rc1
* Re-wrote the arm-smmu changes as suggested by Robin Murphy
* Re-worked the Exynos patches to hopefully not break the
driver anymore
* Fixed a missing mutex_unlock() reported by Marek Szyprowski,
thanks for that.
There is also a git-branch available with these patches applied:
https://git.kernel.org/pub/scm/linux/kernel/git/joro/linux.git/log/?h=iommu-probe-device-v2
Please review.
Thanks,
Joerg
Joerg Roedel (32):
iommu: Move default domain allocation to separate function
iommu/amd: Implement iommu_ops->def_domain_type call-back
iommu/vt-d: Wire up iommu_ops->def_domain_type
iommu/amd: Remove dma_mask check from check_device()
iommu/amd: Return -ENODEV in add_device when device is not handled by
IOMMU
iommu: Add probe_device() and remove_device() call-backs
iommu: Move default domain allocation to iommu_probe_device()
iommu: Keep a list of allocated groups in __iommu_probe_device()
iommu: Move new probe_device path to separate function
iommu: Split off default domain allocation from group assignment
iommu: Move iommu_group_create_direct_mappings() out of
iommu_group_add_device()
iommu: Export bus_iommu_probe() and make is safe for re-probing
iommu/amd: Remove dev_data->passthrough
iommu/amd: Convert to probe/release_device() call-backs
iommu/vt-d: Convert to probe/release_device() call-backs
iommu/arm-smmu: Convert to probe/release_device() call-backs
iommu/pamu: Convert to probe/release_device() call-backs
iommu/s390: Convert to probe/release_device() call-backs
iommu/virtio: Convert to probe/release_device() call-backs
iommu/msm: Convert to probe/release_device() call-backs
iommu/mediatek: Convert to probe/release_device() call-backs
iommu/mediatek-v1 Convert to probe/release_device() call-backs
iommu/qcom: Convert to probe/release_device() call-backs
iommu/rockchip: Convert to probe/release_device() call-backs
iommu/tegra: Convert to probe/release_device() call-backs
iommu/renesas: Convert to probe/release_device() call-backs
iommu/omap: Remove orphan_dev tracking
iommu/omap: Convert to probe/release_device() call-backs
iommu/exynos: Use first SYSMMU in controllers list for IOMMU core
iommu/exynos: Convert to probe/release_device() call-backs
iommu: Remove add_device()/remove_device() code-paths
iommu: Unexport iommu_group_get_for_dev()
Sai Praneeth Prakhya (1):
iommu: Add def_domain_type() callback in iommu_ops
drivers/iommu/amd_iommu.c | 97
drivers/iommu/amd_iommu_types.h | 1 -
drivers/iommu/arm-smmu-v3.c | 38 +--
drivers/iommu/arm-smmu.c| 39 ++--
drivers/iommu/exynos-iommu.c| 24 +-
drivers/iommu/fsl_pamu_domain.c | 22 +-
drivers/iommu/intel-iommu.c | 68 +-
drivers/iommu/iommu.c | 393 +---
drivers/iommu/ipmmu-vmsa.c | 60 ++---
drivers/iommu/msm_iommu.c | 34 +--
drivers/iommu/mtk_iommu.c | 24 +-
drivers/iommu/mtk_iommu_v1.c| 50 ++--
drivers/iommu/omap-iommu.c | 99 ++--
drivers/iommu/qcom_iommu.c | 24 +-
drivers/iommu/rockchip-iommu.c | 26 +--
drivers/iommu/s390-iommu.c | 22 +-
drivers/iommu/tegra-gart.c | 24 +-
drivers/iommu/tegra-smmu.c | 31 +--
drivers/iommu/virtio-iommu.c| 41 +---
include/linux/iommu.h | 21 +-
20 files changed, 533 insertions(+), 605 deletions(-)
--
2.17.1
___
iommu mailing list
io...@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Hi Joerg,
With this patchset, I have an epyc system where if I boot with
iommu=nopt and force a dump I will see some io page faults for a nic
on the system. The vmcore is harvested and the system reboots. I
haven't reproduced it on other systems yet, but without the patchset I
don't see the io page faults during the kdump.
Regards,
Jerry
I just hit an issue on a separate intel based system (kdump iommu=nopt),
where it panics in during intel_iommu_attach_device, in is_aux_domain,
due to device_domain_info being DEFER_DEVICE_DOMAIN_INFO. That doesn't
get set to a valid address until the domain_add_dev_info call.
Is it as simple as the following?
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 29d3940847d3..f1bbeed46a4c 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5053,8 +5053,8 @@ is_aux_domain(struct device *dev, struct iommu_domain
*domain)
{
struct device_domain_info *info = dev->archdata.iommu;
- return info && info->auxd_enabled &&
-
Re: [PATCH v2 00/33] iommu: Move iommu_group setup to IOMMU core code
On Fri May 29 20, Jerry Snitselaar wrote:
On Tue Apr 14 20, Joerg Roedel wrote:
Hi,
here is the second version of this patch-set. The first version with
some more introductory text can be found here:
https://lore.kernel.org/lkml/20200407183742.4344-1-j...@8bytes.org/
Changes v1->v2:
* Rebased to v5.7-rc1
* Re-wrote the arm-smmu changes as suggested by Robin Murphy
* Re-worked the Exynos patches to hopefully not break the
driver anymore
* Fixed a missing mutex_unlock() reported by Marek Szyprowski,
thanks for that.
There is also a git-branch available with these patches applied:
https://git.kernel.org/pub/scm/linux/kernel/git/joro/linux.git/log/?h=iommu-probe-device-v2
Please review.
Thanks,
Joerg
Joerg Roedel (32):
iommu: Move default domain allocation to separate function
iommu/amd: Implement iommu_ops->def_domain_type call-back
iommu/vt-d: Wire up iommu_ops->def_domain_type
iommu/amd: Remove dma_mask check from check_device()
iommu/amd: Return -ENODEV in add_device when device is not handled by
IOMMU
iommu: Add probe_device() and remove_device() call-backs
iommu: Move default domain allocation to iommu_probe_device()
iommu: Keep a list of allocated groups in __iommu_probe_device()
iommu: Move new probe_device path to separate function
iommu: Split off default domain allocation from group assignment
iommu: Move iommu_group_create_direct_mappings() out of
iommu_group_add_device()
iommu: Export bus_iommu_probe() and make is safe for re-probing
iommu/amd: Remove dev_data->passthrough
iommu/amd: Convert to probe/release_device() call-backs
iommu/vt-d: Convert to probe/release_device() call-backs
iommu/arm-smmu: Convert to probe/release_device() call-backs
iommu/pamu: Convert to probe/release_device() call-backs
iommu/s390: Convert to probe/release_device() call-backs
iommu/virtio: Convert to probe/release_device() call-backs
iommu/msm: Convert to probe/release_device() call-backs
iommu/mediatek: Convert to probe/release_device() call-backs
iommu/mediatek-v1 Convert to probe/release_device() call-backs
iommu/qcom: Convert to probe/release_device() call-backs
iommu/rockchip: Convert to probe/release_device() call-backs
iommu/tegra: Convert to probe/release_device() call-backs
iommu/renesas: Convert to probe/release_device() call-backs
iommu/omap: Remove orphan_dev tracking
iommu/omap: Convert to probe/release_device() call-backs
iommu/exynos: Use first SYSMMU in controllers list for IOMMU core
iommu/exynos: Convert to probe/release_device() call-backs
iommu: Remove add_device()/remove_device() code-paths
iommu: Unexport iommu_group_get_for_dev()
Sai Praneeth Prakhya (1):
iommu: Add def_domain_type() callback in iommu_ops
drivers/iommu/amd_iommu.c | 97
drivers/iommu/amd_iommu_types.h | 1 -
drivers/iommu/arm-smmu-v3.c | 38 +--
drivers/iommu/arm-smmu.c| 39 ++--
drivers/iommu/exynos-iommu.c| 24 +-
drivers/iommu/fsl_pamu_domain.c | 22 +-
drivers/iommu/intel-iommu.c | 68 +-
drivers/iommu/iommu.c | 393 +---
drivers/iommu/ipmmu-vmsa.c | 60 ++---
drivers/iommu/msm_iommu.c | 34 +--
drivers/iommu/mtk_iommu.c | 24 +-
drivers/iommu/mtk_iommu_v1.c| 50 ++--
drivers/iommu/omap-iommu.c | 99 ++--
drivers/iommu/qcom_iommu.c | 24 +-
drivers/iommu/rockchip-iommu.c | 26 +--
drivers/iommu/s390-iommu.c | 22 +-
drivers/iommu/tegra-gart.c | 24 +-
drivers/iommu/tegra-smmu.c | 31 +--
drivers/iommu/virtio-iommu.c| 41 +---
include/linux/iommu.h | 21 +-
20 files changed, 533 insertions(+), 605 deletions(-)
--
2.17.1
___
iommu mailing list
io...@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Hi Joerg,
With this patchset, I have an epyc system where if I boot with
iommu=nopt and force a dump I will see some io page faults for a nic
on the system. The vmcore is harvested and the system reboots. I
haven't reproduced it on other systems yet, but without the patchset I
don't see the io page faults during the kdump.
Regards,
Jerry
I just hit an issue on a separate intel based system (kdump iommu=nopt),
where it panics in during intel_iommu_attach_device, in is_aux_domain,
due to device_domain_info being DEFER_DEVICE_DOMAIN_INFO. That doesn't
get set to a valid address until the domain_add_dev_info call.
Is it as simple as the following?
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 29d3940847d3..f1bbeed46a4c 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5053,8 +5053,8 @@ is_aux_domain(struct device *dev, struct iommu_domain
*domain)
{
struct device_domain_info *info = dev->archdata.iommu;
- return info && info->auxd_enabled &&
- domain->type == IOMMU_DOMAIN_U
Re: [PATCH v2 00/33] iommu: Move iommu_group setup to IOMMU core code
On Tue Apr 14 20, Joerg Roedel wrote: Hi, here is the second version of this patch-set. The first version with some more introductory text can be found here: https://lore.kernel.org/lkml/20200407183742.4344-1-j...@8bytes.org/ Changes v1->v2: * Rebased to v5.7-rc1 * Re-wrote the arm-smmu changes as suggested by Robin Murphy * Re-worked the Exynos patches to hopefully not break the driver anymore * Fixed a missing mutex_unlock() reported by Marek Szyprowski, thanks for that. There is also a git-branch available with these patches applied: https://git.kernel.org/pub/scm/linux/kernel/git/joro/linux.git/log/?h=iommu-probe-device-v2 Please review. Thanks, Joerg Joerg Roedel (32): iommu: Move default domain allocation to separate function iommu/amd: Implement iommu_ops->def_domain_type call-back iommu/vt-d: Wire up iommu_ops->def_domain_type iommu/amd: Remove dma_mask check from check_device() iommu/amd: Return -ENODEV in add_device when device is not handled by IOMMU iommu: Add probe_device() and remove_device() call-backs iommu: Move default domain allocation to iommu_probe_device() iommu: Keep a list of allocated groups in __iommu_probe_device() iommu: Move new probe_device path to separate function iommu: Split off default domain allocation from group assignment iommu: Move iommu_group_create_direct_mappings() out of iommu_group_add_device() iommu: Export bus_iommu_probe() and make is safe for re-probing iommu/amd: Remove dev_data->passthrough iommu/amd: Convert to probe/release_device() call-backs iommu/vt-d: Convert to probe/release_device() call-backs iommu/arm-smmu: Convert to probe/release_device() call-backs iommu/pamu: Convert to probe/release_device() call-backs iommu/s390: Convert to probe/release_device() call-backs iommu/virtio: Convert to probe/release_device() call-backs iommu/msm: Convert to probe/release_device() call-backs iommu/mediatek: Convert to probe/release_device() call-backs iommu/mediatek-v1 Convert to probe/release_device() call-backs iommu/qcom: Convert to probe/release_device() call-backs iommu/rockchip: Convert to probe/release_device() call-backs iommu/tegra: Convert to probe/release_device() call-backs iommu/renesas: Convert to probe/release_device() call-backs iommu/omap: Remove orphan_dev tracking iommu/omap: Convert to probe/release_device() call-backs iommu/exynos: Use first SYSMMU in controllers list for IOMMU core iommu/exynos: Convert to probe/release_device() call-backs iommu: Remove add_device()/remove_device() code-paths iommu: Unexport iommu_group_get_for_dev() Sai Praneeth Prakhya (1): iommu: Add def_domain_type() callback in iommu_ops drivers/iommu/amd_iommu.c | 97 drivers/iommu/amd_iommu_types.h | 1 - drivers/iommu/arm-smmu-v3.c | 38 +-- drivers/iommu/arm-smmu.c| 39 ++-- drivers/iommu/exynos-iommu.c| 24 +- drivers/iommu/fsl_pamu_domain.c | 22 +- drivers/iommu/intel-iommu.c | 68 +- drivers/iommu/iommu.c | 393 +--- drivers/iommu/ipmmu-vmsa.c | 60 ++--- drivers/iommu/msm_iommu.c | 34 +-- drivers/iommu/mtk_iommu.c | 24 +- drivers/iommu/mtk_iommu_v1.c| 50 ++-- drivers/iommu/omap-iommu.c | 99 ++-- drivers/iommu/qcom_iommu.c | 24 +- drivers/iommu/rockchip-iommu.c | 26 +-- drivers/iommu/s390-iommu.c | 22 +- drivers/iommu/tegra-gart.c | 24 +- drivers/iommu/tegra-smmu.c | 31 +-- drivers/iommu/virtio-iommu.c| 41 +--- include/linux/iommu.h | 21 +- 20 files changed, 533 insertions(+), 605 deletions(-) -- 2.17.1 ___ iommu mailing list io...@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu Hi Joerg, With this patchset, I have an epyc system where if I boot with iommu=nopt and force a dump I will see some io page faults for a nic on the system. The vmcore is harvested and the system reboots. I haven't reproduced it on other systems yet, but without the patchset I don't see the io page faults during the kdump. Regards, Jerry
Re: [PATCH] iommu: Implement deferred domain attachment
On Mon May 18 20, Joerg Roedel wrote:
On Fri, May 15, 2020 at 08:23:13PM +0100, Robin Murphy wrote:
But that's not what this is; this is (supposed to be) the exact same "don't
actually perform the attach yet" logic as before, just restricting it to
default domains in the one place that it actually needs to be, so as not to
fundamentally bugger up iommu_attach_device() in a way that prevents it from
working as expected at the correct point later.
You are right, that is better. I tested it and it seems to work. Updated
diff attached, with a minor cleanup included. Mind sending it as a
proper patch I can send upstream?
Thanks,
Joerg
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7b375421afba..a9d02bc3ab5b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -693,6 +693,15 @@ static int iommu_group_create_direct_mappings(struct
iommu_group *group,
return ret;
}
+static bool iommu_is_attach_deferred(struct iommu_domain *domain,
+struct device *dev)
+{
+ if (domain->ops->is_attach_deferred)
+ return domain->ops->is_attach_deferred(domain, dev);
+
+ return false;
+}
+
/**
* iommu_group_add_device - add a device to an iommu group
* @group: the group into which to add the device (reference should be held)
@@ -705,6 +714,7 @@ int iommu_group_add_device(struct iommu_group *group,
struct device *dev)
{
int ret, i = 0;
struct group_device *device;
+ struct iommu_domain *domain;
device = kzalloc(sizeof(*device), GFP_KERNEL);
if (!device)
@@ -747,7 +757,8 @@ int iommu_group_add_device(struct iommu_group *group,
struct device *dev)
mutex_lock(>mutex);
list_add_tail(>list, >devices);
- if (group->domain)
+ domain = group->domain;
+ if (domain && !iommu_is_attach_deferred(domain, dev))
ret = __iommu_attach_device(group->domain, dev);
mutex_unlock(>mutex);
if (ret)
@@ -1653,9 +1664,6 @@ static int __iommu_attach_device(struct iommu_domain
*domain,
struct device *dev)
{
int ret;
- if ((domain->ops->is_attach_deferred != NULL) &&
- domain->ops->is_attach_deferred(domain, dev))
- return 0;
if (unlikely(domain->ops->attach_dev == NULL))
return -ENODEV;
@@ -1727,8 +1735,7 @@ EXPORT_SYMBOL_GPL(iommu_sva_unbind_gpasid);
static void __iommu_detach_device(struct iommu_domain *domain,
struct device *dev)
{
- if ((domain->ops->is_attach_deferred != NULL) &&
- domain->ops->is_attach_deferred(domain, dev))
+ if (iommu_is_attach_deferred(domain, dev))
return;
if (unlikely(domain->ops->detach_dev == NULL))
___
iommu mailing list
io...@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
This worked for me as well.
Re: [PATCH] iommu: Implement deferred domain attachment
On Mon May 18 20, Joerg Roedel wrote: On Fri, May 15, 2020 at 08:23:13PM +0100, Robin Murphy wrote: But that's not what this is; this is (supposed to be) the exact same "don't actually perform the attach yet" logic as before, just restricting it to default domains in the one place that it actually needs to be, so as not to fundamentally bugger up iommu_attach_device() in a way that prevents it from working as expected at the correct point later. You are right, that is better. I tested it and it seems to work. Updated diff attached, with a minor cleanup included. Mind sending it as a proper patch I can send upstream? Thanks, Joerg I should have this tested this afternoon.
Re: amd kdump failure with iommu=nopt
On Thu May 14 20, Joerg Roedel wrote:
On Thu, May 14, 2020 at 05:36:23PM +0200, Joerg Roedel wrote:
This commit also removes the deferred attach of the device to its new
domain. Does the attached diff fix the problem for you?
+static int __iommu_attach_device_no_defer(struct iommu_domain *domain,
+ struct device *dev)
+{
if (unlikely(domain->ops->attach_dev == NULL))
return -ENODEV;
ret = domain->ops->attach_dev(domain, dev);
if (!ret)
trace_attach_device_to_domain(dev);
+
return ret;
}
Sorry, this didn't compile, here is an updated version that actually
compiles:
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4050569188be..f54ebb964271 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1889,13 +1889,19 @@ void iommu_domain_free(struct iommu_domain *domain)
}
EXPORT_SYMBOL_GPL(iommu_domain_free);
-static int __iommu_attach_device(struct iommu_domain *domain,
-struct device *dev)
+static bool __iommu_is_attach_deferred(struct iommu_domain *domain,
+ struct device *dev)
+{
+ if (!domain->ops->is_attach_deferred)
+ return false;
+
+ return domain->ops->is_attach_deferred(domain, dev);
+}
+
+static int __iommu_attach_device_no_defer(struct iommu_domain *domain,
+ struct device *dev)
{
int ret;
- if ((domain->ops->is_attach_deferred != NULL) &&
- domain->ops->is_attach_deferred(domain, dev))
- return 0;
if (unlikely(domain->ops->attach_dev == NULL))
return -ENODEV;
@@ -1903,9 +1909,19 @@ static int __iommu_attach_device(struct iommu_domain
*domain,
ret = domain->ops->attach_dev(domain, dev);
if (!ret)
trace_attach_device_to_domain(dev);
+
return ret;
}
+static int __iommu_attach_device(struct iommu_domain *domain,
+struct device *dev)
+{
+ if (__iommu_is_attach_deferred(domain, dev))
+ return 0;
+
+ return __iommu_attach_device_no_defer(domain, dev);
+}
+
int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
{
struct iommu_group *group;
@@ -2023,7 +2039,12 @@ EXPORT_SYMBOL_GPL(iommu_get_domain_for_dev);
*/
struct iommu_domain *iommu_get_dma_domain(struct device *dev)
{
- return dev->iommu_group->default_domain;
+ struct iommu_domain *domain = dev->iommu_group->default_domain;
+
+ if (__iommu_is_attach_deferred(domain, dev))
+ __iommu_attach_device_no_defer(domain, dev);
+
+ return domain;
}
/*
___
iommu mailing list
io...@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Yes, that works.
Tested-by: Jerry Snitselaar
amd kdump failure with iommu=nopt
We've seen kdump failures with recent kernels (5.5, 5.6, 5.7-rc1) on amd systems when iommu is enabled in translation mode. In the cases so far there has been mpt3sas involved, but I'm also seeing io page faults for ahci right before mpt3sas has an io page fault: [ 15.156620] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xfff9b300 flags=0x0020] [ 15.166889] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xfff9b320 flags=0x0020] [ 15.177169] ata2: SATA link up 6.0 Gbps (SStatus 133 SControl 300) [ 15.186100] ata4.00: failed to IDENTIFY (device reports invalid type, err_mask=0x0) [ 15.193786] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f730c0 flags=0x0020] [ 15.204059] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f732c0 flags=0x0020] [ 15.214327] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f734c0 flags=0x0020] [ 15.224597] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f736c0 flags=0x0020] [ 15.234867] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f738c0 flags=0x0020] [ 15.245138] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f73ac0 flags=0x0020] [ 15.255407] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f73cc0 flags=0x0020] [ 15.265677] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f73ec0 flags=0x0020] [ 20.599101] ata2.00: failed to IDENTIFY (INIT_DEV_PARAMS failed, err_mask=0x80) [ 20.916172] ata4: SATA link up 1.5 Gbps (SStatus 113 SControl 300) [ 20.922429] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xfff9b300 flags=0x0020] [ 20.932703] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xfff9b320 flags=0x0020] [ 20.943234] ata2: SATA link up 6.0 Gbps (SStatus 133 SControl 300) [ 20.949430] ata4.00: failed to IDENTIFY (device reports invalid type, err_mask=0x0) [ 20.957115] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f730c0 flags=0x0020] [ 20.967384] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f732c0 flags=0x0020] [ 20.977654] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f734c0 flags=0x0020] [ 20.987923] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f736c0 flags=0x0020] [ 20.998193] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f738c0 flags=0x0020] [ 21.008464] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f73ac0 flags=0x0020] [ 21.018733] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f73cc0 flags=0x0020] [ 21.029005] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f73ec0 flags=0x0020] [ 26.231097] ata2.00: failed to IDENTIFY (INIT_DEV_PARAMS failed, err_mask=0x80) [ 26.238415] ata2: limiting SATA link speed to 3.0 Gbps [ 26.548169] ata4: SATA link up 1.5 Gbps (SStatus 113 SControl 300) [ 26.564483] ata2: SATA link up 6.0 Gbps (SStatus 133 SControl 320) [ 26.571026] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f730c0 flags=0x0020] [ 26.581301] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f732c0 flags=0x0020] [ 26.591568] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f734c0 flags=0x0020] [ 26.601839] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f736c0 flags=0x0020] [ 26.612109] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f738c0 flags=0x0020] [ 26.622377] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f73ac0 flags=0x0020] [ 26.632647] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f73cc0 flags=0x0020] [ 26.642917] ahci :63:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0042 address=0xf1f73ec0 flags=0x0020] [ 26.654047] ata2.00: failed to IDENTIFY (INIT_DEV_PARAMS failed, err_mask=0x80) [ 26.743097] xhci_hcd :05:00.3: Error while assigning device slot ID [ 26.749718] xhci_hcd :05:00.3: Max number of devices this xHCI host supports is 64. [ 26.757730] usb usb1-port2: couldn't allocate usb_device [ 26.987555] mpt3sas version 33.100.00.00 loaded [ 26.994668] mpt3sas_cm0: 63 BIT PCI BUS DMA ADDRESSING SUPPORTED, total mem (226256 kB) [ 27.060443] mpt3sas_cm0: CurrentHostPageSize is 0: Setting default host page size to 4k [ 27.068469] mpt3sas_cm0: MSI-X vectors supported: 96 [ 27.073444] no of cores: 1, max_msix_vectors: -1 [ 27.078244]
Re: [PATCH v4 0/3] Replace private domain with per-group default domain
On Wed May 06 20, Lu Baolu wrote: Some devices are required to use a specific type (identity or dma) of default domain when they are used with a vendor iommu. When the system level default domain type is different from it, the vendor iommu driver has to request a new default domain with either iommu_request_dma_domain_for_dev() or iommu_request_dm_for_dev() in the add_dev() callback. Unfortunately, these two helpers only work when the group hasn't been assigned to any other devices, hence, some vendor iommu driver has to use a private domain if it fails to request a new default one. Joerg proposed an on-going proposal which makes the default domain framework to support configuring per-group default domain during boot process. https://lkml.org/lkml/2020/4/14/616 [This has been applied in iommu/next.] Hence, there is no need to keep the private domain implementation in the Intel IOMMU driver. This patch series aims to remove it. Best regards, baolu Change log: v3->v4: - Make the commit message of the first patch more comprehensive. v2->v3: - Port necessary patches on the top of Joerg's new proposal. https://lkml.org/lkml/2020/4/14/616 The per-group default domain proposed previously in this series will be deprecated due to a race concern between domain switching and device driver probing. v1->v2: - Rename the iommu ops callback to def_domain_type Lu Baolu (3): iommu/vt-d: Allow 32bit devices to uses DMA domain iommu/vt-d: Allow PCI sub-hierarchy to use DMA domain iommu/vt-d: Apply per-device dma_ops drivers/iommu/intel-iommu.c | 396 +++- 1 file changed, 26 insertions(+), 370 deletions(-) -- 2.17.1 Reviewed-by: Jerry Snitselaar
Re: [PATCH v4 1/7] ima: Switch to ima_hash_algo for boot aggregate
On Thu Apr 02 20, Mimi Zohar wrote:
Hi Roberto,
On Wed, 2020-03-25 at 11:47 +0100, Roberto Sassu wrote:
boot_aggregate is the first entry of IMA measurement list. Its purpose is
to link pre-boot measurements to IMA measurements. As IMA was designed to
work with a TPM 1.2, the SHA1 PCR bank was always selected even if a
TPM 2.0 with support for stronger hash algorithms is available.
This patch first tries to find a PCR bank with the IMA default hash
algorithm. If it does not find it, it selects the SHA256 PCR bank for
TPM 2.0 and SHA1 for TPM 1.2. Ultimately, it selects SHA1 also for TPM 2.0
if the SHA256 PCR bank is not found.
If none of the PCR banks above can be found, boot_aggregate file digest is
filled with zeros, as for TPM bypass, making it impossible to perform a
remote attestation of the system.
Cc: sta...@vger.kernel.org # 5.1.x
Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR
read")
Reported-by: Jerry Snitselaar
Suggested-by: James Bottomley
Signed-off-by: Roberto Sassu
Thank you! Â This patch set is now queued in next-integrity-testing
during the open window. Â Jerry, I assume this works for you. Â Could we
get your tag?
thanks!
Mimi
Hi Mimi,
Yes, I no longer get the errors with this patch.
Tested-by: Jerry Snitselaar
Regards,
Jerry
Re: [PATCH] efi/tpm: fix section mismatch warning
On Thu Apr 30 20, Ard Biesheuvel wrote:
On Thu, 30 Apr 2020 at 23:21, Arnd Bergmann wrote:
On Thu, Apr 30, 2020 at 11:15 PM Jerry Snitselaar wrote:
>
> On Wed Apr 29 20, Arnd Bergmann wrote:
> >Building with gcc-10 causes a harmless warning about a section mismatch:
> >
> >WARNING: modpost: vmlinux.o(.text.unlikely+0x5e191): Section mismatch in
reference from the function tpm2_calc_event_log_size() to the function
.init.text:early_memunmap()
> >The function tpm2_calc_event_log_size() references
> >the function __init early_memunmap().
> >This is often because tpm2_calc_event_log_size lacks a __init
> >annotation or the annotation of early_memunmap is wrong.
> >
> >Add the missing annotation.
> >
> >Fixes: e658c82be556 ("efi/tpm: Only set 'efi_tpm_final_log_size' after successful
event log parsing")
> >Signed-off-by: Arnd Bergmann
>
> Minor thing, but should the Fixes be c46f3405692d ("tpm: Reserve the TPM final
events table")? Or what am I missing
> about e658c82be556 that causes this? Just trying to understand what I did. :)
You are right, I misread the git history. Can you fix it up when applying the
patch, or should I resend it?
I can fix it up, no worries.
With the fix applied:
Reviewed-by: Jerry Snitselaar
Re: [PATCH] efi/tpm: fix section mismatch warning
On Wed Apr 29 20, Arnd Bergmann wrote:
Building with gcc-10 causes a harmless warning about a section mismatch:
WARNING: modpost: vmlinux.o(.text.unlikely+0x5e191): Section mismatch in
reference from the function tpm2_calc_event_log_size() to the function
.init.text:early_memunmap()
The function tpm2_calc_event_log_size() references
the function __init early_memunmap().
This is often because tpm2_calc_event_log_size lacks a __init
annotation or the annotation of early_memunmap is wrong.
Add the missing annotation.
Fixes: e658c82be556 ("efi/tpm: Only set 'efi_tpm_final_log_size' after successful
event log parsing")
Signed-off-by: Arnd Bergmann
Minor thing, but should the Fixes be c46f3405692d ("tpm: Reserve the TPM final
events table")? Or what am I missing
about e658c82be556 that causes this? Just trying to understand what I did. :)
Regards,
Jerry
---
drivers/firmware/efi/tpm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 31f9f0e369b9..55b031d2c989 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -16,7 +16,7 @@
int efi_tpm_final_log_size;
EXPORT_SYMBOL(efi_tpm_final_log_size);
-static int tpm2_calc_event_log_size(void *data, int count, void *size_info)
+static int __init tpm2_calc_event_log_size(void *data, int count, void
*size_info)
{
struct tcg_pcr_event2_head *header;
int event_size, size = 0;
--
2.26.0
Re: [PATCH] iommu/amd: Check PM_LEVEL_SIZE() condition in locked section
On Fri Oct 18 19, Joerg Roedel wrote:
On Thu, Oct 17, 2019 at 07:36:51AM -0400, Qian Cai wrote:
> On Oct 16, 2019, at 6:59 PM, Jerry Snitselaar wrote:
>
> I guess the mode level 6 check is really for other potential callers
> increase_address_space, none exist at the moment, and the condition
> of the while loop in alloc_pte should fail if the mode level is 6.
Because there is no locking around iommu_map_page(), if there are
several concurrent callers of it for the same domain, could it be that
it silently corrupt data due to invalid access?
No, that can't happen because increase_address_space locks the domain
before actually doing anything. So the address space can't grow above
domain->mode == 6. But what can happen is that the WARN_ON_ONCE triggers
in there and that the address space is increased multiple times when
only one increase would be sufficient.
To fix this we just need to check the PM_LEVEL_SIZE() condition again
when we hold the lock:
From e930e792a998e89dfd4feef15fbbf289c45124dc Mon Sep 17 00:00:00 2001
From: Joerg Roedel
Date: Fri, 18 Oct 2019 11:34:22 +0200
Subject: [PATCH] iommu/amd: Check PM_LEVEL_SIZE() condition in locked section
The increase_address_space() function has to check the PM_LEVEL_SIZE()
condition again under the domain->lock to avoid a false trigger of the
WARN_ON_ONCE() and to avoid that the address space is increase more
often than necessary.
Reported-by: Qian Cai
Fixes: 754265bcab78 ("iommu/amd: Fix race in increase_address_space()")
Signed-off-by: Joerg Roedel
---
drivers/iommu/amd_iommu.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 2369b8af81f3..a0639e511ffe 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1463,6 +1463,7 @@ static void free_pagetable(struct protection_domain
*domain)
* to 64 bits.
*/
static bool increase_address_space(struct protection_domain *domain,
+ unsigned long address,
gfp_t gfp)
{
unsigned long flags;
@@ -1471,8 +1472,8 @@ static bool increase_address_space(struct
protection_domain *domain,
spin_lock_irqsave(>lock, flags);
- if (WARN_ON_ONCE(domain->mode == PAGE_MODE_6_LEVEL))
- /* address space already 64 bit large */
+ if (address <= PM_LEVEL_SIZE(domain->mode) ||
+ WARN_ON_ONCE(domain->mode == PAGE_MODE_6_LEVEL))
goto out;
pte = (void *)get_zeroed_page(gfp);
@@ -1505,7 +1506,7 @@ static u64 *alloc_pte(struct protection_domain *domain,
BUG_ON(!is_power_of_2(page_size));
while (address > PM_LEVEL_SIZE(domain->mode))
- *updated = increase_address_space(domain, gfp) || *updated;
+ *updated = increase_address_space(domain, address, gfp) ||
*updated;
level = domain->mode - 1;
pte = >pt_root[PM_LEVEL_INDEX(level, address)];
--
2.16.4
Reviewed-by: Jerry Snitselaar
Re: [PATCH -next] iommu/amd: fix a warning in increase_address_space
On Wed Oct 16 19, Qian Cai wrote:
After the commit 754265bcab78 ("iommu/amd: Fix race in
increase_address_space()"), it could still possible trigger a race
condition under some heavy memory pressure below. The race to trigger a
warning is,
CPU0: CPU1:
in alloc_pte(): in increase_address_space():
while (address > PM_LEVEL_SIZE(domain->mode)) [1]
spin_lock_irqsave(>lock
domain->mode+= 1;
spin_unlock_irqrestore(>lock
in increase_address_space():
spin_lock_irqsave(>lock
if (WARN_ON_ONCE(domain->mode == PAGE_MODE_6_LEVEL))
[1] domain->mode = 5
It is unclear the triggering of the warning is the root cause of the
smartpqi offline yet, but let's fix it first by lifting the locking.
WARNING: CPU: 57 PID: 124314 at drivers/iommu/amd_iommu.c:1474
iommu_map_page+0x718/0x7e0
smartpqi :23:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x
address=0xffec flags=0x0010]
smartpqi :23:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x
address=0xffec1000 flags=0x0010]
CPU: 57 PID: 124314 Comm: oom01 Tainted: G O
Hardware name: HPE ProLiant DL385 Gen10/ProLiant DL385 Gen10, BIOS A40
07/10/2019
RIP: 0010:iommu_map_page+0x718/0x7e0
Code: 88 a5 70 ff ff ff e9 5d fa ff ff 48 8b b5 70 ff ff ff 4c 89 ef e8
08 32 2f 00 41 80 fc 01 0f 87 b7 3d 00 00 41 83 e4 01 eb be <0f> 0b 48
8b b5 70 ff ff ff 4c 89 ef e8 e7 31 2f 00 eb dd 0f 0b 48
RSP: 0018:888da4816cb8 EFLAGS: 00010046
RAX: RBX: 8885fe689000 RCX: 96f4a6c4
RDX: 0007 RSI: dc00 RDI: 8885fe689124
RBP: 888da4816da8 R08: ed10bfcd120e R09: ed10bfcd120e
R10: ed10bfcd120d R11: 8885fe68906b R12:
smartpqi :23:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x
address=0xffec1a00 flags=0x0010]
R13: 8885fe689068 R14: 8885fe689124 R15:
smartpqi :23:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x
address=0xffec1e00 flags=0x0010]
FS: 7f29722ba700() GS:88902f88()
knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 7f27f82d8000 CR3: 00102ed9c000 CR4: 003406e0
Call Trace:
smartpqi :23:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x
address=0xffec2000 flags=0x0010]
map_sg+0x1ce/0x2f0
smartpqi :23:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x
address=0xffec2400 flags=0x0010]
scsi_dma_map+0xd7/0x160
pqi_raid_submit_scsi_cmd_with_io_request+0x1b8/0x420 [smartpqi]
smartpqi :23:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x
address=0xffec2800 flags=0x0010]
pqi_scsi_queue_command+0x8ab/0xe00 [smartpqi]
smartpqi :23:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x
address=0xffec2c00 flags=0x0010]
scsi_queue_rq+0xd19/0x1360
smartpqi :23:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x
address=0xffec3000 flags=0x0010]
__blk_mq_try_issue_directly+0x295/0x3f0
smartpqi :23:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x
address=0xffec3400 flags=0x0010]
AMD-Vi: Event logged [IO_PAGE_FAULT device=23:00.0 domain=0x
address=0xffec3800 flags=0x0010]
blk_mq_request_issue_directly+0xb5/0x100
AMD-Vi: Event logged [IO_PAGE_FAULT device=23:00.0 domain=0x
address=0xffec3c00 flags=0x0010]
blk_mq_try_issue_list_directly+0xa9/0x160
blk_mq_sched_insert_requests+0x228/0x380
blk_mq_flush_plug_list+0x448/0x7e0
blk_flush_plug_list+0x1eb/0x230
blk_finish_plug+0x43/0x5d
shrink_node_memcg+0x9c5/0x1550
smartpqi :23:00.0: controller is offline: status code 0x14803
smartpqi :23:00.0: controller offline
Fixes: 754265bcab78 ("iommu/amd: Fix race in increase_address_space()")
Signed-off-by: Qian Cai
---
BTW, Joerg, this line from the commit "iommu/amd: Remove domain->updated" looks
suspicious. Not sure what the purpose of it.
*updated = increase_address_space(domain, gfp) || *updated;
Looking at it again I think that isn't an issue really, it would just
not lose updated being set in a previous loop iteration, but now
I'm wondering about the loop itself. In the cases where it would return
false, how does the evaluation of the condition for the while loop
change?
drivers/iommu/amd_iommu.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 2369b8af81f3..a5754068aa29 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -1465,12 +1465,9 @@ static void free_pagetable(struct protection_domain
*domain)
static bool increase_address_space(struct protection_domain *domain,
gfp_t gfp)
{
- unsigned long flags;
bool ret = false;
u64 *pte;
- spin_lock_irqsave(>lock, flags);
-
if (WARN_ON_ONCE(domain->mode ==
Re: [Patch v7 0/4] Create and consolidate trusted keys subsystem
On Fri Oct 11 19, Jarkko Sakkinen wrote:
On Mon, Oct 07, 2019 at 10:55:31AM +0530, Sumit Garg wrote:
This patch-set does restructuring of trusted keys code to create and
consolidate trusted keys subsystem.
Also, patch #2 replaces tpm1_buf code used in security/keys/trusted.c and
crypto/asymmertic_keys/asym_tpm.c files to use the common tpm_buf code.
Changes in v7:
1. Rebased to top of tpmdd/master
2. Patch #4: update tpm2 trusted keys code to use tpm_send() instead of
tpm_transmit_cmd() which is an internal function.
Changes in v6:
1. Switch TPM asymmetric code also to use common tpm_buf code. These
changes required patches #1 and #2 update, so I have dropped review
tags from those patches.
2. Incorporated miscellaneous comments from Jarkko.
Changes in v5:
1. Drop 5/5 patch as its more relavant along with TEE patch-set.
2. Add Reviewed-by tag for patch #2.
3. Fix build failure when "CONFIG_HEADER_TEST" and
"CONFIG_KERNEL_HEADER_TEST" config options are enabled.
4. Misc changes to rename files.
Changes in v4:
1. Separate patch for export of tpm_buf code to include/linux/tpm.h
2. Change TPM1.x trusted keys code to use common tpm_buf
3. Keep module name as trusted.ko only
Changes in v3:
Move TPM2 trusted keys code to trusted keys subsystem.
Changes in v2:
Split trusted keys abstraction patch for ease of review.
Sumit Garg (4):
tpm: Move tpm_buf code to include/linux/
KEYS: Use common tpm_buf for trusted and asymmetric keys
KEYS: trusted: Create trusted keys subsystem
KEYS: trusted: Move TPM2 trusted keys code
crypto/asymmetric_keys/asym_tpm.c | 101 +++
drivers/char/tpm/tpm-interface.c | 56
drivers/char/tpm/tpm.h | 226 ---
drivers/char/tpm/tpm2-cmd.c| 307
include/Kbuild | 1 -
include/keys/{trusted.h => trusted_tpm.h} | 49 +---
include/linux/tpm.h| 251 ++--
security/keys/Makefile | 2 +-
security/keys/trusted-keys/Makefile| 8 +
.../{trusted.c => trusted-keys/trusted_tpm1.c} | 96 +++
security/keys/trusted-keys/trusted_tpm2.c | 314 +
11 files changed, 652 insertions(+), 759 deletions(-)
rename include/keys/{trusted.h => trusted_tpm.h} (77%)
create mode 100644 security/keys/trusted-keys/Makefile
rename security/keys/{trusted.c => trusted-keys/trusted_tpm1.c} (94%)
create mode 100644 security/keys/trusted-keys/trusted_tpm2.c
--
2.7.4
I fixed a merge conflict caused by James' commit. Already pushed.
Compiling test kernel ATM i.e. tested-by's will follow later.
/Jarkko
Are you missing patch 4 on master?
Re: [PATCH 0/2] iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM
On Wed Oct 09 19, Bjorn Helgaas wrote: From: Bjorn Helgaas I think intel-iommu.c depends on CONFIG_AMD_IOMMU in an undesirable way: When CONFIG_INTEL_IOMMU_SVM=y, iommu_enable_dev_iotlb() calls PRI interfaces (pci_reset_pri() and pci_enable_pri()), but those are only implemented when CONFIG_PCI_PRI is enabled. If CONFIG_PCI_PRI is not enabled, there are stubs that just return failure. The INTEL_IOMMU_SVM Kconfig does nothing with PCI_PRI, but AMD_IOMMU selects PCI_PRI. So if AMD_IOMMU is enabled, intel-iommu.c gets the full PRI interfaces. If AMD_IOMMU is not enabled, it gets the PRI stubs. This seems wrong. The first patch here makes INTEL_IOMMU_SVM select PCI_PRI so intel-iommu.c always gets the full PRI interfaces. The second patch moves pci_prg_resp_pasid_required(), which simply returns a bit from the PCI capability, from #ifdef CONFIG_PCI_PASID to #ifdef CONFIG_PCI_PRI. This is related because INTEL_IOMMU_SVM already *does* select PCI_PASID, so it previously always got pci_prg_resp_pasid_required() even though it got stubs for other PRI things. Since these are related and I have several follow-on ATS-related patches in the queue, I'd like to take these both via the PCI tree. Bjorn Helgaas (2): iommu/vt-d: Select PCI_PRI for INTEL_IOMMU_SVM PCI/ATS: Move pci_prg_resp_pasid_required() to CONFIG_PCI_PRI drivers/iommu/Kconfig | 1 + drivers/pci/ats.c | 55 +++-- include/linux/pci-ats.h | 11 - 3 files changed, 31 insertions(+), 36 deletions(-) -- 2.23.0.581.g78d2f28ef7-goog ___ iommu mailing list io...@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu Reviewed-by: Jerry Snitselaar
Re: [PATCH] tpm: add check after commands attribs tab allocation
On Mon Oct 07 19, Tadeusz Struk wrote:
devm_kcalloc() can fail and return NULL so we need to check for that.
Fixes: 58472f5cd4f6f ("tpm: validate TPM 2.0 commands")
Signed-off-by: Tadeusz Struk
---
drivers/char/tpm/tpm2-cmd.c |4
1 file changed, 4 insertions(+)
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index ba9acae83bff..5817dfe5c5d2 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -939,6 +939,10 @@ static int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip)
chip->cc_attrs_tbl = devm_kcalloc(>dev, 4, nr_commands,
GFP_KERNEL);
+ if (!chip->cc_attrs_tbl) {
+ rc = -ENOMEM;
+ goto out;
+ }
rc = tpm_buf_init(, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_CAPABILITY);
if (rc)
Reviewed-by: Jerry Snitselaar
Re: [PATCH v3 1/2] tpm: Use GFP_KERNEL for allocating struct tpm_buf
On Mon Oct 07 19, James Bottomley wrote: From: James Bottomley Subject: [PATCH] tpm: use GFP kernel for tpm_buf allocations The current code uses GFP_HIGHMEM, which is wrong because GFP_HIGHMEM (on 32 bit systems) is memory ordinarily inaccessible to the kernel and should only be used for allocations affecting userspace. In order to make highmem visible to the kernel on 32 bit it has to be kmapped, which consumes valuable entries in the kmap region. Since the tpm_buf is only ever used in the kernel, switch to using a GFP_KERNEL allocation so as not to waste kmap space on 32 bits. Fixes: a74f8b36352e (tpm: introduce tpm_buf) Signed-off-by: James Bottomley --- Reviewed-by: Jerry Snitselaar
[tip: efi/urgent] efi/tpm: Only set 'efi_tpm_final_log_size' after successful event log parsing
The following commit has been merged into the efi/urgent branch of tip:
Commit-ID: e658c82be5561412c5e83b5e74e9da4830593f3e
Gitweb:
https://git.kernel.org/tip/e658c82be5561412c5e83b5e74e9da4830593f3e
Author:Jerry Snitselaar
AuthorDate:Wed, 02 Oct 2019 18:59:02 +02:00
Committer: Ingo Molnar
CommitterDate: Mon, 07 Oct 2019 15:24:36 +02:00
efi/tpm: Only set 'efi_tpm_final_log_size' after successful event log parsing
If __calc_tpm2_event_size() fails to parse an event it will return 0,
resulting tpm2_calc_event_log_size() returning -1. Currently there is
no check of this return value, and 'efi_tpm_final_log_size' can end up
being set to this negative value resulting in a crash like this one:
BUG: unable to handle page fault for address: bc8fc00866ad
#PF: supervisor read access in kernel mode
#PF: error_code(0x) - not-present page
RIP: 0010:memcpy_erms+0x6/0x10
Call Trace:
tpm_read_log_efi()
tpm_bios_log_setup()
tpm_chip_register()
tpm_tis_core_init.cold.9+0x28c/0x466
tpm_tis_plat_probe()
platform_drv_probe()
...
Also __calc_tpm2_event_size() returns a size of 0 when it fails
to parse an event, so update function documentation to reflect this.
The root cause of the issue that caused the failure of event parsing
in this case is resolved by Peter Jone's patchset dealing with large
event logs where crossing over a page boundary causes the page with
the event count to be unmapped.
Signed-off-by: Jerry Snitselaar
Signed-off-by: Ard Biesheuvel
Cc: Ben Dooks
Cc: Dave Young
Cc: Jarkko Sakkinen
Cc: Linus Torvalds
Cc: Lukas Wunner
Cc: Lyude Paul
Cc: Matthew Garrett
Cc: Octavian Purdila
Cc: Peter Jones
Cc: Peter Zijlstra
Cc: Scott Talbert
Cc: Thomas Gleixner
Cc: linux-...@vger.kernel.org
Cc: linux-integr...@vger.kernel.org
Cc: sta...@vger.kernel.org
Fixes: c46f3405692de ("tpm: Reserve the TPM final events table")
Link: https://lkml.kernel.org/r/20191002165904.8819-6-ard.biesheu...@linaro.org
Signed-off-by: Ingo Molnar
---
drivers/firmware/efi/tpm.c | 9 -
include/linux/tpm_eventlog.h | 2 +-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index b9ae5c6..703469c 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -85,11 +85,18 @@ int __init efi_tpm_eventlog_init(void)
final_tbl->nr_events,
log_tbl->log);
}
+
+ if (tbl_size < 0) {
+ pr_err(FW_BUG "Failed to parse event in TPM Final Events
Log\n");
+ goto out_calc;
+ }
+
memblock_reserve((unsigned long)final_tbl,
tbl_size + sizeof(*final_tbl));
- early_memunmap(final_tbl, sizeof(*final_tbl));
efi_tpm_final_log_size = tbl_size;
+out_calc:
+ early_memunmap(final_tbl, sizeof(*final_tbl));
out:
early_memunmap(log_tbl, sizeof(*log_tbl));
return ret;
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index b50cc3a..131ea1b 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -152,7 +152,7 @@ struct tcg_algorithm_info {
* total. Once we've done this we know the offset of the data length field,
* and can calculate the total size of the event.
*
- * Return: size of the event on success, <0 on failure
+ * Return: size of the event on success, 0 on failure
*/
static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
Re: [PATCH] KEYS: asym_tpm: Switch to get_random_bytes()
On Fri Oct 04 19, Jerry Snitselaar wrote: On Fri Oct 04 19, James Bottomley wrote: On Fri, 2019-10-04 at 11:33 -0700, Jerry Snitselaar wrote: On Fri Oct 04 19, James Bottomley wrote: On Fri, 2019-10-04 at 21:22 +0300, Jarkko Sakkinen wrote: > On Thu, Oct 03, 2019 at 04:59:37PM -0700, James Bottomley wrote: > > I think the principle of using multiple RNG sources for strong > > keys is a sound one, so could I propose a compromise: We have > > a tpm subsystem random number generator that, when asked for > > random bytes first extracts bytes from the TPM RNG and > > places it into the kernel entropy pool and then asks for > > random bytes from the kernel RNG? That way, it will always have > > the entropy to satisfy the request and in the worst case, where > > the kernel has picked up no other entropy sources at all it > > will be equivalent to what we have now (single entropy source) > > but usually it will be a much better mixed entropy source. > > I think we should rely the existing architecture where TPM is > contributing to the entropy pool as hwrng. That doesn't seem to work: when I trace what happens I see us inject 32 bytes of entropy at boot time, but never again. I think the problem is the kernel entropy pool is push not pull and we have no triggering event in the TPM to get us to push. I suppose we could set a timer to do this or perhaps there is a pull hook and we haven't wired it up correctly? James Shouldn't hwrng_fillfn be pulling from it? It should, but the problem seems to be it only polls the "current" hw rng ... it doesn't seem to have a concept that there may be more than one. What happens, according to a brief reading of the code, is when multiple are registered, it determines what the "best" one is and then only pulls from that. What I think it should be doing is filling from all of them using the entropy quality to adjust how many bits we get. James Most of them don't even set quality, including the tpm, so they end up at the end of the list. For the ones that do I'm not sure how they determined the value. For example virtio-rng sets quality to 1000. I should have added that I like that idea though.
Re: [PATCH v3 2/2] tpm: Detach page allocation from tpm_buf
On Fri Oct 04 19, James Bottomley wrote:
On Fri, 2019-10-04 at 13:37 -0400, Mimi Zohar wrote:
On Fri, 2019-10-04 at 09:37 -0700, James Bottomley wrote:
> On Thu, 2019-10-03 at 21:51 +0300, Jarkko Sakkinen wrote:
> > As has been seen recently, binding the buffer allocation and
> > tpm_buf
> > together is sometimes far from optimal.
>
> Can you elaborate on this a bit more? I must have missed the
> discussion.
Refer to e13cd21ffd50 ("tpm: Wrap the buffer from the caller to
tpm_buf in tpm_send()") for the details.
Yes, I get that, but to my mind that calls for moving the
tpm_init/destroy_buf into the callers of tpm_send (which, for the most
part, already exist), which means there's no need to separate the buf
and data lifetimes.
James
Sumit has been working on a patchset that does this. His patchset
converts both the asymmetric keys and trusted keys code to using the
tpm_buf manipulation functions.
Re: [PATCH v4 0/4] tpm: add update_durations class op to allow override of chip supplied values
On Wed Oct 02 19, Jarkko Sakkinen wrote: On Mon, Sep 02, 2019 at 07:27:32AM -0700, Jerry Snitselaar wrote: We've run into a case where a customer has an STM TPM 1.2 chip (version 1.2.8.28), that is getting into an inconsistent state and they end up getting tpm transmit errors. In really old tpm code this wasn't seen because the code that grabbed the duration values from the chip could fail silently, and would proceed to just use default values and move forward. More recent code though successfully gets the duration values from the chip, and using those values this particular chip version gets into the state seen by the customer. The idea with this patchset is to provide a facility like the update_timeouts operation to allow the override of chip supplied values. changes from v3: * Assign value to version when tpm1_getcap is successful for TPM 1.1 device not when it fails. changes from v2: * Added patch 1/3 * Rework tpm_tis_update_durations to make use of new version structs and pull tpm1_getcap calls out of loop. changes from v1: * Remove unneeded newline * Formatting cleanups * Change tpm_tis_update_durations to be a void function, and use chip->duration_adjusted to track whether adjustment was made. Jarkko Sakkinen (1): tpm: Remove duplicate code from caps_show() in tpm-sysfs.c Jerry Snitselaar (2): tpm: provide a way to override the chip returned durations tpm_tis: override durations for STM tpm with firmware 1.2.8.28 I applied to my master branch. Probably hard to get wide testing given the "niche" case when the issue happens. Should be sufficient that the commonc case still works. /Jarkko Yeah, it is a pain. The people with the problem systems tested an earlier version of Alexey's patches. I have a system with a different rev STM device, so I did some testing with a modified patch that keyed off that revision, but it will be hard to get it wide exposure.
Re: [PATCH 3/3] tpm: Fix TPM 1.2 Shutdown sequence to prevent future TPM operations
On Wed Oct 02 19, Jerry Snitselaar wrote:
On Wed Oct 02 19, Sasha Levin wrote:
On Wed, Oct 02, 2019 at 03:57:58PM +0200, Greg KH wrote:
On Wed, Oct 02, 2019 at 04:14:44PM +0300, Jarkko Sakkinen wrote:
From: Vadim Sukhomlinov
commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 upstream
TPM 2.0 Shutdown involve sending TPM2_Shutdown to TPM chip and disabling
future TPM operations. TPM 1.2 behavior was different, future TPM
operations weren't disabled, causing rare issues. This patch ensures
that future TPM operations are disabled.
Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
Cc: sta...@vger.kernel.org
Signed-off-by: Vadim Sukhomlinov
[dianders: resolved merge conflicts with mainline]
Signed-off-by: Douglas Anderson
Reviewed-by: Jarkko Sakkinen
Signed-off-by: Jarkko Sakkinen
---
drivers/char/tpm/tpm-chip.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
What kernel version(s) is this for?
It would go to 4.19, we've recently reverted an incorrect backport of
this patch.
Jarkko, why is this patch 3/3? We haven't seen the first two on the
mailing list, do we need anything besides this patch?
--
Thanks,
Sasha
It looks like there was a problem mailing the earlier patchset, and patches 1
and 2
weren't cc'd to stable, but patch 3 was.
Is linux-stab...@vger.kernel.org a valid address?
Re: [PATCH 3/3] tpm: Fix TPM 1.2 Shutdown sequence to prevent future TPM operations
On Wed Oct 02 19, Sasha Levin wrote:
On Wed, Oct 02, 2019 at 03:57:58PM +0200, Greg KH wrote:
On Wed, Oct 02, 2019 at 04:14:44PM +0300, Jarkko Sakkinen wrote:
From: Vadim Sukhomlinov
commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 upstream
TPM 2.0 Shutdown involve sending TPM2_Shutdown to TPM chip and disabling
future TPM operations. TPM 1.2 behavior was different, future TPM
operations weren't disabled, causing rare issues. This patch ensures
that future TPM operations are disabled.
Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
Cc: sta...@vger.kernel.org
Signed-off-by: Vadim Sukhomlinov
[dianders: resolved merge conflicts with mainline]
Signed-off-by: Douglas Anderson
Reviewed-by: Jarkko Sakkinen
Signed-off-by: Jarkko Sakkinen
---
drivers/char/tpm/tpm-chip.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
What kernel version(s) is this for?
It would go to 4.19, we've recently reverted an incorrect backport of
this patch.
Jarkko, why is this patch 3/3? We haven't seen the first two on the
mailing list, do we need anything besides this patch?
--
Thanks,
Sasha
It looks like there was a problem mailing the earlier patchset, and patches 1
and 2
weren't cc'd to stable, but patch 3 was.
Re: [PATCH v4 0/4] tpm: add update_durations class op to allow override of chip supplied values
On Mon Sep 02 19, Jerry Snitselaar wrote: We've run into a case where a customer has an STM TPM 1.2 chip (version 1.2.8.28), that is getting into an inconsistent state and they end up getting tpm transmit errors. In really old tpm code this wasn't seen because the code that grabbed the duration values from the chip could fail silently, and would proceed to just use default values and move forward. More recent code though successfully gets the duration values from the chip, and using those values this particular chip version gets into the state seen by the customer. The idea with this patchset is to provide a facility like the update_timeouts operation to allow the override of chip supplied values. changes from v3: * Assign value to version when tpm1_getcap is successful for TPM 1.1 device not when it fails. changes from v2: * Added patch 1/3 * Rework tpm_tis_update_durations to make use of new version structs and pull tpm1_getcap calls out of loop. changes from v1: * Remove unneeded newline * Formatting cleanups * Change tpm_tis_update_durations to be a void function, and use chip->duration_adjusted to track whether adjustment was made. Jarkko Sakkinen (1): tpm: Remove duplicate code from caps_show() in tpm-sysfs.c Jerry Snitselaar (2): tpm: provide a way to override the chip returned durations tpm_tis: override durations for STM tpm with firmware 1.2.8.28 Anyone else have any feedback on this patchset?
Re: [PATCH] tpm: Detach page allocation from tpm_buf
On Thu Sep 26 19, Jarkko Sakkinen wrote:
As has been seen recently, binding the buffer allocation and tpm_buf
together is sometimes far from optimal. The buffer might come from the
caller namely when tpm_send() is used by another subsystem. In addition we
can stability in call sites w/o rollback (e.g. power events)>
Take allocation out of the tpm_buf framework and make it purely a wrapper
for the data buffer.
Link: https://patchwork.kernel.org/patch/11146585/
Cc: Mimi Zohar
Cc: Jerry Snitselaar
Cc: James Bottomley
Cc: Sumit Garg
Cc: Stefan Berger
Signed-off-by: Jarkko Sakkinen
---
v2:
* In tpm2_get_random(), TPM2_CC_GET_RANDOM was accidently switch to
TPM2_CC_PCR_EXTEND. Now it has been switched back.
drivers/char/tpm/tpm-sysfs.c | 19 ++-
drivers/char/tpm/tpm.h| 40 ++---
drivers/char/tpm/tpm1-cmd.c | 114 +
drivers/char/tpm/tpm2-cmd.c | 265 +++---
drivers/char/tpm/tpm2-space.c | 64 +---
drivers/char/tpm/tpm_vtpm_proxy.c | 24 +--
6 files changed, 333 insertions(+), 193 deletions(-)
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index edfa89160010..eeb90c9225b9 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -32,21 +32,26 @@ struct tpm_readpubek_out {
static ssize_t pubek_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
- struct tpm_buf tpm_buf;
- struct tpm_readpubek_out *out;
- int i;
- char *str = buf;
struct tpm_chip *chip = to_tpm_chip(dev);
+ struct tpm_readpubek_out *out;
+ struct page *data_page;
+ struct tpm_buf tpm_buf;
char anti_replay[20];
+ char *str = buf;
+ int i;
memset(_replay, 0, sizeof(anti_replay));
if (tpm_try_get_ops(chip))
return 0;
- if (tpm_buf_init(_buf, TPM_TAG_RQU_COMMAND, TPM_ORD_READPUBEK))
+ data_page = alloc_page(GFP_HIGHUSER);
+ if (!data_page)
goto out_ops;
+ tpm_buf_reset(_buf, kmap(data_page), PAGE_SIZE, TPM_TAG_RQU_COMMAND,
+ TPM_ORD_READPUBEK);
+
tpm_buf_append(_buf, anti_replay, sizeof(anti_replay));
if (tpm_transmit_cmd(chip, _buf, READ_PUBEK_RESULT_MIN_BODY_SIZE,
@@ -83,7 +88,9 @@ static ssize_t pubek_show(struct device *dev, struct
device_attribute *attr,
}
out_buf:
- tpm_buf_destroy(_buf);
+ kunmap(data_page);
+ __free_page(data_page);
+
out_ops:
tpm_put_ops(chip);
return str - buf;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7fea3e0ca86..45316e5d2d36 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -284,36 +284,30 @@ enum tpm_buf_flags {
};
struct tpm_buf {
- struct page *data_page;
- unsigned int flags;
u8 *data;
+ unsigned int size;
+ unsigned int flags;
};
-static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
+static inline void tpm_buf_reset(struct tpm_buf *buf, u8 *data,
+ unsigned int size, u16 tag, u32 ordinal)
{
- struct tpm_header *head = (struct tpm_header *)buf->data;
+ struct tpm_header *head = (struct tpm_header *)data;
- head->tag = cpu_to_be16(tag);
- head->length = cpu_to_be32(sizeof(*head));
- head->ordinal = cpu_to_be32(ordinal);
-}
-
-static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
-{
- buf->data_page = alloc_page(GFP_HIGHUSER);
- if (!buf->data_page)
- return -ENOMEM;
+ /* sanity check */
+ if (size < TPM_HEADER_SIZE) {
+ WARN(1, "tpm_buf: overflow\n");
+ buf->flags |= TPM_BUF_OVERFLOW;
+ return;
+ }
+ buf->data = data;
+ buf->size = size;
buf->flags = 0;
- buf->data = kmap(buf->data_page);
- tpm_buf_reset(buf, tag, ordinal);
- return 0;
-}
-static inline void tpm_buf_destroy(struct tpm_buf *buf)
-{
- kunmap(buf->data_page);
- __free_page(buf->data_page);
+ head->tag = cpu_to_be16(tag);
+ head->length = cpu_to_be32(sizeof(*head));
+ head->ordinal = cpu_to_be32(ordinal);
}
static inline u32 tpm_buf_length(struct tpm_buf *buf)
@@ -341,7 +335,7 @@ static inline void tpm_buf_append(struct tpm_buf *buf,
if (buf->flags & TPM_BUF_OVERFLOW)
return;
- if ((len + new_len) > PAGE_SIZE) {
+ if ((len + new_len) > buf->size) {
WARN(1, "tpm_buf: overflow\n");
buf->flags |= TPM_BUF_OVERFLOW;
return;
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 149e953ca369..2753699454ab 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -323,19 +323,25 @@ unsigned long tpm1_calc_ordinal_duration(struct tpm_ch
Re: [PATCH] tpm: Wrap the buffer from the caller to tpm_buf in tpm_send()
On Mon Sep 16 19, Jerry Snitselaar wrote:
On Mon Sep 16 19, Jarkko Sakkinen wrote:
tpm_send() does not give anymore the result back to the caller. This
would require another memcpy(), which kind of tells that the whole
approach is somewhat broken. Instead, as Mimi suggested, this commit
just wraps the data to the tpm_buf, and thus the result will not go to
the garbage.
Obviously this assumes from the caller that it passes large enough
buffer, which makes the whole API somewhat broken because it could be
different size than @buflen but since trusted keys is the only module
using this API right now I think that this fix is sufficient for the
moment.
In the near future the plan is to replace the parameters with a tpm_buf
created by the caller.
Reported-by: Mimi Zohar
Suggested-by: Mimi Zohar
Cc: sta...@vger.kernel.org
Fixes: 412eb585587a ("use tpm_buf in tpm_transmit_cmd() as the IO parameter")
Signed-off-by: Jarkko Sakkinen
---
drivers/char/tpm/tpm-interface.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index d9ace5480665..2459d36dd8cc 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -358,13 +358,9 @@ int tpm_send(struct tpm_chip *chip, void *cmd, size_t
buflen)
if (!chip)
return -ENODEV;
- rc = tpm_buf_init(, 0, 0);
- if (rc)
- goto out;
-
- memcpy(buf.data, cmd, buflen);
+ buf.data = cmd;
rc = tpm_transmit_cmd(chip, , 0, "attempting to a send a command");
- tpm_buf_destroy();
+
out:
tpm_put_ops(chip);
return rc;
--
2.20.1
Nothing uses the out label any longer so it should be dropped as well, but
other than that...
Acked-by: Jerry Snitselaar
sigh (wrong emacs macro hit), that should be:
Reviewed-by: Jerry Snitselaar
Re: [PATCH] tpm: Wrap the buffer from the caller to tpm_buf in tpm_send()
On Mon Sep 16 19, Jarkko Sakkinen wrote:
tpm_send() does not give anymore the result back to the caller. This
would require another memcpy(), which kind of tells that the whole
approach is somewhat broken. Instead, as Mimi suggested, this commit
just wraps the data to the tpm_buf, and thus the result will not go to
the garbage.
Obviously this assumes from the caller that it passes large enough
buffer, which makes the whole API somewhat broken because it could be
different size than @buflen but since trusted keys is the only module
using this API right now I think that this fix is sufficient for the
moment.
In the near future the plan is to replace the parameters with a tpm_buf
created by the caller.
Reported-by: Mimi Zohar
Suggested-by: Mimi Zohar
Cc: sta...@vger.kernel.org
Fixes: 412eb585587a ("use tpm_buf in tpm_transmit_cmd() as the IO parameter")
Signed-off-by: Jarkko Sakkinen
---
drivers/char/tpm/tpm-interface.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index d9ace5480665..2459d36dd8cc 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -358,13 +358,9 @@ int tpm_send(struct tpm_chip *chip, void *cmd, size_t
buflen)
if (!chip)
return -ENODEV;
- rc = tpm_buf_init(, 0, 0);
- if (rc)
- goto out;
-
- memcpy(buf.data, cmd, buflen);
+ buf.data = cmd;
rc = tpm_transmit_cmd(chip, , 0, "attempting to a send a command");
- tpm_buf_destroy();
+
out:
tpm_put_ops(chip);
return rc;
--
2.20.1
Nothing uses the out label any longer so it should be dropped as well, but
other than that...
Acked-by: Jerry Snitselaar
Re: [PATCH] tpm: Call tpm_put_ops() when the validation for @digests fails
On Tue Sep 10 19, Jarkko Sakkinen wrote:
The chip is not released when the validation for @digests fails. Add
tpm_put_ops() to the failure path.
Cc: sta...@vger.kernel.org
Reported-by: Roberto Sassu
Signed-off-by: Jarkko Sakkinen
Reviewed-by: Jerry Snitselaar
---
drivers/char/tpm/tpm-interface.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 208e5ba40e6e..c7eeb40feac8 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -320,18 +320,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
if (!chip)
return -ENODEV;
- for (i = 0; i < chip->nr_allocated_banks; i++)
- if (digests[i].alg_id != chip->allocated_banks[i].alg_id)
- return -EINVAL;
+ for (i = 0; i < chip->nr_allocated_banks; i++) {
+ if (digests[i].alg_id != chip->allocated_banks[i].alg_id) {
+ rc = EINVAL;
+ goto out;
+ }
+ }
if (chip->flags & TPM_CHIP_FLAG_TPM2) {
rc = tpm2_pcr_extend(chip, pcr_idx, digests);
- tpm_put_ops(chip);
- return rc;
+ goto out;
}
rc = tpm1_pcr_extend(chip, pcr_idx, digests[0].digest,
"attempting extend a PCR value");
+
+out:
tpm_put_ops(chip);
return rc;
}
--
2.20.1
Re: [PATCH v4] tpm: Parse event log from TPM2 ACPI table
On Fri Aug 30 19, Jordan Hand wrote:
For systems with a TPM2 chip which use ACPI to expose event logs, retrieve the
crypto-agile event log from the TPM2 ACPI table. The TPM2 table is defined
in section 7.3 of the TCG ACPI Specification (see link).
The TPM2 table is used by SeaBIOS in place of the TCPA table when the system's
TPM is version 2.0 to denote (among other metadata) the location of the
crypto-agile log.
Link: https://trustedcomputinggroup.org/resource/tcg-acpi-specification/
Signed-off-by: Jordan Hand
---
drivers/char/tpm/eventlog/acpi.c | 60 ++--
1 file changed, 41 insertions(+), 19 deletions(-)
diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
index 63ada5e53f13..38a8bcec1dd5 100644
--- a/drivers/char/tpm/eventlog/acpi.c
+++ b/drivers/char/tpm/eventlog/acpi.c
@@ -41,17 +41,23 @@ struct acpi_tcpa {
};
};
+/* If an event log is present, the TPM2 ACPI table will contain the full
+ * trailer
+ */
+
/* read binary bios log */
int tpm_read_log_acpi(struct tpm_chip *chip)
{
- struct acpi_tcpa *buff;
+ struct acpi_table_header *buff;
+ struct acpi_tcpa *tcpa;
+ struct acpi_tpm2_trailer *tpm2_trailer;
acpi_status status;
void __iomem *virt;
u64 len, start;
+ int log_type;
struct tpm_bios_log *log;
-
- if (chip->flags & TPM_CHIP_FLAG_TPM2)
- return -ENODEV;
+ bool is_tpm2 = chip->flags & TPM_CHIP_FLAG_TPM2;
+ acpi_string table_sig;
log = >log;
@@ -61,26 +67,42 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
if (!chip->acpi_dev_handle)
return -ENODEV;
- /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
- status = acpi_get_table(ACPI_SIG_TCPA, 1,
- (struct acpi_table_header **));
+ /* Find TCPA or TPM2 entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
+ table_sig = is_tpm2 ? ACPI_SIG_TPM2 : ACPI_SIG_TCPA;
+ status = acpi_get_table(table_sig, 1, );
if (ACPI_FAILURE(status))
return -ENODEV;
- switch(buff->platform_class) {
- case BIOS_SERVER:
- len = buff->server.log_max_len;
- start = buff->server.log_start_addr;
- break;
- case BIOS_CLIENT:
- default:
- len = buff->client.log_max_len;
- start = buff->client.log_start_addr;
- break;
+ if (!is_tpm2) {
+ tcpa = (struct acpi_tcpa *)buff;
+ switch (tcpa->platform_class) {
+ case BIOS_SERVER:
+ len = tcpa->server.log_max_len;
+ start = tcpa->server.log_start_addr;
+ break;
+ case BIOS_CLIENT:
+ default:
+ len = tcpa->client.log_max_len;
+ start = tcpa->client.log_start_addr;
+ break;
+ }
+ log_type = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
+ } else if (buff->length ==
+ sizeof(struct acpi_table_tpm2) +
+ sizeof(struct acpi_tpm2_trailer)) {
+ tpm2_trailer = (struct acpi_tpm2_trailer *)buff;
+
+ len = tpm2_trailer.minimum_log_length;
+ start = tpm2_trailer.log_address;
Are your builds not failing here? Both v3 and v4 have this.
+ log_type = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
+ } else {
+ return -ENODEV;
}
+
if (!len) {
- dev_warn(>dev, "%s: TCPA log area empty\n", __func__);
+ dev_warn(>dev, "%s: %s log area empty\n",
+__func__, table_sig);
return -EIO;
}
@@ -98,7 +120,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
memcpy_fromio(log->bios_event_log, virt, len);
acpi_os_unmap_iomem(virt, len);
- return EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
+ return log_type;
err:
kfree(log->bios_event_log);
--
2.20.1
Re: [PATCH AUTOSEL 4.19 126/167] tpm: Fix TPM 1.2 Shutdown sequence to prevent future TPM operations
On Tue Sep 03 19, Doug Anderson wrote:
Hi,
On Tue, Sep 3, 2019 at 9:28 AM Sasha Levin wrote:
From: Vadim Sukhomlinov
[ Upstream commit db4d8cb9c9f2af71c4d087817160d866ed572cc9 ]
TPM 2.0 Shutdown involve sending TPM2_Shutdown to TPM chip and disabling
future TPM operations. TPM 1.2 behavior was different, future TPM
operations weren't disabled, causing rare issues. This patch ensures
that future TPM operations are disabled.
Fixes: d1bd4a792d39 ("tpm: Issue a TPM2_Shutdown for TPM2 devices.")
Cc: sta...@vger.kernel.org
Signed-off-by: Vadim Sukhomlinov
[dianders: resolved merge conflicts with mainline]
Signed-off-by: Douglas Anderson
Reviewed-by: Jarkko Sakkinen
Signed-off-by: Jarkko Sakkinen
Signed-off-by: Sasha Levin
---
drivers/char/tpm/tpm-chip.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
Jarkko: did you deal with the issues that came up in response to my
post? Are you happy with this going into 4.19 stable at this point?
I notice this has your Signed-off-by so maybe?
I think that is just the signed-off-by chain coming from the upstream patch.
Jarkko mentioned getting to the backports after Linux Plumbers, which is next
week.
-Doug
[PATCH v4 2/3] tpm: provide a way to override the chip returned durations
Patch adds method ->update_durations to override returned
durations in case TPM chip misbehaves for TPM 1.2 drivers.
Cc: Peter Huewe
Cc: Jason Gunthorpe
Signed-off-by: Alexey Klimov
Signed-off-by: Jerry Snitselaar
Reviewed-by: Jarkko Sakkinen
---
drivers/char/tpm/tpm1-cmd.c | 15 +++
include/linux/tpm.h | 2 ++
2 files changed, 17 insertions(+)
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 149e953ca369..ca7158fa6e6c 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -343,6 +343,7 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
{
cap_t cap;
unsigned long timeout_old[4], timeout_chip[4], timeout_eff[4];
+ unsigned long durations[3];
ssize_t rc;
rc = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, , NULL,
@@ -427,6 +428,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_long));
chip->duration[TPM_LONG_LONG] = 0; /* not used under 1.2 */
+ /*
+* Provide the ability for vendor overrides of duration values in case
+* of misreporting.
+*/
+ if (chip->ops->update_durations)
+ chip->ops->update_durations(chip, durations);
+
+ if (chip->duration_adjusted) {
+ dev_info(>dev, HW_ERR "Adjusting reported durations.");
+ chip->duration[TPM_SHORT] = durations[0];
+ chip->duration[TPM_MEDIUM] = durations[1];
+ chip->duration[TPM_LONG] = durations[2];
+ }
+
/* The Broadcom BCM0102 chipset in a Dell Latitude D820 gets the above
* value wrong and apparently reports msecs rather than usecs. So we
* fix up the resulting too-small TPM_SHORT value to make things work.
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 53c0ea9ec9df..bb1d1ac7081d 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -67,6 +67,8 @@ struct tpm_class_ops {
u8 (*status) (struct tpm_chip *chip);
void (*update_timeouts)(struct tpm_chip *chip,
unsigned long *timeout_cap);
+ void (*update_durations)(struct tpm_chip *chip,
+unsigned long *duration_cap);
int (*go_idle)(struct tpm_chip *chip);
int (*cmd_ready)(struct tpm_chip *chip);
int (*request_locality)(struct tpm_chip *chip, int loc);
--
2.21.0
[PATCH v4 3/3] tpm_tis: override durations for STM tpm with firmware 1.2.8.28
There was revealed a bug in the STM TPM chipset used in Dell R415s.
Bug is observed so far only on chipset firmware 1.2.8.28
(1.2 TPM, device-id 0x0, rev-id 78). After some number of
operations chipset hangs and stays in inconsistent state:
tpm_tis 00:09: Operation Timed out
tpm_tis 00:09: tpm_transmit: tpm_send: error -5
Durations returned by the chip are the same like on other
firmware revisions but apparently with specifically 1.2.8.28 fw
durations should be reset to 2 minutes to enable tpm chip work
properly. No working way of updating firmware was found.
This patch adds implementation of ->update_durations method
that matches only STM devices with specific firmware version.
Cc: Peter Huewe
Cc: Jarkko Sakkinen
Cc: Jason Gunthorpe
Signed-off-by: Alexey Klimov
Signed-off-by: Jerry Snitselaar
Reviewed-by: Jarkko Sakkinen
---
drivers/char/tpm/tpm_tis_core.c | 79 +
1 file changed, 79 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index c3181ea9f271..27c6ca031e23 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -506,6 +506,84 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf,
size_t len)
return rc;
}
+struct tis_vendor_durations_override {
+ u32 did_vid;
+ struct tpm1_version version;
+ unsigned long durations[3];
+};
+
+static const struct tis_vendor_durations_override vendor_dur_overrides[] = {
+ /* STMicroelectronics 0x104a */
+ { 0x104a,
+ { 1, 2, 8, 28 },
+ { (2 * 60 * HZ), (2 * 60 * HZ), (2 * 60 * HZ) } },
+};
+
+static void tpm_tis_update_durations(struct tpm_chip *chip,
+unsigned long *duration_cap)
+{
+ struct tpm_tis_data *priv = dev_get_drvdata(>dev);
+ struct tpm1_version *version;
+ u32 did_vid;
+ int i, rc;
+ cap_t cap;
+
+ chip->duration_adjusted = false;
+
+ if (chip->ops->clk_enable != NULL)
+ chip->ops->clk_enable(chip, true);
+
+ rc = tpm_tis_read32(priv, TPM_DID_VID(0), _vid);
+ if (rc < 0) {
+ dev_warn(>dev, "%s: failed to read did_vid. %d\n",
+__func__, rc);
+ goto out;
+ }
+
+ /* Try to get a TPM version 1.2 or 1.1 TPM_CAP_VERSION_INFO */
+ rc = tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
+"attempting to determine the 1.2 version",
+sizeof(cap.version2));
+ if (!rc) {
+ version =
+ } else {
+ rc = tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
+"attempting to determine the 1.1 version",
+sizeof(cap.version1));
+
+ if (rc)
+ goto out;
+
+ version =
+ }
+
+ for (i = 0; i != ARRAY_SIZE(vendor_dur_overrides); i++) {
+ if (vendor_dur_overrides[i].did_vid != did_vid)
+ continue;
+
+ if ((version->major ==
+vendor_dur_overrides[i].version.major) &&
+ (version->minor ==
+vendor_dur_overrides[i].version.minor) &&
+ (version->rev_major ==
+vendor_dur_overrides[i].version.rev_major) &&
+ (version->rev_minor ==
+vendor_dur_overrides[i].version.rev_minor)) {
+
+ memcpy(duration_cap,
+ vendor_dur_overrides[i].durations,
+ sizeof(vendor_dur_overrides[i].durations));
+
+ chip->duration_adjusted = true;
+ goto out;
+ }
+ }
+
+out:
+ if (chip->ops->clk_enable != NULL)
+ chip->ops->clk_enable(chip, false);
+}
+
struct tis_vendor_timeout_override {
u32 did_vid;
unsigned long timeout_us[4];
@@ -842,6 +920,7 @@ static const struct tpm_class_ops tpm_tis = {
.send = tpm_tis_send,
.cancel = tpm_tis_ready,
.update_timeouts = tpm_tis_update_timeouts,
+ .update_durations = tpm_tis_update_durations,
.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_canceled = tpm_tis_req_canceled,
--
2.21.0
[PATCH v4 1/3] tpm: Remove duplicate code from caps_show() in tpm-sysfs.c
From: Jarkko Sakkinen
Replace existing TPM 1.x version structs with new structs that consolidate
the common parts into a single struct so that code duplication is no longer
needed in caps_show().
Cc: Peter Huewe
Cc: Jason Gunthorpe
Cc: Alexey Klimov
Signed-off-by: Jarkko Sakkinen
Reviewed-by: Jerry Snitselaar
Tested-by: Jerry Snitselaar
---
drivers/char/tpm/tpm-sysfs.c | 45 ++--
drivers/char/tpm/tpm.h | 23 --
2 files changed, 33 insertions(+), 35 deletions(-)
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index d9caedda075b..eb05d5df4759 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -217,6 +217,7 @@ static ssize_t caps_show(struct device *dev, struct
device_attribute *attr,
char *buf)
{
struct tpm_chip *chip = to_tpm_chip(dev);
+ struct tpm1_version *version;
ssize_t rc = 0;
char *str = buf;
cap_t cap;
@@ -232,31 +233,31 @@ static ssize_t caps_show(struct device *dev, struct
device_attribute *attr,
str += sprintf(str, "Manufacturer: 0x%x\n",
be32_to_cpu(cap.manufacturer_id));
- /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */
- rc = tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
+ /* TPM 1.2 */
+ if (!tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
"attempting to determine the 1.2 version",
-sizeof(cap.tpm_version_1_2));
- if (!rc) {
- str += sprintf(str,
- "TCG version: %d.%d\nFirmware version: %d.%d\n",
- cap.tpm_version_1_2.Major,
- cap.tpm_version_1_2.Minor,
- cap.tpm_version_1_2.revMajor,
- cap.tpm_version_1_2.revMinor);
- } else {
- /* Otherwise just use TPM_STRUCT_VER */
- if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
- "attempting to determine the 1.1 version",
- sizeof(cap.tpm_version)))
- goto out_ops;
- str += sprintf(str,
- "TCG version: %d.%d\nFirmware version: %d.%d\n",
- cap.tpm_version.Major,
- cap.tpm_version.Minor,
- cap.tpm_version.revMajor,
- cap.tpm_version.revMinor);
+sizeof(cap.version2))) {
+ version =
+ goto out_print;
}
+
+ /* TPM 1.1 */
+ if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
+ "attempting to determine the 1.1 version",
+ sizeof(cap.version1))) {
+ goto out_ops;
+ }
+
+ version =
+
+out_print:
+ str += sprintf(str,
+ "TCG version: %d.%d\nFirmware version: %d.%d\n",
+ version->major, version->minor,
+ version->rev_major, version->rev_minor);
+
rc = str - buf;
+
out_ops:
tpm_put_ops(chip);
return rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7fea3e0ca86..a4f74dd02a35 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -186,19 +186,16 @@ structstclear_flags_t {
u8 bGlobalLock;
} __packed;
-struct tpm_version_t {
- u8 Major;
- u8 Minor;
- u8 revMajor;
- u8 revMinor;
+struct tpm1_version {
+ u8 major;
+ u8 minor;
+ u8 rev_major;
+ u8 rev_minor;
} __packed;
-struct tpm_version_1_2_t {
- __be16 tag;
- u8 Major;
- u8 Minor;
- u8 revMajor;
- u8 revMinor;
+struct tpm1_version2 {
+ __be16 tag;
+ struct tpm1_version version;
} __packed;
struct timeout_t {
@@ -243,8 +240,8 @@ typedef union {
struct stclear_flags_t stclear_flags;
__u8owned;
__be32 num_pcrs;
- struct tpm_version_t tpm_version;
- struct tpm_version_1_2_t tpm_version_1_2;
+ struct tpm1_version version1;
+ struct tpm1_version2 version2;
__be32 manufacturer_id;
struct timeout_t timeout;
struct duration_t duration;
--
2.21.0
[PATCH v4 0/4] tpm: add update_durations class op to allow override of chip supplied values
We've run into a case where a customer has an STM TPM 1.2 chip (version 1.2.8.28), that is getting into an inconsistent state and they end up getting tpm transmit errors. In really old tpm code this wasn't seen because the code that grabbed the duration values from the chip could fail silently, and would proceed to just use default values and move forward. More recent code though successfully gets the duration values from the chip, and using those values this particular chip version gets into the state seen by the customer. The idea with this patchset is to provide a facility like the update_timeouts operation to allow the override of chip supplied values. changes from v3: * Assign value to version when tpm1_getcap is successful for TPM 1.1 device not when it fails. changes from v2: * Added patch 1/3 * Rework tpm_tis_update_durations to make use of new version structs and pull tpm1_getcap calls out of loop. changes from v1: * Remove unneeded newline * Formatting cleanups * Change tpm_tis_update_durations to be a void function, and use chip->duration_adjusted to track whether adjustment was made. Jarkko Sakkinen (1): tpm: Remove duplicate code from caps_show() in tpm-sysfs.c Jerry Snitselaar (2): tpm: provide a way to override the chip returned durations tpm_tis: override durations for STM tpm with firmware 1.2.8.28
Re: [PATCH 1/3 v3] tpm: Remove duplicate code from caps_show() in tpm-sysfs.c
On Thu Aug 29 19, Jerry Snitselaar wrote:
From: Jarkko Sakkinen
Replace existing TPM 1.x version structs with new structs that consolidate
the common parts into a single struct so that code duplication is no longer
needed in caps_show().
Cc: Peter Huewe
Cc: Jason Gunthorpe
Cc: Alexey Klimov
Signed-off-by: Jarkko Sakkinen
Reviewed-by: Jerry Snitselaar
Tested-by: Jerry Snitselaar
---
drivers/char/tpm/tpm-sysfs.c | 44 ++--
drivers/char/tpm/tpm.h | 23 ---
2 files changed, 32 insertions(+), 35 deletions(-)
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index d9caedda075b..e0550f0cfd8f 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -217,6 +217,7 @@ static ssize_t caps_show(struct device *dev, struct
device_attribute *attr,
char *buf)
{
struct tpm_chip *chip = to_tpm_chip(dev);
+ struct tpm1_version *version;
ssize_t rc = 0;
char *str = buf;
cap_t cap;
@@ -232,31 +233,30 @@ static ssize_t caps_show(struct device *dev, struct
device_attribute *attr,
str += sprintf(str, "Manufacturer: 0x%x\n",
be32_to_cpu(cap.manufacturer_id));
- /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */
- rc = tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
+ /* TPM 1.2 */
+ if (!tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
"attempting to determine the 1.2 version",
-sizeof(cap.tpm_version_1_2));
- if (!rc) {
- str += sprintf(str,
- "TCG version: %d.%d\nFirmware version: %d.%d\n",
- cap.tpm_version_1_2.Major,
- cap.tpm_version_1_2.Minor,
- cap.tpm_version_1_2.revMajor,
- cap.tpm_version_1_2.revMinor);
- } else {
- /* Otherwise just use TPM_STRUCT_VER */
- if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
- "attempting to determine the 1.1 version",
- sizeof(cap.tpm_version)))
- goto out_ops;
- str += sprintf(str,
- "TCG version: %d.%d\nFirmware version: %d.%d\n",
- cap.tpm_version.Major,
- cap.tpm_version.Minor,
- cap.tpm_version.revMajor,
- cap.tpm_version.revMinor);
+sizeof(cap.version2))) {
+ version =
+ goto out_print;
}
+
+ /* TPM 1.1 */
+ if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
+ "attempting to determine the 1.1 version",
+ sizeof(cap.version1))) {
+ version =
+ goto out_ops;
+ }
Jarkko, does the following change to the above block look good? I'll
submit a v4. With the current patch it is setting version when the
tpm1_getcap fails for 1.1 instead of when it succeeds. I only have
1.2 and 2.0 devices, so I didn't hit it when testing your patch.
+ /* TPM 1.1 */
+ if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
+ "attempting to determine the 1.1 version",
+ sizeof(cap.version1)))
+ goto out_ops;
+
+ version =
+
+out_print:
+ str += sprintf(str,
+ "TCG version: %d.%d\nFirmware version: %d.%d\n",
+ version->major, version->minor,
+ version->rev_major, version->rev_minor);
+
rc = str - buf;
+
out_ops:
tpm_put_ops(chip);
return rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7fea3e0ca86..a4f74dd02a35 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -186,19 +186,16 @@ structstclear_flags_t {
u8 bGlobalLock;
} __packed;
-struct tpm_version_t {
- u8 Major;
- u8 Minor;
- u8 revMajor;
- u8 revMinor;
+struct tpm1_version {
+ u8 major;
+ u8 minor;
+ u8 rev_major;
+ u8 rev_minor;
} __packed;
-struct tpm_version_1_2_t {
- __be16 tag;
- u8 Major;
- u8 Minor;
- u8 revMajor;
- u8 revMinor;
+struct tpm1_version2 {
+ __be16 tag;
+ struct tpm1_version version;
} __packed;
struct timeout_t {
@@ -243,8 +240,8 @@ typedef union {
struct stclear_flags_t stclear_flags;
__u8owned;
__be32 num_pcrs;
- struct tpm_version_t tpm_version;
- struct tpm_version_1_2_t tpm_version_1_2;
+ struct tpm1_version version1;
+ struct tpm1_version2 version2;
__be32 manufacturer_id;
struct timeout_t timeout;
struct duration_t duration;
--
2.21.0
Re: [PATCH] tpm: Remove duplicate code from caps_show() in tpm-sysfs.c
On Thu Aug 29 19, Jarkko Sakkinen wrote:
Replace existing TPM 1.x version structs with new structs that consolidate
the common parts into a single struct so that code duplication is no longer
needed in caps_show().
Cc: Alexey Klimov
Cc: Jerry Snitselaar
Signed-off-by: Jarkko Sakkinen
---
Jerry, Alexey: Plese include this to the next version of your patches.
This a low priority patch alone so it does not need to be merge upfront.
drivers/char/tpm/tpm-sysfs.c | 44 ++--
drivers/char/tpm/tpm.h | 23 ---
2 files changed, 32 insertions(+), 35 deletions(-)
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index edfa89160010..8064fea2de59 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -217,6 +217,7 @@ static ssize_t caps_show(struct device *dev, struct
device_attribute *attr,
char *buf)
{
struct tpm_chip *chip = to_tpm_chip(dev);
+ struct tpm1_version *version;
ssize_t rc = 0;
char *str = buf;
cap_t cap;
@@ -232,31 +233,30 @@ static ssize_t caps_show(struct device *dev, struct
device_attribute *attr,
str += sprintf(str, "Manufacturer: 0x%x\n",
be32_to_cpu(cap.manufacturer_id));
- /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */
- rc = tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
+ /* TPM 1.2 */
+ if (!tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
"attempting to determine the 1.2 version",
-sizeof(cap.tpm_version_1_2));
- if (!rc) {
- str += sprintf(str,
- "TCG version: %d.%d\nFirmware version: %d.%d\n",
- cap.tpm_version_1_2.Major,
- cap.tpm_version_1_2.Minor,
- cap.tpm_version_1_2.revMajor,
- cap.tpm_version_1_2.revMinor);
- } else {
- /* Otherwise just use TPM_STRUCT_VER */
- if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
- "attempting to determine the 1.1 version",
- sizeof(cap.tpm_version)))
- goto out_ops;
- str += sprintf(str,
- "TCG version: %d.%d\nFirmware version: %d.%d\n",
- cap.tpm_version.Major,
- cap.tpm_version.Minor,
- cap.tpm_version.revMajor,
- cap.tpm_version.revMinor);
+sizeof(cap.version2))) {
+ version =
+ goto out_print;
}
+
+ /* TPM 1.1 */
+ if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
+ "attempting to determine the 1.1 version",
+ sizeof(cap.version1))) {
+ version =
Actually looking at this again, this seems wrong. The version assignment here
should be below this if, or in an else block, yes?
+ goto out_ops;
+ }
+
+out_print:
+ str += sprintf(str,
+ "TCG version: %d.%d\nFirmware version: %d.%d\n",
+ version->major, version->minor,
+ version->rev_major, version->rev_minor);
+
rc = str - buf;
+
out_ops:
tpm_put_ops(chip);
return rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7fea3e0ca86..a4f74dd02a35 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -186,19 +186,16 @@ structstclear_flags_t {
u8 bGlobalLock;
} __packed;
-struct tpm_version_t {
- u8 Major;
- u8 Minor;
- u8 revMajor;
- u8 revMinor;
+struct tpm1_version {
+ u8 major;
+ u8 minor;
+ u8 rev_major;
+ u8 rev_minor;
} __packed;
-struct tpm_version_1_2_t {
- __be16 tag;
- u8 Major;
- u8 Minor;
- u8 revMajor;
- u8 revMinor;
+struct tpm1_version2 {
+ __be16 tag;
+ struct tpm1_version version;
} __packed;
struct timeout_t {
@@ -243,8 +240,8 @@ typedef union {
struct stclear_flags_t stclear_flags;
__u8owned;
__be32 num_pcrs;
- struct tpm_version_t tpm_version;
- struct tpm_version_1_2_t tpm_version_1_2;
+ struct tpm1_version version1;
+ struct tpm1_version2 version2;
__be32 manufacturer_id;
struct timeout_t timeout;
struct duration_t duration;
--
2.20.1
[PATCH 2/3 v3] tpm: provide a way to override the chip returned durations
Patch adds method ->update_durations to override returned
durations in case TPM chip misbehaves for TPM 1.2 drivers.
Cc: Peter Huewe
Cc: Jarkko Sakkinen
Cc: Jason Gunthorpe
Signed-off-by: Alexey Klimov
Signed-off-by: Jerry Snitselaar
---
v3: no changes
v2: newline cleanup as requested by Jarkko
drivers/char/tpm/tpm1-cmd.c | 15 +++
include/linux/tpm.h | 2 ++
2 files changed, 17 insertions(+)
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 149e953ca369..ca7158fa6e6c 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -343,6 +343,7 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
{
cap_t cap;
unsigned long timeout_old[4], timeout_chip[4], timeout_eff[4];
+ unsigned long durations[3];
ssize_t rc;
rc = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, , NULL,
@@ -427,6 +428,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
usecs_to_jiffies(be32_to_cpu(cap.duration.tpm_long));
chip->duration[TPM_LONG_LONG] = 0; /* not used under 1.2 */
+ /*
+* Provide the ability for vendor overrides of duration values in case
+* of misreporting.
+*/
+ if (chip->ops->update_durations)
+ chip->ops->update_durations(chip, durations);
+
+ if (chip->duration_adjusted) {
+ dev_info(>dev, HW_ERR "Adjusting reported durations.");
+ chip->duration[TPM_SHORT] = durations[0];
+ chip->duration[TPM_MEDIUM] = durations[1];
+ chip->duration[TPM_LONG] = durations[2];
+ }
+
/* The Broadcom BCM0102 chipset in a Dell Latitude D820 gets the above
* value wrong and apparently reports msecs rather than usecs. So we
* fix up the resulting too-small TPM_SHORT value to make things work.
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 53c0ea9ec9df..bb1d1ac7081d 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -67,6 +67,8 @@ struct tpm_class_ops {
u8 (*status) (struct tpm_chip *chip);
void (*update_timeouts)(struct tpm_chip *chip,
unsigned long *timeout_cap);
+ void (*update_durations)(struct tpm_chip *chip,
+unsigned long *duration_cap);
int (*go_idle)(struct tpm_chip *chip);
int (*cmd_ready)(struct tpm_chip *chip);
int (*request_locality)(struct tpm_chip *chip, int loc);
--
2.21.0
[PATCH 1/3 v3] tpm: Remove duplicate code from caps_show() in tpm-sysfs.c
From: Jarkko Sakkinen
Replace existing TPM 1.x version structs with new structs that consolidate
the common parts into a single struct so that code duplication is no longer
needed in caps_show().
Cc: Peter Huewe
Cc: Jason Gunthorpe
Cc: Alexey Klimov
Signed-off-by: Jarkko Sakkinen
Reviewed-by: Jerry Snitselaar
Tested-by: Jerry Snitselaar
---
drivers/char/tpm/tpm-sysfs.c | 44 ++--
drivers/char/tpm/tpm.h | 23 ---
2 files changed, 32 insertions(+), 35 deletions(-)
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index d9caedda075b..e0550f0cfd8f 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -217,6 +217,7 @@ static ssize_t caps_show(struct device *dev, struct
device_attribute *attr,
char *buf)
{
struct tpm_chip *chip = to_tpm_chip(dev);
+ struct tpm1_version *version;
ssize_t rc = 0;
char *str = buf;
cap_t cap;
@@ -232,31 +233,30 @@ static ssize_t caps_show(struct device *dev, struct
device_attribute *attr,
str += sprintf(str, "Manufacturer: 0x%x\n",
be32_to_cpu(cap.manufacturer_id));
- /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */
- rc = tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
+ /* TPM 1.2 */
+ if (!tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
"attempting to determine the 1.2 version",
-sizeof(cap.tpm_version_1_2));
- if (!rc) {
- str += sprintf(str,
- "TCG version: %d.%d\nFirmware version: %d.%d\n",
- cap.tpm_version_1_2.Major,
- cap.tpm_version_1_2.Minor,
- cap.tpm_version_1_2.revMajor,
- cap.tpm_version_1_2.revMinor);
- } else {
- /* Otherwise just use TPM_STRUCT_VER */
- if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
- "attempting to determine the 1.1 version",
- sizeof(cap.tpm_version)))
- goto out_ops;
- str += sprintf(str,
- "TCG version: %d.%d\nFirmware version: %d.%d\n",
- cap.tpm_version.Major,
- cap.tpm_version.Minor,
- cap.tpm_version.revMajor,
- cap.tpm_version.revMinor);
+sizeof(cap.version2))) {
+ version =
+ goto out_print;
}
+
+ /* TPM 1.1 */
+ if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
+ "attempting to determine the 1.1 version",
+ sizeof(cap.version1))) {
+ version =
+ goto out_ops;
+ }
+
+out_print:
+ str += sprintf(str,
+ "TCG version: %d.%d\nFirmware version: %d.%d\n",
+ version->major, version->minor,
+ version->rev_major, version->rev_minor);
+
rc = str - buf;
+
out_ops:
tpm_put_ops(chip);
return rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7fea3e0ca86..a4f74dd02a35 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -186,19 +186,16 @@ structstclear_flags_t {
u8 bGlobalLock;
} __packed;
-struct tpm_version_t {
- u8 Major;
- u8 Minor;
- u8 revMajor;
- u8 revMinor;
+struct tpm1_version {
+ u8 major;
+ u8 minor;
+ u8 rev_major;
+ u8 rev_minor;
} __packed;
-struct tpm_version_1_2_t {
- __be16 tag;
- u8 Major;
- u8 Minor;
- u8 revMajor;
- u8 revMinor;
+struct tpm1_version2 {
+ __be16 tag;
+ struct tpm1_version version;
} __packed;
struct timeout_t {
@@ -243,8 +240,8 @@ typedef union {
struct stclear_flags_t stclear_flags;
__u8owned;
__be32 num_pcrs;
- struct tpm_version_t tpm_version;
- struct tpm_version_1_2_t tpm_version_1_2;
+ struct tpm1_version version1;
+ struct tpm1_version2 version2;
__be32 manufacturer_id;
struct timeout_t timeout;
struct duration_t duration;
--
2.21.0
[PATCH 0/3 v3] tpm: add update_durations class op to allow override of chip supplied values
We've run into a case where a customer has an STM TPM 1.2 chip (version 1.2.8.28), that is getting into an inconsistent state and they end up getting tpm transmit errors. In really old tpm code this wasn't seen because the code that grabbed the duration values from the chip could fail silently, and would proceed to just use default values and move forward. More recent code though successfully gets the duration values from the chip, and using those values this particular chip version gets into the state seen by the customer. The idea with this patchset is to provide a facility like the update_timeouts operation to allow the override of chip supplied values. v3: Rework update_durations to make use of new version structs and pull tpm1_getcap calls out of loop. v2: Update Alexey's v1 submission with suggestions made by Jarkko
[PATCH 3/3 v3] tpm_tis: override durations for STM tpm with firmware 1.2.8.28
There was revealed a bug in the STM TPM chipset used in Dell R415s.
Bug is observed so far only on chipset firmware 1.2.8.28
(1.2 TPM, device-id 0x0, rev-id 78). After some number of
operations chipset hangs and stays in inconsistent state:
tpm_tis 00:09: Operation Timed out
tpm_tis 00:09: tpm_transmit: tpm_send: error -5
Durations returned by the chip are the same like on other
firmware revisions but apparently with specifically 1.2.8.28 fw
durations should be reset to 2 minutes to enable tpm chip work
properly. No working way of updating firmware was found.
This patch adds implementation of ->update_durations method
that matches only STM devices with specific firmware version.
Cc: Peter Huewe
Cc: Jarkko Sakkinen
Cc: Jason Gunthorpe
Signed-off-by: Alexey Klimov
Signed-off-by: Jerry Snitselaar
---
v3: Rework update_durations to make use of new version structs
from 1/3 patch, and move tpm1_getcap calls out of the loop.
v2: Make suggested changes from Jarkko
- change struct field name to durations from durs
- formatting cleanups
- turn into void function like update_timeouts and
use chip->duration_adjusted to track whether adjustment occurred.
drivers/char/tpm/tpm_tis_core.c | 79 +
1 file changed, 79 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index c3181ea9f271..27c6ca031e23 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -506,6 +506,84 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf,
size_t len)
return rc;
}
+struct tis_vendor_durations_override {
+ u32 did_vid;
+ struct tpm1_version version;
+ unsigned long durations[3];
+};
+
+static const struct tis_vendor_durations_override vendor_dur_overrides[] = {
+ /* STMicroelectronics 0x104a */
+ { 0x104a,
+ { 1, 2, 8, 28 },
+ { (2 * 60 * HZ), (2 * 60 * HZ), (2 * 60 * HZ) } },
+};
+
+static void tpm_tis_update_durations(struct tpm_chip *chip,
+unsigned long *duration_cap)
+{
+ struct tpm_tis_data *priv = dev_get_drvdata(>dev);
+ struct tpm1_version *version;
+ u32 did_vid;
+ int i, rc;
+ cap_t cap;
+
+ chip->duration_adjusted = false;
+
+ if (chip->ops->clk_enable != NULL)
+ chip->ops->clk_enable(chip, true);
+
+ rc = tpm_tis_read32(priv, TPM_DID_VID(0), _vid);
+ if (rc < 0) {
+ dev_warn(>dev, "%s: failed to read did_vid. %d\n",
+__func__, rc);
+ goto out;
+ }
+
+ /* Try to get a TPM version 1.2 or 1.1 TPM_CAP_VERSION_INFO */
+ rc = tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
+"attempting to determine the 1.2 version",
+sizeof(cap.version2));
+ if (!rc) {
+ version =
+ } else {
+ rc = tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
+"attempting to determine the 1.1 version",
+sizeof(cap.version1));
+
+ if (rc)
+ goto out;
+
+ version =
+ }
+
+ for (i = 0; i != ARRAY_SIZE(vendor_dur_overrides); i++) {
+ if (vendor_dur_overrides[i].did_vid != did_vid)
+ continue;
+
+ if ((version->major ==
+vendor_dur_overrides[i].version.major) &&
+ (version->minor ==
+vendor_dur_overrides[i].version.minor) &&
+ (version->rev_major ==
+vendor_dur_overrides[i].version.rev_major) &&
+ (version->rev_minor ==
+vendor_dur_overrides[i].version.rev_minor)) {
+
+ memcpy(duration_cap,
+ vendor_dur_overrides[i].durations,
+ sizeof(vendor_dur_overrides[i].durations));
+
+ chip->duration_adjusted = true;
+ goto out;
+ }
+ }
+
+out:
+ if (chip->ops->clk_enable != NULL)
+ chip->ops->clk_enable(chip, false);
+}
+
struct tis_vendor_timeout_override {
u32 did_vid;
unsigned long timeout_us[4];
@@ -842,6 +920,7 @@ static const struct tpm_class_ops tpm_tis = {
.send = tpm_tis_send,
.cancel = tpm_tis_ready,
.update_timeouts = tpm_tis_update_timeouts,
+ .update_durations = tpm_tis_update_durations,
.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
.req_canceled = tpm_tis_req_canceled,
--
2.21.0
Re: [PATCH] tpm: Remove duplicate code from caps_show() in tpm-sysfs.c
On Thu Aug 29 19, Jarkko Sakkinen wrote:
Replace existing TPM 1.x version structs with new structs that consolidate
the common parts into a single struct so that code duplication is no longer
needed in caps_show().
Cc: Alexey Klimov
Cc: Jerry Snitselaar
Signed-off-by: Jarkko Sakkinen
---
Jerry, Alexey: Plese include this to the next version of your patches.
This a low priority patch alone so it does not need to be merge upfront.
Will do. I'll add my Reviewed-by and Tested-by to it and submit it with v3.
drivers/char/tpm/tpm-sysfs.c | 44 ++--
drivers/char/tpm/tpm.h | 23 ---
2 files changed, 32 insertions(+), 35 deletions(-)
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
index edfa89160010..8064fea2de59 100644
--- a/drivers/char/tpm/tpm-sysfs.c
+++ b/drivers/char/tpm/tpm-sysfs.c
@@ -217,6 +217,7 @@ static ssize_t caps_show(struct device *dev, struct
device_attribute *attr,
char *buf)
{
struct tpm_chip *chip = to_tpm_chip(dev);
+ struct tpm1_version *version;
ssize_t rc = 0;
char *str = buf;
cap_t cap;
@@ -232,31 +233,30 @@ static ssize_t caps_show(struct device *dev, struct
device_attribute *attr,
str += sprintf(str, "Manufacturer: 0x%x\n",
be32_to_cpu(cap.manufacturer_id));
- /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */
- rc = tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
+ /* TPM 1.2 */
+ if (!tpm1_getcap(chip, TPM_CAP_VERSION_1_2, ,
"attempting to determine the 1.2 version",
-sizeof(cap.tpm_version_1_2));
- if (!rc) {
- str += sprintf(str,
- "TCG version: %d.%d\nFirmware version: %d.%d\n",
- cap.tpm_version_1_2.Major,
- cap.tpm_version_1_2.Minor,
- cap.tpm_version_1_2.revMajor,
- cap.tpm_version_1_2.revMinor);
- } else {
- /* Otherwise just use TPM_STRUCT_VER */
- if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
- "attempting to determine the 1.1 version",
- sizeof(cap.tpm_version)))
- goto out_ops;
- str += sprintf(str,
- "TCG version: %d.%d\nFirmware version: %d.%d\n",
- cap.tpm_version.Major,
- cap.tpm_version.Minor,
- cap.tpm_version.revMajor,
- cap.tpm_version.revMinor);
+sizeof(cap.version2))) {
+ version =
+ goto out_print;
}
+
+ /* TPM 1.1 */
+ if (tpm1_getcap(chip, TPM_CAP_VERSION_1_1, ,
+ "attempting to determine the 1.1 version",
+ sizeof(cap.version1))) {
+ version =
+ goto out_ops;
+ }
+
+out_print:
+ str += sprintf(str,
+ "TCG version: %d.%d\nFirmware version: %d.%d\n",
+ version->major, version->minor,
+ version->rev_major, version->rev_minor);
+
rc = str - buf;
+
out_ops:
tpm_put_ops(chip);
return rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7fea3e0ca86..a4f74dd02a35 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -186,19 +186,16 @@ structstclear_flags_t {
u8 bGlobalLock;
} __packed;
-struct tpm_version_t {
- u8 Major;
- u8 Minor;
- u8 revMajor;
- u8 revMinor;
+struct tpm1_version {
+ u8 major;
+ u8 minor;
+ u8 rev_major;
+ u8 rev_minor;
} __packed;
-struct tpm_version_1_2_t {
- __be16 tag;
- u8 Major;
- u8 Minor;
- u8 revMajor;
- u8 revMinor;
+struct tpm1_version2 {
+ __be16 tag;
+ struct tpm1_version version;
} __packed;
struct timeout_t {
@@ -243,8 +240,8 @@ typedef union {
struct stclear_flags_t stclear_flags;
__u8owned;
__be32 num_pcrs;
- struct tpm_version_t tpm_version;
- struct tpm_version_1_2_t tpm_version_1_2;
+ struct tpm1_version version1;
+ struct tpm1_version2 version2;
__be32 manufacturer_id;
struct timeout_t timeout;
struct duration_t duration;
--
2.20.1
