| Message ID | 20140731095328.GB31192@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <tiejun.chen@intel.com> >>> >>> 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. 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); 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 > > ---- > 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 <mst@redhat.com> > > --> > > 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; > >
On 2014/7/31 18:12, 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 <tiejun.chen@intel.com> >>>> >>>> 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. 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); > > 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? > I think I can do this inside that into xen_igd_passthrough_pc_machine_pci_bus_init(). There, we can just follow pci_bus = i440fx_init() since create_pseudo_pch_isa_bridge() just needs pci_bus as a parameter. Thanks Tiejun
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 <tiejun.chen@intel.com> > >>> > >>>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? > 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 <mst@redhat.com> > > > >--> > > > >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; > > > >
On Thu, Jul 31, 2014 at 08:10:53PM +0800, Chen, Tiejun wrote: > On 2014/7/31 18:12, 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 <tiejun.chen@intel.com> > >>>> > >>>>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. 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); > > > > 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? > > > > I think I can do this inside that into > xen_igd_passthrough_pc_machine_pci_bus_init(). There, we can just follow > pci_bus = i440fx_init() since create_pseudo_pch_isa_bridge() just needs > pci_bus as a parameter. > > Thanks > Tiejun Do it in xen_igd_i440fx_class_init: constructor for your class. This device is not hot-pluggable right? If so I would prefer not to make this xen specific. Instead, find the device and tweak the id when you initialize the card.
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 <tiejun.chen@intel.com> >>>>> >>>>> 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 <mst@redhat.com> >>> >>> --> >>> >>> 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; >>> >>> >
On 2014/7/31 23:47, Michael S. Tsirkin wrote: > On Thu, Jul 31, 2014 at 08:10:53PM +0800, Chen, Tiejun wrote: >> On 2014/7/31 18:12, 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 <tiejun.chen@intel.com> >>>>>> >>>>>> 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. 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); >>> >>> 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? >>> >> >> I think I can do this inside that into >> xen_igd_passthrough_pc_machine_pci_bus_init(). There, we can just follow >> pci_bus = i440fx_init() since create_pseudo_pch_isa_bridge() just needs >> pci_bus as a parameter. >> >> Thanks >> Tiejun > > Do it in xen_igd_i440fx_class_init: constructor for your class. > This device is not hot-pluggable right? > If so I would prefer not to make this xen specific. > Instead, find the device and tweak the id when you > initialize the card. Michael, I mean in v3 patches series to do something like, xen_igd_passthrough_pc_machine_pci_bus_init() { if (pci_enabled) { pci_bus = i440fx_init(); create_pseudo_pch_isa_bridge() } Thanks Tiejun
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 <tiejun.chen@intel.com> >>> >>> 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 <mst@redhat.com> 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; > >
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;