qemu-devel
[Top][All Lists]
Advanced

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

Re: [PATCH 39/42] hw/isa/piix: Unexport PIIXState


From: Mark Cave-Ayland
Subject: Re: [PATCH 39/42] hw/isa/piix: Unexport PIIXState
Date: Sun, 18 Sep 2022 21:21:09 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0

On 01/09/2022 17:26, Bernhard Beschow wrote:

The - deliberately exported - components can still be accessed
via QOM properties.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
  hw/isa/piix.c                 | 52 +++++++++++++++++++++++++++++++++
  include/hw/southbridge/piix.h | 54 -----------------------------------
  2 files changed, 52 insertions(+), 54 deletions(-)

diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index e413d7e792..c503a6e836 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -26,20 +26,72 @@
  #include "qemu/osdep.h"
  #include "qemu/range.h"
  #include "qapi/error.h"
+#include "qom/object.h"
+#include "hw/acpi/piix4.h"
  #include "hw/dma/i8257.h"
+#include "hw/ide/pci.h"
  #include "hw/intc/i8259.h"
  #include "hw/southbridge/piix.h"
  #include "hw/timer/i8254.h"
  #include "hw/irq.h"
  #include "hw/qdev-properties.h"
  #include "hw/isa/isa.h"
+#include "hw/pci/pci.h"
+#include "hw/qdev-properties.h"
+#include "hw/rtc/mc146818rtc.h"
+#include "hw/usb/hcd-uhci.h"
  #include "hw/xen/xen.h"
  #include "sysemu/runstate.h"
  #include "migration/vmstate.h"
  #include "hw/acpi/acpi_aml_interface.h"
+#define PIIX_NUM_PIRQS 4ULL /* PIRQ[A-D] */
  #define XEN_PIIX_NUM_PIRQS      128ULL
+struct PIIXState {
+    PCIDevice dev;
+
+    /*
+     * bitmap to track pic levels.
+     * The pic level is the logical OR of all the PCI irqs mapped to it
+     * So one PIC level is tracked by PIIX_NUM_PIRQS bits.
+     *
+     * PIRQ is mapped to PIC pins, we track it by
+     * PIIX_NUM_PIRQS * ISA_NUM_IRQS = 64 bits with
+     * pic_irq * PIIX_NUM_PIRQS + pirq
+     */
+#if ISA_NUM_IRQS * PIIX_NUM_PIRQS > 64
+#error "unable to encode pic state in 64bit in pic_levels."
+#endif
+    uint64_t pic_levels;
+
+    /* This member isn't used. Just for save/load compatibility */
+    int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
+    uint8_t pci_irq_reset_mappings[PIIX_NUM_PIRQS];
+
+    ISAPICState pic;
+    RTCState rtc;
+    PCIIDEState ide;
+    UHCIState uhci;
+    PIIX4PMState pm;
+
+    uint32_t smb_io_base;
+
+    /* Reset Control Register contents */
+    uint8_t rcr;
+
+    /* IO memory region for Reset Control Register (PIIX_RCR_IOPORT) */
+    MemoryRegion rcr_mem;
+
+    bool has_acpi;
+    bool has_usb;
+    bool smm_enabled;
+};
+typedef struct PIIXState PIIXState;
+
+DECLARE_INSTANCE_CHECKER(PIIXState, PIIX_PCI_DEVICE,
+                         TYPE_PIIX3_PCI_DEVICE)
+
  static void piix_set_irq_pic(PIIXState *piix, int pic_irq)
  {
      qemu_set_irq(piix->pic.in_irqs[pic_irq],
diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
index c9fa0f1aa6..0edc23710c 100644
--- a/include/hw/southbridge/piix.h
+++ b/include/hw/southbridge/piix.h
@@ -12,14 +12,6 @@
  #ifndef HW_SOUTHBRIDGE_PIIX_H
  #define HW_SOUTHBRIDGE_PIIX_H
-#include "hw/pci/pci.h"
-#include "qom/object.h"
-#include "hw/acpi/piix4.h"
-#include "hw/ide/pci.h"
-#include "hw/intc/i8259.h"
-#include "hw/rtc/mc146818rtc.h"
-#include "hw/usb/hcd-uhci.h"
-
  /* PIRQRC[A:D]: PIRQx Route Control Registers */
  #define PIIX_PIRQCA 0x60
  #define PIIX_PIRQCB 0x61
@@ -32,53 +24,7 @@
   */
  #define PIIX_RCR_IOPORT 0xcf9
-#define PIIX_NUM_PIRQS 4ULL /* PIRQ[A-D] */
-
-struct PIIXState {
-    PCIDevice dev;
-
-    /*
-     * bitmap to track pic levels.
-     * The pic level is the logical OR of all the PCI irqs mapped to it
-     * So one PIC level is tracked by PIIX_NUM_PIRQS bits.
-     *
-     * PIRQ is mapped to PIC pins, we track it by
-     * PIIX_NUM_PIRQS * ISA_NUM_IRQS = 64 bits with
-     * pic_irq * PIIX_NUM_PIRQS + pirq
-     */
-#if ISA_NUM_IRQS * PIIX_NUM_PIRQS > 64
-#error "unable to encode pic state in 64bit in pic_levels."
-#endif
-    uint64_t pic_levels;
-
-    /* This member isn't used. Just for save/load compatibility */
-    int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
-    uint8_t pci_irq_reset_mappings[PIIX_NUM_PIRQS];
-
-    ISAPICState pic;
-    RTCState rtc;
-    PCIIDEState ide;
-    UHCIState uhci;
-    PIIX4PMState pm;
-
-    uint32_t smb_io_base;
-
-    /* Reset Control Register contents */
-    uint8_t rcr;
-
-    /* IO memory region for Reset Control Register (PIIX_RCR_IOPORT) */
-    MemoryRegion rcr_mem;
-
-    bool has_acpi;
-    bool has_usb;
-    bool smm_enabled;
-};
-typedef struct PIIXState PIIXState;
-
  #define TYPE_PIIX3_PCI_DEVICE "pci-piix3"
-DECLARE_INSTANCE_CHECKER(PIIXState, PIIX_PCI_DEVICE,
-                         TYPE_PIIX3_PCI_DEVICE)
-
  #define TYPE_PIIX3_DEVICE "PIIX3"
  #define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
  #define TYPE_PIIX4_PCI_DEVICE "piix4-isa"

I don't think that this is the right way to go here - whilst the definition of public and private can be a little vague, the general aim should be for the QOM type struct and macros to be in the corresponding .h file and the implementation in the .c file. In effect this ensures that anyone who wants to use a TYPE_FOO will include foo.h which helps make it easier to keep track of dependencies.

Looking at TYPE_PIIX3_PCI_DEVICE I'm wondering why this couldn't be OBJECT_SIMPLE_TYPE instead of DECLARE_INSTANCE_CHECKER with this series?


ATB,

Mark.



reply via email to

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