Re: [PATCH] hw/core: Introduce proxy-pic

[Bernhard Beschow <shentey@gmail.com>]

Am 5. Januar 2023 09:50:03 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>
>
>Am 4. Januar 2023 22:22:01 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>>On 04/01/2023 19:53, Bernhard Beschow wrote:
>>
>>> Having a proxy PIC allows for ISA PICs to be created and wired up in
>>> southbridges. This is especially useful 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.
>>> 
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>> Message-Id: <20221022150508.26830-15-shentey@gmail.com>
>>> ---
>>> Changes since v4:
>>> * Change license to GPL-2.0-or-later and use SPDX-License-Identifier
>>> * Fix typo in commit message
>>> ---
>>>   include/hw/core/proxy-pic.h | 38 ++++++++++++++++++++++++++
>>>   hw/core/proxy-pic.c         | 54 +++++++++++++++++++++++++++++++++++++
>>>   MAINTAINERS                 |  2 ++
>>>   hw/core/Kconfig             |  3 +++
>>>   hw/core/meson.build         |  1 +
>>>   5 files changed, 98 insertions(+)
>>>   create mode 100644 include/hw/core/proxy-pic.h
>>>   create mode 100644 hw/core/proxy-pic.c
>>> 
>>> diff --git a/include/hw/core/proxy-pic.h b/include/hw/core/proxy-pic.h
>>> new file mode 100644
>>> index 0000000000..32bc7936bd
>>> --- /dev/null
>>> +++ b/include/hw/core/proxy-pic.h
>>> @@ -0,0 +1,38 @@
>>> +/*
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + *
>>> + * Proxy interrupt controller device.
>>> + *
>>> + * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
>>> + */
>>> +
>>> +#ifndef HW_PROXY_PIC_H
>>> +#define HW_PROXY_PIC_H
>>> +
>>> +#include "hw/qdev-core.h"
>>> +#include "qom/object.h"
>>> +#include "hw/irq.h"
>>> +
>>> +#define TYPE_PROXY_PIC "proxy-pic"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(ProxyPICState, PROXY_PIC)
>>> +
>>> +#define MAX_PROXY_PIC_LINES 16
>>> +
>>> +/**
>>> + * This is a simple device which has 16 pairs of GPIO input and output lines.
>>> + * Any change on an input line is forwarded to the respective output.
>>> + *
>>> + * QEMU interface:
>>> + *  + 16 unnamed GPIO inputs: the input lines
>>> + *  + 16 unnamed GPIO outputs: the output lines
>>> + */
>>
>>Re-reading this as a standalone patch, I can understand now why Phil was asking about device properties etc. because aside from the commit message, it isn't particularly clear that this is a workaround for QEMU's PIC devices and accelerator implementations not (yet) supporting direct wiring with qdev gpios. I would definitely argue that it is a special purpose and not a generic device.
>>
>>I apologise that this is quite late in the review process, however given that this wasn't immediately clear I do think it is worth making a few minor changes. Perhaps something like:
>>
>>- Update the comment above in proxy_pic.h clarifying that this is only for wiring up
>>  ISA PICs (similar to the commit message) until gpios can be used
>
>Will do.
>
>>- Move the .c and .h files from hw/core/proxy-pic.c and include/hw/core/proxy_pic.h
>>  to hw/i386/proxy-pic.c and include/hw/i386/proxy_pic.h to provide a strong hint
>>  that the device is restricted to x86-only
>
>The device gets used in PIIX4 as well, i.e. MIPS, too. I therefore think it is not x86 but rather PIC specific. I propose to move it back to hw/intc/i8259 where it was implemented until v2: https://patchew.org/QEMU/20221022150508.26830-1-shentey@gmail.com/20221022150508.26830-15-shentey@gmail.com/ . I can also rename the device back to isa-pic to make things more obvious.

I've submitted v5 of the series. Mark, would you be available for review?

For Phil's convenience I've pushed a git tag here: https://github.com/shentok/qemu/commits/piix-consolidate-v5

