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

["Chen, Tiejun" <tiejun.chen@intel.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 <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;
>
>