[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [v2][PATCH 2/5] hw:pci-host:piix: split i440fx_init

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@xxxxxxxxx>

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 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");

        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?


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?


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@xxxxxxxxxx>


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,
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) \

@@ -91,7 +90,6 @@ typedef struct PIIX3State {
      MemoryRegion rcr_mem;
  } PIIX3State;

-#define TYPE_I440FX_PCI_DEVICE "i440FX"
  #define I440FX_PCI_DEVICE(obj) \

@@ -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), 

-    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;

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.