Re: [Qemu-devel] [PATCH v1 00/13] q35 patches for pci tree

[Jason Baron <jbaron@redhat.com>]

On Wed, Oct 31, 2012 at 07:55:13AM -0500, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Tue, Oct 30, 2012 at 02:20:35PM -0500, Anthony Liguori wrote:
> >> Jason Baron <jbaron@redhat.com> writes:
> >> 
> >> > Hi,
> >> >
> >> > Re-base of my previous q35 patches on top of Michael Tsirkin's pci
> >> > tree.
> >> 
> >> I don't want this to come in through the pci tree.
> >
> > OK so you want to merge directly?
> 
> Yes.
> 
> >
> >> This is not just
> >> another PCI device and the ramifications are pretty big since this will
> >> become the main machine model.
> >
> > OTOH it's not going to be the main machine model in 1.3 so maybe they
> > are not that big, yet.
> 
> I think it's important to avoid introducing a lot of unmodelled code
> regardless of whether it's the main machine model or not.
> 
> There's plenty of time to fix it for 1.3 though so it shouldn't be a big deal.
> 
> Regards,
> 
> Anthony Liguori
> 
> >

I was looking at some of past pc refactoring patches, and they are
fairly involved: 

Subject: 	[Qemu-devel] [PATCH 0/6] refactor PC machine, i440fx and piix3 to take advantage of QOM
http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg04711.html

I'm not sure that level of modeling/refactoring can be safely be done in
1.3. I'm hoping you had something less invasive in mind.

It might be helpful to look at the core 'q35' data structures:


'Northbridge':

q35.h:

typedef struct GMCHPCIHost {
    PCIExpressHost      host;
    PCIDevice    *dev; //points to the GMCHPCIState
} GMCHPCIHost;

q35.h:

typedef struct GMCHPCIState {
    PCIDevice   d;
    /*
     * GMCH_PCIHost   *gmch_host;
     * In order to get GMCH_PCIHost
     *  PCIDevice -> qdev -> parent_bus -> qdev -upcast-> GMCH_PCIHost
     */
    MemoryRegion *ram_memory;
    MemoryRegion *pci_address_space;
    MemoryRegion *system_memory;
    PAMMemoryRegion pam_regions[13];
    MemoryRegion smram_region;
    MemoryRegion pci_hole;
    MemoryRegion pci_hole_64bit;
    uint8_t smm_enabled;
} GMCHPCIState;

So the memory controller is really the GMCHPCIState structure. We could
create a generic 'MemoryController' class and embed it in GMCHPCIState,
and then embed GMCHPCIState into GMCHPCIHost. Adding the PAM routines
to the 'MemoryController'.

Then when we initialize GMCHPCIHost, we can also initialize GMCHPCIState
and the 'MemoryController'. Next, set the user passed parameters and then
finishing initializing all 3 structures?


'SouthBride':

ich9.h:

typedef struct ICH9LPCState {
    /* ICH9 LPC PCI to ISA bridge */
    PCIDevice d;

    /* (pci device, intx) -> pirq
     * In real chipset case, the unused slots are never used
     * as ICH9 supports only D25-D32 irq routing.
     * On the other hand in qemu case, any slot/function can be
     * populated
     * via command line option.
     * So fallback interrupt routing for any devices in any slots is
     * necessary.
    */
    uint8_t irr[PCI_SLOT_MAX][PCI_NUM_PINS];

    APMState apm;
    ICH9LPCPMRegs pm;
    uint32_t sci_level; /* track sci level */

    /* 10.1 Chipset Configuration registers(Memory Space)
     which is pointed by RCBA */
    uint8_t chip_config[ICH9_CC_SIZE];
    /* isa bus */
    ISABus *isa_bus;
    MemoryRegion rbca_mem;

    qemu_irq *pic;
    qemu_irq *ioapic;
} ICH9LPCState;

hw/smbus_ich9.c:

typedef struct ICH9SMBState {
    PCIDevice dev;

    PMSMBus smb;
    MemoryRegion mem_bar;
} ICH9SMBState;

hw/ide/ahci.h:

typedef struct AHCIPCIState {
    PCIDevice card;
    AHCIState ahci;
} AHCIPCIState;


Here too, we probably could create a generic 'ICH9' device and embed the
LPC, SMB, and ahci controller in it. Initialize all the devices, set
the desired parameters, and finalize their creation.

That would still be a lot of code churn. Maybe you can be more specific
about what you're looking for?

Also, will this include refactoring i440fx/piix3? 

Thanks,

-Jason