Re: [PATCH 10/20] hw/arm: Open-code pflash_cfi01_register()
On Thu, Jan 5, 2023 at 6:43 AM Philippe Mathieu-Daudé wrote: > > pflash_cfi01_register() hides an implicit sysbus mapping of > MMIO region #0. This is not practical in a heterogeneous world > where multiple cores use different address spaces. In order to > remove to remove pflash_cfi01_register() from the pflash API, duplicated "to remove" > open-code it as a qdev creation call followed by an explicit > sysbus mapping. > > Signed-off-by: Philippe Mathieu-Daudé > --- > hw/arm/collie.c | 15 +-- > hw/arm/gumstix.c | 19 +-- > hw/arm/mainstone.c | 13 - > hw/arm/omap_sx1.c| 22 ++ > hw/arm/versatilepb.c | 13 - > hw/arm/z2.c | 10 +++--- > 6 files changed, 59 insertions(+), 33 deletions(-) > Otherwise, Reviewed-by: Bin Meng
Re: [PATCH v5 00/31] Consolidate PIIX south bridges
On 05/01/2023 14:31, Bernhard Beschow wrote: This series consolidates the implementations of the PIIX3 and PIIX4 south bridges and is an extended version of [1]. The motivation is to share as much code as possible and to bring both device models to feature parity such that perhaps PIIX4 can become a drop-in-replacement for PIIX3 in the pc machine. This could resolve the "Frankenstein" PIIX4-PM problem in PIIX3 discussed on this list before. The series is structured as follows: First, PIIX3 is changed to instantiate internal devices itself, like PIIX4 does already. Second, PIIX3 gets prepared for the merge with PIIX4 which includes some fixes, cleanups, and renamings. Third, the same is done for PIIX4. In step four the implementations are merged. Since some consolidations could be done easier with merged implementations, the consolidation continues in step five which concludes the series. Note that the first three patches are only included to avoid merge conflicts with mips-next -- please ignore. One particular challenge in this series was that the PIC of PIIX3 used to be instantiated outside of the south bridge while some sub functions require a PIC with populated qemu_irqs. This has been solved by introducing a proxy PIC which furthermore allows PIIX3 to be agnostic towards the virtualization technology used (KVM, TCG, Xen). Due to consolidation PIIX4 gained the proxy PIC as well. Another challenge was dealing with optional devices where Peter already gave advice in [1] which this series implements. Last but not least there might be some opportunity to consolidate VM state handling, probably by reusing the one from PIIX3. Since I'm not very familiar with the requirements I didn't touch it so far. v5: - Pick up Reviewed-by tags from https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg00116.html - Add patch to make usage of the isa_pic global more type-safe - Re-introduce isa-pic as PIC specific proxy (Mark) Note that both patches are unreviewed -> Mark? Furthermore, patch 'hw/i386/pc_piix: Associate pci_map_irq_fn as soon as PCI bus is created' needs review and could be merged into https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03312.html . Testing done: * make check * Boot live CD: * `qemu-system-x86_64 -M pc -m 2G -accel kvm -cpu host -cdrom manjaro-kde-21.3.2-220704-linux515.iso` * `qemu-system-x86_64 -M q35 -m 2G -accel kvm -cpu host -cdrom manjaro-kde-21.3.2-220704-linux515.iso` * 'qemu-system-mips64el -M malta -kernel vmlinux-3.2.0-4-5kc-malta -hda debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=ttyS0"` Based-on: <20221120150550.63059-1-shen...@gmail.com> "[PATCH v2 0/3] Decouple INTx-to-LNKx routing from south bridges" v4: - Rebase onto "[PATCH v2 0/3] Decouple INTx-to-LNKx routing from south bridges" since it is already queued via mips-next. This eliminates patches 'hw/isa/piix3: Prefix pci_slot_get_pirq() with "piix3_"' and 'hw/isa/piix4: Prefix pci_slot_get_pirq() with "piix4_"'. - Squash 'hw/isa/piix: Drop the "3" from the PIIX base class' into 'hw/isa/piix3: Rename typedef PIIX3State to PIIXState'. I originally only split these patches since I wasn't sure whether renaming a type was allowed. - Add new patch 'hw/i386/pc_piix: Associate pci_map_irq_fn as soon as PCI bus is created' for forther cleanup of INTx-to-LNKx route decoupling. v3: - Introduce one TYPE_ICH9_USB_UHCI(fn) rather than several TYPE_ICH9_USB_UHCIx (Philippe) - Make proxy PIC generic (Philippe) - Track Malta's PIIX dependencies through KConfig - Rebase onto Philippe's 'hw/isa/piix4: Remove MIPS Malta specific bits' series [3] - Also rebase onto latest master to resolve merge conflicts. This required copying Philippe's series as first three patches - please ignore. v2: - Introduce TYPE_ defines for IDE and USB device models (Mark) - Omit unexporting of PIIXState (Mark) - Improve commit message of patch 5 to mention reset triggering through PCI configuration space (Mark) - Move reviewed patches w/o dependencies to the bottom of the series for early upstreaming [1] https://lists.nongnu.org/archive/html/qemu-devel/2022-07/msg02348.html [2] https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03310.html [3] https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg05367.html Bernhard Beschow (28): hw/mips/Kconfig: Track Malta's PIIX dependencies via Kconfig hw/usb/hcd-uhci: Introduce TYPE_ defines for device models hw/i386/pc_piix: Associate pci_map_irq_fn as soon as PCI bus is created hw/i386/pc_piix: Allow for setting properties before realizing PIIX3 south bridge hw/i386/pc: Create RTC controllers in south bridges hw/i386/pc: No need for rtc_state to be an out-parameter hw/isa/piix3: Create USB controller in host device hw/isa/piix3: Create power management controller in host device hw/intc/i8259: Make using the isa_pic singleton more type-safe hw/intc/i8259: Introduce i8259
Re: [PATCH v5 24/31] hw/isa/piix4: Reuse struct PIIXState from PIIX3
On 05/01/2023 14:32, Bernhard Beschow wrote:
Now that PIIX4 also uses the "proxy-pic", both implementations
Should "proxy-pic" be replaced with "isa-pic" (or even TYPE_ISA_PIC) here?
can share the same struct.
Signed-off-by: Bernhard Beschow
Reviewed-by: Michael S. Tsirkin
Message-Id: <20221022150508.26830-34-shen...@gmail.com>
---
hw/isa/piix4.c | 51 +++---
1 file changed, 15 insertions(+), 36 deletions(-)
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index eae4db0182..ce88377630 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -42,32 +42,10 @@
#include "sysemu/runstate.h"
#include "qom/object.h"
-struct PIIX4State {
-PCIDevice dev;
-
-ISAPICState pic;
-RTCState rtc;
-PCIIDEState ide;
-UHCIState uhci;
-PIIX4PMState pm;
-
-uint32_t smb_io_base;
-
-/* Reset Control Register */
-MemoryRegion rcr_mem;
-uint8_t rcr;
-
-bool has_acpi;
-bool has_usb;
-bool smm_enabled;
-};
-
-OBJECT_DECLARE_SIMPLE_TYPE(PIIX4State, PIIX4_PCI_DEVICE)
-
static void piix4_set_irq(void *opaque, int irq_num, int level)
{
int i, pic_irq, pic_level;
-PIIX4State *s = opaque;
+PIIXState *s = opaque;
PCIBus *bus = pci_get_bus(>dev);
/* now we change the pic irq level according to the piix irq mappings */
@@ -87,7 +65,7 @@ static void piix4_set_irq(void *opaque, int irq_num, int
level)
static void piix4_isa_reset(DeviceState *dev)
{
-PIIX4State *d = PIIX4_PCI_DEVICE(dev);
+PIIXState *d = PIIX_PCI_DEVICE(dev);
uint8_t *pci_conf = d->dev.config;
pci_conf[0x04] = 0x07; // master, memory and I/O
@@ -122,12 +100,13 @@ static void piix4_isa_reset(DeviceState *dev)
pci_conf[0xac] = 0x00;
pci_conf[0xae] = 0x00;
+d->pic_levels = 0; /* not used in PIIX4 */
d->rcr = 0;
}
static int piix4_post_load(void *opaque, int version_id)
{
-PIIX4State *s = opaque;
+PIIXState *s = opaque;
if (version_id == 2) {
s->rcr = 0;
@@ -142,8 +121,8 @@ static const VMStateDescription vmstate_piix4 = {
.minimum_version_id = 2,
.post_load = piix4_post_load,
.fields = (VMStateField[]) {
-VMSTATE_PCI_DEVICE(dev, PIIX4State),
-VMSTATE_UINT8_V(rcr, PIIX4State, 3),
+VMSTATE_PCI_DEVICE(dev, PIIXState),
+VMSTATE_UINT8_V(rcr, PIIXState, 3),
VMSTATE_END_OF_LIST()
}
};
@@ -151,7 +130,7 @@ static const VMStateDescription vmstate_piix4 = {
static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
unsigned int len)
{
-PIIX4State *s = opaque;
+PIIXState *s = opaque;
if (val & 4) {
qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
@@ -163,7 +142,7 @@ static void piix4_rcr_write(void *opaque, hwaddr addr,
uint64_t val,
static uint64_t piix4_rcr_read(void *opaque, hwaddr addr, unsigned int len)
{
-PIIX4State *s = opaque;
+PIIXState *s = opaque;
return s->rcr;
}
@@ -180,7 +159,7 @@ static const MemoryRegionOps piix4_rcr_ops = {
static void piix4_realize(PCIDevice *dev, Error **errp)
{
-PIIX4State *s = PIIX4_PCI_DEVICE(dev);
+PIIXState *s = PIIX_PCI_DEVICE(dev);
PCIBus *pci_bus = pci_get_bus(dev);
ISABus *isa_bus;
@@ -250,7 +229,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
static void piix4_init(Object *obj)
{
-PIIX4State *s = PIIX4_PCI_DEVICE(obj);
+PIIXState *s = PIIX_PCI_DEVICE(obj);
object_initialize_child(obj, "pic", >pic, TYPE_ISA_PIC);
object_initialize_child(obj, "rtc", >rtc, TYPE_MC146818_RTC);
@@ -258,10 +237,10 @@ static void piix4_init(Object *obj)
}
static Property piix4_props[] = {
-DEFINE_PROP_UINT32("smb_io_base", PIIX4State, smb_io_base, 0),
-DEFINE_PROP_BOOL("has-acpi", PIIX4State, has_acpi, true),
-DEFINE_PROP_BOOL("has-usb", PIIX4State, has_usb, true),
-DEFINE_PROP_BOOL("smm-enabled", PIIX4State, smm_enabled, false),
+DEFINE_PROP_UINT32("smb_io_base", PIIXState, smb_io_base, 0),
+DEFINE_PROP_BOOL("has-acpi", PIIXState, has_acpi, true),
+DEFINE_PROP_BOOL("has-usb", PIIXState, has_usb, true),
+DEFINE_PROP_BOOL("smm-enabled", PIIXState, smm_enabled, false),
DEFINE_PROP_END_OF_LIST(),
};
@@ -289,7 +268,7 @@ static void piix4_class_init(ObjectClass *klass, void *data)
static const TypeInfo piix4_info = {
.name = TYPE_PIIX4_PCI_DEVICE,
.parent= TYPE_PCI_DEVICE,
-.instance_size = sizeof(PIIX4State),
+.instance_size = sizeof(PIIXState),
.instance_init = piix4_init,
.class_init= piix4_class_init,
.interfaces = (InterfaceInfo[]) {
ATB,
Mark.
Re: [PATCH v5 22/31] hw/isa/piix4: Remove unused inbound ISA interrupt lines
On 05/01/2023 14:32, Bernhard Beschow wrote:
The Malta board, which is the only user of PIIX4, doesn't connect to the
exported interrupt lines. PIIX3 doesn't expose such intterupt lines
typo here: s/intterupt/interrupt/
either, so remove them for PIIX4 for simplicity and consistency.
Signed-off-by: Bernhard Beschow
Reviewed-by: Michael S. Tsirkin
Message-Id: <20221022150508.26830-32-shen...@gmail.com>
---
hw/isa/piix4.c | 8
1 file changed, 8 deletions(-)
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index de4133f573..9edaa5de3e 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -155,12 +155,6 @@ static void piix4_request_i8259_irq(void *opaque, int irq,
int level)
qemu_set_irq(s->cpu_intr, level);
}
-static void piix4_set_i8259_irq(void *opaque, int irq, int level)
-{
-PIIX4State *s = opaque;
-qemu_set_irq(s->isa[irq], level);
-}
-
static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
unsigned int len)
{
@@ -204,8 +198,6 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
return;
}
-qdev_init_gpio_in_named(DEVICE(dev), piix4_set_i8259_irq,
-"isa", ISA_NUM_IRQS);
qdev_init_gpio_out_named(DEVICE(dev), >cpu_intr,
"intr", 1);
ATB,
Mark.
Re: [PATCH v5 13/31] hw/intc/i8259: Introduce i8259 proxy "isa-pic"
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
Signed-off-by: Bernhard Beschow
---
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", >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
+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(_info);
+type_register_static(_info);
}
type_init(pic_register_types)
ATB,
Mark.
Re: [PATCH v5 12/31] hw/intc/i8259: Make using the isa_pic singleton more type-safe
On 05/01/2023 14:32, Bernhard Beschow wrote:
This even spares some casts in hot code paths along the way.
Signed-off-by: Bernhard Beschow
---
Note: The next patch will introduce a class "isa-pic", which is
shall not be confused with the isa_pic singleton.
---
include/hw/intc/i8259.h | 6 +++---
include/qemu/typedefs.h | 1 +
hw/intc/i8259.c | 11 ---
3 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/include/hw/intc/i8259.h b/include/hw/intc/i8259.h
index e2b1e8c59a..a0e34dd990 100644
--- a/include/hw/intc/i8259.h
+++ b/include/hw/intc/i8259.h
@@ -3,10 +3,10 @@
/* i8259.c */
-extern DeviceState *isa_pic;
+extern PICCommonState *isa_pic;
qemu_irq *i8259_init(ISABus *bus, qemu_irq parent_irq);
qemu_irq *kvm_i8259_init(ISABus *bus);
-int pic_get_output(DeviceState *d);
-int pic_read_irq(DeviceState *d);
+int pic_get_output(PICCommonState *s);
+int pic_read_irq(PICCommonState *s);
#endif
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 688408e048..3d5944d2a4 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -98,6 +98,7 @@ typedef struct PCIExpressDevice PCIExpressDevice;
typedef struct PCIExpressHost PCIExpressHost;
typedef struct PCIHostDeviceAddress PCIHostDeviceAddress;
typedef struct PCIHostState PCIHostState;
+typedef struct PICCommonState PICCommonState;
typedef struct PostcopyDiscardState PostcopyDiscardState;
typedef struct Property Property;
typedef struct PropertyInfo PropertyInfo;
diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
index cc4e21ffec..0261f087b2 100644
--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -55,7 +55,7 @@ struct PICClass {
#ifdef DEBUG_IRQ_LATENCY
static int64_t irq_time[16];
#endif
-DeviceState *isa_pic;
+PICCommonState *isa_pic;
static PICCommonState *slave_pic;
/* return the highest priority found in mask (highest = smallest
@@ -173,9 +173,8 @@ static void pic_intack(PICCommonState *s, int irq)
pic_update_irq(s);
}
-int pic_read_irq(DeviceState *d)
+int pic_read_irq(PICCommonState *s)
{
-PICCommonState *s = PIC_COMMON(d);
int irq, intno;
irq = pic_get_irq(s);
@@ -354,10 +353,8 @@ static uint64_t pic_ioport_read(void *opaque, hwaddr addr,
return ret;
}
-int pic_get_output(DeviceState *d)
+int pic_get_output(PICCommonState *s)
{
-PICCommonState *s = PIC_COMMON(d);
-
return (pic_get_irq(s) >= 0);
}
@@ -426,7 +423,7 @@ qemu_irq *i8259_init(ISABus *bus, qemu_irq parent_irq)
irq_set[i] = qdev_get_gpio_in(dev, i);
}
-isa_pic = dev;
+isa_pic = PIC_COMMON(dev);
isadev = i8259_init_chip(TYPE_I8259, bus, false);
dev = DEVICE(isadev);
I actually had a similar thought when I was looking at the i8259 code and it seems
sensible to me, so:
Reviewed-by: Mark Cave-Ayland
ATB,
Mark.
