qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Mark Cave-Ayland
Subject: Re: [PATCH] hw/core: Introduce proxy-pic
Date: Wed, 4 Jan 2023 22:22:01 +0000
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0

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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]