diff mbox series

[v3,5/8] acpi: Enable TPM IRQ

Message ID 20200616205721.1191408-6-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 16, 2020, 8:57 p.m. UTC 
From: Stefan Berger <stefanb@linux.ibm.com>

Move the TPM TIS IRQ to unused IRQ 13, which is the only one accepted by
Windows. Query for the TPM's irq number and enable the TPM IRQ unless
TPM_IRQ_DISABLED is returned.

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 17, 2020, 7:39 a.m. UTC | #1 
On Wed, Jun 17, 2020 at 12:57 AM 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 the only one accepted by
> Windows. Query for the TPM's irq number and enable the TPM IRQ unless
> TPM_IRQ_DISABLED is returned.
>
> 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..bb9a7f8497 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)) {
> +                int8_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 != TPM_IRQ_DISABLED) {
> +                    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 d5caee9771..d356f2e06e 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_ISA_IRQ             5
> +#define TPM_TIS_ISA_IRQ             13    /* only one possible */
>  #define TPM_TIS_SYSBUS_IRQ          5
>
>  #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
> --
> 2.24.1
>
Eric Auger June 17, 2020, 8:22 a.m. UTC | #2 
Hi Stefan,

On 6/16/20 10:57 PM, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
> 
> Move the TPM TIS IRQ to unused IRQ 13, which is the only one accepted by
> Windows. Query for the TPM's irq number and enable the TPM IRQ unless
> TPM_IRQ_DISABLED is returned.
> 
> 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..bb9a7f8497 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)) {
> +                int8_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 != TPM_IRQ_DISABLED) {
Out of curiosity what is the goal to expose the irq num as a property
settable by the end-user if only 13 is known to work in all cases. At
least shouldn't we warn the end-user in case he attempts to change the
default value?

Thanks

Eric
> +                    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 d5caee9771..d356f2e06e 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_ISA_IRQ             5
> +#define TPM_TIS_ISA_IRQ             13    /* only one possible */
>  #define TPM_TIS_SYSBUS_IRQ          5
>  
>  #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */
>
Stefan Berger June 17, 2020, 11:59 a.m. UTC | #3 
On 6/17/20 4:22 AM, Auger Eric wrote:
> Hi Stefan,
>
> On 6/16/20 10:57 PM, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> Move the TPM TIS IRQ to unused IRQ 13, which is the only one accepted by
>> Windows. Query for the TPM's irq number and enable the TPM IRQ unless
>> TPM_IRQ_DISABLED is returned.
>>
>> 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..bb9a7f8497 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)) {
>> +                int8_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 != TPM_IRQ_DISABLED) {
> Out of curiosity what is the goal to expose the irq num as a property
> settable by the end-user if only 13 is known to work in all cases. At
> least shouldn't we warn the end-user in case he attempts to change the
> default value?

For Windows only IRQ 13 works (and I am not sure whether this has always 
been like this), Linux accepts several other ones. As for exposing it to 
the end-user, I may have taken this from soundblaster (sb16.c), which 
also exposes it. If someone plays around with the irq numbers I would 
say they must have some more Pc knowledge than  just trying random numbers.


    Stefan
Michael S. Tsirkin June 18, 2020, 8:12 p.m. UTC | #4 
On Wed, Jun 17, 2020 at 07:59:51AM -0400, Stefan Berger wrote:
> On 6/17/20 4:22 AM, Auger Eric wrote:
> > Hi Stefan,
> > 
> > On 6/16/20 10:57 PM, Stefan Berger wrote:
> > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > 
> > > Move the TPM TIS IRQ to unused IRQ 13, which is the only one accepted by
> > > Windows. Query for the TPM's irq number and enable the TPM IRQ unless
> > > TPM_IRQ_DISABLED is returned.
> > > 
> > > 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..bb9a7f8497 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)) {
> > > +                int8_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 != TPM_IRQ_DISABLED) {
> > Out of curiosity what is the goal to expose the irq num as a property
> > settable by the end-user if only 13 is known to work in all cases. At
> > least shouldn't we warn the end-user in case he attempts to change the
> > default value?
> 
> For Windows only IRQ 13 works (and I am not sure whether this has always
> been like this), Linux accepts several other ones. As for exposing it to the
> end-user, I may have taken this from soundblaster (sb16.c), which also
> exposes it. If someone plays around with the irq numbers I would say they
> must have some more Pc knowledge than  just trying random numbers.
> 
> 
>    Stefan

So is this useful to anyone? If no I'd say drop it.
I'm guessing sb16 has it since it is useful for running extremely old OSes which might
have weird quirks for a specific hardware.
Stefan Berger June 18, 2020, 8:57 p.m. UTC | #5 
On 6/18/20 4:12 PM, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2020 at 07:59:51AM -0400, Stefan Berger wrote:
>> On 6/17/20 4:22 AM, Auger Eric wrote:
>>> Hi Stefan,
>>>
>>> On 6/16/20 10:57 PM, Stefan Berger wrote:
>>>> From: Stefan Berger <stefanb@linux.ibm.com>
>>>>
>>>> Move the TPM TIS IRQ to unused IRQ 13, which is the only one accepted by
>>>> Windows. Query for the TPM's irq number and enable the TPM IRQ unless
>>>> TPM_IRQ_DISABLED is returned.
>>>>
>>>> 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..bb9a7f8497 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)) {
>>>> +                int8_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 != TPM_IRQ_DISABLED) {
>>> Out of curiosity what is the goal to expose the irq num as a property
>>> settable by the end-user if only 13 is known to work in all cases. At
>>> least shouldn't we warn the end-user in case he attempts to change the
>>> default value?
>> For Windows only IRQ 13 works (and I am not sure whether this has always
>> been like this), Linux accepts several other ones. As for exposing it to the
>> end-user, I may have taken this from soundblaster (sb16.c), which also
>> exposes it. If someone plays around with the irq numbers I would say they
>> must have some more Pc knowledge than  just trying random numbers.
>>
>>
>>    Stefan
> So is this useful to anyone? If no I'd say drop it.


So we can remove command line options?


> I'm guessing sb16 has it since it is useful for running extremely old OSes which might
> have weird quirks for a specific hardware.
>
diff mbox series

Patch

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 900f786d08..bb9a7f8497 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)) {
+                int8_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 != TPM_IRQ_DISABLED) {
+                    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 d5caee9771..d356f2e06e 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_ISA_IRQ             5
+#define TPM_TIS_ISA_IRQ             13    /* only one possible */
 #define TPM_TIS_SYSBUS_IRQ          5
 
 #define TPM_TIS_NUM_LOCALITIES      5     /* per spec */