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

[Bernhard Beschow]

Hi Mark,

Am 7. Januar 2023 23:45:39 UTC schrieb Mark Cave-Ayland 
<mark.cave-ayland@ilande.co.uk>:
>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);

This wouldn't work for the Malta machine where TYPE_I8259 is used even in the 
case of kvm_enabled(). Furthermore, the PC machine may instantiate yet another 
interrupt controller here in Xen mode. Hence my sentence in the cover letter of 
making PIIX agnostic about the virtualization technology used. Let's discuss 
future directions elsewhere for the sake of separation of concerns ;)

>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."?

I'd omit the last sentence for now.

>Other than that the implementation looks sensible to me, so:
>
>Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Thanks!

Best regards,
Bernhard

>> +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.