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 17:25, Chen, Tiejun wrote:
> 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.
>

Looks this is true after a quick test, so I will update this then send v5.

Thanks
Tiejun