Best regards,
Bernhard
>
>What do you think?
>
>Best regards,
>Bernhard
>
>>
>>I think this makes it more obvious what the device is doing, and also prevent its usage leaking into other places in the codebase. In fact in its current form there is no need for device properties to configure the PIC lines, since legacy x86 PICs always have 16 (ISA) IRQ lines.
>>
>>> +struct ProxyPICState {
>>> +    /*< private >*/
>>> +    struct DeviceState parent_obj;
>>> +    /*< public >*/
>>> +
>>> +    qemu_irq in_irqs[MAX_PROXY_PIC_LINES];
>>> +    qemu_irq out_irqs[MAX_PROXY_PIC_LINES];
>>> +};
>>> +
>>> +#endif /* HW_PROXY_PIC_H */
>>> diff --git a/hw/core/proxy-pic.c b/hw/core/proxy-pic.c
>>> new file mode 100644
>>> index 0000000000..40fd70b9e2
>>> --- /dev/null
>>> +++ b/hw/core/proxy-pic.c
>>> @@ -0,0 +1,54 @@
>>> +/*
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + *
>>> + * Proxy interrupt controller device.
>>> + *
>>> + * Copyright (c) 2022 Bernhard Beschow <shentey@gmail.com>
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "hw/core/proxy-pic.h"
>>> +
>>> +static void proxy_pic_set_irq(void *opaque, int irq, int level)
>>> +{
>>> +    ProxyPICState *s = opaque;
>>> +
>>> +    qemu_set_irq(s->out_irqs[irq], level);
>>> +}
>>> +
>>> +static void proxy_pic_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    ProxyPICState *s = PROXY_PIC(dev);
>>> +
>>> +    qdev_init_gpio_in(DEVICE(s), proxy_pic_set_irq, MAX_PROXY_PIC_LINES);
>>> +    qdev_init_gpio_out(DEVICE(s), s->out_irqs, MAX_PROXY_PIC_LINES);
>>> +
>>> +    for (int i = 0; i < MAX_PROXY_PIC_LINES; ++i) {
>>> +        s->in_irqs[i] = qdev_get_gpio_in(DEVICE(s), i);
>>> +    }
>>> +}
>>> +
>>> +static void proxy_pic_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +
>>> +    /* No state to reset or migrate */
>>> +    dc->realize = proxy_pic_realize;
>>> +
>>> +    /* Reason: Needs to be wired up to work */
>>> +    dc->user_creatable = false;
>>> +}
>>> +
>>> +static const TypeInfo proxy_pic_info = {
>>> +    .name          = TYPE_PROXY_PIC,
>>> +    .parent        = TYPE_DEVICE,
>>> +    .instance_size = sizeof(ProxyPICState),
>>> +    .class_init = proxy_pic_class_init,
>>> +};
>>> +
>>> +static void split_irq_register_types(void)
>>> +{
>>> +    type_register_static(&proxy_pic_info);
>>> +}
>>> +
>>> +type_init(split_irq_register_types)
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 7a40d4d865..295a76bfbd 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -1674,6 +1674,7 @@ S: Supported
>>>   F: hw/char/debugcon.c
>>>   F: hw/char/parallel*
>>>   F: hw/char/serial*
>>> +F: hw/core/proxy-pic.c
>>>   F: hw/dma/i8257*
>>>   F: hw/i2c/pm_smbus.c
>>>   F: hw/input/pckbd.c
>>> @@ -1690,6 +1691,7 @@ F: hw/watchdog/wdt_ib700.c
>>>   F: hw/watchdog/wdt_i6300esb.c
>>>   F: include/hw/display/vga.h
>>>   F: include/hw/char/parallel.h
>>> +F: include/hw/core/proxy-pic.h
>>>   F: include/hw/dma/i8257.h
>>>   F: include/hw/i2c/pm_smbus.h
>>>   F: include/hw/input/i8042.h
>>> diff --git a/hw/core/Kconfig b/hw/core/Kconfig
>>> index 9397503656..a7224f4ca0 100644
>>> --- a/hw/core/Kconfig
>>> +++ b/hw/core/Kconfig
>>> @@ -22,6 +22,9 @@ config OR_IRQ
>>>   config PLATFORM_BUS
>>>       bool
>>>   +config PROXY_PIC
>>> +    bool
>>> +
>>>   config REGISTER
>>>       bool
>>>   diff --git a/hw/core/meson.build b/hw/core/meson.build
>>> index 7a4d02b6c0..e86aef6ec3 100644
>>> --- a/hw/core/meson.build
>>> +++ b/hw/core/meson.build
>>> @@ -30,6 +30,7 @@ softmmu_ss.add(when: ['CONFIG_GUEST_LOADER', fdt], if_true: files('guest-loader.
>>>   softmmu_ss.add(when: 'CONFIG_OR_IRQ', if_true: files('or-irq.c'))
>>>   softmmu_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('platform-bus.c'))
>>>   softmmu_ss.add(when: 'CONFIG_PTIMER', if_true: files('ptimer.c'))
>>> +softmmu_ss.add(when: 'CONFIG_PROXY_PIC', if_true: files('proxy-pic.c'))
>>>   softmmu_ss.add(when: 'CONFIG_REGISTER', if_true: files('register.c'))
>>>   softmmu_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
>>>   softmmu_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
>>
>>
>>ATB,
>>
>>Mark.