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: Bernhard Beschow
Subject: Re: [PATCH 39/42] hw/isa/piix: Unexport PIIXState
Date: Sun, 18 Sep 2022 21:31:43 +0000

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.




reply via email to

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