Re: [PATCH v5 12/31] hw/intc/i8259: Make using the isa_pic singleton more type-safe

[Mark Cave-Ayland]

On 05/01/2023 14:32, Bernhard Beschow wrote:

> This even spares some casts in hot code paths along the way.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>
> ---
> Note: The next patch will introduce a class "isa-pic", which is
> shall not be confused with the isa_pic singleton.
> ---
>   include/hw/intc/i8259.h |  6 +++---
>   include/qemu/typedefs.h |  1 +
>   hw/intc/i8259.c         | 11 ++++-------
>   3 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/intc/i8259.h b/include/hw/intc/i8259.h
> index e2b1e8c59a..a0e34dd990 100644
> --- a/include/hw/intc/i8259.h
> +++ b/include/hw/intc/i8259.h
> @@ -3,10 +3,10 @@
>   
>   /* i8259.c */
>   
> -extern DeviceState *isa_pic;
> +extern PICCommonState *isa_pic;
>   qemu_irq *i8259_init(ISABus *bus, qemu_irq parent_irq);
>   qemu_irq *kvm_i8259_init(ISABus *bus);
> -int pic_get_output(DeviceState *d);
> -int pic_read_irq(DeviceState *d);
> +int pic_get_output(PICCommonState *s);
> +int pic_read_irq(PICCommonState *s);
>   
>   #endif
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 688408e048..3d5944d2a4 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -98,6 +98,7 @@ typedef struct PCIExpressDevice PCIExpressDevice;
>   typedef struct PCIExpressHost PCIExpressHost;
>   typedef struct PCIHostDeviceAddress PCIHostDeviceAddress;
>   typedef struct PCIHostState PCIHostState;
> +typedef struct PICCommonState PICCommonState;
>   typedef struct PostcopyDiscardState PostcopyDiscardState;
>   typedef struct Property Property;
>   typedef struct PropertyInfo PropertyInfo;
> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> index cc4e21ffec..0261f087b2 100644
> --- a/hw/intc/i8259.c
> +++ b/hw/intc/i8259.c
> @@ -55,7 +55,7 @@ struct PICClass {
>   #ifdef DEBUG_IRQ_LATENCY
>   static int64_t irq_time[16];
>   #endif
> -DeviceState *isa_pic;
> +PICCommonState *isa_pic;
>   static PICCommonState *slave_pic;
>   
>   /* return the highest priority found in mask (highest = smallest
> @@ -173,9 +173,8 @@ static void pic_intack(PICCommonState *s, int irq)
>       pic_update_irq(s);
>   }
>   
> -int pic_read_irq(DeviceState *d)
> +int pic_read_irq(PICCommonState *s)
>   {
> -    PICCommonState *s = PIC_COMMON(d);
>       int irq, intno;
>   
>       irq = pic_get_irq(s);
> @@ -354,10 +353,8 @@ static uint64_t pic_ioport_read(void *opaque, hwaddr addr,
>       return ret;
>   }
>   
> -int pic_get_output(DeviceState *d)
> +int pic_get_output(PICCommonState *s)
>   {
> -    PICCommonState *s = PIC_COMMON(d);
> -
>       return (pic_get_irq(s) >= 0);
>   }
>   
> @@ -426,7 +423,7 @@ qemu_irq *i8259_init(ISABus *bus, qemu_irq parent_irq)
>           irq_set[i] = qdev_get_gpio_in(dev, i);
>       }
>   
> -    isa_pic = dev;
> +    isa_pic = PIC_COMMON(dev);
>   
>       isadev = i8259_init_chip(TYPE_I8259, bus, false);
>       dev = DEVICE(isadev);

I actually had a similar thought when I was looking at the i8259 code and it seems 
sensible to me, so:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.