| Message ID | 20250501183628.87479-14-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/i386/pc: Remove deprecated 2.6 and 2.7 PC machines | expand |
On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote: > The IOAPICCommonState::version integer was only set > in the hw_compat_2_7[] array, via the 'version=0x11' > property. We removed all machines using that array, > lets remove that property, simplify by only using the > default version (defined as IOAPIC_VER_DEF). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/intc/ioapic_internal.h | 3 +-- > hw/intc/ioapic.c | 18 ++---------------- > hw/intc/ioapic_common.c | 2 +- > 3 files changed, 4 insertions(+), 19 deletions(-) > > diff --git a/hw/intc/ioapic_internal.h b/hw/intc/ioapic_internal.h > index 51205767f44..330ce195222 100644 > --- a/hw/intc/ioapic_internal.h > +++ b/hw/intc/ioapic_internal.h > @@ -82,7 +82,7 @@ > #define IOAPIC_ID_MASK 0xf > > #define IOAPIC_VER_ENTRIES_SHIFT 16 > - > +#define IOAPIC_VER_DEF 0x20 > > #define TYPE_IOAPIC_COMMON "ioapic-common" > OBJECT_DECLARE_TYPE(IOAPICCommonState, IOAPICCommonClass, IOAPIC_COMMON) > @@ -104,7 +104,6 @@ struct IOAPICCommonState { > uint32_t irr; > uint64_t ioredtbl[IOAPIC_NUM_PINS]; > Notifier machine_done; > - uint8_t version; > uint64_t irq_count[IOAPIC_NUM_PINS]; > int irq_level[IOAPIC_NUM_PINS]; > int irq_eoi[IOAPIC_NUM_PINS]; > diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c > index 133bef852d1..5cc97767d9d 100644 > --- a/hw/intc/ioapic.c > +++ b/hw/intc/ioapic.c > @@ -315,7 +315,7 @@ ioapic_mem_read(void *opaque, hwaddr addr, unsigned int size) > val = s->id << IOAPIC_ID_SHIFT; > break; > case IOAPIC_REG_VER: > - val = s->version | > + val = IOAPIC_VER_DEF | > ((IOAPIC_NUM_PINS - 1) << IOAPIC_VER_ENTRIES_SHIFT); > break; > default: > @@ -411,8 +411,7 @@ ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val, > } > break; > case IOAPIC_EOI: > - /* Explicit EOI is only supported for IOAPIC version 0x20 */ > - if (size != 4 || s->version != 0x20) { > + if (size != 4) { > break; > } > ioapic_eoi_broadcast(val); > @@ -444,18 +443,10 @@ static void ioapic_machine_done_notify(Notifier *notifier, void *data) > #endif > } > > -#define IOAPIC_VER_DEF 0x20 > - > static void ioapic_realize(DeviceState *dev, Error **errp) > { > IOAPICCommonState *s = IOAPIC_COMMON(dev); > > - if (s->version != 0x11 && s->version != 0x20) { > - error_setg(errp, "IOAPIC only supports version 0x11 or 0x20 " > - "(default: 0x%x).", IOAPIC_VER_DEF); > - return; > - } > - > memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s, > "ioapic", 0x1000); > > @@ -476,10 +467,6 @@ static void ioapic_unrealize(DeviceState *dev) > timer_free(s->delayed_ioapic_service_timer); > } > > -static const Property ioapic_properties[] = { > - DEFINE_PROP_UINT8("version", IOAPICCommonState, version, IOAPIC_VER_DEF), > -}; > - > static void ioapic_class_init(ObjectClass *klass, const void *data) > { > IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass); > @@ -493,7 +480,6 @@ static void ioapic_class_init(ObjectClass *klass, const void *data) > */ > k->post_load = ioapic_update_kvm_routes; > device_class_set_legacy_reset(dc, ioapic_reset_common); > - device_class_set_props(dc, ioapic_properties); > } > > static const TypeInfo ioapic_info = { > diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c > index fce3486e519..8b3e2ba9384 100644 > --- a/hw/intc/ioapic_common.c > +++ b/hw/intc/ioapic_common.c > @@ -83,7 +83,7 @@ static void ioapic_print_redtbl(GString *buf, IOAPICCommonState *s) > int i; > > g_string_append_printf(buf, "ioapic0: ver=0x%x id=0x%02x sel=0x%02x", > - s->version, s->id, s->ioregsel); > + IOAPIC_VER_DEF, s->id, s->ioregsel); > if (s->ioregsel) { > g_string_append_printf(buf, " (redir[%u])\n", > (s->ioregsel - IOAPIC_REG_REDTBL_BASE) >> 1); Mildly curious that other than the reported version the version field doesn't appear to control anything else - was the original bug that QEMU implemented a 0x20 IOAPIC but incorrectly reported it as a 0x11 IOAPIC? Anyhow the diff itself looks good so: Reviewed-by: Mark Cave-Ayland <mark.caveayland@nutanix.com> ATB, Mark.
On 2/5/25 11:31, Mark Cave-Ayland wrote: > On 01/05/2025 19:36, Philippe Mathieu-Daudé wrote: > >> The IOAPICCommonState::version integer was only set >> in the hw_compat_2_7[] array, via the 'version=0x11' >> property. We removed all machines using that array, >> lets remove that property, simplify by only using the >> default version (defined as IOAPIC_VER_DEF). >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> hw/intc/ioapic_internal.h | 3 +-- >> hw/intc/ioapic.c | 18 ++---------------- >> hw/intc/ioapic_common.c | 2 +- >> 3 files changed, 4 insertions(+), 19 deletions(-) >> >> diff --git a/hw/intc/ioapic_internal.h b/hw/intc/ioapic_internal.h >> index 51205767f44..330ce195222 100644 >> --- a/hw/intc/ioapic_internal.h >> +++ b/hw/intc/ioapic_internal.h >> @@ -82,7 +82,7 @@ >> #define IOAPIC_ID_MASK 0xf >> #define IOAPIC_VER_ENTRIES_SHIFT 16 >> - >> +#define IOAPIC_VER_DEF 0x20 >> #define TYPE_IOAPIC_COMMON "ioapic-common" >> OBJECT_DECLARE_TYPE(IOAPICCommonState, IOAPICCommonClass, >> IOAPIC_COMMON) >> @@ -104,7 +104,6 @@ struct IOAPICCommonState { >> uint32_t irr; >> uint64_t ioredtbl[IOAPIC_NUM_PINS]; >> Notifier machine_done; >> - uint8_t version; >> uint64_t irq_count[IOAPIC_NUM_PINS]; >> int irq_level[IOAPIC_NUM_PINS]; >> int irq_eoi[IOAPIC_NUM_PINS]; >> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c >> index 133bef852d1..5cc97767d9d 100644 >> --- a/hw/intc/ioapic.c >> +++ b/hw/intc/ioapic.c >> @@ -315,7 +315,7 @@ ioapic_mem_read(void *opaque, hwaddr addr, >> unsigned int size) >> val = s->id << IOAPIC_ID_SHIFT; >> break; >> case IOAPIC_REG_VER: >> - val = s->version | >> + val = IOAPIC_VER_DEF | >> ((IOAPIC_NUM_PINS - 1) << IOAPIC_VER_ENTRIES_SHIFT); >> break; >> default: >> @@ -411,8 +411,7 @@ ioapic_mem_write(void *opaque, hwaddr addr, >> uint64_t val, >> } >> break; >> case IOAPIC_EOI: >> - /* Explicit EOI is only supported for IOAPIC version 0x20 */ >> - if (size != 4 || s->version != 0x20) { >> + if (size != 4) { >> break; >> } >> ioapic_eoi_broadcast(val); >> @@ -444,18 +443,10 @@ static void ioapic_machine_done_notify(Notifier >> *notifier, void *data) >> #endif >> } >> -#define IOAPIC_VER_DEF 0x20 >> - >> static void ioapic_realize(DeviceState *dev, Error **errp) >> { >> IOAPICCommonState *s = IOAPIC_COMMON(dev); >> - if (s->version != 0x11 && s->version != 0x20) { >> - error_setg(errp, "IOAPIC only supports version 0x11 or 0x20 " >> - "(default: 0x%x).", IOAPIC_VER_DEF); >> - return; >> - } >> - >> memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s, >> "ioapic", 0x1000); >> @@ -476,10 +467,6 @@ static void ioapic_unrealize(DeviceState *dev) >> timer_free(s->delayed_ioapic_service_timer); >> } >> -static const Property ioapic_properties[] = { >> - DEFINE_PROP_UINT8("version", IOAPICCommonState, version, >> IOAPIC_VER_DEF), >> -}; >> - >> static void ioapic_class_init(ObjectClass *klass, const void *data) >> { >> IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass); >> @@ -493,7 +480,6 @@ static void ioapic_class_init(ObjectClass *klass, >> const void *data) >> */ >> k->post_load = ioapic_update_kvm_routes; >> device_class_set_legacy_reset(dc, ioapic_reset_common); >> - device_class_set_props(dc, ioapic_properties); >> } >> static const TypeInfo ioapic_info = { >> diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c >> index fce3486e519..8b3e2ba9384 100644 >> --- a/hw/intc/ioapic_common.c >> +++ b/hw/intc/ioapic_common.c >> @@ -83,7 +83,7 @@ static void ioapic_print_redtbl(GString *buf, >> IOAPICCommonState *s) >> int i; >> g_string_append_printf(buf, "ioapic0: ver=0x%x id=0x%02x >> sel=0x%02x", >> - s->version, s->id, s->ioregsel); >> + IOAPIC_VER_DEF, s->id, s->ioregsel); >> if (s->ioregsel) { >> g_string_append_printf(buf, " (redir[%u])\n", >> (s->ioregsel - >> IOAPIC_REG_REDTBL_BASE) >> 1); > > Mildly curious that other than the reported version the version field > doesn't appear to control anything else - was the original bug that QEMU > implemented a 0x20 IOAPIC but incorrectly reported it as a 0x11 IOAPIC? I'll mention: commit 20fd4b7b6d9282fe0cb83601f1821f31bd257458 Author: Peter Xu <peterx@redhat.com> Date: Mon Aug 1 21:59:19 2016 +0800 x86: ioapic: add support for explicit EOI Some old Linux kernels (upstream before v4.0), or any released RHEL kernels has problem in sending APIC EOI when IR is enabled. Meanwhile, many of them only support explicit EOI for IOAPIC, which is only introduced in IOAPIC version 0x20. This patch provide a way to boost QEMU IOAPIC to version 0x20, in order for QEMU to correctly receive EOI messages. Without boosting IOAPIC version to 0x20, kernels before commit d32932d ("x86/irq: Convert IOAPIC to use hierarchical irqdomain interfaces") will have trouble enabling both IR and level-triggered interrupt devices (like e1000). To upgrade IOAPIC to version 0x20, we need to specify: -global ioapic.version=0x20 To be compatible with old systems, 0x11 will still be the default IOAPIC version. Here 0x11 and 0x20 are the only versions to be supported. One thing to mention: this patch only applies to emulated IOAPIC. It does not affect kernel IOAPIC behavior. (see also:) commit 048a2e8869cb7e26013e40d860c9ebdf8e28c2ac Author: Peter Xu <peterx@redhat.com> Date: Fri Sep 23 13:33:15 2016 +0800 x86: ioapic: boost default version to 0x20 It's 2.8 now, and maybe it's time to switch IOAPIC default version to 0x20. Signed-off-by: Peter Xu <peterx@redhat.com> Message-Id: <1474608795-23058-1-git-send-email-peterx@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> diff --git a/include/hw/compat.h b/include/hw/compat.h index a1d66944924..46412b229a7 100644 --- a/include/hw/compat.h +++ b/include/hw/compat.h @@ -8,0 +9,4 @@ + },{\ + .driver = "ioapic",\ + .property = "version",\ + .value = "0x11",\ diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c index 31791b09860..fd9208fde08 100644 --- a/hw/intc/ioapic.c +++ b/hw/intc/ioapic.c @@ -419 +419 @@ static Property ioapic_properties[] = { - DEFINE_PROP_UINT8("version", IOAPICCommonState, version, 0x11), + DEFINE_PROP_UINT8("version", IOAPICCommonState, version, 0x20), > > Anyhow the diff itself looks good so: > > Reviewed-by: Mark Cave-Ayland <mark.caveayland@nutanix.com> Thanks!
diff --git a/hw/intc/ioapic_internal.h b/hw/intc/ioapic_internal.h index 51205767f44..330ce195222 100644 --- a/hw/intc/ioapic_internal.h +++ b/hw/intc/ioapic_internal.h @@ -82,7 +82,7 @@ #define IOAPIC_ID_MASK 0xf #define IOAPIC_VER_ENTRIES_SHIFT 16 - +#define IOAPIC_VER_DEF 0x20 #define TYPE_IOAPIC_COMMON "ioapic-common" OBJECT_DECLARE_TYPE(IOAPICCommonState, IOAPICCommonClass, IOAPIC_COMMON) @@ -104,7 +104,6 @@ struct IOAPICCommonState { uint32_t irr; uint64_t ioredtbl[IOAPIC_NUM_PINS]; Notifier machine_done; - uint8_t version; uint64_t irq_count[IOAPIC_NUM_PINS]; int irq_level[IOAPIC_NUM_PINS]; int irq_eoi[IOAPIC_NUM_PINS]; diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c index 133bef852d1..5cc97767d9d 100644 --- a/hw/intc/ioapic.c +++ b/hw/intc/ioapic.c @@ -315,7 +315,7 @@ ioapic_mem_read(void *opaque, hwaddr addr, unsigned int size) val = s->id << IOAPIC_ID_SHIFT; break; case IOAPIC_REG_VER: - val = s->version | + val = IOAPIC_VER_DEF | ((IOAPIC_NUM_PINS - 1) << IOAPIC_VER_ENTRIES_SHIFT); break; default: @@ -411,8 +411,7 @@ ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val, } break; case IOAPIC_EOI: - /* Explicit EOI is only supported for IOAPIC version 0x20 */ - if (size != 4 || s->version != 0x20) { + if (size != 4) { break; } ioapic_eoi_broadcast(val); @@ -444,18 +443,10 @@ static void ioapic_machine_done_notify(Notifier *notifier, void *data) #endif } -#define IOAPIC_VER_DEF 0x20 - static void ioapic_realize(DeviceState *dev, Error **errp) { IOAPICCommonState *s = IOAPIC_COMMON(dev); - if (s->version != 0x11 && s->version != 0x20) { - error_setg(errp, "IOAPIC only supports version 0x11 or 0x20 " - "(default: 0x%x).", IOAPIC_VER_DEF); - return; - } - memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s, "ioapic", 0x1000); @@ -476,10 +467,6 @@ static void ioapic_unrealize(DeviceState *dev) timer_free(s->delayed_ioapic_service_timer); } -static const Property ioapic_properties[] = { - DEFINE_PROP_UINT8("version", IOAPICCommonState, version, IOAPIC_VER_DEF), -}; - static void ioapic_class_init(ObjectClass *klass, const void *data) { IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass); @@ -493,7 +480,6 @@ static void ioapic_class_init(ObjectClass *klass, const void *data) */ k->post_load = ioapic_update_kvm_routes; device_class_set_legacy_reset(dc, ioapic_reset_common); - device_class_set_props(dc, ioapic_properties); } static const TypeInfo ioapic_info = { diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c index fce3486e519..8b3e2ba9384 100644 --- a/hw/intc/ioapic_common.c +++ b/hw/intc/ioapic_common.c @@ -83,7 +83,7 @@ static void ioapic_print_redtbl(GString *buf, IOAPICCommonState *s) int i; g_string_append_printf(buf, "ioapic0: ver=0x%x id=0x%02x sel=0x%02x", - s->version, s->id, s->ioregsel); + IOAPIC_VER_DEF, s->id, s->ioregsel); if (s->ioregsel) { g_string_append_printf(buf, " (redir[%u])\n", (s->ioregsel - IOAPIC_REG_REDTBL_BASE) >> 1);
The IOAPICCommonState::version integer was only set in the hw_compat_2_7[] array, via the 'version=0x11' property. We removed all machines using that array, lets remove that property, simplify by only using the default version (defined as IOAPIC_VER_DEF). Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/intc/ioapic_internal.h | 3 +-- hw/intc/ioapic.c | 18 ++---------------- hw/intc/ioapic_common.c | 2 +- 3 files changed, 4 insertions(+), 19 deletions(-)