Re: [Qemu-devel] [v4][PATCH 3/5] I440FX_PCI_DEVICE: add pci_type to index

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/8/12 16:54, Michael S. Tsirkin wrote:
> On Mon, Aug 11, 2014 at 10:50:48AM +0800, Chen, Tiejun wrote:
>> On 2014/8/11 4:27, Michael S. Tsirkin wrote:
>>> On Thu, Aug 07, 2014 at 09:40:49AM +0800, Chen, Tiejun wrote:
>>>>
>>>>
>>>> On 2014/8/7 5:07, Michael S. Tsirkin wrote:
>>>>> On Wed, Aug 06, 2014 at 06:17:02PM +0800, Chen, Tiejun wrote:
>>>>>> On 2014/8/6 17:45, Michael S. Tsirkin wrote:
>>>>>>> On Wed, Aug 06, 2014 at 02:50:33PM +0800, Tiejun Chen wrote:
>>>>>>>> We need to use this index to reuse this macro later
>>>>>>>>
>>>>>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>>>>>
>>>>>>> Which index?
>>>>>>> Most users don't need to change.
>>>>>>> Just open-code OBJECT_CHECK where necessary, or add
>>>>>>> a new wrapper.
>>>>>>
>>>>>> Okay so what about this?
>>>>>>
>>>>>>      hw:pci-host:piix: define I440FX_PCI_DEVICE_FROM_TYPE
>>>>>>
>>>>>>      We need to introduce I440FX_PCI_DEVICE_FROM_TYPE to get
>>>>>>      object with type, then we can reuse i440fx_init() simply.
>>>>>>
>>>>>>      Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>>>>
>>>>>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>>>>>> index 0cd82b8..8c74653 100644
>>>>>> --- a/hw/pci-host/piix.c
>>>>>> +++ b/hw/pci-host/piix.c
>>>>>> @@ -93,6 +93,9 @@ typedef struct PIIX3State {
>>>>>>   #define I440FX_PCI_DEVICE(obj) \
>>>>>>       OBJECT_CHECK(PCII440FXState, (obj), TYPE_I440FX_PCI_DEVICE)
>>>>>>
>>>>>> +#define I440FX_PCI_DEVICE_FROM_TYPE(obj, type) \
>>>>>> +    OBJECT_CHECK(PCII440FXState, (obj), type)
>>>>>
>>>>> This is just wrong. If you are casting to PCII440FXState,
>>>>
>>>> Why? We will have two different QOM typenames of PCII440FXStates.
>>>>
>>>>> there is no reason not to use TYPE_I440FX_PCI_DEVICE.
>>>>
>>>> As you know we already have this original,
>>>>
>>>> static const TypeInfo i440fx_info = {
>>>>      .name          = TYPE_I440FX_PCI_DEVICE,
>>>>      .parent        = TYPE_PCI_DEVICE,
>>>>      .instance_size = sizeof(PCII440FXState),
>>>>      .class_init    = i440fx_class_init,
>>>> };
>>>>
>>>> and in patch #4, we will register that new host bridge to IGD passthrough:
>>>>
>>>> static const TypeInfo xen_igd_passthrough_i440fx_info = {
>>>>      .name          = TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE,
>>>>      .parent        = TYPE_PCI_DEVICE,
>>>>      .instance_size = sizeof(PCII440FXState),
>>>>      .class_init    = xen_igd_passthrough_i440fx_class_init,
>>>> };
>>>
>>> My idea is to inherit TYPE_I440FX_PCI_DEVICE instead.
>>
>> So here, this mean xen_igd_passthrough_i440fx_info's parent should be
>> TYPE_I440FX_PCI_DEVICE, right?
>>
>>> Then you can reuse regular piix code which casts to
>>> TYPE_I440FX_PCI_DEVICE because
>>> TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE would
>>> be a subclass of TYPE_I440FX_PCI_DEVICE.
>>>
>>
>> So since then, we can reuse i440fx_initfn.
>>
>> As a summary, what we should do is like the following,
>>
>> xen:hw:pci-host:piix: create host bridge to passthrough
>>
>> Implement a pci host bridge specific to passthrough. Actually
>> this just inherits the standard one.
>>
>> This is based on http://patchwork.ozlabs.org/patch/363810/.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   hw/pci-host/piix.c   | 28 ++++++++++++++++++++++++++++
>>   include/hw/i386/pc.h |  2 ++
>>   2 files changed, 30 insertions(+)
>>
>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> index 0cd82b8..2ccd9ee 100644
>> --- a/hw/pci-host/piix.c
>> +++ b/hw/pci-host/piix.c
>> @@ -703,6 +703,33 @@ static const TypeInfo i440fx_info = {
>>       .class_init    = i440fx_class_init,
>>   };
>>
>> +static void xen_igd_passthrough_i440fx_class_init(ObjectClass *klass, void
>> *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> +
>> +    k->init = i440fx_initfn;
>> +    k->vendor_id = PCI_VENDOR_ID_INTEL;
>> +    k->device_id = PCI_DEVICE_ID_INTEL_82441;
>> +    k->revision = 0x02;
>> +    k->class_id = PCI_CLASS_BRIDGE_HOST;
>> +    dc->desc = "IGD PT XEN Host bridge";
>> +    dc->vmsd = &vmstate_i440fx;
>> +    /*
>> +     * PCI-facing part of the host bridge, not usable without the
>> +     * host-facing part, which can't be device_add'ed, yet.
>> +     */
>> +    dc->cannot_instantiate_with_device_add_yet = true;
>> +    dc->hotpluggable   = false;
>> +}
>> +
>
> A bunch of code still duplicated here. Only override what you have to.

You mean all stuffs can be inherited from its own parent, even including 
vendor_id/device_id?

If yes, currently the follows should be enough,

static void xen_igd_passthrough_i440fx_class_init(ObjectClass *klass, 
void *data)
{
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);

     dc->desc = "IGD PT XEN Host bridge";
}

We will add k->config_write/k->config_read here when we introduce IGD 
passthrough later.

Thanks
Tiejun

>
>
>> +static const TypeInfo xen_igd_passthrough_i440fx_info = {
>> +    .name          = TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE,
>> +    .parent        = TYPE_I440FX_PCI_DEVICE,
>> +    .instance_size = sizeof(PCII440FXState),
>> +    .class_init    = xen_igd_passthrough_i440fx_class_init,
>> +};
>> +
>>   static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
>>                                                   PCIBus *rootbus)
>>   {
>> @@ -744,6 +771,7 @@ static const TypeInfo i440fx_pcihost_info = {
>>   static void i440fx_register_types(void)
>>   {
>>       type_register_static(&i440fx_info);
>> +    type_register_static(&xen_igd_passthrough_i440fx_info);
>>       type_register_static(&piix3_info);
>>       type_register_static(&piix3_xen_info);
>>       type_register_static(&i440fx_pcihost_info);
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 11fb72f..de34aa6 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -238,6 +238,8 @@ typedef struct PCII440FXState PCII440FXState;
>>   #define TYPE_I440FX_PCI_HOST_BRIDGE "i440FX-pcihost"
>>   #define TYPE_I440FX_PCI_DEVICE "i440FX"
>>
>> +#define TYPE_XEN_IGD_PASSTHROUGH_I440FX_PCI_DEVICE
>> "xen-igd-passthrough-i440FX"
>> +
>>   PCIBus *i440fx_init(const char *host_type, const char *pci_type,
>>                       PCII440FXState **pi440fx_state, int *piix_devfn,
>>                       ISABus **isa_bus, qemu_irq *pic,
>> --
>> 1.9.1
>>
>> Thanks
>> Tiejun
>
>