1. Patchew
  2. QEMU
  3. View series

[PATCH] hw/core: Introduce proxy-pic

Bernhard Beschow posted 1 patch 1 year, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230104195351.3418-1-shentey@gmail.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
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
[PATCH] hw/core: Introduce proxy-pic
Posted by Bernhard Beschow 1 year, 3 months ago 
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
+ */
+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'))
-- 
2.39.0
Re: [PATCH] hw/core: Introduce proxy-pic
Posted by Mark Cave-Ayland 1 year, 3 months ago 
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

- 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

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.
Re: [PATCH] hw/core: Introduce proxy-pic
Posted by Bernhard Beschow 1 year, 3 months ago 

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.

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.
Re: [PATCH] hw/core: Introduce proxy-pic
Posted by Bernhard Beschow 1 year, 3 months ago 

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.

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.