Re: [PATCH v2 13/16] hw/intc/ioapic: Remove IOAPICCommonState::version field

[Mark Cave-Ayland <mark.caveayland@nutanix.com>]

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.