| Message ID | 20201011193229.3210774-5-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/isa: Introduce definitions for default IRQ values | expand |
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(),
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(), > > >
On Sun, Oct 11, 2020 at 09:32:23PM +0200, Philippe Mathieu-Daudé wrote: > The TPM TIS device uses IRQ #5 by default. Add this > default definition to the IsaIrqNumber enum. IRQ 5 has no fixed assignment. All kinds of add-on isa cards (sound, net) used to use irq #5 by default because that one wasn't assigned otherwise. Seems these days tpm and ipmi use it ... take care, Gerd
On 10/13/20 9:20 AM, Gerd Hoffmann wrote: > On Sun, Oct 11, 2020 at 09:32:23PM +0200, Philippe Mathieu-Daudé wrote: >> The TPM TIS device uses IRQ #5 by default. Add this >> default definition to the IsaIrqNumber enum. > > IRQ 5 has no fixed assignment. All kinds of add-on isa cards (sound, > net) used to use irq #5 by default because that one wasn't assigned > otherwise. Seems these days tpm and ipmi use it ... Yes, I'll drop this patch. Thanks, Phil.
On 10/13/20 4:26 AM, Philippe Mathieu-Daudé wrote: > On 10/13/20 9:20 AM, Gerd Hoffmann wrote: >> On Sun, Oct 11, 2020 at 09:32:23PM +0200, Philippe >> Mathieu-Daudé wrote: >>> The TPM TIS device uses IRQ #5 by default. Add this >>> default definition to the IsaIrqNumber enum. >> >> IRQ 5 has no fixed assignment. All kinds of add-on isa cards (sound, >> net) used to use irq #5 by default because that one wasn't assigned >> otherwise. Seems these days tpm and ipmi use it ... > > Yes, I'll drop this patch. I think this patch is good but maybe the name should be a different one. Rather than having these numbers in the code you could maybe call it something like this here, which makes grepping through the code a bit easier: ISA_IRQ_IRQ5 = 5, Regards, Stefan > > Thanks, > > Phil.
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(), }; 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(),
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(-)