From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55885) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XECRI-0008U8-8m for qemu-devel@nongnu.org; Mon, 04 Aug 2014 03:12:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XECRD-00019R-5v for qemu-devel@nongnu.org; Mon, 04 Aug 2014 03:12:12 -0400 Received: from mga01.intel.com ([192.55.52.88]:60784) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XECRC-000199-UZ for qemu-devel@nongnu.org; Mon, 04 Aug 2014 03:12:07 -0400 Message-ID: <53DF3235.3020108@intel.com> Date: Mon, 04 Aug 2014 15:11:49 +0800 From: "Chen, Tiejun" MIME-Version: 1.0 References: <1406788299-8394-1-git-send-email-tiejun.chen@intel.com> <1406788299-8394-3-git-send-email-tiejun.chen@intel.com> <20140731091050.GA31192@redhat.com> <53DA0BD1.9080509@intel.com> <20140731095328.GB31192@redhat.com> In-Reply-To: <20140731095328.GB31192@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: pbonzini@redhat.com, xen-devel@lists.xensource.com, qemu-devel@nongnu.org, stefano.stabellini@eu.citrix.com On 2014/7/31 17:53, Michael S. Tsirkin wrote: > On Thu, Jul 31, 2014 at 05:26:41PM +0800, Chen, Tiejun wrote: >> On 2014/7/31 17:10, Michael S. Tsirkin wrote: >>> On Thu, Jul 31, 2014 at 02:31:36PM +0800, Tiejun Chen wrote: >>>> We'd like to split i440fx_init and then we can share something >>>> with other stuff. >>>> >>>> Signed-off-by: Tiejun Chen >>> >>> I think this is too much work for very little benefit. >>> Just pass const char *type to i440fx_init. >> >> You know we will introduce that faked PCIe device represented that PCH >> later, > > Later when? On top of this patch series? Would like to see it all > before applying this ... > >> so how to distinguish them? Are you saying I should check the type >> like this? >> >> if(Xen-Type) >> {} >> else >> {} >> > > No! Put the code in init function for the respective class, > pass type as an argument: > > ---- > i440fx: make types configurable at run-time > > Xen wants to supply a different pci and host devices, > inheriting i440fx devices. Make types configurable. > > Signed-off-by: Michael S. Tsirkin Looks this is not enough as well since after we use patch #3 to introduce that host bridge, @@ -93,6 +93,9 @@ typedef struct PIIX3State { #define I440FX_PCI_DEVICE(obj) \ OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE) +#define XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE(obj) \ + OBJECT_CHECK(PCII440FXState, (obj), TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE) + struct PCII440FXState { /*< private >*/ PCIDevice parent_obj; Inside i440fx_init(), we still need to replace that macro, I440FX_PCI_DEVICE, with XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE in xenigd case. So looks we'd better split that i440fx_init() to address this point. Thanks Tiejun > > --> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index be8fdfe..86f295a 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -230,7 +230,11 @@ extern int no_hpet; > struct PCII440FXState; > typedef struct PCII440FXState PCII440FXState; > > -PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix_devfn, > +#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost" > +#define TYPE_I440FX_PCI_DEVICE "i440FX" > + > +PCIBus *i440fx_init(const char *host_type, const char *pci_type, > + PCII440FXState **pi440fx_state, int *piix_devfn, > ISABus **isa_bus, qemu_irq *pic, > MemoryRegion *address_space_mem, > MemoryRegion *address_space_io, > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 31125b7..e0979cd 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -194,7 +194,9 @@ static void pc_init1(MachineState *machine, > } > > if (pci_enabled) { > - pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi, > + pci_bus = i440fx_init(TYPE_I440FX_PCI_HOST_BRIDGE, > + TYPE_I440FX_PCI_DEVICE, > + &i440fx_state, &piix3_devfn, &isa_bus, gsi, > system_memory, system_io, machine->ram_size, > below_4g_mem_size, > above_4g_mem_size, > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c > index e0e0946..0cd82b8 100644 > --- a/hw/pci-host/piix.c > +++ b/hw/pci-host/piix.c > @@ -40,7 +40,6 @@ > * http://download.intel.com/design/chipsets/datashts/29054901.pdf > */ > > -#define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost" > #define I440FX_PCI_HOST_BRIDGE(obj) \ > OBJECT_CHECK(I440FXState, (obj), TYPE_I440FX_PCI_HOST_BRIDGE) > > @@ -91,7 +90,6 @@ typedef struct PIIX3State { > MemoryRegion rcr_mem; > } PIIX3State; > > -#define TYPE_I440FX_PCI_DEVICE "i440FX" > #define I440FX_PCI_DEVICE(obj) \ > OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE) > > @@ -305,7 +303,8 @@ static int i440fx_initfn(PCIDevice *dev) > return 0; > } > > -PCIBus *i440fx_init(PCII440FXState **pi440fx_state, > +PCIBus *i440fx_init(const char *host_type, const char *pci_type, > + PCII440FXState **pi440fx_state, > int *piix3_devfn, > ISABus **isa_bus, qemu_irq *pic, > MemoryRegion *address_space_mem, > @@ -325,7 +324,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, > unsigned i; > I440FXState *i440fx; > > - dev = qdev_create(NULL, TYPE_I440FX_PCI_HOST_BRIDGE); > + dev = qdev_create(NULL, host_type); > s = PCI_HOST_BRIDGE(dev); > b = pci_bus_new(dev, NULL, pci_address_space, > address_space_io, 0, TYPE_PCI_BUS); > @@ -333,7 +332,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, > object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL); > qdev_init_nofail(dev); > > - d = pci_create_simple(b, 0, TYPE_I440FX_PCI_DEVICE); > + d = pci_create_simple(b, 0, pci_type); > *pi440fx_state = I440FX_PCI_DEVICE(d); > f = *pi440fx_state; > f->system_memory = address_space_mem; > >