On Thu, 8 May 2025 15:35:47 +0200
Philippe Mathieu-Daudé <[email protected]> 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).
>
> For the record, this field was introduced in commit
> 20fd4b7b6d9 ("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
> >
that crutch might be in-use by external users, and even if they use
0x20, removing property will break CLI.
I'd deprecate it first and then remove.
> > 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.
looking at the code removed, default is 0x20 which doesn't match above
statement. Have something changed between then and now (missing ref to
0x20 becoming default)?
> >
> > One thing to mention: this patch only applies to emulated IOAPIC. It
> > does not affect kernel IOAPIC behavior.
>
> Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
> Reviewed-by: Mark Cave-Ayland <[email protected]>
> ---
> 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);
