| Message ID | 20230105143228.244965-14-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Consolidate PIIX south bridges | expand |
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.
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.
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. + */ +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)
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(+)