From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56717) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XD2gY-0008JT-04 for qemu-devel@nongnu.org; Thu, 31 Jul 2014 22:35:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XD2gR-0006En-W9 for qemu-devel@nongnu.org; Thu, 31 Jul 2014 22:35:09 -0400 Received: from mga01.intel.com ([192.55.52.88]:46923) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XD2gR-0006AJ-J5 for qemu-devel@nongnu.org; Thu, 31 Jul 2014 22:35:03 -0400 Message-ID: <53DAFCD4.5070103@intel.com> Date: Fri, 01 Aug 2014 10:35:00 +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> <53DA1690.6020905@intel.com> <20140731154417.GB3898@redhat.com> In-Reply-To: <20140731154417.GB3898@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 23:44, Michael S. Tsirkin wrote: > On Thu, Jul 31, 2014 at 06:12:32PM +0800, Chen, Tiejun wrote: >> 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 ... >> >> I will send this with other IGD stuff after this series is fine to you since >> its just creating a simple PCIe device. >> >> I think you should know this whole story since as you guys discussed we >> don't fix that PCH at 1f.0. > > > And this works with all legacy drivers and with windows > drivers? If we want to support these drivers, we still need to fix PCH at 1f.0. I think we can discuss this in detail when I send patch to address this point later. But now I don't think that block this patches series since this is about how to do *inside* that creating PCIe device subfunction. This is also why I try to the whole IGD patches, respectively since they always bring more comments or discussion, then it will be difficult to cover them at the same time by myself. I hope I can do this step by step address all your comments until you guys expect :) Thanks Tiejun > > >> So it may be like this, >> >> static int create_pseudo_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev) >> { >> >> struct PCIDevice *dev; >> >> char rid; >> >> /* We havt to use a simple PCI device to fake this ISA bridge >> * to avoid making some confusion to BIOS and ACPI. >> */ >> dev = pci_create(bus, -1, "pseudo-intel-pch-isa-bridge"); >> >> qdev_init_nofail(&dev->qdev); >> pci_config_set_vendor_id(dev->config, XEN_SUBSYSTEM_ID)); >> ^ >> I don't remember this exactly. >> pci_config_set_device_id(dev->config, hdev->device_id); > > This is a hack anyway, how about reverse-decoding > required device id from the supplied card instead of > poking at the host? > You can do it when card is initialized. > >> >> return 0; >> } >>> >>>> 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: >> >> If you mean we don't introduce any "if/else", I still don't understand how >> to insert such that function above, could you show this exactly? >> >> Tiejun > > Stick it in the constructor for your xen pt device. > >>> >>> ---- >>> 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 >>> >>> --> >>> >>> 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; >>> >>> >