Am 18. September 2022 20:21:09 UTC schrieb Mark Cave-Ayland
<mark.cave-ayl...@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 <shen...@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.
