Re: [PATCH v4 24/27] hw/intc/ioapic: Remove IOAPICCommonState::version field

[Igor Mammedov <imammedo@redhat.com>]

On Thu,  8 May 2025 15:35:47 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> 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é <philmd@linaro.org>
> Reviewed-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> ---
>  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);