[PATCH v2 0/5] tpm: Enable usage of TPM TIS with interrupts

[PATCH v2 0/5] tpm: Enable usage of TPM TIS with interrupts

[Stefan Berger]

This series of patches enables the usage of the TPM TIS with interrupts.
We use the unused IRQ 13, which is also accepted by Windows.

    Stefan

v1->v2:
 - Added updated DSDT

Stefan Berger (5):
  tpm_tis: Allow lowering of IRQ also when locality is not active
  tpm: Extend TPMIfClass with get_irqnum() function
  tests: Temporarily ignore DSDT table differences
  acpi: Enable TPM IRQ
  tests: Add updated DSDT

 hw/i386/acpi-build.c         |  11 +++++------
 hw/tpm/tpm_tis_common.c      |   4 ----
 hw/tpm/tpm_tis_isa.c         |   9 +++++++++
 hw/tpm/tpm_tis_sysbus.c      |   9 +++++++++
 include/hw/acpi/tpm.h        |   2 +-
 include/sysemu/tpm.h         |  10 ++++++++++
 tests/data/acpi/q35/DSDT.tis | Bin 8357 -> 8360 bytes
 7 files changed, 34 insertions(+), 11 deletions(-)

-- 
2.24.1

[PATCH v2 1/5] tpm_tis: Allow lowering of IRQ also when locality is not active

[Stefan Berger]

From: Stefan Berger <stefanb@linux.ibm.com>

This patch fixes a bug that occurs when using interrupts. It
allows to lower the IRQ also when a locality is not active.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 hw/tpm/tpm_tis_common.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c
index 1af4bce139..0f42696f1f 100644
--- a/hw/tpm/tpm_tis_common.c
+++ b/hw/tpm/tpm_tis_common.c
@@ -601,10 +601,6 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
         /* hard wired -- ignore */
         break;
     case TPM_TIS_REG_INT_STATUS:
-        if (s->active_locty != locty) {
-            break;
-        }
-
         /* clearing of interrupt flags */
         if (((val & TPM_TIS_INTERRUPTS_SUPPORTED)) &&
             (s->loc[locty].ints & TPM_TIS_INTERRUPTS_SUPPORTED)) {
-- 
2.24.1

[PATCH v2 2/5] tpm: Extend TPMIfClass with get_irqnum() function

[Stefan Berger]

From: Stefan Berger <stefanb@sbct-2.pok.ibm.com>

Implement get_irqnum() as part of the TPMIfClass to be get the assigned
IRQ number.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 hw/tpm/tpm_tis_isa.c    |  9 +++++++++
 hw/tpm/tpm_tis_sysbus.c |  9 +++++++++
 include/sysemu/tpm.h    | 10 ++++++++++
 3 files changed, 28 insertions(+)

diff --git a/hw/tpm/tpm_tis_isa.c b/hw/tpm/tpm_tis_isa.c
index 30ba37079d..63b62f4c21 100644
--- a/hw/tpm/tpm_tis_isa.c
+++ b/hw/tpm/tpm_tis_isa.c
@@ -80,6 +80,14 @@ static enum TPMVersion tpm_tis_isa_get_tpm_version(TPMIf *ti)
     return tpm_tis_get_tpm_version(s);
 }
 
+static uint8_t tpm_tis_isa_get_irqnum(TPMIf *ti)
+{
+    TPMStateISA *isadev = TPM_TIS_ISA(ti);
+    TPMState *s = &isadev->state;
+
+    return s->irq_num;
+}
+
 static void tpm_tis_isa_reset(DeviceState *dev)
 {
     TPMStateISA *isadev = TPM_TIS_ISA(dev);
@@ -148,6 +156,7 @@ static void tpm_tis_isa_class_init(ObjectClass *klass, void *data)
     dc->reset = tpm_tis_isa_reset;
     tc->request_completed = tpm_tis_isa_request_completed;
     tc->get_version = tpm_tis_isa_get_tpm_version;
+    tc->get_irqnum = tpm_tis_isa_get_irqnum;
 }
 
 static const TypeInfo tpm_tis_isa_info = {
diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
index eced1fc843..6cf45e5057 100644
--- a/hw/tpm/tpm_tis_sysbus.c
+++ b/hw/tpm/tpm_tis_sysbus.c
@@ -80,6 +80,14 @@ static enum TPMVersion tpm_tis_sysbus_get_tpm_version(TPMIf *ti)
     return tpm_tis_get_tpm_version(s);
 }
 
+static uint8_t tpm_tis_sysbus_get_irqnum(TPMIf *ti)
+{
+    TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(ti);
+    TPMState *s = &sbdev->state;
+
+    return s->irq_num;
+}
+
 static void tpm_tis_sysbus_reset(DeviceState *dev)
 {
     TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(dev);
@@ -137,6 +145,7 @@ static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data)
     dc->reset = tpm_tis_sysbus_reset;
     tc->request_completed = tpm_tis_sysbus_request_completed;
     tc->get_version = tpm_tis_sysbus_get_tpm_version;
+    tc->get_irqnum = tpm_tis_sysbus_get_irqnum;
 }
 
 static const TypeInfo tpm_tis_sysbus_info = {
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index 03fb25941c..b94a8a2b16 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -41,6 +41,7 @@ typedef struct TPMIfClass {
     enum TpmModel model;
     void (*request_completed)(TPMIf *obj, int ret);
     enum TPMVersion (*get_version)(TPMIf *obj);
+    uint8_t (*get_irqnum)(TPMIf *obj);
 } TPMIfClass;
 
 #define TYPE_TPM_TIS_ISA            "tpm-tis"
@@ -74,4 +75,13 @@ static inline TPMVersion tpm_get_version(TPMIf *ti)
     return TPM_IF_GET_CLASS(ti)->get_version(ti);
 }
 
+static inline uint8_t tpm_get_irqnum(TPMIf *ti)
+{
+    if (!ti || !TPM_IF_GET_CLASS(ti)->get_irqnum) {
+        return 0;
+    }
+
+    return TPM_IF_GET_CLASS(ti)->get_irqnum(ti);
+}
+
 #endif /* QEMU_TPM_H */
-- 
2.24.1

[PATCH v2 3/5] tests: Temporarily ignore DSDT table differences

[Stefan Berger]

Ignore DSDT table differences before enabling IRQ support for TPM.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
CC: Michael S. Tsirkin <mst@redhat.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..bb4ce8967b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,2 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/q35/DSDT.tis",
-- 
2.24.1

