diff mbox series

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

Message ID 20200615142327.671546-5-stefanb@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show
Series tpm: Enable usage of TPM TIS with interrupts | expand

Commit Message

Stefan Berger June 15, 2020, 2:23 p.m. UTC 
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(-)

Comments

Marc-André Lureau June 15, 2020, 3:13 p.m. UTC | #1 
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
>
Stefan Berger June 15, 2020, 5:11 p.m. UTC | #2 
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
>>
Eric Auger June 16, 2020, 7:44 a.m. UTC | #3 
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
>>>
> 
>
Eric Auger June 16, 2020, 1:01 p.m. UTC | #4 
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
>>>
>
Stefan Berger June 16, 2020, 2:05 p.m. UTC | #5 
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
>>>>
Eric Auger June 16, 2020, 2:36 p.m. UTC | #6 
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
>>>>>
> 
>
diff mbox series

Patch

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