Re: [PATCH v5 13/31] hw/intc/i8259: Introduce i8259 proxy "isa-pic"

[Mark Cave-Ayland]

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

> Having an i8259 proxy allows for ISA PICs to be created and wired up in
> southbridges. This is especially interesting for PIIX3 for two reasons:
> First, the southbridge doesn't need to care about the virtualization
> technology used (KVM, TCG, Xen) due to in-IRQs (where devices get
> attached) and out-IRQs (which will trigger the IRQs of the respective
> virtualization technology) are separated. Second, since the in-IRQs are
> populated with fully initialized qemu_irq's, they can already be wired
> up inside PIIX3.
>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/intc/i8259.h | 19 +++++++++++++++++++
>   hw/intc/i8259.c         | 27 +++++++++++++++++++++++++++
>   2 files changed, 46 insertions(+)
>
> diff --git a/include/hw/intc/i8259.h b/include/hw/intc/i8259.h
> index a0e34dd990..f666f5ee09 100644
> --- a/include/hw/intc/i8259.h
> +++ b/include/hw/intc/i8259.h
> @@ -1,6 +1,25 @@
>   #ifndef HW_I8259_H
>   #define HW_I8259_H
>   
> +#include "qom/object.h"
> +#include "hw/isa/isa.h"
> +#include "qemu/typedefs.h"
> +
> +#define TYPE_ISA_PIC "isa-pic"
> +OBJECT_DECLARE_SIMPLE_TYPE(ISAPICState, ISA_PIC)
> +
> +/*
> + * TYPE_ISA_PIC is currently a PIC proxy which allows for interrupt wiring in
> + * a virtualization technology agnostic way. It could be turned into a proper
> + * GPIO-based ISA PIC in the future.
> + */

I would say that the last sentence isn't true, since when all PIC implementations 
have been converted to qdev the mechanism for choosing the PIC implementation is 
currently unspecified. As an example once this has been done "isa-pic" could be 
removed completely and the code in the following patch changed to something like:

    object_initialize_child(obj, "pic", &d->pic, kvm_enabled() ?  
TYPE_KVM_I8259 :
                            TYPE_I8259);

Perhaps change the last sentence to something like: "It provides a temporary bridge 
between older x86 code where qemu_irqs are passed around directly and devices that 
use qdev gpios."?

Other than that the implementation looks sensible to me, so:

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

> +struct ISAPICState {
> +    DeviceState parent_obj;
> +
> +    qemu_irq in_irqs[ISA_NUM_IRQS];
> +    qemu_irq out_irqs[ISA_NUM_IRQS];
> +};
> +
>   /* i8259.c */
>   
>   extern PICCommonState *isa_pic;
> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> index 0261f087b2..e99d02136d 100644
> --- a/hw/intc/i8259.c
> +++ b/hw/intc/i8259.c
> @@ -455,9 +455,36 @@ static const TypeInfo i8259_info = {
>       .class_size = sizeof(PICClass),
>   };
>   
> +static void isapic_set_irq(void *opaque, int irq, int level)
> +{
> +    ISAPICState *s = opaque;
> +
> +    qemu_set_irq(s->out_irqs[irq], level);
> +}
> +
> +static void isapic_init(Object *obj)
> +{
> +    ISAPICState *s = ISA_PIC(obj);
> +
> +    qdev_init_gpio_in(DEVICE(s), isapic_set_irq, ISA_NUM_IRQS);
> +    qdev_init_gpio_out(DEVICE(s), s->out_irqs, ISA_NUM_IRQS);
> +
> +    for (int i = 0; i < ISA_NUM_IRQS; ++i) {
> +        s->in_irqs[i] = qdev_get_gpio_in(DEVICE(s), i);
> +    }
> +}
> +
> +static const TypeInfo isapic_info = {
> +    .name          = TYPE_ISA_PIC,
> +    .parent        = TYPE_DEVICE,
> +    .instance_size = sizeof(ISAPICState),
> +    .instance_init = isapic_init,
> +};
> +
>   static void pic_register_types(void)
>   {
>       type_register_static(&i8259_info);
> +    type_register_static(&isapic_info);
>   }
>   
>   type_init(pic_register_types)


ATB,

Mark.