| Message ID | 20220901162613.6939-40-shentey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Consolidate PIIX south bridges | expand |
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.
Am 18. September 2022 20:21:09 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >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. So essentially I'd omit this patch from the series... >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? ... and instead add one which replaces DECLARE_INSTANCE_CHECKER with OBJECT_SIMPLE_TYPE here? Best regards, Bernhard > > >ATB, > >Mark.
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"
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(-)