Re: [PATCH 04/10] hw/isa: Add the ISA_IRQ_TPM_DEFAULT definition

[Stefan Berger <stefanb@linux.ibm.com>]

On 10/11/20 3:32 PM, Philippe Mathieu-Daudé wrote:
> The TPM TIS device uses IRQ #5 by default. Add this
> default definition to the IsaIrqNumber enum.
>
> Avoid magic values in the code, replace them by the
> newly introduced definition.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   include/hw/isa/isa.h   | 1 +
>   hw/i386/acpi-build.c   | 2 +-
>   hw/ipmi/isa_ipmi_bt.c  | 2 +-
>   hw/ipmi/isa_ipmi_kcs.c | 2 +-
>   hw/tpm/tpm_tis_isa.c   | 2 +-
>   5 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index 519296d5823..e4f2aed004f 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -11,6 +11,7 @@
>   enum IsaIrqNumber {
>       ISA_IRQ_KBD_DEFAULT =  1,
>       ISA_IRQ_SER_DEFAULT =  4,
> +    ISA_IRQ_TPM_DEFAULT =  5,
>       ISA_NUM_IRQS        = 16
>   };
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 45ad2f95334..2b6038ab015 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1886,7 +1886,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>                       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)); */
> +                /* aml_append(crs, aml_irq_no_flags(ISA_IRQ_TPM_DEFAULT)); */
>                   aml_append(dev, aml_name_decl("_CRS", crs));
>
>                   tpm_build_ppi_acpi(tpm, dev);
> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
> index b7c2ad557b2..13a92bd2c44 100644
> --- a/hw/ipmi/isa_ipmi_bt.c
> +++ b/hw/ipmi/isa_ipmi_bt.c
> @@ -137,7 +137,7 @@ static void *isa_ipmi_bt_get_backend_data(IPMIInterface *ii)
>
>   static Property ipmi_isa_properties[] = {
>       DEFINE_PROP_UINT32("ioport", ISAIPMIBTDevice, bt.io_base,  0xe4),
> -    DEFINE_PROP_INT32("irq",   ISAIPMIBTDevice, isairq,  5),
> +    DEFINE_PROP_INT32("irq", ISAIPMIBTDevice, isairq, ISA_IRQ_TPM_DEFAULT),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>
> diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
> index 7dd6bf0040a..c956b539688 100644
> --- a/hw/ipmi/isa_ipmi_kcs.c
> +++ b/hw/ipmi/isa_ipmi_kcs.c
> @@ -144,7 +144,7 @@ static void *isa_ipmi_kcs_get_backend_data(IPMIInterface *ii)
>
>   static Property ipmi_isa_properties[] = {
>       DEFINE_PROP_UINT32("ioport", ISAIPMIKCSDevice, kcs.io_base,  0xca2),
> -    DEFINE_PROP_INT32("irq",   ISAIPMIKCSDevice, isairq,  5),
> +    DEFINE_PROP_INT32("irq", ISAIPMIKCSDevice, isairq, ISA_IRQ_TPM_DEFAULT),
>       DEFINE_PROP_END_OF_LIST(),
>   };


I don't know what these devices do but this looks like an unwanted clash.


> diff --git a/hw/tpm/tpm_tis_isa.c b/hw/tpm/tpm_tis_isa.c
> index 6fd876eebf1..5a4afda42df 100644
> --- a/hw/tpm/tpm_tis_isa.c
> +++ b/hw/tpm/tpm_tis_isa.c
> @@ -91,7 +91,7 @@ static void tpm_tis_isa_reset(DeviceState *dev)
>   }
>
>   static Property tpm_tis_isa_properties[] = {
> -    DEFINE_PROP_UINT32("irq", TPMStateISA, state.irq_num, TPM_TIS_IRQ),
> +    DEFINE_PROP_UINT32("irq", TPMStateISA, state.irq_num, ISA_IRQ_TPM_DEFAULT),
>       DEFINE_PROP_TPMBE("tpmdev", TPMStateISA, state.be_driver),
>       DEFINE_PROP_BOOL("ppi", TPMStateISA, state.ppi_enabled, true),
>       DEFINE_PROP_END_OF_LIST(),