Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

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

On 2014/8/25 13:18, Chen, Tiejun wrote:
> On 2014/8/24 19:12, Michael S. Tsirkin wrote:
>> On Fri, Aug 22, 2014 at 09:06:20AM +0800, Chen, Tiejun wrote:
>>> On 2014/8/22 0:16, Konrad Rzeszutek Wilk wrote:
>>>> On Thu, Aug 21, 2014 at 09:28:28AM +0800, Tiejun Chen wrote:
>>>>> Currenjly this ISA bridge should be fixed at 1f.0, and pass the
>>>>
>>>> Currently
>>>
>>> Fixed.
>>>
>>>>
>>>>> real vendor/device ids as the driver expect.
>>>>
>>>> Could you add a bit more description to this patch please? Explain
>>>> the rationale, etc.
>>>
>>> So rephrase as follows:
>>>
>>> xen:i386:pc_piix: create isa bridge specific to IGD passthrough
>>>
>>> Currently IGD drivers always need to access PCH by 1f.0,
>>
>> OK
>>
>>> and
>>> identify PCH type with its own real vendor/device ids. This type
>>> value help driver initialize correctly.
>>
>> instead: PCH vendor/device id is used to identify the card.
>
> Okay.
>
>>
>>>>>
>>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>>> ---
>>>>>   hw/i386/pc_piix.c | 24 +++++++++++++++++++++++-
>>>>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>>> index 7710724..b131fa3 100644
>>>>> --- a/hw/i386/pc_piix.c
>>>>> +++ b/hw/i386/pc_piix.c
>>>>> @@ -50,7 +50,8 @@
>>>>>   #include "cpu.h"
>>>>>   #include "qemu/error-report.h"
>>>>>   #ifdef CONFIG_XEN
>>>>> -#  include <xen/hvm/hvm_info_table.h>
>>>>> +#include <xen/hvm/hvm_info_table.h>
>>>>> +#include <hw/xen/xen-host-pci-device.h>
>>>>>   #endif
>>>>>
>>>>>   #define MAX_IDE_BUS 2
>>>>> @@ -463,6 +464,26 @@ static void pc_xen_hvm_init(MachineState
>>>>> *machine)
>>>>>       }
>>>>>   }
>>>>>
>>>>> +static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
>>>>> +{
>>>>> +    struct PCIDevice *dev;
>>>>> +    XenHostPCIDevice hdev;
>>>>> +    int r = 0;
>>>>> +
>>>>> +    /* This shoudl be fixed at 1f.0 then pass vendor/device ids.
>>>>
>>>> should
>>>>
>>>> However I would remove the comment as it does not add anything extra
>>>> to the function. It is pretty clear what it is doing.
>>>>
>>>> What would help is if you said:
>>>>
>>>> Must be fixed at 1f.0 because .. bla blah
>>>
>>> Like the patch description, so what about this,
>>>
>>>      /* Currently IGD drivers always need to access PCH by 1f.0, and
>>>       * identify PCH type with its own real vendor/device ids.
>>>       */
>>>
>>> Thanks
>>> Tiejun
>>>
>>>>
>>>>> +     */
>>>>> +    dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
>>>>> +                            "xen-igd-passthrough-isa-bridge");
>>>>> +    if (dev) {
>>>>> +        r = xen_host_pci_device_get(&hdev, 0, 0, PCI_DEVFN(0x1f,
>>>>> 0), 0);
>>>>> +        if (!r) {
>>>>> +            pci_config_set_vendor_id(dev->config, hdev.vendor_id);
>>>>> +            pci_config_set_device_id(dev->config, hdev.device_id);
>>
>> Can you, instead, implement the reverse logic, probing
>> the card and supplying the correct device id for PCH?
>>
>
> Here what is your so-called reverse logic as I already asked you
> previously? Do you mean I should list all PCHs with a combo illustrated
> with the vendor/device id in advance? Then look up if we can find a

Michael,

Could you explain this exactly? Then I can try follow-up your idea ASAP 
if this is necessary and possible.

Thanks
Tiejun

> matched PCH? If yes, what is that benefit you expect in passthrough
> case? Shouldn't we pass these info to VM directly in passthrough case?
>
> Thanks
> Tiejun
>
>
>