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

[Philippe Mathieu-Daudé]

On 10/11/20 10:28 PM, Stefan Berger wrote:
> 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.

Yes, sorry :( Maybe better to drop this patch.

> > 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(),