[PATCH v2 4/5] acpi: Enable TPM IRQ

[Stefan Berger]

From: Stefan Berger <stefanb@linux.ibm.com>

Move the TPM TIS IRQ to unused IRQ 13, which is also accepted by Windows.
Query for the TPM's irq number and enable the TPM IRQ if not zero.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
CC: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/acpi-build.c  | 11 +++++------
 include/hw/acpi/tpm.h |  2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 900f786d08..747defe1ce 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2021,6 +2021,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
             build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en);
 
             if (TPM_IS_TIS_ISA(tpm)) {
+                uint8_t irq = tpm_get_irqnum(tpm);
                 if (misc->tpm_version == TPM_VERSION_2_0) {
                     dev = aml_device("TPM");
                     aml_append(dev, aml_name_decl("_HID",
@@ -2035,12 +2036,10 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
                 crs = aml_resource_template();
                 aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
                            TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
-                /*
-                    FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs,
-                    Rewrite to take IRQ from TPM device model and
-                    fix default IRQ value there to use some unused IRQ
-                 */
-                /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
+
+                if (irq) {
+                    aml_append(crs, aml_irq_no_flags(irq));
+                }
                 aml_append(dev, aml_name_decl("_CRS", crs));
 
                 tpm_build_ppi_acpi(tpm, dev);
diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
index 1a2a57a21f..063a9eb42a 100644
--- a/include/hw/acpi/tpm.h
+++ b/include/hw/acpi/tpm.h
@@ -24,7 +24,7 @@
 #define TPM_TIS_ADDR_BASE           0xFED40000
 #define TPM_TIS_ADDR_SIZE           0x5000
 
-#define TPM_TIS_IRQ                 5
+#define TPM_TIS_IRQ                 13
 
 #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
 #define TPM_TIS_LOCALITY_SHIFT      12
-- 
2.24.1

[PATCH v2 5/5] tests: Add updated DSDT

[Stefan Berger]

Add the updated DSDT following the interrupt enablement.

@@ -5,13 +5,13 @@
  *
  * Disassembling to symbolic ASL+ operators
  *
- * Disassembly of tests/data/acpi/q35/DSDT.tis, Mon Jun 15 09:57:05 2020
+ * Disassembly of /tmp/aml-Y77YL0, Mon Jun 15 09:57:05 2020
  *
  * Original Table Header:
  *     Signature        "DSDT"
- *     Length           0x000020A5 (8357)
+ *     Length           0x000020A8 (8360)
  *     Revision         0x01 **** 32-bit table (V1), no 64-bit math support
- *     Checksum         0xAD
+ *     Checksum         0x77
  *     OEM ID           "BOCHS "
  *     OEM Table ID     "BXPCDSDT"
  *     OEM Revision     0x00000001 (1)
@@ -3162,6 +3162,8 @@
                         0xFED40000,         // Address Base
                         0x00005000,         // Address Length
                         )
+                    IRQNoFlags ()
+                        {13}
                 })
                 OperationRegion (TPP2, SystemMemory, 0xFED45100, 0x5A)
                 Field (TPP2, AnyAcc, NoLock, Preserve)
**

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
CC: Michael S. Tsirkin <mst@redhat.com>
---
 tests/data/acpi/q35/DSDT.tis                | Bin 8357 -> 8360 bytes
 tests/qtest/bios-tables-test-allowed-diff.h |   1 -
 2 files changed, 1 deletion(-)

diff --git a/tests/data/acpi/q35/DSDT.tis b/tests/data/acpi/q35/DSDT.tis
index 56b6fb0c3298517d080e38fea05a748b9f1dba54..3f9db960aa05d399fa7f8449e6db688788211832 100644
GIT binary patch
delta 64
zcmZ4LxWbXkCD<iog#rTuWBEp|KeC)oS~2m#PVoX>llkS`nVeK7N60A%iEs(FaWXJ6
UFkJb^5Wv8o#GtUbT~3Y(068!Z;Q#;t

delta 61
zcmZ4CxYUu$CD<iosR9E7<Jyf}e`GoRHDls~o#F-DC-cj>Gx@7bj*wH}7v$n=<78lD
RV7T&+A%KBlbC;YP695=#58(g+

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index bb4ce8967b..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,2 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/q35/DSDT.tis",
-- 
2.24.1

Re: [PATCH v2 1/5] tpm_tis: Allow lowering of IRQ also when locality is not active

[Marc-André Lureau]

Hi

On Mon, Jun 15, 2020 at 6:23 PM Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
>
> From: Stefan Berger <stefanb@linux.ibm.com>
>
> This patch fixes a bug that occurs when using interrupts. It
> allows to lower the IRQ also when a locality is not active.
>

Can you quote the specification, or is it purely based on testing
(Windows & Linux) or checking expected behaviour from Linux code?

> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  hw/tpm/tpm_tis_common.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c
> index 1af4bce139..0f42696f1f 100644
> --- a/hw/tpm/tpm_tis_common.c
> +++ b/hw/tpm/tpm_tis_common.c
> @@ -601,10 +601,6 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
>          /* hard wired -- ignore */
>          break;
>      case TPM_TIS_REG_INT_STATUS:
> -        if (s->active_locty != locty) {
> -            break;
> -        }
> -
>          /* clearing of interrupt flags */
>          if (((val & TPM_TIS_INTERRUPTS_SUPPORTED)) &&
>              (s->loc[locty].ints & TPM_TIS_INTERRUPTS_SUPPORTED)) {
> --
> 2.24.1
>

Re: [PATCH v2 2/5] tpm: Extend TPMIfClass with get_irqnum() function

[Marc-André Lureau]

Hi

On Mon, Jun 15, 2020 at 6:23 PM Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
>
> From: Stefan Berger <stefanb@sbct-2.pok.ibm.com>
>
> Implement get_irqnum() as part of the TPMIfClass to be get the assigned

^^ to get

> IRQ number.
>

Since it is TIS ISA specific (at least for now), perhaps a dedicated
tpm_tis_get_irq_num() is more appropriate?

> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  hw/tpm/tpm_tis_isa.c    |  9 +++++++++
>  hw/tpm/tpm_tis_sysbus.c |  9 +++++++++
>  include/sysemu/tpm.h    | 10 ++++++++++
>  3 files changed, 28 insertions(+)
>
> diff --git a/hw/tpm/tpm_tis_isa.c b/hw/tpm/tpm_tis_isa.c
> index 30ba37079d..63b62f4c21 100644
> --- a/hw/tpm/tpm_tis_isa.c
> +++ b/hw/tpm/tpm_tis_isa.c
> @@ -80,6 +80,14 @@ static enum TPMVersion tpm_tis_isa_get_tpm_version(TPMIf *ti)
>      return tpm_tis_get_tpm_version(s);
>  }
>
> +static uint8_t tpm_tis_isa_get_irqnum(TPMIf *ti)
> +{
> +    TPMStateISA *isadev = TPM_TIS_ISA(ti);
> +    TPMState *s = &isadev->state;
> +
> +    return s->irq_num;
> +}
> +
>  static void tpm_tis_isa_reset(DeviceState *dev)
>  {
>      TPMStateISA *isadev = TPM_TIS_ISA(dev);
> @@ -148,6 +156,7 @@ static void tpm_tis_isa_class_init(ObjectClass *klass, void *data)
>      dc->reset = tpm_tis_isa_reset;
>      tc->request_completed = tpm_tis_isa_request_completed;
>      tc->get_version = tpm_tis_isa_get_tpm_version;
> +    tc->get_irqnum = tpm_tis_isa_get_irqnum;
>  }
>
>  static const TypeInfo tpm_tis_isa_info = {
> diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
> index eced1fc843..6cf45e5057 100644
> --- a/hw/tpm/tpm_tis_sysbus.c
> +++ b/hw/tpm/tpm_tis_sysbus.c
> @@ -80,6 +80,14 @@ static enum TPMVersion tpm_tis_sysbus_get_tpm_version(TPMIf *ti)
>      return tpm_tis_get_tpm_version(s);
>  }
>
> +static uint8_t tpm_tis_sysbus_get_irqnum(TPMIf *ti)
> +{
> +    TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(ti);
> +    TPMState *s = &sbdev->state;
> +
> +    return s->irq_num;
> +}
> +
>  static void tpm_tis_sysbus_reset(DeviceState *dev)
>  {
>      TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(dev);
> @@ -137,6 +145,7 @@ static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data)
>      dc->reset = tpm_tis_sysbus_reset;
>      tc->request_completed = tpm_tis_sysbus_request_completed;
>      tc->get_version = tpm_tis_sysbus_get_tpm_version;
> +    tc->get_irqnum = tpm_tis_sysbus_get_irqnum;
>  }
>
>  static const TypeInfo tpm_tis_sysbus_info = {
> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> index 03fb25941c..b94a8a2b16 100644
> --- a/include/sysemu/tpm.h
> +++ b/include/sysemu/tpm.h
> @@ -41,6 +41,7 @@ typedef struct TPMIfClass {
>      enum TpmModel model;
>      void (*request_completed)(TPMIf *obj, int ret);
>      enum TPMVersion (*get_version)(TPMIf *obj);
> +    uint8_t (*get_irqnum)(TPMIf *obj);
>  } TPMIfClass;
>
>  #define TYPE_TPM_TIS_ISA            "tpm-tis"
> @@ -74,4 +75,13 @@ static inline TPMVersion tpm_get_version(TPMIf *ti)
>      return TPM_IF_GET_CLASS(ti)->get_version(ti);
>  }
>
> +static inline uint8_t tpm_get_irqnum(TPMIf *ti)
> +{
> +    if (!ti || !TPM_IF_GET_CLASS(ti)->get_irqnum) {
> +        return 0;
> +    }
> +
> +    return TPM_IF_GET_CLASS(ti)->get_irqnum(ti);
> +}
> +
>  #endif /* QEMU_TPM_H */
> --
> 2.24.1
>

Re: [PATCH v2 3/5] tests: Temporarily ignore DSDT table differences

[Marc-André Lureau]

On Mon, Jun 15, 2020 at 6:27 PM Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
>
> Ignore DSDT table differences before enabling IRQ support for TPM.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> CC: Michael S. Tsirkin <mst@redhat.com>

make sense, to split implementation & test commits:

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  tests/qtest/bios-tables-test-allowed-diff.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index dfb8523c8b..bb4ce8967b 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1 +1,2 @@
>  /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/q35/DSDT.tis",
> --
> 2.24.1
>

Re: [PATCH v2 4/5] acpi: Enable TPM IRQ

[Marc-André Lureau]

On Mon, Jun 15, 2020 at 6:24 PM Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
>
> From: Stefan Berger <stefanb@linux.ibm.com>
>
> Move the TPM TIS IRQ to unused IRQ 13, which is also accepted by Windows.
> Query for the TPM's irq number and enable the TPM IRQ if not zero.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> CC: Michael S. Tsirkin <mst@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  hw/i386/acpi-build.c  | 11 +++++------
>  include/hw/acpi/tpm.h |  2 +-
>  2 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 900f786d08..747defe1ce 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2021,6 +2021,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>              build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en);
>
>              if (TPM_IS_TIS_ISA(tpm)) {
> +                uint8_t irq = tpm_get_irqnum(tpm);
>                  if (misc->tpm_version == TPM_VERSION_2_0) {
>                      dev = aml_device("TPM");
>                      aml_append(dev, aml_name_decl("_HID",
> @@ -2035,12 +2036,10 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>                  crs = aml_resource_template();
>                  aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
>                             TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
> -                /*
> -                    FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs,
> -                    Rewrite to take IRQ from TPM device model and
> -                    fix default IRQ value there to use some unused IRQ
> -                 */
> -                /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
> +
> +                if (irq) {
> +                    aml_append(crs, aml_irq_no_flags(irq));
> +                }
>                  aml_append(dev, aml_name_decl("_CRS", crs));
>
>                  tpm_build_ppi_acpi(tpm, dev);
> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> index 1a2a57a21f..063a9eb42a 100644
> --- a/include/hw/acpi/tpm.h
> +++ b/include/hw/acpi/tpm.h
> @@ -24,7 +24,7 @@
>  #define TPM_TIS_ADDR_BASE           0xFED40000
>  #define TPM_TIS_ADDR_SIZE           0x5000
>
> -#define TPM_TIS_IRQ                 5
> +#define TPM_TIS_IRQ                 13
>
>  #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
>  #define TPM_TIS_LOCALITY_SHIFT      12
> --
> 2.24.1
>

Re: [PATCH v2 1/5] tpm_tis: Allow lowering of IRQ also when locality is not active

[Stefan Berger]

On 6/15/20 11:06 AM, Marc-André Lureau wrote:
> Hi
>
> On Mon, Jun 15, 2020 at 6:23 PM Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> This patch fixes a bug that occurs when using interrupts. It
>> allows to lower the IRQ also when a locality is not active.
>>
> Can you quote the specification, or is it purely based on testing
> (Windows & Linux) or checking expected behaviour from Linux code?

There's no spec for this detail. It's purely based on testing.

also: Linux 5.x had interrupt support for a short while but this was 
removed due to some laptops receiving an interrupt storm following 
enablement: https://www.spinics.net/lists/linux-integrity/msg11870.html  
[ reason seems to be misconfiguration of some GPIO pin]

I tested it with this patch: https://lkml.org/lkml/2019/8/20/421

5.4 kernel: 
https://elixir.bootlin.com/linux/v5.4/source/drivers/char/tpm/tpm_tis_core.c#L983


    Stefan




>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   hw/tpm/tpm_tis_common.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c
>> index 1af4bce139..0f42696f1f 100644
>> --- a/hw/tpm/tpm_tis_common.c
>> +++ b/hw/tpm/tpm_tis_common.c
>> @@ -601,10 +601,6 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
>>           /* hard wired -- ignore */
>>           break;
>>       case TPM_TIS_REG_INT_STATUS:
>> -        if (s->active_locty != locty) {
>> -            break;
>> -        }
>> -
>>           /* clearing of interrupt flags */
>>           if (((val & TPM_TIS_INTERRUPTS_SUPPORTED)) &&
>>               (s->loc[locty].ints & TPM_TIS_INTERRUPTS_SUPPORTED)) {
>> --
>> 2.24.1
>>

Re: [PATCH v2 2/5] tpm: Extend TPMIfClass with get_irqnum() function

[Stefan Berger]

On 6/15/20 11:11 AM, Marc-André Lureau wrote:
> Hi
>
> On Mon, Jun 15, 2020 at 6:23 PM Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> From: Stefan Berger <stefanb@sbct-2.pok.ibm.com>
>>
>> Implement get_irqnum() as part of the TPMIfClass to be get the assigned
> ^^ to get
>
>> IRQ number.
>>
> Since it is TIS ISA specific (at least for now), perhaps a dedicated
> tpm_tis_get_irq_num() is more appropriate?


There's no caller from ARM at the moment but at least it is preparing it 
to support getting the IRQ number since it also allows passing it in via 
command lie. So it wouldn't have any side-effects on ARM for as long as 
no extra code was added there. And once sysbus was to use it, it would 
funnel through the same tpm_get_irqnum code. Other option is to drop the 
parts for sysbus entirely.


>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   hw/tpm/tpm_tis_isa.c    |  9 +++++++++
>>   hw/tpm/tpm_tis_sysbus.c |  9 +++++++++
>>   include/sysemu/tpm.h    | 10 ++++++++++
>>   3 files changed, 28 insertions(+)
>>
>> diff --git a/hw/tpm/tpm_tis_isa.c b/hw/tpm/tpm_tis_isa.c
>> index 30ba37079d..63b62f4c21 100644
>> --- a/hw/tpm/tpm_tis_isa.c
>> +++ b/hw/tpm/tpm_tis_isa.c
>> @@ -80,6 +80,14 @@ static enum TPMVersion tpm_tis_isa_get_tpm_version(TPMIf *ti)
>>       return tpm_tis_get_tpm_version(s);
>>   }
>>
>> +static uint8_t tpm_tis_isa_get_irqnum(TPMIf *ti)
>> +{
>> +    TPMStateISA *isadev = TPM_TIS_ISA(ti);
>> +    TPMState *s = &isadev->state;
>> +
>> +    return s->irq_num;
>> +}
>> +
>>   static void tpm_tis_isa_reset(DeviceState *dev)
>>   {
>>       TPMStateISA *isadev = TPM_TIS_ISA(dev);
>> @@ -148,6 +156,7 @@ static void tpm_tis_isa_class_init(ObjectClass *klass, void *data)
>>       dc->reset = tpm_tis_isa_reset;
>>       tc->request_completed = tpm_tis_isa_request_completed;
>>       tc->get_version = tpm_tis_isa_get_tpm_version;
>> +    tc->get_irqnum = tpm_tis_isa_get_irqnum;
>>   }
>>
>>   static const TypeInfo tpm_tis_isa_info = {
>> diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
>> index eced1fc843..6cf45e5057 100644
>> --- a/hw/tpm/tpm_tis_sysbus.c
>> +++ b/hw/tpm/tpm_tis_sysbus.c
>> @@ -80,6 +80,14 @@ static enum TPMVersion tpm_tis_sysbus_get_tpm_version(TPMIf *ti)
>>       return tpm_tis_get_tpm_version(s);
>>   }
>>
>> +static uint8_t tpm_tis_sysbus_get_irqnum(TPMIf *ti)
>> +{
>> +    TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(ti);
>> +    TPMState *s = &sbdev->state;
>> +
>> +    return s->irq_num;
>> +}
>> +
>>   static void tpm_tis_sysbus_reset(DeviceState *dev)
>>   {
>>       TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(dev);
>> @@ -137,6 +145,7 @@ static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data)
>>       dc->reset = tpm_tis_sysbus_reset;
>>       tc->request_completed = tpm_tis_sysbus_request_completed;
>>       tc->get_version = tpm_tis_sysbus_get_tpm_version;
>> +    tc->get_irqnum = tpm_tis_sysbus_get_irqnum;
>>   }
>>
>>   static const TypeInfo tpm_tis_sysbus_info = {
>> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
>> index 03fb25941c..b94a8a2b16 100644
>> --- a/include/sysemu/tpm.h
>> +++ b/include/sysemu/tpm.h
>> @@ -41,6 +41,7 @@ typedef struct TPMIfClass {
>>       enum TpmModel model;
>>       void (*request_completed)(TPMIf *obj, int ret);
>>       enum TPMVersion (*get_version)(TPMIf *obj);
>> +    uint8_t (*get_irqnum)(TPMIf *obj);
>>   } TPMIfClass;
>>
>>   #define TYPE_TPM_TIS_ISA            "tpm-tis"
>> @@ -74,4 +75,13 @@ static inline TPMVersion tpm_get_version(TPMIf *ti)
>>       return TPM_IF_GET_CLASS(ti)->get_version(ti);
>>   }
>>
>> +static inline uint8_t tpm_get_irqnum(TPMIf *ti)
>> +{
>> +    if (!ti || !TPM_IF_GET_CLASS(ti)->get_irqnum) {
>> +        return 0;
>> +    }
>> +
>> +    return TPM_IF_GET_CLASS(ti)->get_irqnum(ti);
>> +}
>> +
>>   #endif /* QEMU_TPM_H */
>> --
>> 2.24.1
>>

Re: [PATCH v2 2/5] tpm: Extend TPMIfClass with get_irqnum() function

[Philippe Mathieu-Daudé]

On 6/15/20 4:23 PM, Stefan Berger wrote:
> From: Stefan Berger <stefanb@sbct-2.pok.ibm.com>
> 
> Implement get_irqnum() as part of the TPMIfClass to be get the assigned
> IRQ number.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  hw/tpm/tpm_tis_isa.c    |  9 +++++++++
>  hw/tpm/tpm_tis_sysbus.c |  9 +++++++++
>  include/sysemu/tpm.h    | 10 ++++++++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/hw/tpm/tpm_tis_isa.c b/hw/tpm/tpm_tis_isa.c
> index 30ba37079d..63b62f4c21 100644
> --- a/hw/tpm/tpm_tis_isa.c
> +++ b/hw/tpm/tpm_tis_isa.c
> @@ -80,6 +80,14 @@ static enum TPMVersion tpm_tis_isa_get_tpm_version(TPMIf *ti)
>      return tpm_tis_get_tpm_version(s);
>  }
>  
> +static uint8_t tpm_tis_isa_get_irqnum(TPMIf *ti)
> +{
> +    TPMStateISA *isadev = TPM_TIS_ISA(ti);
> +    TPMState *s = &isadev->state;
> +
> +    return s->irq_num;
> +}
> +
>  static void tpm_tis_isa_reset(DeviceState *dev)
>  {
>      TPMStateISA *isadev = TPM_TIS_ISA(dev);
> @@ -148,6 +156,7 @@ static void tpm_tis_isa_class_init(ObjectClass *klass, void *data)
>      dc->reset = tpm_tis_isa_reset;
>      tc->request_completed = tpm_tis_isa_request_completed;
>      tc->get_version = tpm_tis_isa_get_tpm_version;
> +    tc->get_irqnum = tpm_tis_isa_get_irqnum;
>  }
>  
>  static const TypeInfo tpm_tis_isa_info = {
> diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
> index eced1fc843..6cf45e5057 100644
> --- a/hw/tpm/tpm_tis_sysbus.c
> +++ b/hw/tpm/tpm_tis_sysbus.c
> @@ -80,6 +80,14 @@ static enum TPMVersion tpm_tis_sysbus_get_tpm_version(TPMIf *ti)
>      return tpm_tis_get_tpm_version(s);
>  }
>  
> +static uint8_t tpm_tis_sysbus_get_irqnum(TPMIf *ti)
> +{
> +    TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(ti);
> +    TPMState *s = &sbdev->state;
> +
> +    return s->irq_num;
> +}
> +
>  static void tpm_tis_sysbus_reset(DeviceState *dev)
>  {
>      TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(dev);
> @@ -137,6 +145,7 @@ static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data)
>      dc->reset = tpm_tis_sysbus_reset;
>      tc->request_completed = tpm_tis_sysbus_request_completed;
>      tc->get_version = tpm_tis_sysbus_get_tpm_version;
> +    tc->get_irqnum = tpm_tis_sysbus_get_irqnum;
>  }
>  
>  static const TypeInfo tpm_tis_sysbus_info = {
> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> index 03fb25941c..b94a8a2b16 100644
> --- a/include/sysemu/tpm.h
> +++ b/include/sysemu/tpm.h
> @@ -41,6 +41,7 @@ typedef struct TPMIfClass {
>      enum TpmModel model;
>      void (*request_completed)(TPMIf *obj, int ret);
>      enum TPMVersion (*get_version)(TPMIf *obj);
> +    uint8_t (*get_irqnum)(TPMIf *obj);
>  } TPMIfClass;
>  
>  #define TYPE_TPM_TIS_ISA            "tpm-tis"
> @@ -74,4 +75,13 @@ static inline TPMVersion tpm_get_version(TPMIf *ti)
>      return TPM_IF_GET_CLASS(ti)->get_version(ti);
>  }
>  
> +static inline uint8_t tpm_get_irqnum(TPMIf *ti)
> +{
> +    if (!ti || !TPM_IF_GET_CLASS(ti)->get_irqnum) {
> +        return 0;

0 is valid...

Shouldn't we use an Error* argument?

Else return -1 when invalid?

> +    }
> +
> +    return TPM_IF_GET_CLASS(ti)->get_irqnum(ti);
> +}
> +
>  #endif /* QEMU_TPM_H */
> 

Re: [PATCH v2 2/5] tpm: Extend TPMIfClass with get_irqnum() function

[Stefan Berger]

On 6/15/20 11:54 AM, Philippe Mathieu-Daudé wrote:
> On 6/15/20 4:23 PM, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@sbct-2.pok.ibm.com>
>>
>> Implement get_irqnum() as part of the TPMIfClass to be get the assigned
>> IRQ number.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   hw/tpm/tpm_tis_isa.c    |  9 +++++++++
>>   hw/tpm/tpm_tis_sysbus.c |  9 +++++++++
>>   include/sysemu/tpm.h    | 10 ++++++++++
>>   3 files changed, 28 insertions(+)
>>
>> diff --git a/hw/tpm/tpm_tis_isa.c b/hw/tpm/tpm_tis_isa.c
>> index 30ba37079d..63b62f4c21 100644
>> --- a/hw/tpm/tpm_tis_isa.c
>> +++ b/hw/tpm/tpm_tis_isa.c
>> @@ -80,6 +80,14 @@ static enum TPMVersion tpm_tis_isa_get_tpm_version(TPMIf *ti)
>>       return tpm_tis_get_tpm_version(s);
>>   }
>>   
>> +static uint8_t tpm_tis_isa_get_irqnum(TPMIf *ti)
>> +{
>> +    TPMStateISA *isadev = TPM_TIS_ISA(ti);
>> +    TPMState *s = &isadev->state;
>> +
>> +    return s->irq_num;
>> +}
>> +
>>   static void tpm_tis_isa_reset(DeviceState *dev)
>>   {
>>       TPMStateISA *isadev = TPM_TIS_ISA(dev);
>> @@ -148,6 +156,7 @@ static void tpm_tis_isa_class_init(ObjectClass *klass, void *data)
>>       dc->reset = tpm_tis_isa_reset;
>>       tc->request_completed = tpm_tis_isa_request_completed;
>>       tc->get_version = tpm_tis_isa_get_tpm_version;
>> +    tc->get_irqnum = tpm_tis_isa_get_irqnum;
>>   }
>>   
>>   static const TypeInfo tpm_tis_isa_info = {
>> diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
>> index eced1fc843..6cf45e5057 100644
>> --- a/hw/tpm/tpm_tis_sysbus.c
>> +++ b/hw/tpm/tpm_tis_sysbus.c
>> @@ -80,6 +80,14 @@ static enum TPMVersion tpm_tis_sysbus_get_tpm_version(TPMIf *ti)
>>       return tpm_tis_get_tpm_version(s);
>>   }
>>   
>> +static uint8_t tpm_tis_sysbus_get_irqnum(TPMIf *ti)
>> +{
>> +    TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(ti);
>> +    TPMState *s = &sbdev->state;
>> +
>> +    return s->irq_num;
>> +}
>> +
>>   static void tpm_tis_sysbus_reset(DeviceState *dev)
>>   {
>>       TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(dev);
>> @@ -137,6 +145,7 @@ static void tpm_tis_sysbus_class_init(ObjectClass *klass, void *data)
>>       dc->reset = tpm_tis_sysbus_reset;
>>       tc->request_completed = tpm_tis_sysbus_request_completed;
>>       tc->get_version = tpm_tis_sysbus_get_tpm_version;
>> +    tc->get_irqnum = tpm_tis_sysbus_get_irqnum;
>>   }
>>   
>>   static const TypeInfo tpm_tis_sysbus_info = {
>> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
>> index 03fb25941c..b94a8a2b16 100644
>> --- a/include/sysemu/tpm.h
>> +++ b/include/sysemu/tpm.h
>> @@ -41,6 +41,7 @@ typedef struct TPMIfClass {
>>       enum TpmModel model;
>>       void (*request_completed)(TPMIf *obj, int ret);
>>       enum TPMVersion (*get_version)(TPMIf *obj);
>> +    uint8_t (*get_irqnum)(TPMIf *obj);
>>   } TPMIfClass;
>>   
>>   #define TYPE_TPM_TIS_ISA            "tpm-tis"
>> @@ -74,4 +75,13 @@ static inline TPMVersion tpm_get_version(TPMIf *ti)
>>       return TPM_IF_GET_CLASS(ti)->get_version(ti);
>>   }
>>   
>> +static inline uint8_t tpm_get_irqnum(TPMIf *ti)
>> +{
>> +    if (!ti || !TPM_IF_GET_CLASS(ti)->get_irqnum) {
>> +        return 0;
> 0 is valid...
>
> Shouldn't we use an Error* argument?
>
> Else return -1 when invalid?

Indeed. Let me fix this. Thanks.


>
>> +    }
>> +
>> +    return TPM_IF_GET_CLASS(ti)->get_irqnum(ti);
>> +}
>> +
>>   #endif /* QEMU_TPM_H */
>>

Re: [PATCH v2 4/5] acpi: Enable TPM IRQ

[Stefan Berger]

On 6/15/20 11:13 AM, Marc-André Lureau wrote:
>
>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>> index 1a2a57a21f..063a9eb42a 100644
>> --- a/include/hw/acpi/tpm.h
>> +++ b/include/hw/acpi/tpm.h
>> @@ -24,7 +24,7 @@
>>   #define TPM_TIS_ADDR_BASE           0xFED40000
>>   #define TPM_TIS_ADDR_SIZE           0x5000
>>
>> -#define TPM_TIS_IRQ                 5
>> +#define TPM_TIS_IRQ                 13


Eric,

  does this change have any negative side effects on ARM? If you prefer, 
we can split this part here up into TPM_TIS_ISA_IRQ and TPM_TIS_SYSBUS 
IRQ and leave the latter at '5' because we know that this is working.

    Stefan


>>
>>   #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
>>   #define TPM_TIS_LOCALITY_SHIFT      12
>> --
>> 2.24.1
>>

Re: [PATCH v2 4/5] acpi: Enable TPM IRQ

[Auger Eric]

Hi Stefan,

On 6/15/20 7:11 PM, Stefan Berger wrote:
> On 6/15/20 11:13 AM, Marc-André Lureau wrote:
>>
>>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>>> index 1a2a57a21f..063a9eb42a 100644
>>> --- a/include/hw/acpi/tpm.h
>>> +++ b/include/hw/acpi/tpm.h
>>> @@ -24,7 +24,7 @@
>>>   #define TPM_TIS_ADDR_BASE           0xFED40000
>>>   #define TPM_TIS_ADDR_SIZE           0x5000
>>>
>>> -#define TPM_TIS_IRQ                 5
>>> +#define TPM_TIS_IRQ                 13
> 
> 
> Eric,
> 
>  does this change have any negative side effects on ARM? If you prefer,
> we can split this part here up into TPM_TIS_ISA_IRQ and TPM_TIS_SYSBUS
> IRQ and leave the latter at '5' because we know that this is working.

I just gave it a try and it does not seem to introduce any regression
with automatic LUKS decryption. I will take more time to review the code
though.

Thanks

Eric

> 
>    Stefan
> 
> 
>>>
>>>   #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
>>>   #define TPM_TIS_LOCALITY_SHIFT      12
>>> -- 
>>> 2.24.1
>>>
> 
> 

Re: [PATCH v2 1/5] tpm_tis: Allow lowering of IRQ also when locality is not active

[Auger Eric]

Hi Stefan,

On 6/15/20 4:23 PM, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
> 
> This patch fixes a bug that occurs when using interrupts. It
> allows to lower the IRQ also when a locality is not active.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---

>  hw/tpm/tpm_tis_common.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c
> index 1af4bce139..0f42696f1f 100644
> --- a/hw/tpm/tpm_tis_common.c
> +++ b/hw/tpm/tpm_tis_common.c
> @@ -601,10 +601,6 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
>          /* hard wired -- ignore */
>          break;
>      case TPM_TIS_REG_INT_STATUS:
> -        if (s->active_locty != locty) {
> -            break;
> -        }
> -
>          /* clearing of interrupt flags */
>          if (((val & TPM_TIS_INTERRUPTS_SUPPORTED)) &&
>              (s->loc[locty].ints & TPM_TIS_INTERRUPTS_SUPPORTED)) {
> 

Re: [PATCH v2 2/5] tpm: Extend TPMIfClass with get_irqnum() function

[Auger Eric]

Hi Stefan,

On 6/15/20 5:44 PM, Stefan Berger wrote:
> On 6/15/20 11:11 AM, Marc-André Lureau wrote:
>> Hi
>>
>> On Mon, Jun 15, 2020 at 6:23 PM Stefan Berger
>> <stefanb@linux.vnet.ibm.com> wrote:
>>> From: Stefan Berger <stefanb@sbct-2.pok.ibm.com>
>>>
>>> Implement get_irqnum() as part of the TPMIfClass to be get the assigned
>> ^^ to get
>>
>>> IRQ number.
>>>
>> Since it is TIS ISA specific (at least for now), perhaps a dedicated
>> tpm_tis_get_irq_num() is more appropriate?
> 
> 
> There's no caller from ARM at the moment but at least it is preparing it
> to support getting the IRQ number since it also allows passing it in via
> command lie. So it wouldn't have any side-effects on ARM for as long as
> no extra code was added there. And once sysbus was to use it, it would
> funnel through the same tpm_get_irqnum code. Other option is to drop the
> parts for sysbus entirely.

As there is no caller on ARM side, I think I would leave the ops
undefined for the sysbus device. But as already reported it does not
break anything on ARM side.

Thanks

Eric
> 
> 
>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>> ---
>>>   hw/tpm/tpm_tis_isa.c    |  9 +++++++++
>>>   hw/tpm/tpm_tis_sysbus.c |  9 +++++++++
>>>   include/sysemu/tpm.h    | 10 ++++++++++
>>>   3 files changed, 28 insertions(+)
>>>
>>> diff --git a/hw/tpm/tpm_tis_isa.c b/hw/tpm/tpm_tis_isa.c
>>> index 30ba37079d..63b62f4c21 100644
>>> --- a/hw/tpm/tpm_tis_isa.c
>>> +++ b/hw/tpm/tpm_tis_isa.c
>>> @@ -80,6 +80,14 @@ static enum TPMVersion
>>> tpm_tis_isa_get_tpm_version(TPMIf *ti)
>>>       return tpm_tis_get_tpm_version(s);
>>>   }
>>>
>>> +static uint8_t tpm_tis_isa_get_irqnum(TPMIf *ti)
>>> +{
>>> +    TPMStateISA *isadev = TPM_TIS_ISA(ti);
>>> +    TPMState *s = &isadev->state;
>>> +
>>> +    return s->irq_num;
>>> +}
>>> +
>>>   static void tpm_tis_isa_reset(DeviceState *dev)
>>>   {
>>>       TPMStateISA *isadev = TPM_TIS_ISA(dev);
>>> @@ -148,6 +156,7 @@ static void tpm_tis_isa_class_init(ObjectClass
>>> *klass, void *data)
>>>       dc->reset = tpm_tis_isa_reset;
>>>       tc->request_completed = tpm_tis_isa_request_completed;
>>>       tc->get_version = tpm_tis_isa_get_tpm_version;
>>> +    tc->get_irqnum = tpm_tis_isa_get_irqnum;
>>>   }
>>>
>>>   static const TypeInfo tpm_tis_isa_info = {
>>> diff --git a/hw/tpm/tpm_tis_sysbus.c b/hw/tpm/tpm_tis_sysbus.c
>>> index eced1fc843..6cf45e5057 100644
>>> --- a/hw/tpm/tpm_tis_sysbus.c
>>> +++ b/hw/tpm/tpm_tis_sysbus.c
>>> @@ -80,6 +80,14 @@ static enum TPMVersion
>>> tpm_tis_sysbus_get_tpm_version(TPMIf *ti)
>>>       return tpm_tis_get_tpm_version(s);
>>>   }
>>>
>>> +static uint8_t tpm_tis_sysbus_get_irqnum(TPMIf *ti)
>>> +{
>>> +    TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(ti);
>>> +    TPMState *s = &sbdev->state;
>>> +
>>> +    return s->irq_num;
>>> +}
>>> +
>>>   static void tpm_tis_sysbus_reset(DeviceState *dev)
>>>   {
>>>       TPMStateSysBus *sbdev = TPM_TIS_SYSBUS(dev);
>>> @@ -137,6 +145,7 @@ static void tpm_tis_sysbus_class_init(ObjectClass
>>> *klass, void *data)
>>>       dc->reset = tpm_tis_sysbus_reset;
>>>       tc->request_completed = tpm_tis_sysbus_request_completed;
>>>       tc->get_version = tpm_tis_sysbus_get_tpm_version;
>>> +    tc->get_irqnum = tpm_tis_sysbus_get_irqnum;
>>>   }
>>>
>>>   static const TypeInfo tpm_tis_sysbus_info = {
>>> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
>>> index 03fb25941c..b94a8a2b16 100644
>>> --- a/include/sysemu/tpm.h
>>> +++ b/include/sysemu/tpm.h
>>> @@ -41,6 +41,7 @@ typedef struct TPMIfClass {
>>>       enum TpmModel model;
>>>       void (*request_completed)(TPMIf *obj, int ret);
>>>       enum TPMVersion (*get_version)(TPMIf *obj);
>>> +    uint8_t (*get_irqnum)(TPMIf *obj);
>>>   } TPMIfClass;
>>>
>>>   #define TYPE_TPM_TIS_ISA            "tpm-tis"
>>> @@ -74,4 +75,13 @@ static inline TPMVersion tpm_get_version(TPMIf *ti)
>>>       return TPM_IF_GET_CLASS(ti)->get_version(ti);
>>>   }
>>>
>>> +static inline uint8_t tpm_get_irqnum(TPMIf *ti)
>>> +{
>>> +    if (!ti || !TPM_IF_GET_CLASS(ti)->get_irqnum) {
>>> +        return 0;
>>> +    }
>>> +
>>> +    return TPM_IF_GET_CLASS(ti)->get_irqnum(ti);
>>> +}
>>> +
>>>   #endif /* QEMU_TPM_H */
>>> -- 
>>> 2.24.1
>>>
> 

Re: [PATCH v2 3/5] tests: Temporarily ignore DSDT table differences

[Auger Eric]

On 6/15/20 4:23 PM, Stefan Berger wrote:
> Ignore DSDT table differences before enabling IRQ support for TPM.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---

>  tests/qtest/bios-tables-test-allowed-diff.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index dfb8523c8b..bb4ce8967b 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1 +1,2 @@
>  /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/q35/DSDT.tis",
> 

Re: [PATCH v2 4/5] acpi: Enable TPM IRQ

[Auger Eric]

Hi Stefan,

On 6/15/20 7:11 PM, Stefan Berger wrote:
> On 6/15/20 11:13 AM, Marc-André Lureau wrote:
>>
>>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>>> index 1a2a57a21f..063a9eb42a 100644
>>> --- a/include/hw/acpi/tpm.h
>>> +++ b/include/hw/acpi/tpm.h
>>> @@ -24,7 +24,7 @@
>>>   #define TPM_TIS_ADDR_BASE           0xFED40000
>>>   #define TPM_TIS_ADDR_SIZE           0x5000
>>>
>>> -#define TPM_TIS_IRQ                 5
>>> +#define TPM_TIS_IRQ                 13
> 
> 
> Eric,
> 
>  does this change have any negative side effects on ARM? If you prefer,
> we can split this part here up into TPM_TIS_ISA_IRQ and TPM_TIS_SYSBUS
> IRQ and leave the latter at '5' because we know that this is working.
The IRQ is not advertised in dt nor ACPI on ARM. However it is
advertised in the capability reg and in the vector. reg So I think this
should be fixed? I guess on ARM we will pick up a completely different
IRQ num, allocated from the platform bus slot.

Thanks

Eric
> 
>    Stefan
> 
> 
>>>
>>>   #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
>>>   #define TPM_TIS_LOCALITY_SHIFT      12
>>> -- 
>>> 2.24.1
>>>
> 

Re: [PATCH v2 4/5] acpi: Enable TPM IRQ

[Stefan Berger]

On 6/16/20 9:01 AM, Auger Eric wrote:
> Hi Stefan,
>
> On 6/15/20 7:11 PM, Stefan Berger wrote:
>> On 6/15/20 11:13 AM, Marc-André Lureau wrote:
>>>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>>>> index 1a2a57a21f..063a9eb42a 100644
>>>> --- a/include/hw/acpi/tpm.h
>>>> +++ b/include/hw/acpi/tpm.h
>>>> @@ -24,7 +24,7 @@
>>>>    #define TPM_TIS_ADDR_BASE           0xFED40000
>>>>    #define TPM_TIS_ADDR_SIZE           0x5000
>>>>
>>>> -#define TPM_TIS_IRQ                 5
>>>> +#define TPM_TIS_IRQ                 13
>>
>> Eric,
>>
>>   does this change have any negative side effects on ARM? If you prefer,
>> we can split this part here up into TPM_TIS_ISA_IRQ and TPM_TIS_SYSBUS
>> IRQ and leave the latter at '5' because we know that this is working.
> The IRQ is not advertised in dt nor ACPI on ARM. However it is
> advertised in the capability reg and in the vector. reg So I think this
> should be fixed? I guess on ARM we will pick up a completely different
> IRQ num, allocated from the platform bus slot.


The specification

https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p04_r0p37_pub-1.pdf

declares several fields in the Interface Capability Register (table 23, 
pdf page 89) to be mandatory and they must be set to '1'. So I would not 
want to touch those. We can set the interrupt vector register to '0' in 
case interrupts are not supported. Following the spec 0 means that no 
interrupts are supported. I will now split TPM_TIS_IRQ into 
TPM_TIS_ISA_IRQ and TPM_TIS_SYSBUS_IRQ and will in the end set 
TPM_TIS_SYSBUS_IRQ to 'disabled', indicating that IRQs are not 
supported, though they should work even though on ARM there may not be a 
driver to test this with. Does this sound ok?


    Stefan


>
> Thanks
>
> Eric
>>     Stefan
>>
>>
>>>>    #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
>>>>    #define TPM_TIS_LOCALITY_SHIFT      12
>>>> -- 
>>>> 2.24.1
>>>>

Re: [PATCH v2 4/5] acpi: Enable TPM IRQ

[Auger Eric]

Hi Stefan,

On 6/16/20 4:05 PM, Stefan Berger wrote:
> On 6/16/20 9:01 AM, Auger Eric wrote:
>> Hi Stefan,
>>
>> On 6/15/20 7:11 PM, Stefan Berger wrote:
>>> On 6/15/20 11:13 AM, Marc-André Lureau wrote:
>>>>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>>>>> index 1a2a57a21f..063a9eb42a 100644
>>>>> --- a/include/hw/acpi/tpm.h
>>>>> +++ b/include/hw/acpi/tpm.h
>>>>> @@ -24,7 +24,7 @@
>>>>>    #define TPM_TIS_ADDR_BASE           0xFED40000
>>>>>    #define TPM_TIS_ADDR_SIZE           0x5000
>>>>>
>>>>> -#define TPM_TIS_IRQ                 5
>>>>> +#define TPM_TIS_IRQ                 13
>>>
>>> Eric,
>>>
>>>   does this change have any negative side effects on ARM? If you prefer,
>>> we can split this part here up into TPM_TIS_ISA_IRQ and TPM_TIS_SYSBUS
>>> IRQ and leave the latter at '5' because we know that this is working.
>> The IRQ is not advertised in dt nor ACPI on ARM. However it is
>> advertised in the capability reg and in the vector. reg So I think this
>> should be fixed? I guess on ARM we will pick up a completely different
>> IRQ num, allocated from the platform bus slot.
> 
> 
> The specification
> 
> https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p04_r0p37_pub-1.pdf
> 
> 
> declares several fields in the Interface Capability Register (table 23,
> pdf page 89) to be mandatory and they must be set to '1'. So I would not
> want to touch those. We can set the interrupt vector register to '0' in
> case interrupts are not supported. Following the spec 0 means that no
> interrupts are supported. I will now split TPM_TIS_IRQ into
> TPM_TIS_ISA_IRQ and TPM_TIS_SYSBUS_IRQ and will in the end set
> TPM_TIS_SYSBUS_IRQ to 'disabled', indicating that IRQs are not
> supported, though they should work even though on ARM there may not be a
> driver to test this with. Does this sound ok?

Yes it does.

Thanks

Eric
> 
> 
>    Stefan
> 
> 
>>
>> Thanks
>>
>> Eric
>>>     Stefan
>>>
>>>
>>>>>    #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
>>>>>    #define TPM_TIS_LOCALITY_SHIFT      12
>>>>> -- 
>>>>> 2.24.1
>>>>>
> 
>