diff mbox

[2/2] xen:i386:pc_piix: create isa bridge specific to IGD passthrough

Message ID 1408584508-5946-3-git-send-email-tiejun.chen@intel.com
State New
Headers show

Commit Message

Tiejun Chen Aug. 21, 2014, 1:28 a.m. UTC 
Currenjly this ISA bridge should be fixed at 1f.0, and pass the
real vendor/device ids as the driver expect.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 hw/i386/pc_piix.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

Comments

Konrad Rzeszutek Wilk Aug. 21, 2014, 4:16 p.m. UTC | #1 
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

> real vendor/device ids as the driver expect.

Could you add a bit more description to this patch please? Explain
the rationale, etc.
> 
> 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

> +     */
> +    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);
> +        } else
> +            fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
> +    }
> +}
> +
>  static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
>  {
>      PCIBus *bus;
> @@ -472,6 +493,7 @@ static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
>      bus = pci_find_primary_bus();
>      if (bus != NULL) {
>          pci_create_simple(bus, -1, "xen-platform");
> +        xen_igd_passthrough_isa_bridge_create(bus);
>      }
>  }
>  #endif
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
Tiejun Chen Aug. 22, 2014, 1:06 a.m. UTC | #2 
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, and
identify PCH type with its own real vendor/device ids. This type
value help driver initialize correctly.

>>
>> 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);
>> +        } else
>> +            fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
>> +    }
>> +}
>> +
>>   static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
>>   {
>>       PCIBus *bus;
>> @@ -472,6 +493,7 @@ static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
>>       bus = pci_find_primary_bus();
>>       if (bus != NULL) {
>>           pci_create_simple(bus, -1, "xen-platform");
>> +        xen_igd_passthrough_isa_bridge_create(bus);
>>       }
>>   }
>>   #endif
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
Michael S. Tsirkin Aug. 22, 2014, 6:23 p.m. UTC | #3 
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
> real vendor/device ids as the driver expect.
> 
> 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.
> +     */
> +    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);

No need to override vendor ID, it's intel anyway.

> +            pci_config_set_device_id(dev->config, hdev.device_id);

Please don't do this directly.
Instead, add a property specifying device ID.
Xen can then set this property in Xen code.


> +        } else
> +            fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
> +    }
> +}
> +
>  static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
>  {
>      PCIBus *bus;
> @@ -472,6 +493,7 @@ static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
>      bus = pci_find_primary_bus();
>      if (bus != NULL) {
>          pci_create_simple(bus, -1, "xen-platform");
> +        xen_igd_passthrough_isa_bridge_create(bus);
>      }
>  }
>  #endif
> -- 
> 1.9.1
Michael S. Tsirkin Aug. 24, 2014, 11:12 a.m. UTC | #4 
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.

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

> >>+        } else
> >>+            fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
> >>+    }
> >>+}
> >>+
> >>  static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
> >>  {
> >>      PCIBus *bus;
> >>@@ -472,6 +493,7 @@ static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
> >>      bus = pci_find_primary_bus();
> >>      if (bus != NULL) {
> >>          pci_create_simple(bus, -1, "xen-platform");
> >>+        xen_igd_passthrough_isa_bridge_create(bus);
> >>      }
> >>  }
> >>  #endif
> >>--
> >>1.9.1
> >>
> >>
> >>_______________________________________________
> >>Xen-devel mailing list
> >>Xen-devel@lists.xen.org
> >>http://lists.xen.org/xen-devel
> >
Tiejun Chen Aug. 25, 2014, 5:18 a.m. UTC | #5 
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 
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
Tiejun Chen Aug. 26, 2014, 2:49 a.m. UTC | #6 
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
>
>
>
Tiejun Chen Aug. 28, 2014, 12:56 a.m. UTC | #7 
>>>>>> +     */
>>>>>> +    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,
>

Ping.

Thanks
Tiejun

> 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
>>
>>
>>
>
>
>
Tiejun Chen Aug. 29, 2014, 1:28 a.m. UTC | #8 
On 2014/8/28 8:56, Chen, Tiejun wrote:
>>>>>>> +     */
>>>>>>> +    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,
>>
>
> Ping.
>
> Thanks
> Tiejun
>
>> Could you explain this exactly? Then I can try follow-up your idea ASAP
>> if this is necessary and possible.

Michel,

Could you give us some explanation for your "reverse logic" when you're 
free?

Thanks
Tiejun

>>
>> 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
>>>
>>>
>>>
>>
>>
>>
>
>
>
Michael S. Tsirkin Aug. 31, 2014, 8:58 a.m. UTC | #9 
On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
> 
> 
> On 2014/8/28 8:56, Chen, Tiejun wrote:
> >>>>>>>+     */
> >>>>>>>+    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,
> >>
> >
> >Ping.
> >
> >Thanks
> >Tiejun
> >
> >>Could you explain this exactly? Then I can try follow-up your idea ASAP
> >>if this is necessary and possible.
> 
> Michel,
> 
> Could you give us some explanation for your "reverse logic" when you're
> free?
> 
> Thanks
> Tiejun

So future drivers will look at device ID for the card
and figure out how things should work from there.
Old drivers still poke at device id of the chipset for this,
but maybe qemu can do what newer drivers do:
look at the card and figure out what guest should do,
then present the appropriate chipset id.

This is based on what Jesse said:
	Practically speaking, we could probably assume specific GPU/PCH combos,
	as I don't think they're generally mixed across generations, though SNB
	and IVB did have compatible sockets, so there is the possibility of
	mixing CPT and PPT PCHs, but those are register identical as far as the
	graphics driver is concerned, so even that should be safe.

so the idea is to have a reverse table mapping GPU back to PCH.
Present to guest the ID that will let it assume the correct GPU.

the problem with these tables is they are hard to keep up to date
as new hardware comes out, but as future hardware won't need
these hacks, we shall be fine.


> >>
> >>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
> >>>
> >>>
> >>>
> >>
> >>
> >>
> >
> >
> >
Tiejun Chen Sept. 1, 2014, 2:50 a.m. UTC | #10 
On 2014/8/31 16:58, Michael S. Tsirkin wrote:
> On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
>>
>>
>> On 2014/8/28 8:56, Chen, Tiejun wrote:
>>>>>>>>> +     */
>>>>>>>>> +    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,
>>>>
>>>
>>> Ping.
>>>
>>> Thanks
>>> Tiejun
>>>
>>>> Could you explain this exactly? Then I can try follow-up your idea ASAP
>>>> if this is necessary and possible.
>>
>> Michel,
>>
>> Could you give us some explanation for your "reverse logic" when you're
>> free?
>>
>> Thanks
>> Tiejun
>
> So future drivers will look at device ID for the card
> and figure out how things should work from there.
> Old drivers still poke at device id of the chipset for this,
> but maybe qemu can do what newer drivers do:
> look at the card and figure out what guest should do,
> then present the appropriate chipset id.
>
> This is based on what Jesse said:
> 	Practically speaking, we could probably assume specific GPU/PCH combos,
> 	as I don't think they're generally mixed across generations, though SNB
> 	and IVB did have compatible sockets, so there is the possibility of
> 	mixing CPT and PPT PCHs, but those are register identical as far as the
> 	graphics driver is concerned, so even that should be safe.
>

Michael,

Thanks for your explanation.

> so the idea is to have a reverse table mapping GPU back to PCH.
> Present to guest the ID that will let it assume the correct GPU.

I guess you mean we should create to maintain such a table:

[GPU Card: device_id(s), PCH: device_id]

Then each time, instead of exposing that real PCH device id directly, 
qemu first can get the real GPU device id to lookup this table to 
present a appropriate PCH's device id to the guest.

And looks here that appropriate PCH's device id is not always a that 
real PCH's device id. Right? If I'm wrong please correct me.

>
> the problem with these tables is they are hard to keep up to date

Yeah. But I think currently we can just start from some modern CPU such 
as HSW and BDW, then things could be easy.

Allen,

Any idea to this suggestion?

> as new hardware comes out, but as future hardware won't need
> these hacks, we shall be fine.

Yeah.

Thanks
Tiejun

>
>
>>>>
>>>> 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
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>
Michael S. Tsirkin Sept. 1, 2014, 6:05 a.m. UTC | #11 
On Mon, Sep 01, 2014 at 10:50:37AM +0800, Chen, Tiejun wrote:
> On 2014/8/31 16:58, Michael S. Tsirkin wrote:
> >On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
> >>
> >>
> >>On 2014/8/28 8:56, Chen, Tiejun wrote:
> >>>>>>>>>+     */
> >>>>>>>>>+    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,
> >>>>
> >>>
> >>>Ping.
> >>>
> >>>Thanks
> >>>Tiejun
> >>>
> >>>>Could you explain this exactly? Then I can try follow-up your idea ASAP
> >>>>if this is necessary and possible.
> >>
> >>Michel,
> >>
> >>Could you give us some explanation for your "reverse logic" when you're
> >>free?
> >>
> >>Thanks
> >>Tiejun
> >
> >So future drivers will look at device ID for the card
> >and figure out how things should work from there.
> >Old drivers still poke at device id of the chipset for this,
> >but maybe qemu can do what newer drivers do:
> >look at the card and figure out what guest should do,
> >then present the appropriate chipset id.
> >
> >This is based on what Jesse said:
> >	Practically speaking, we could probably assume specific GPU/PCH combos,
> >	as I don't think they're generally mixed across generations, though SNB
> >	and IVB did have compatible sockets, so there is the possibility of
> >	mixing CPT and PPT PCHs, but those are register identical as far as the
> >	graphics driver is concerned, so even that should be safe.
> >
> 
> Michael,
> 
> Thanks for your explanation.
> 
> >so the idea is to have a reverse table mapping GPU back to PCH.
> >Present to guest the ID that will let it assume the correct GPU.
> 
> I guess you mean we should create to maintain such a table:
> 
> [GPU Card: device_id(s), PCH: device_id]
> 
> Then each time, instead of exposing that real PCH device id directly, qemu
> first can get the real GPU device id to lookup this table to present a
> appropriate PCH's device id to the guest.
> 
> And looks here that appropriate PCH's device id is not always a that real
> PCH's device id. Right? If I'm wrong please correct me.

Exactly: we don't really care what the PCH ID is - we only need it
for the guest driver to do the right thing.

> >
> >the problem with these tables is they are hard to keep up to date
> 
> Yeah. But I think currently we can just start from some modern CPU such as
> HSW and BDW, then things could be easy.
> 
> Allen,
> 
> Any idea to this suggestion?
> 
> >as new hardware comes out, but as future hardware won't need
> >these hacks, we shall be fine.
> 
> Yeah.
> 
> Thanks
> Tiejun
> 
> >
> >
> >>>>
> >>>>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
> >>>>>
> >>>>>
> >>>>>
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>>
> >
Tiejun Chen Sept. 1, 2014, 7:49 a.m. UTC | #12 
On 2014/9/1 14:05, Michael S. Tsirkin wrote:
> On Mon, Sep 01, 2014 at 10:50:37AM +0800, Chen, Tiejun wrote:
>> On 2014/8/31 16:58, Michael S. Tsirkin wrote:
>>> On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
>>>>
>>>>
>>>> On 2014/8/28 8:56, Chen, Tiejun wrote:
>>>>>>>>>>> +     */
>>>>>>>>>>> +    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,
>>>>>>
>>>>>
>>>>> Ping.
>>>>>
>>>>> Thanks
>>>>> Tiejun
>>>>>
>>>>>> Could you explain this exactly? Then I can try follow-up your idea ASAP
>>>>>> if this is necessary and possible.
>>>>
>>>> Michel,
>>>>
>>>> Could you give us some explanation for your "reverse logic" when you're
>>>> free?
>>>>
>>>> Thanks
>>>> Tiejun
>>>
>>> So future drivers will look at device ID for the card
>>> and figure out how things should work from there.
>>> Old drivers still poke at device id of the chipset for this,
>>> but maybe qemu can do what newer drivers do:
>>> look at the card and figure out what guest should do,
>>> then present the appropriate chipset id.
>>>
>>> This is based on what Jesse said:
>>> 	Practically speaking, we could probably assume specific GPU/PCH combos,
>>> 	as I don't think they're generally mixed across generations, though SNB
>>> 	and IVB did have compatible sockets, so there is the possibility of
>>> 	mixing CPT and PPT PCHs, but those are register identical as far as the
>>> 	graphics driver is concerned, so even that should be safe.
>>>
>>
>> Michael,
>>
>> Thanks for your explanation.
>>
>>> so the idea is to have a reverse table mapping GPU back to PCH.
>>> Present to guest the ID that will let it assume the correct GPU.
>>
>> I guess you mean we should create to maintain such a table:
>>
>> [GPU Card: device_id(s), PCH: device_id]
>>
>> Then each time, instead of exposing that real PCH device id directly, qemu
>> first can get the real GPU device id to lookup this table to present a
>> appropriate PCH's device id to the guest.
>>
>> And looks here that appropriate PCH's device id is not always a that real
>> PCH's device id. Right? If I'm wrong please correct me.
>
> Exactly: we don't really care what the PCH ID is - we only need it
> for the guest driver to do the right thing.

Agreed.

I need to ask Allen to check if one given GPU card device id is always 
corresponding to one given PCH on both HSW and BDW currently. If yes, I 
can do this quickly. Otherwise I need some time to establish that sort 
of connection. 	

Thanks
Tiejun

>
>>>
>>> the problem with these tables is they are hard to keep up to date
>>
>> Yeah. But I think currently we can just start from some modern CPU such as
>> HSW and BDW, then things could be easy.
>>
>> Allen,
>>
>> Any idea to this suggestion?
>>
>>> as new hardware comes out, but as future hardware won't need
>>> these hacks, we shall be fine.
>>
>> Yeah.
>>
>> Thanks
>> Tiejun
>>
>>>
>>>
>>>>>>
>>>>>> 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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>
>
>
Kay, Allen M Sept. 3, 2014, 1:40 a.m. UTC | #13 
> -----Original Message-----
> From: Chen, Tiejun
> Sent: Monday, September 01, 2014 12:50 AM
> To: Michael S. Tsirkin
> Cc: xen-devel@lists.xensource.com; Kay, Allen M; qemu-devel@nongnu.org;
> Konrad Rzeszutek Wilk
> Subject: Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen:i386:pc_piix: create
> isa bridge specific to IGD passthrough
> 
> On 2014/9/1 14:05, Michael S. Tsirkin wrote:
> > On Mon, Sep 01, 2014 at 10:50:37AM +0800, Chen, Tiejun wrote:
> >> On 2014/8/31 16:58, Michael S. Tsirkin wrote:
> >>> On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
> >>>>
> >>>>
> >>>> On 2014/8/28 8:56, Chen, Tiejun wrote:
> >>>>>>>>>>> +     */
> >>>>>>>>>>> +    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,
> >>>>>>
> >>>>>
> >>>>> Ping.
> >>>>>
> >>>>> Thanks
> >>>>> Tiejun
> >>>>>
> >>>>>> Could you explain this exactly? Then I can try follow-up your
> >>>>>> idea ASAP if this is necessary and possible.
> >>>>
> >>>> Michel,
> >>>>
> >>>> Could you give us some explanation for your "reverse logic" when
> >>>> you're free?
> >>>>
> >>>> Thanks
> >>>> Tiejun
> >>>
> >>> So future drivers will look at device ID for the card and figure out
> >>> how things should work from there.
> >>> Old drivers still poke at device id of the chipset for this, but
> >>> maybe qemu can do what newer drivers do:
> >>> look at the card and figure out what guest should do, then present
> >>> the appropriate chipset id.
> >>>
> >>> This is based on what Jesse said:
> >>> 	Practically speaking, we could probably assume specific GPU/PCH
> combos,
> >>> 	as I don't think they're generally mixed across generations, though
> SNB
> >>> 	and IVB did have compatible sockets, so there is the possibility of
> >>> 	mixing CPT and PPT PCHs, but those are register identical as far as the
> >>> 	graphics driver is concerned, so even that should be safe.
> >>>
> >>
> >> Michael,
> >>
> >> Thanks for your explanation.
> >>
> >>> so the idea is to have a reverse table mapping GPU back to PCH.
> >>> Present to guest the ID that will let it assume the correct GPU.
> >>
> >> I guess you mean we should create to maintain such a table:
> >>
> >> [GPU Card: device_id(s), PCH: device_id]
> >>
> >> Then each time, instead of exposing that real PCH device id directly,
> >> qemu first can get the real GPU device id to lookup this table to
> >> present a appropriate PCH's device id to the guest.
> >>
> >> And looks here that appropriate PCH's device id is not always a that
> >> real PCH's device id. Right? If I'm wrong please correct me.
> >
> > Exactly: we don't really care what the PCH ID is - we only need it for
> > the guest driver to do the right thing.
> 
> Agreed.
> 
> I need to ask Allen to check if one given GPU card device id is always
> corresponding to one given PCH on both HSW and BDW currently. If yes, I can
> do this quickly. Otherwise I need some time to establish that sort
> of connection.
> 

If I understand this correctly, the only difference is instead of reading PCH DevID/RevID from the host hardware, QEMU inserts those values into PCH virtual device by looking at the reverse mapping table it maintains.

I agree the downside of doing this is the reverse mapping table may be hard to maintain.

What is the advantage of doing this instead of having QEMU reading it from the host?  Is it to test to make sure reverse mapping methods works before it is adopted in the new drivers?

> Thanks
> Tiejun
> 
> >
> >>>
> >>> the problem with these tables is they are hard to keep up to date
> >>
> >> Yeah. But I think currently we can just start from some modern CPU
> >> such as HSW and BDW, then things could be easy.
> >>
> >> Allen,
> >>
> >> Any idea to this suggestion?
> >>
> >>> as new hardware comes out, but as future hardware won't need these
> >>> hacks, we shall be fine.
> >>
> >> Yeah.
> >>
> >> Thanks
> >> Tiejun
> >>
> >>>
> >>>
> >>>>>>
> >>>>>> 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
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>
> >
> >

Allen
Michael S. Tsirkin Sept. 3, 2014, 6:27 a.m. UTC | #14 
On Wed, Sep 03, 2014 at 01:40:45AM +0000, Kay, Allen M wrote:
> 
> 
> > -----Original Message-----
> > From: Chen, Tiejun
> > Sent: Monday, September 01, 2014 12:50 AM
> > To: Michael S. Tsirkin
> > Cc: xen-devel@lists.xensource.com; Kay, Allen M; qemu-devel@nongnu.org;
> > Konrad Rzeszutek Wilk
> > Subject: Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen:i386:pc_piix: create
> > isa bridge specific to IGD passthrough
> > 
> > On 2014/9/1 14:05, Michael S. Tsirkin wrote:
> > > On Mon, Sep 01, 2014 at 10:50:37AM +0800, Chen, Tiejun wrote:
> > >> On 2014/8/31 16:58, Michael S. Tsirkin wrote:
> > >>> On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
> > >>>>
> > >>>>
> > >>>> On 2014/8/28 8:56, Chen, Tiejun wrote:
> > >>>>>>>>>>> +     */
> > >>>>>>>>>>> +    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,
> > >>>>>>
> > >>>>>
> > >>>>> Ping.
> > >>>>>
> > >>>>> Thanks
> > >>>>> Tiejun
> > >>>>>
> > >>>>>> Could you explain this exactly? Then I can try follow-up your
> > >>>>>> idea ASAP if this is necessary and possible.
> > >>>>
> > >>>> Michel,
> > >>>>
> > >>>> Could you give us some explanation for your "reverse logic" when
> > >>>> you're free?
> > >>>>
> > >>>> Thanks
> > >>>> Tiejun
> > >>>
> > >>> So future drivers will look at device ID for the card and figure out
> > >>> how things should work from there.
> > >>> Old drivers still poke at device id of the chipset for this, but
> > >>> maybe qemu can do what newer drivers do:
> > >>> look at the card and figure out what guest should do, then present
> > >>> the appropriate chipset id.
> > >>>
> > >>> This is based on what Jesse said:
> > >>> 	Practically speaking, we could probably assume specific GPU/PCH
> > combos,
> > >>> 	as I don't think they're generally mixed across generations, though
> > SNB
> > >>> 	and IVB did have compatible sockets, so there is the possibility of
> > >>> 	mixing CPT and PPT PCHs, but those are register identical as far as the
> > >>> 	graphics driver is concerned, so even that should be safe.
> > >>>
> > >>
> > >> Michael,
> > >>
> > >> Thanks for your explanation.
> > >>
> > >>> so the idea is to have a reverse table mapping GPU back to PCH.
> > >>> Present to guest the ID that will let it assume the correct GPU.
> > >>
> > >> I guess you mean we should create to maintain such a table:
> > >>
> > >> [GPU Card: device_id(s), PCH: device_id]
> > >>
> > >> Then each time, instead of exposing that real PCH device id directly,
> > >> qemu first can get the real GPU device id to lookup this table to
> > >> present a appropriate PCH's device id to the guest.
> > >>
> > >> And looks here that appropriate PCH's device id is not always a that
> > >> real PCH's device id. Right? If I'm wrong please correct me.
> > >
> > > Exactly: we don't really care what the PCH ID is - we only need it for
> > > the guest driver to do the right thing.
> > 
> > Agreed.
> > 
> > I need to ask Allen to check if one given GPU card device id is always
> > corresponding to one given PCH on both HSW and BDW currently. If yes, I can
> > do this quickly. Otherwise I need some time to establish that sort
> > of connection.
> > 
> 
> If I understand this correctly, the only difference is instead of reading PCH DevID/RevID from the host hardware, QEMU inserts those values into PCH virtual device by looking at the reverse mapping table it maintains.
> 
> I agree the downside of doing this is the reverse mapping table may be hard to maintain.

Point is it won't be, since future devices will not need this hack.
So we just need the table for the existing ones, build it
once and forget about it.

> What is the advantage of doing this instead of having QEMU reading it from the host?  Is it to test to make sure reverse mapping methods works before it is adopted in the new drivers?

That's one.
But a more important one is security: we don't want QEMU
to poke at any new files in the host if we can help it,
otherwise all distros have to change their LSM policies
to allow this. In particular when using kvm with VFIO, QEMU does not
poke at /sys/bus/pci/ at all anymore, and let's keep it that way.

> > Thanks
> > Tiejun
> > 
> > >
> > >>>
> > >>> the problem with these tables is they are hard to keep up to date
> > >>
> > >> Yeah. But I think currently we can just start from some modern CPU
> > >> such as HSW and BDW, then things could be easy.
> > >>
> > >> Allen,
> > >>
> > >> Any idea to this suggestion?
> > >>
> > >>> as new hardware comes out, but as future hardware won't need these
> > >>> hacks, we shall be fine.
> > >>
> > >> Yeah.
> > >>
> > >> Thanks
> > >> Tiejun
> > >>
> > >>>
> > >>>
> > >>>>>>
> > >>>>>> 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
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>
> > >
> > >
> 
> Allen
Kay, Allen M Sept. 3, 2014, 3:06 p.m. UTC | #15 
> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Tuesday, September 02, 2014 11:27 PM
> To: Kay, Allen M
> Cc: Chen, Tiejun; xen-devel@lists.xensource.com; qemu-
> devel@nongnu.org; Konrad Rzeszutek Wilk
> Subject: Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen:i386:pc_piix: create
> isa bridge specific to IGD passthrough
> 
> On Wed, Sep 03, 2014 at 01:40:45AM +0000, Kay, Allen M wrote:
> >
> >
> > > -----Original Message-----
> > > From: Chen, Tiejun
> > > Sent: Monday, September 01, 2014 12:50 AM
> > > To: Michael S. Tsirkin
> > > Cc: xen-devel@lists.xensource.com; Kay, Allen M;
> > > qemu-devel@nongnu.org; Konrad Rzeszutek Wilk
> > > Subject: Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen:i386:pc_piix:
> > > create isa bridge specific to IGD passthrough
> > >
> > > On 2014/9/1 14:05, Michael S. Tsirkin wrote:
> > > > On Mon, Sep 01, 2014 at 10:50:37AM +0800, Chen, Tiejun wrote:
> > > >> On 2014/8/31 16:58, Michael S. Tsirkin wrote:
> > > >>> On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
> > > >>>>
> > > >>>>
> > > >>>> On 2014/8/28 8:56, Chen, Tiejun wrote:
> > > >>>>>>>>>>> +     */
> > > >>>>>>>>>>> +    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,
> > > >>>>>>
> > > >>>>>
> > > >>>>> Ping.
> > > >>>>>
> > > >>>>> Thanks
> > > >>>>> Tiejun
> > > >>>>>
> > > >>>>>> Could you explain this exactly? Then I can try follow-up your
> > > >>>>>> idea ASAP if this is necessary and possible.
> > > >>>>
> > > >>>> Michel,
> > > >>>>
> > > >>>> Could you give us some explanation for your "reverse logic"
> > > >>>> when you're free?
> > > >>>>
> > > >>>> Thanks
> > > >>>> Tiejun
> > > >>>
> > > >>> So future drivers will look at device ID for the card and figure
> > > >>> out how things should work from there.
> > > >>> Old drivers still poke at device id of the chipset for this, but
> > > >>> maybe qemu can do what newer drivers do:
> > > >>> look at the card and figure out what guest should do, then
> > > >>> present the appropriate chipset id.
> > > >>>
> > > >>> This is based on what Jesse said:
> > > >>> 	Practically speaking, we could probably assume specific
> GPU/PCH
> > > combos,
> > > >>> 	as I don't think they're generally mixed across generations,
> > > >>> though
> > > SNB
> > > >>> 	and IVB did have compatible sockets, so there is the
> possibility of
> > > >>> 	mixing CPT and PPT PCHs, but those are register identical as
> far as the
> > > >>> 	graphics driver is concerned, so even that should be safe.
> > > >>>
> > > >>
> > > >> Michael,
> > > >>
> > > >> Thanks for your explanation.
> > > >>
> > > >>> so the idea is to have a reverse table mapping GPU back to PCH.
> > > >>> Present to guest the ID that will let it assume the correct GPU.
> > > >>
> > > >> I guess you mean we should create to maintain such a table:
> > > >>
> > > >> [GPU Card: device_id(s), PCH: device_id]
> > > >>
> > > >> Then each time, instead of exposing that real PCH device id
> > > >> directly, qemu first can get the real GPU device id to lookup
> > > >> this table to present a appropriate PCH's device id to the guest.
> > > >>
> > > >> And looks here that appropriate PCH's device id is not always a
> > > >> that real PCH's device id. Right? If I'm wrong please correct me.
> > > >
> > > > Exactly: we don't really care what the PCH ID is - we only need it
> > > > for the guest driver to do the right thing.
> > >
> > > Agreed.
> > >
> > > I need to ask Allen to check if one given GPU card device id is
> > > always corresponding to one given PCH on both HSW and BDW currently.
> > > If yes, I can do this quickly. Otherwise I need some time to
> > > establish that sort of connection.
> > >
> >
> > If I understand this correctly, the only difference is instead of reading PCH
> DevID/RevID from the host hardware, QEMU inserts those values into PCH
> virtual device by looking at the reverse mapping table it maintains.
> >
> > I agree the downside of doing this is the reverse mapping table may be
> hard to maintain.
> 
> Point is it won't be, since future devices will not need this hack.
> So we just need the table for the existing ones, build it once and forget
> about it.
> 

Agree.

> > What is the advantage of doing this instead of having QEMU reading it from
> the host?  Is it to test to make sure reverse mapping methods works before
> it is adopted in the new drivers?
> 
> That's one.
> But a more important one is security: we don't want QEMU to poke at any
> new files in the host if we can help it, otherwise all distros have to change
> their LSM policies to allow this. In particular when using kvm with VFIO,
> QEMU does not poke at /sys/bus/pci/ at all anymore, and let's keep it that
> way.
> 

OK.  I now see this is from KVM support perspective.  Thanks for the explanation.

> > > Thanks
> > > Tiejun
> > >
> > > >
> > > >>>
> > > >>> the problem with these tables is they are hard to keep up to
> > > >>> date
> > > >>
> > > >> Yeah. But I think currently we can just start from some modern
> > > >> CPU such as HSW and BDW, then things could be easy.
> > > >>
> > > >> Allen,
> > > >>
> > > >> Any idea to this suggestion?
> > > >>
> > > >>> as new hardware comes out, but as future hardware won't need
> > > >>> these hacks, we shall be fine.
> > > >>
> > > >> Yeah.
> > > >>
> > > >> Thanks
> > > >> Tiejun
> > > >>
> > > >>>
> > > >>>
> > > >>>>>>
> > > >>>>>> 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
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>
> > > >
> > > >
> >
> > Allen
Tiejun Chen Sept. 28, 2014, 2:59 a.m. UTC | #16 
On 2014/9/3 9:40, Kay, Allen M wrote:
>
>
>> -----Original Message-----
>> From: Chen, Tiejun
>> Sent: Monday, September 01, 2014 12:50 AM
>> To: Michael S. Tsirkin
>> Cc: xen-devel@lists.xensource.com; Kay, Allen M; qemu-devel@nongnu.org;
>> Konrad Rzeszutek Wilk
>> Subject: Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen:i386:pc_piix: create
>> isa bridge specific to IGD passthrough
>>
>> On 2014/9/1 14:05, Michael S. Tsirkin wrote:
>>> On Mon, Sep 01, 2014 at 10:50:37AM +0800, Chen, Tiejun wrote:
>>>> On 2014/8/31 16:58, Michael S. Tsirkin wrote:
>>>>> On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
>>>>>>
>>>>>>
>>>>>> On 2014/8/28 8:56, Chen, Tiejun wrote:
>>>>>>>>>>>>> +     */
>>>>>>>>>>>>> +    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,
>>>>>>>>
>>>>>>>
>>>>>>> Ping.
>>>>>>>
>>>>>>> Thanks
>>>>>>> Tiejun
>>>>>>>
>>>>>>>> Could you explain this exactly? Then I can try follow-up your
>>>>>>>> idea ASAP if this is necessary and possible.
>>>>>>
>>>>>> Michel,
>>>>>>
>>>>>> Could you give us some explanation for your "reverse logic" when
>>>>>> you're free?
>>>>>>
>>>>>> Thanks
>>>>>> Tiejun
>>>>>
>>>>> So future drivers will look at device ID for the card and figure out
>>>>> how things should work from there.
>>>>> Old drivers still poke at device id of the chipset for this, but
>>>>> maybe qemu can do what newer drivers do:
>>>>> look at the card and figure out what guest should do, then present
>>>>> the appropriate chipset id.
>>>>>
>>>>> This is based on what Jesse said:
>>>>> 	Practically speaking, we could probably assume specific GPU/PCH
>> combos,
>>>>> 	as I don't think they're generally mixed across generations, though
>> SNB
>>>>> 	and IVB did have compatible sockets, so there is the possibility of
>>>>> 	mixing CPT and PPT PCHs, but those are register identical as far as the
>>>>> 	graphics driver is concerned, so even that should be safe.
>>>>>
>>>>
>>>> Michael,
>>>>
>>>> Thanks for your explanation.
>>>>
>>>>> so the idea is to have a reverse table mapping GPU back to PCH.
>>>>> Present to guest the ID that will let it assume the correct GPU.
>>>>
>>>> I guess you mean we should create to maintain such a table:
>>>>
>>>> [GPU Card: device_id(s), PCH: device_id]
>>>>
>>>> Then each time, instead of exposing that real PCH device id directly,
>>>> qemu first can get the real GPU device id to lookup this table to
>>>> present a appropriate PCH's device id to the guest.
>>>>
>>>> And looks here that appropriate PCH's device id is not always a that
>>>> real PCH's device id. Right? If I'm wrong please correct me.
>>>
>>> Exactly: we don't really care what the PCH ID is - we only need it for
>>> the guest driver to do the right thing.
>>
>> Agreed.
>>
>> I need to ask Allen to check if one given GPU card device id is always
>> corresponding to one given PCH on both HSW and BDW currently. If yes, I can
>> do this quickly. Otherwise I need some time to establish that sort
>> of connection.

Michael,

Sorry for this delay response but please keep in mind we really are in 
this process.

You know this involve different GPU components we have to take some time 
to communicate or even discuss internal.

Now I have a draft codes, could you take a look at this? I hope that 
comment can help us to understand something, but if you have any 
question, we can further respond inline.

---
typedef struct {
     uint16_t gpu_device_id;
     uint16_t pch_device_id;
} XenIGDDeviceIDInfo;

/* In real world different GPU should have different PCH. But actually
  * the different PCH DIDs likely map to different PCH SKUs. We do the
  * same thing for the GPU. For PCH, the different SKUs are going to be
  * all the same silicon design and implementation, just different
  * features turn on and off with fuses. The SW interfaces should be
  * consistent across all SKUs in a given family (eg LPT). But just same
  * features may not be supported.
  *
  * Most of these different PCH features probably don't matter to the
  * Gfx driver, but obviously any difference in display port connections
  * will so it should be fine with any PCH in case of passthrough.
  *
  * So currently use one PCH version, 0x8c4e, to cover all HSW scenarios,
  * 0x9cc3 for BDW.
  */
static const XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
     /* HSW Classic */
     {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
     {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
     {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
     {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
     {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
     /* HSW ULT */
     {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
     {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
     {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
     {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
     {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
     {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
     /* HSW CRW */
     {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
     {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
     /* HSW Server */
     {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
     /* HSW SRVR */
     {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
     /* BSW */
     {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
     {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
     {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
     {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
     {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
     {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
     {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
     {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
     {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
     {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
     {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
};

static void xen_igd_passthrough_pciisabridge_get_pci_device_id(Object *obj,
                                                                Visitor *v,
                                                                void 
*opaque,
                                                                const 
char *name,
                                                                Error 
**errp)
{
     uint32_t value = 0;
     XenHostPCIDevice hdev;
     int r = 0, num;

     r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
     if (!r) {
         value = hdev.device_id;

         num = sizeof(xen_igd_combo_id_infos)/sizeof(uint16_t);
         for (r = 0; r < num; r++)
             if (value == xen_igd_combo_id_infos[r].gpu_device_id)
                 value = xen_igd_combo_id_infos[r].pch_device_id;
     }

     visit_type_uint32(v, &value, name, errp);
}

static void xen_igd_passthrough_isa_bridge_initfn(Object *obj)
{
     object_property_add(obj, "device-id", "int",
                         xen_igd_passthrough_pciisabridge_get_pci_device_id,
                         NULL, NULL, NULL, NULL);
}

Thanks
Tiejun


>>
>
> If I understand this correctly, the only difference is instead of reading PCH DevID/RevID from the host hardware, QEMU inserts those values into PCH virtual device by looking at the reverse mapping table it maintains.
>
> I agree the downside of doing this is the reverse mapping table may be hard to maintain.
>
> What is the advantage of doing this instead of having QEMU reading it from the host?  Is it to test to make sure reverse mapping methods works before it is adopted in the new drivers?
>
>> Thanks
>> Tiejun
>>
>>>
>>>>>
>>>>> the problem with these tables is they are hard to keep up to date
>>>>
>>>> Yeah. But I think currently we can just start from some modern CPU
>>>> such as HSW and BDW, then things could be easy.
>>>>
>>>> Allen,
>>>>
>>>> Any idea to this suggestion?
>>>>
>>>>> as new hardware comes out, but as future hardware won't need these
>>>>> hacks, we shall be fine.
>>>>
>>>> Yeah.
>>>>
>>>> Thanks
>>>> Tiejun
>>>>
>>>>>
>>>>>
>>>>>>>>
>>>>>>>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>
>>>
>
> Allen
>
>
Tiejun Chen Sept. 28, 2014, 5:38 a.m. UTC | #17 
On 2014/9/28 10:59, Chen, Tiejun wrote:
> On 2014/9/3 9:40, Kay, Allen M wrote:
>>
>>
>>> -----Original Message-----
>>> From: Chen, Tiejun
>>> Sent: Monday, September 01, 2014 12:50 AM
>>> To: Michael S. Tsirkin
>>> Cc: xen-devel@lists.xensource.com; Kay, Allen M; qemu-devel@nongnu.org;
>>> Konrad Rzeszutek Wilk
>>> Subject: Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen:i386:pc_piix:
>>> create
>>> isa bridge specific to IGD passthrough
>>>
>>> On 2014/9/1 14:05, Michael S. Tsirkin wrote:
>>>> On Mon, Sep 01, 2014 at 10:50:37AM +0800, Chen, Tiejun wrote:
>>>>> On 2014/8/31 16:58, Michael S. Tsirkin wrote:
>>>>>> On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2014/8/28 8:56, Chen, Tiejun wrote:
>>>>>>>>>>>>>> +     */
>>>>>>>>>>>>>> +    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,
>>>>>>>>>
>>>>>>>>
>>>>>>>> Ping.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Tiejun
>>>>>>>>
>>>>>>>>> Could you explain this exactly? Then I can try follow-up your
>>>>>>>>> idea ASAP if this is necessary and possible.
>>>>>>>
>>>>>>> Michel,
>>>>>>>
>>>>>>> Could you give us some explanation for your "reverse logic" when
>>>>>>> you're free?
>>>>>>>
>>>>>>> Thanks
>>>>>>> Tiejun
>>>>>>
>>>>>> So future drivers will look at device ID for the card and figure out
>>>>>> how things should work from there.
>>>>>> Old drivers still poke at device id of the chipset for this, but
>>>>>> maybe qemu can do what newer drivers do:
>>>>>> look at the card and figure out what guest should do, then present
>>>>>> the appropriate chipset id.
>>>>>>
>>>>>> This is based on what Jesse said:
>>>>>>     Practically speaking, we could probably assume specific GPU/PCH
>>> combos,
>>>>>>     as I don't think they're generally mixed across generations,
>>>>>> though
>>> SNB
>>>>>>     and IVB did have compatible sockets, so there is the
>>>>>> possibility of
>>>>>>     mixing CPT and PPT PCHs, but those are register identical as
>>>>>> far as the
>>>>>>     graphics driver is concerned, so even that should be safe.
>>>>>>
>>>>>
>>>>> Michael,
>>>>>
>>>>> Thanks for your explanation.
>>>>>
>>>>>> so the idea is to have a reverse table mapping GPU back to PCH.
>>>>>> Present to guest the ID that will let it assume the correct GPU.
>>>>>
>>>>> I guess you mean we should create to maintain such a table:
>>>>>
>>>>> [GPU Card: device_id(s), PCH: device_id]
>>>>>
>>>>> Then each time, instead of exposing that real PCH device id directly,
>>>>> qemu first can get the real GPU device id to lookup this table to
>>>>> present a appropriate PCH's device id to the guest.
>>>>>
>>>>> And looks here that appropriate PCH's device id is not always a that
>>>>> real PCH's device id. Right? If I'm wrong please correct me.
>>>>
>>>> Exactly: we don't really care what the PCH ID is - we only need it for
>>>> the guest driver to do the right thing.
>>>
>>> Agreed.
>>>
>>> I need to ask Allen to check if one given GPU card device id is always
>>> corresponding to one given PCH on both HSW and BDW currently. If yes,
>>> I can
>>> do this quickly. Otherwise I need some time to establish that sort
>>> of connection.
>
> Michael,
>
> Sorry for this delay response but please keep in mind we really are in
> this process.
>
> You know this involve different GPU components we have to take some time
> to communicate or even discuss internal.
>
> Now I have a draft codes, could you take a look at this? I hope that
> comment can help us to understand something, but if you have any
> question, we can further respond inline.
>
> ---
> typedef struct {
>      uint16_t gpu_device_id;
>      uint16_t pch_device_id;
> } XenIGDDeviceIDInfo;
>
> /* In real world different GPU should have different PCH. But actually
>   * the different PCH DIDs likely map to different PCH SKUs. We do the
>   * same thing for the GPU. For PCH, the different SKUs are going to be
>   * all the same silicon design and implementation, just different
>   * features turn on and off with fuses. The SW interfaces should be
>   * consistent across all SKUs in a given family (eg LPT). But just same
>   * features may not be supported.
>   *
>   * Most of these different PCH features probably don't matter to the
>   * Gfx driver, but obviously any difference in display port connections
>   * will so it should be fine with any PCH in case of passthrough.
>   *
>   * So currently use one PCH version, 0x8c4e, to cover all HSW scenarios,
>   * 0x9cc3 for BDW.
>   */
> static const XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
>      /* HSW Classic */
>      {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
>      {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
>      {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
>      {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
>      {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
>      /* HSW ULT */
>      {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
>      {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
>      {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
>      {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
>      {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
>      {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
>      /* HSW CRW */
>      {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
>      {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
>      /* HSW Server */
>      {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
>      /* HSW SRVR */
>      {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
>      /* BSW */
>      {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
>      {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
>      {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
>      {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
>      {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
>      {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
>      {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
>      {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
>      {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
>      {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
>      {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
> };
>
> static void xen_igd_passthrough_pciisabridge_get_pci_device_id(Object *obj,
>                                                                 Visitor *v,
>                                                                 void
> *opaque,
>                                                                 const
> char *name,
>                                                                 Error
> **errp)
> {
>      uint32_t value = 0;
>      XenHostPCIDevice hdev;
>      int r = 0, num;
>
>      r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
>      if (!r) {
>          value = hdev.device_id;
>
>          num = sizeof(xen_igd_combo_id_infos)/sizeof(uint16_t);

Sorry for this one typo:

sizeof(xen_igd_combo_id_infos)/sizeof(xen_igd_combo_id_infos[0]);

Tiejun

>          for (r = 0; r < num; r++)
>              if (value == xen_igd_combo_id_infos[r].gpu_device_id)
>                  value = xen_igd_combo_id_infos[r].pch_device_id;
>      }
>
>      visit_type_uint32(v, &value, name, errp);
> }
>
> static void xen_igd_passthrough_isa_bridge_initfn(Object *obj)
> {
>      object_property_add(obj, "device-id", "int",
>
> xen_igd_passthrough_pciisabridge_get_pci_device_id,
>                          NULL, NULL, NULL, NULL);
> }
>
> Thanks
> Tiejun
>
>
>>>
>>
>> If I understand this correctly, the only difference is instead of
>> reading PCH DevID/RevID from the host hardware, QEMU inserts those
>> values into PCH virtual device by looking at the reverse mapping table
>> it maintains.
>>
>> I agree the downside of doing this is the reverse mapping table may be
>> hard to maintain.
>>
>> What is the advantage of doing this instead of having QEMU reading it
>> from the host?  Is it to test to make sure reverse mapping methods
>> works before it is adopted in the new drivers?
>>
>>> Thanks
>>> Tiejun
>>>
>>>>
>>>>>>
>>>>>> the problem with these tables is they are hard to keep up to date
>>>>>
>>>>> Yeah. But I think currently we can just start from some modern CPU
>>>>> such as HSW and BDW, then things could be easy.
>>>>>
>>>>> Allen,
>>>>>
>>>>> Any idea to this suggestion?
>>>>>
>>>>>> as new hardware comes out, but as future hardware won't need these
>>>>>> hacks, we shall be fine.
>>>>>
>>>>> Yeah.
>>>>>
>>>>> Thanks
>>>>> Tiejun
>>>>>
>>>>>>
>>>>>>
>>>>>>>>>
>>>>>>>>> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>>
>>
>> Allen
>>
>>
>
>
>
Michael S. Tsirkin Sept. 29, 2014, 10:01 a.m. UTC | #18 
On Sun, Sep 28, 2014 at 10:59:05AM +0800, Chen, Tiejun wrote:
> On 2014/9/3 9:40, Kay, Allen M wrote:
> >
> >
> >>-----Original Message-----
> >>From: Chen, Tiejun
> >>Sent: Monday, September 01, 2014 12:50 AM
> >>To: Michael S. Tsirkin
> >>Cc: xen-devel@lists.xensource.com; Kay, Allen M; qemu-devel@nongnu.org;
> >>Konrad Rzeszutek Wilk
> >>Subject: Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen:i386:pc_piix: create
> >>isa bridge specific to IGD passthrough
> >>
> >>On 2014/9/1 14:05, Michael S. Tsirkin wrote:
> >>>On Mon, Sep 01, 2014 at 10:50:37AM +0800, Chen, Tiejun wrote:
> >>>>On 2014/8/31 16:58, Michael S. Tsirkin wrote:
> >>>>>On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
> >>>>>>
> >>>>>>
> >>>>>>On 2014/8/28 8:56, Chen, Tiejun wrote:
> >>>>>>>>>>>>>+     */
> >>>>>>>>>>>>>+    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,
> >>>>>>>>
> >>>>>>>
> >>>>>>>Ping.
> >>>>>>>
> >>>>>>>Thanks
> >>>>>>>Tiejun
> >>>>>>>
> >>>>>>>>Could you explain this exactly? Then I can try follow-up your
> >>>>>>>>idea ASAP if this is necessary and possible.
> >>>>>>
> >>>>>>Michel,
> >>>>>>
> >>>>>>Could you give us some explanation for your "reverse logic" when
> >>>>>>you're free?
> >>>>>>
> >>>>>>Thanks
> >>>>>>Tiejun
> >>>>>
> >>>>>So future drivers will look at device ID for the card and figure out
> >>>>>how things should work from there.
> >>>>>Old drivers still poke at device id of the chipset for this, but
> >>>>>maybe qemu can do what newer drivers do:
> >>>>>look at the card and figure out what guest should do, then present
> >>>>>the appropriate chipset id.
> >>>>>
> >>>>>This is based on what Jesse said:
> >>>>>	Practically speaking, we could probably assume specific GPU/PCH
> >>combos,
> >>>>>	as I don't think they're generally mixed across generations, though
> >>SNB
> >>>>>	and IVB did have compatible sockets, so there is the possibility of
> >>>>>	mixing CPT and PPT PCHs, but those are register identical as far as the
> >>>>>	graphics driver is concerned, so even that should be safe.
> >>>>>
> >>>>
> >>>>Michael,
> >>>>
> >>>>Thanks for your explanation.
> >>>>
> >>>>>so the idea is to have a reverse table mapping GPU back to PCH.
> >>>>>Present to guest the ID that will let it assume the correct GPU.
> >>>>
> >>>>I guess you mean we should create to maintain such a table:
> >>>>
> >>>>[GPU Card: device_id(s), PCH: device_id]
> >>>>
> >>>>Then each time, instead of exposing that real PCH device id directly,
> >>>>qemu first can get the real GPU device id to lookup this table to
> >>>>present a appropriate PCH's device id to the guest.
> >>>>
> >>>>And looks here that appropriate PCH's device id is not always a that
> >>>>real PCH's device id. Right? If I'm wrong please correct me.
> >>>
> >>>Exactly: we don't really care what the PCH ID is - we only need it for
> >>>the guest driver to do the right thing.
> >>
> >>Agreed.
> >>
> >>I need to ask Allen to check if one given GPU card device id is always
> >>corresponding to one given PCH on both HSW and BDW currently. If yes, I can
> >>do this quickly. Otherwise I need some time to establish that sort
> >>of connection.
> 
> Michael,
> 
> Sorry for this delay response but please keep in mind we really are in this
> process.
> 
> You know this involve different GPU components we have to take some time to
> communicate or even discuss internal.
> 
> Now I have a draft codes, could you take a look at this? I hope that comment
> can help us to understand something, but if you have any question, we can
> further respond inline.
> 
> ---
> typedef struct {
>     uint16_t gpu_device_id;
>     uint16_t pch_device_id;
> } XenIGDDeviceIDInfo;
> 
> /* In real world different GPU should have different PCH. But actually
>  * the different PCH DIDs likely map to different PCH SKUs. We do the
>  * same thing for the GPU. For PCH, the different SKUs are going to be
>  * all the same silicon design and implementation, just different
>  * features turn on and off with fuses. The SW interfaces should be
>  * consistent across all SKUs in a given family (eg LPT). But just same
>  * features may not be supported.
>  *
>  * Most of these different PCH features probably don't matter to the
>  * Gfx driver, but obviously any difference in display port connections
>  * will so it should be fine with any PCH in case of passthrough.
>  *
>  * So currently use one PCH version, 0x8c4e, to cover all HSW scenarios,
>  * 0x9cc3 for BDW.

Pls write Haswell and Broadwell in full in comment.

>  */
> static const XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
>     /* HSW Classic */
>     {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
>     {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
>     {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
>     {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
>     {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
>     /* HSW ULT */
>     {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
>     {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
>     {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
>     {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
>     {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
>     {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
>     /* HSW CRW */
>     {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
>     {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
>     /* HSW Server */
>     {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
>     /* HSW SRVR */
>     {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
>     /* BSW */
>     {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
>     {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
>     {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
>     {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
>     {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
>     {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
>     {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
>     {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
>     {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
>     {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
>     {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
> };
> 
> static void xen_igd_passthrough_pciisabridge_get_pci_device_id(Object *obj,
>                                                                Visitor *v,
>                                                                void *opaque,
>                                                                const char
> *name,
>                                                                Error **errp)
> {
>     uint32_t value = 0;
>     XenHostPCIDevice hdev;
>     int r = 0, num;
> 
>     r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
>     if (!r) {
>         value = hdev.device_id;
> 
>         num = sizeof(xen_igd_combo_id_infos)/sizeof(uint16_t);
>         for (r = 0; r < num; r++)
>             if (value == xen_igd_combo_id_infos[r].gpu_device_id)
>                 value = xen_igd_combo_id_infos[r].pch_device_id;

that's messy, I would use different variables for ID
of GPU and PCH.

>     }
> 
>     visit_type_uint32(v, &value, name, errp);


what I was suggesting is to start with the GPU device ID
(you can get it e.g. from the config array).
Use that to look up the PCH ID in xen_igd_combo_id_infos.
If there, override the PCH ID.

If not there, this is a new device so its driver will
not look at PCH at all, we can make it whatever or
skip it completely.

This seems to almost do this, however
- Why are you looking at host PCH device ID at all?
- Why don't you look at the GPU device ID?


> }
> 
> static void xen_igd_passthrough_isa_bridge_initfn(Object *obj)
> {
>     object_property_add(obj, "device-id", "int",
>                         xen_igd_passthrough_pciisabridge_get_pci_device_id,
>                         NULL, NULL, NULL, NULL);
> }

OK and what reads this property?

> Thanks
> Tiejun
> 
> 
> >>
> >
> >If I understand this correctly, the only difference is instead of reading PCH DevID/RevID from the host hardware, QEMU inserts those values into PCH virtual device by looking at the reverse mapping table it maintains.
> >
> >I agree the downside of doing this is the reverse mapping table may be hard to maintain.
> >
> >What is the advantage of doing this instead of having QEMU reading it from the host?  Is it to test to make sure reverse mapping methods works before it is adopted in the new drivers?
> >
> >>Thanks
> >>Tiejun
> >>
> >>>
> >>>>>
> >>>>>the problem with these tables is they are hard to keep up to date
> >>>>
> >>>>Yeah. But I think currently we can just start from some modern CPU
> >>>>such as HSW and BDW, then things could be easy.
> >>>>
> >>>>Allen,
> >>>>
> >>>>Any idea to this suggestion?
> >>>>
> >>>>>as new hardware comes out, but as future hardware won't need these
> >>>>>hacks, we shall be fine.
> >>>>
> >>>>Yeah.
> >>>>
> >>>>Thanks
> >>>>Tiejun
> >>>>
> >>>>>
> >>>>>
> >>>>>>>>
> >>>>>>>>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
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>
> >>>
> >>>
> >
> >Allen
> >
> >
Tiejun Chen Sept. 30, 2014, 2:43 a.m. UTC | #19 
On 2014/9/29 18:01, Michael S. Tsirkin wrote:
> On Sun, Sep 28, 2014 at 10:59:05AM +0800, Chen, Tiejun wrote:
>> On 2014/9/3 9:40, Kay, Allen M wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Chen, Tiejun
>>>> Sent: Monday, September 01, 2014 12:50 AM
>>>> To: Michael S. Tsirkin
>>>> Cc: xen-devel@lists.xensource.com; Kay, Allen M; qemu-devel@nongnu.org;
>>>> Konrad Rzeszutek Wilk
>>>> Subject: Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen:i386:pc_piix: create
>>>> isa bridge specific to IGD passthrough
>>>>
>>>> On 2014/9/1 14:05, Michael S. Tsirkin wrote:
>>>>> On Mon, Sep 01, 2014 at 10:50:37AM +0800, Chen, Tiejun wrote:
>>>>>> On 2014/8/31 16:58, Michael S. Tsirkin wrote:
>>>>>>> On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2014/8/28 8:56, Chen, Tiejun wrote:
>>>>>>>>>>>>>>> +     */
>>>>>>>>>>>>>>> +    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,
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Ping.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Tiejun
>>>>>>>>>
>>>>>>>>>> Could you explain this exactly? Then I can try follow-up your
>>>>>>>>>> idea ASAP if this is necessary and possible.
>>>>>>>>
>>>>>>>> Michel,
>>>>>>>>
>>>>>>>> Could you give us some explanation for your "reverse logic" when
>>>>>>>> you're free?
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Tiejun
>>>>>>>
>>>>>>> So future drivers will look at device ID for the card and figure out
>>>>>>> how things should work from there.
>>>>>>> Old drivers still poke at device id of the chipset for this, but
>>>>>>> maybe qemu can do what newer drivers do:
>>>>>>> look at the card and figure out what guest should do, then present
>>>>>>> the appropriate chipset id.
>>>>>>>
>>>>>>> This is based on what Jesse said:
>>>>>>> 	Practically speaking, we could probably assume specific GPU/PCH
>>>> combos,
>>>>>>> 	as I don't think they're generally mixed across generations, though
>>>> SNB
>>>>>>> 	and IVB did have compatible sockets, so there is the possibility of
>>>>>>> 	mixing CPT and PPT PCHs, but those are register identical as far as the
>>>>>>> 	graphics driver is concerned, so even that should be safe.
>>>>>>>
>>>>>>
>>>>>> Michael,
>>>>>>
>>>>>> Thanks for your explanation.
>>>>>>
>>>>>>> so the idea is to have a reverse table mapping GPU back to PCH.
>>>>>>> Present to guest the ID that will let it assume the correct GPU.
>>>>>>
>>>>>> I guess you mean we should create to maintain such a table:
>>>>>>
>>>>>> [GPU Card: device_id(s), PCH: device_id]
>>>>>>
>>>>>> Then each time, instead of exposing that real PCH device id directly,
>>>>>> qemu first can get the real GPU device id to lookup this table to
>>>>>> present a appropriate PCH's device id to the guest.
>>>>>>
>>>>>> And looks here that appropriate PCH's device id is not always a that
>>>>>> real PCH's device id. Right? If I'm wrong please correct me.
>>>>>
>>>>> Exactly: we don't really care what the PCH ID is - we only need it for
>>>>> the guest driver to do the right thing.
>>>>
>>>> Agreed.
>>>>
>>>> I need to ask Allen to check if one given GPU card device id is always
>>>> corresponding to one given PCH on both HSW and BDW currently. If yes, I can
>>>> do this quickly. Otherwise I need some time to establish that sort
>>>> of connection.
>>
>> Michael,
>>
>> Sorry for this delay response but please keep in mind we really are in this
>> process.
>>
>> You know this involve different GPU components we have to take some time to
>> communicate or even discuss internal.
>>
>> Now I have a draft codes, could you take a look at this? I hope that comment
>> can help us to understand something, but if you have any question, we can
>> further respond inline.
>>
>> ---
>> typedef struct {
>>      uint16_t gpu_device_id;
>>      uint16_t pch_device_id;
>> } XenIGDDeviceIDInfo;
>>
>> /* In real world different GPU should have different PCH. But actually
>>   * the different PCH DIDs likely map to different PCH SKUs. We do the
>>   * same thing for the GPU. For PCH, the different SKUs are going to be
>>   * all the same silicon design and implementation, just different
>>   * features turn on and off with fuses. The SW interfaces should be
>>   * consistent across all SKUs in a given family (eg LPT). But just same
>>   * features may not be supported.
>>   *
>>   * Most of these different PCH features probably don't matter to the
>>   * Gfx driver, but obviously any difference in display port connections
>>   * will so it should be fine with any PCH in case of passthrough.
>>   *
>>   * So currently use one PCH version, 0x8c4e, to cover all HSW scenarios,
>>   * 0x9cc3 for BDW.
>
> Pls write Haswell and Broadwell in full in comment.

Are you saying I should list all PCH device ids both of HSW and BDW here 
like this format?

HSWGT1D: 0xABCD
...

>
>>   */
>> static const XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
>>      /* HSW Classic */
>>      {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
>>      {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
>>      {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
>>      {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
>>      {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
>>      /* HSW ULT */
>>      {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
>>      {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
>>      {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
>>      {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
>>      {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
>>      {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
>>      /* HSW CRW */
>>      {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
>>      {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
>>      /* HSW Server */
>>      {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
>>      /* HSW SRVR */
>>      {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
>>      /* BSW */
>>      {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
>>      {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
>>      {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
>>      {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
>>      {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
>>      {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
>>      {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
>>      {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
>>      {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
>>      {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
>>      {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
>> };
>>
>> static void xen_igd_passthrough_pciisabridge_get_pci_device_id(Object *obj,
>>                                                                 Visitor *v,
>>                                                                 void *opaque,
>>                                                                 const char
>> *name,
>>                                                                 Error **errp)
>> {
>>      uint32_t value = 0;
>>      XenHostPCIDevice hdev;
>>      int r = 0, num;
>>
>>      r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
>>      if (!r) {
>>          value = hdev.device_id;
>>
>>          num = sizeof(xen_igd_combo_id_infos)/sizeof(uint16_t);
>>          for (r = 0; r < num; r++)
>>              if (value == xen_igd_combo_id_infos[r].gpu_device_id)
>>                  value = xen_igd_combo_id_infos[r].pch_device_id;
>
> that's messy, I would use different variables for ID
> of GPU and PCH.

         for (r = 0; r < num; r++)
             if (gpu_id == xen_igd_combo_id_infos[r].gpu_device_id)
                 pch_id = xen_igd_combo_id_infos[r].pch_device_id;

>
>>      }
>>
>>      visit_type_uint32(v, &value, name, errp);
>
>
> what I was suggesting is to start with the GPU device ID
> (you can get it e.g. from the config array).

I think current codes is doing this:

     r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
     if (!r) {
         gpu_id = hdev.device_id;

Here xen_host_pci_device_get() is a wrapper of reading pci sysfs.

> Use that to look up the PCH ID in xen_igd_combo_id_infos.
> If there, override the PCH ID.
>
> If not there, this is a new device so its driver will
> not look at PCH at all, we can make it whatever or
> skip it completely.

I think we should return one initialized value, 0xffff, since often this 
represents an invalid PCI device.

>
> This seems to almost do this, however
> - Why are you looking at host PCH device ID at all?
> - Why don't you look at the GPU device ID?

We still fix this bridge at 1f.0, and current our implementation can 
cover our requirement and safe. I means this bridge should not be used 
for other use cases, so if its still be accessed we mightn't take care 
of them, right?

>
>
>> }
>>
>> static void xen_igd_passthrough_isa_bridge_initfn(Object *obj)
>> {
>>      object_property_add(obj, "device-id", "int",
>>                          xen_igd_passthrough_pciisabridge_get_pci_device_id,
>>                          NULL, NULL, NULL, NULL);
>> }
>
> OK and what reads this property?

In sequent patch I will do something like this,

@@ -464,6 +464,32 @@ static void pc_xen_hvm_init(MachineState *machine)
      }
  }

+static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
+{
+    struct PCIDevice *dev;
+    Error *local_err = NULL;
+    uint16_t device_id = 0xffff;
+
+    /* Currently IGD drivers always need to access PCH by 1f.0. */
+    dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
+                            "xen-igd-passthrough-isa-bridge");
+
+    /* Identify PCH card with its own real vendor/device ids.
+     * Here that vendor id is always PCI_VENDOR_ID_INTEL.
+     */
+    if (dev) {
+        device_id = (uint16_t)object_property_get_int(OBJECT(dev),
+                                                      "device-id",
+                                                      &local_err);
+        if ((!local_err) && (device_id != 0xffff)) {
+            pci_config_set_device_id(dev->config, device_id);
+            return;
+        }
+    }
+
+    fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
+}
+
  static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
  {
      PCIBus *bus;
@@ -473,6 +499,7 @@ static void 
xen_igd_passthrough_pc_hvm_init(MachineState *machine)
      bus = pci_find_primary_bus();
      if (bus != NULL) {
          pci_create_simple(bus, -1, "xen-platform");
+        xen_igd_passthrough_isa_bridge_create(bus);
      }
  }
  #endif

Tiejun

>
>> Thanks
>> Tiejun
>>
>>
>>>>
>>>
>>> If I understand this correctly, the only difference is instead of reading PCH DevID/RevID from the host hardware, QEMU inserts those values into PCH virtual device by looking at the reverse mapping table it maintains.
>>>
>>> I agree the downside of doing this is the reverse mapping table may be hard to maintain.
>>>
>>> What is the advantage of doing this instead of having QEMU reading it from the host?  Is it to test to make sure reverse mapping methods works before it is adopted in the new drivers?
>>>
>>>> Thanks
>>>> Tiejun
>>>>
>>>>>
>>>>>>>
>>>>>>> the problem with these tables is they are hard to keep up to date
>>>>>>
>>>>>> Yeah. But I think currently we can just start from some modern CPU
>>>>>> such as HSW and BDW, then things could be easy.
>>>>>>
>>>>>> Allen,
>>>>>>
>>>>>> Any idea to this suggestion?
>>>>>>
>>>>>>> as new hardware comes out, but as future hardware won't need these
>>>>>>> hacks, we shall be fine.
>>>>>>
>>>>>> Yeah.
>>>>>>
>>>>>> Thanks
>>>>>> Tiejun
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>> Allen
>>>
>>>
>
Tiejun Chen Oct. 7, 2014, 1:02 a.m. UTC | #20 
Ping...

Thanks
Tiejun

On 2014/9/30 10:43, Chen, Tiejun wrote:
> On 2014/9/29 18:01, Michael S. Tsirkin wrote:
>> On Sun, Sep 28, 2014 at 10:59:05AM +0800, Chen, Tiejun wrote:
>>> On 2014/9/3 9:40, Kay, Allen M wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Chen, Tiejun
>>>>> Sent: Monday, September 01, 2014 12:50 AM
>>>>> To: Michael S. Tsirkin
>>>>> Cc: xen-devel@lists.xensource.com; Kay, Allen M;
>>>>> qemu-devel@nongnu.org;
>>>>> Konrad Rzeszutek Wilk
>>>>> Subject: Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen:i386:pc_piix:
>>>>> create
>>>>> isa bridge specific to IGD passthrough
>>>>>
>>>>> On 2014/9/1 14:05, Michael S. Tsirkin wrote:
>>>>>> On Mon, Sep 01, 2014 at 10:50:37AM +0800, Chen, Tiejun wrote:
>>>>>>> On 2014/8/31 16:58, Michael S. Tsirkin wrote:
>>>>>>>> On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2014/8/28 8:56, Chen, Tiejun wrote:
>>>>>>>>>>>>>>>> +     */
>>>>>>>>>>>>>>>> +    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,
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Ping.
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> Tiejun
>>>>>>>>>>
>>>>>>>>>>> Could you explain this exactly? Then I can try follow-up your
>>>>>>>>>>> idea ASAP if this is necessary and possible.
>>>>>>>>>
>>>>>>>>> Michel,
>>>>>>>>>
>>>>>>>>> Could you give us some explanation for your "reverse logic" when
>>>>>>>>> you're free?
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Tiejun
>>>>>>>>
>>>>>>>> So future drivers will look at device ID for the card and figure
>>>>>>>> out
>>>>>>>> how things should work from there.
>>>>>>>> Old drivers still poke at device id of the chipset for this, but
>>>>>>>> maybe qemu can do what newer drivers do:
>>>>>>>> look at the card and figure out what guest should do, then present
>>>>>>>> the appropriate chipset id.
>>>>>>>>
>>>>>>>> This is based on what Jesse said:
>>>>>>>>     Practically speaking, we could probably assume specific GPU/PCH
>>>>> combos,
>>>>>>>>     as I don't think they're generally mixed across generations,
>>>>>>>> though
>>>>> SNB
>>>>>>>>     and IVB did have compatible sockets, so there is the
>>>>>>>> possibility of
>>>>>>>>     mixing CPT and PPT PCHs, but those are register identical as
>>>>>>>> far as the
>>>>>>>>     graphics driver is concerned, so even that should be safe.
>>>>>>>>
>>>>>>>
>>>>>>> Michael,
>>>>>>>
>>>>>>> Thanks for your explanation.
>>>>>>>
>>>>>>>> so the idea is to have a reverse table mapping GPU back to PCH.
>>>>>>>> Present to guest the ID that will let it assume the correct GPU.
>>>>>>>
>>>>>>> I guess you mean we should create to maintain such a table:
>>>>>>>
>>>>>>> [GPU Card: device_id(s), PCH: device_id]
>>>>>>>
>>>>>>> Then each time, instead of exposing that real PCH device id
>>>>>>> directly,
>>>>>>> qemu first can get the real GPU device id to lookup this table to
>>>>>>> present a appropriate PCH's device id to the guest.
>>>>>>>
>>>>>>> And looks here that appropriate PCH's device id is not always a that
>>>>>>> real PCH's device id. Right? If I'm wrong please correct me.
>>>>>>
>>>>>> Exactly: we don't really care what the PCH ID is - we only need it
>>>>>> for
>>>>>> the guest driver to do the right thing.
>>>>>
>>>>> Agreed.
>>>>>
>>>>> I need to ask Allen to check if one given GPU card device id is always
>>>>> corresponding to one given PCH on both HSW and BDW currently. If
>>>>> yes, I can
>>>>> do this quickly. Otherwise I need some time to establish that sort
>>>>> of connection.
>>>
>>> Michael,
>>>
>>> Sorry for this delay response but please keep in mind we really are
>>> in this
>>> process.
>>>
>>> You know this involve different GPU components we have to take some
>>> time to
>>> communicate or even discuss internal.
>>>
>>> Now I have a draft codes, could you take a look at this? I hope that
>>> comment
>>> can help us to understand something, but if you have any question, we
>>> can
>>> further respond inline.
>>>
>>> ---
>>> typedef struct {
>>>      uint16_t gpu_device_id;
>>>      uint16_t pch_device_id;
>>> } XenIGDDeviceIDInfo;
>>>
>>> /* In real world different GPU should have different PCH. But actually
>>>   * the different PCH DIDs likely map to different PCH SKUs. We do the
>>>   * same thing for the GPU. For PCH, the different SKUs are going to be
>>>   * all the same silicon design and implementation, just different
>>>   * features turn on and off with fuses. The SW interfaces should be
>>>   * consistent across all SKUs in a given family (eg LPT). But just same
>>>   * features may not be supported.
>>>   *
>>>   * Most of these different PCH features probably don't matter to the
>>>   * Gfx driver, but obviously any difference in display port connections
>>>   * will so it should be fine with any PCH in case of passthrough.
>>>   *
>>>   * So currently use one PCH version, 0x8c4e, to cover all HSW
>>> scenarios,
>>>   * 0x9cc3 for BDW.
>>
>> Pls write Haswell and Broadwell in full in comment.
>
> Are you saying I should list all PCH device ids both of HSW and BDW here
> like this format?
>
> HSWGT1D: 0xABCD
> ...
>
>>
>>>   */
>>> static const XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
>>>      /* HSW Classic */
>>>      {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
>>>      {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
>>>      {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
>>>      {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
>>>      {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
>>>      /* HSW ULT */
>>>      {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
>>>      {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
>>>      {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
>>>      {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
>>>      {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
>>>      {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
>>>      /* HSW CRW */
>>>      {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
>>>      {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
>>>      /* HSW Server */
>>>      {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
>>>      /* HSW SRVR */
>>>      {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
>>>      /* BSW */
>>>      {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
>>>      {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
>>>      {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
>>>      {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
>>>      {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
>>>      {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
>>>      {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
>>>      {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
>>>      {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
>>>      {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
>>>      {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
>>> };
>>>
>>> static void xen_igd_passthrough_pciisabridge_get_pci_device_id(Object
>>> *obj,
>>>
>>> Visitor *v,
>>>                                                                 void
>>> *opaque,
>>>                                                                 const
>>> char
>>> *name,
>>>                                                                 Error
>>> **errp)
>>> {
>>>      uint32_t value = 0;
>>>      XenHostPCIDevice hdev;
>>>      int r = 0, num;
>>>
>>>      r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
>>>      if (!r) {
>>>          value = hdev.device_id;
>>>
>>>          num = sizeof(xen_igd_combo_id_infos)/sizeof(uint16_t);
>>>          for (r = 0; r < num; r++)
>>>              if (value == xen_igd_combo_id_infos[r].gpu_device_id)
>>>                  value = xen_igd_combo_id_infos[r].pch_device_id;
>>
>> that's messy, I would use different variables for ID
>> of GPU and PCH.
>
>          for (r = 0; r < num; r++)
>              if (gpu_id == xen_igd_combo_id_infos[r].gpu_device_id)
>                  pch_id = xen_igd_combo_id_infos[r].pch_device_id;
>
>>
>>>      }
>>>
>>>      visit_type_uint32(v, &value, name, errp);
>>
>>
>> what I was suggesting is to start with the GPU device ID
>> (you can get it e.g. from the config array).
>
> I think current codes is doing this:
>
>      r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
>      if (!r) {
>          gpu_id = hdev.device_id;
>
> Here xen_host_pci_device_get() is a wrapper of reading pci sysfs.
>
>> Use that to look up the PCH ID in xen_igd_combo_id_infos.
>> If there, override the PCH ID.
>>
>> If not there, this is a new device so its driver will
>> not look at PCH at all, we can make it whatever or
>> skip it completely.
>
> I think we should return one initialized value, 0xffff, since often this
> represents an invalid PCI device.
>
>>
>> This seems to almost do this, however
>> - Why are you looking at host PCH device ID at all?
>> - Why don't you look at the GPU device ID?
>
> We still fix this bridge at 1f.0, and current our implementation can
> cover our requirement and safe. I means this bridge should not be used
> for other use cases, so if its still be accessed we mightn't take care
> of them, right?
>
>>
>>
>>> }
>>>
>>> static void xen_igd_passthrough_isa_bridge_initfn(Object *obj)
>>> {
>>>      object_property_add(obj, "device-id", "int",
>>>
>>> xen_igd_passthrough_pciisabridge_get_pci_device_id,
>>>                          NULL, NULL, NULL, NULL);
>>> }
>>
>> OK and what reads this property?
>
> In sequent patch I will do something like this,
>
> @@ -464,6 +464,32 @@ static void pc_xen_hvm_init(MachineState *machine)
>       }
>   }
>
> +static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
> +{
> +    struct PCIDevice *dev;
> +    Error *local_err = NULL;
> +    uint16_t device_id = 0xffff;
> +
> +    /* Currently IGD drivers always need to access PCH by 1f.0. */
> +    dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
> +                            "xen-igd-passthrough-isa-bridge");
> +
> +    /* Identify PCH card with its own real vendor/device ids.
> +     * Here that vendor id is always PCI_VENDOR_ID_INTEL.
> +     */
> +    if (dev) {
> +        device_id = (uint16_t)object_property_get_int(OBJECT(dev),
> +                                                      "device-id",
> +                                                      &local_err);
> +        if ((!local_err) && (device_id != 0xffff)) {
> +            pci_config_set_device_id(dev->config, device_id);
> +            return;
> +        }
> +    }
> +
> +    fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
> +}
> +
>   static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
>   {
>       PCIBus *bus;
> @@ -473,6 +499,7 @@ static void
> xen_igd_passthrough_pc_hvm_init(MachineState *machine)
>       bus = pci_find_primary_bus();
>       if (bus != NULL) {
>           pci_create_simple(bus, -1, "xen-platform");
> +        xen_igd_passthrough_isa_bridge_create(bus);
>       }
>   }
>   #endif
>
> Tiejun
>
>>
>>> Thanks
>>> Tiejun
>>>
>>>
>>>>>
>>>>
>>>> If I understand this correctly, the only difference is instead of
>>>> reading PCH DevID/RevID from the host hardware, QEMU inserts those
>>>> values into PCH virtual device by looking at the reverse mapping
>>>> table it maintains.
>>>>
>>>> I agree the downside of doing this is the reverse mapping table may
>>>> be hard to maintain.
>>>>
>>>> What is the advantage of doing this instead of having QEMU reading
>>>> it from the host?  Is it to test to make sure reverse mapping
>>>> methods works before it is adopted in the new drivers?
>>>>
>>>>> Thanks
>>>>> Tiejun
>>>>>
>>>>>>
>>>>>>>>
>>>>>>>> the problem with these tables is they are hard to keep up to date
>>>>>>>
>>>>>>> Yeah. But I think currently we can just start from some modern CPU
>>>>>>> such as HSW and BDW, then things could be easy.
>>>>>>>
>>>>>>> Allen,
>>>>>>>
>>>>>>> Any idea to this suggestion?
>>>>>>>
>>>>>>>> as new hardware comes out, but as future hardware won't need these
>>>>>>>> hacks, we shall be fine.
>>>>>>>
>>>>>>> Yeah.
>>>>>>>
>>>>>>> Thanks
>>>>>>> Tiejun
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 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
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>> Allen
>>>>
>>>>
>>
>
>
>
Michael S. Tsirkin Oct. 7, 2014, 7:26 a.m. UTC | #21 
On Tue, Sep 30, 2014 at 10:43:09AM +0800, Chen, Tiejun wrote:
> On 2014/9/29 18:01, Michael S. Tsirkin wrote:
> >On Sun, Sep 28, 2014 at 10:59:05AM +0800, Chen, Tiejun wrote:
> >>On 2014/9/3 9:40, Kay, Allen M wrote:
> >>>
> >>>
> >>>>-----Original Message-----
> >>>>From: Chen, Tiejun
> >>>>Sent: Monday, September 01, 2014 12:50 AM
> >>>>To: Michael S. Tsirkin
> >>>>Cc: xen-devel@lists.xensource.com; Kay, Allen M; qemu-devel@nongnu.org;
> >>>>Konrad Rzeszutek Wilk
> >>>>Subject: Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen:i386:pc_piix: create
> >>>>isa bridge specific to IGD passthrough
> >>>>
> >>>>On 2014/9/1 14:05, Michael S. Tsirkin wrote:
> >>>>>On Mon, Sep 01, 2014 at 10:50:37AM +0800, Chen, Tiejun wrote:
> >>>>>>On 2014/8/31 16:58, Michael S. Tsirkin wrote:
> >>>>>>>On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>On 2014/8/28 8:56, Chen, Tiejun wrote:
> >>>>>>>>>>>>>>>+     */
> >>>>>>>>>>>>>>>+    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,
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>Ping.
> >>>>>>>>>
> >>>>>>>>>Thanks
> >>>>>>>>>Tiejun
> >>>>>>>>>
> >>>>>>>>>>Could you explain this exactly? Then I can try follow-up your
> >>>>>>>>>>idea ASAP if this is necessary and possible.
> >>>>>>>>
> >>>>>>>>Michel,
> >>>>>>>>
> >>>>>>>>Could you give us some explanation for your "reverse logic" when
> >>>>>>>>you're free?
> >>>>>>>>
> >>>>>>>>Thanks
> >>>>>>>>Tiejun
> >>>>>>>
> >>>>>>>So future drivers will look at device ID for the card and figure out
> >>>>>>>how things should work from there.
> >>>>>>>Old drivers still poke at device id of the chipset for this, but
> >>>>>>>maybe qemu can do what newer drivers do:
> >>>>>>>look at the card and figure out what guest should do, then present
> >>>>>>>the appropriate chipset id.
> >>>>>>>
> >>>>>>>This is based on what Jesse said:
> >>>>>>>	Practically speaking, we could probably assume specific GPU/PCH
> >>>>combos,
> >>>>>>>	as I don't think they're generally mixed across generations, though
> >>>>SNB
> >>>>>>>	and IVB did have compatible sockets, so there is the possibility of
> >>>>>>>	mixing CPT and PPT PCHs, but those are register identical as far as the
> >>>>>>>	graphics driver is concerned, so even that should be safe.
> >>>>>>>
> >>>>>>
> >>>>>>Michael,
> >>>>>>
> >>>>>>Thanks for your explanation.
> >>>>>>
> >>>>>>>so the idea is to have a reverse table mapping GPU back to PCH.
> >>>>>>>Present to guest the ID that will let it assume the correct GPU.
> >>>>>>
> >>>>>>I guess you mean we should create to maintain such a table:
> >>>>>>
> >>>>>>[GPU Card: device_id(s), PCH: device_id]
> >>>>>>
> >>>>>>Then each time, instead of exposing that real PCH device id directly,
> >>>>>>qemu first can get the real GPU device id to lookup this table to
> >>>>>>present a appropriate PCH's device id to the guest.
> >>>>>>
> >>>>>>And looks here that appropriate PCH's device id is not always a that
> >>>>>>real PCH's device id. Right? If I'm wrong please correct me.
> >>>>>
> >>>>>Exactly: we don't really care what the PCH ID is - we only need it for
> >>>>>the guest driver to do the right thing.
> >>>>
> >>>>Agreed.
> >>>>
> >>>>I need to ask Allen to check if one given GPU card device id is always
> >>>>corresponding to one given PCH on both HSW and BDW currently. If yes, I can
> >>>>do this quickly. Otherwise I need some time to establish that sort
> >>>>of connection.
> >>
> >>Michael,
> >>
> >>Sorry for this delay response but please keep in mind we really are in this
> >>process.
> >>
> >>You know this involve different GPU components we have to take some time to
> >>communicate or even discuss internal.
> >>
> >>Now I have a draft codes, could you take a look at this? I hope that comment
> >>can help us to understand something, but if you have any question, we can
> >>further respond inline.
> >>
> >>---
> >>typedef struct {
> >>     uint16_t gpu_device_id;
> >>     uint16_t pch_device_id;
> >>} XenIGDDeviceIDInfo;
> >>
> >>/* In real world different GPU should have different PCH. But actually
> >>  * the different PCH DIDs likely map to different PCH SKUs. We do the
> >>  * same thing for the GPU. For PCH, the different SKUs are going to be
> >>  * all the same silicon design and implementation, just different
> >>  * features turn on and off with fuses. The SW interfaces should be
> >>  * consistent across all SKUs in a given family (eg LPT). But just same
> >>  * features may not be supported.
> >>  *
> >>  * Most of these different PCH features probably don't matter to the
> >>  * Gfx driver, but obviously any difference in display port connections
> >>  * will so it should be fine with any PCH in case of passthrough.
> >>  *
> >>  * So currently use one PCH version, 0x8c4e, to cover all HSW scenarios,
> >>  * 0x9cc3 for BDW.
> >
> >Pls write Haswell and Broadwell in full in comment.
> 
> Are you saying I should list all PCH device ids both of HSW and BDW here
> like this format?
> 
> HSWGT1D: 0xABCD
> ...

I am merely saying don't say "HSW, BDW" in comments. Say "Haswell,
Broadwell, or if you like HWS(Haswell), BDW(Broadwell).

> >
> >>  */
> >>static const XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
> >>     /* HSW Classic */
> >>     {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
> >>     {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
> >>     {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
> >>     {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
> >>     {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
> >>     /* HSW ULT */
> >>     {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
> >>     {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
> >>     {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
> >>     {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
> >>     {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
> >>     {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
> >>     /* HSW CRW */
> >>     {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
> >>     {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
> >>     /* HSW Server */
> >>     {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
> >>     /* HSW SRVR */
> >>     {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
> >>     /* BSW */
> >>     {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
> >>     {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
> >>     {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
> >>     {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
> >>     {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
> >>     {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
> >>     {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
> >>     {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
> >>     {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
> >>     {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
> >>     {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
> >>};
> >>
> >>static void xen_igd_passthrough_pciisabridge_get_pci_device_id(Object *obj,
> >>                                                                Visitor *v,
> >>                                                                void *opaque,
> >>                                                                const char
> >>*name,
> >>                                                                Error **errp)
> >>{
> >>     uint32_t value = 0;
> >>     XenHostPCIDevice hdev;
> >>     int r = 0, num;
> >>
> >>     r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
> >>     if (!r) {
> >>         value = hdev.device_id;
> >>
> >>         num = sizeof(xen_igd_combo_id_infos)/sizeof(uint16_t);
> >>         for (r = 0; r < num; r++)
> >>             if (value == xen_igd_combo_id_infos[r].gpu_device_id)
> >>                 value = xen_igd_combo_id_infos[r].pch_device_id;
> >
> >that's messy, I would use different variables for ID
> >of GPU and PCH.
> 
>         for (r = 0; r < num; r++)
>             if (gpu_id == xen_igd_combo_id_infos[r].gpu_device_id)
>                 pch_id = xen_igd_combo_id_infos[r].pch_device_id;

yea

> >
> >>     }
> >>
> >>     visit_type_uint32(v, &value, name, errp);
> >
> >
> >what I was suggesting is to start with the GPU device ID
> >(you can get it e.g. from the config array).
> 
> I think current codes is doing this:
> 
>     r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
>     if (!r) {
>         gpu_id = hdev.device_id;
> 
> Here xen_host_pci_device_get() is a wrapper of reading pci sysfs.


Do you want the device ID of the GPU?
So please don't poke at it directly in sysfs.
Instead, find the GPU's PCIDevice and do
	gpu->config_read();
or just get from gpu->config.

> >Use that to look up the PCH ID in xen_igd_combo_id_infos.
> >If there, override the PCH ID.
> >
> >If not there, this is a new device so its driver will
> >not look at PCH at all, we can make it whatever or
> >skip it completely.
> 
> I think we should return one initialized value, 0xffff, since often this
> represents an invalid PCI device.

I'm not sure you really want an invalid PCI device,
would be nicer not to have a PCH device there at all.


> >
> >This seems to almost do this, however
> >- Why are you looking at host PCH device ID at all?
> >- Why don't you look at the GPU device ID?
> 
> We still fix this bridge at 1f.0, and current our implementation can cover
> our requirement and safe. I means this bridge should not be used for other
> use cases, so if its still be accessed we mightn't take care of them, right?

I was asking about the host bridge device, not guest.

> >
> >
> >>}
> >>
> >>static void xen_igd_passthrough_isa_bridge_initfn(Object *obj)
> >>{
> >>     object_property_add(obj, "device-id", "int",
> >>                         xen_igd_passthrough_pciisabridge_get_pci_device_id,
> >>                         NULL, NULL, NULL, NULL);
> >>}
> >
> >OK and what reads this property?
> 
> In sequent patch I will do something like this,
> 
> @@ -464,6 +464,32 @@ static void pc_xen_hvm_init(MachineState *machine)
>      }
>  }
> 
> +static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
> +{
> +    struct PCIDevice *dev;
> +    Error *local_err = NULL;
> +    uint16_t device_id = 0xffff;
> +
> +    /* Currently IGD drivers always need to access PCH by 1f.0. */
> +    dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
> +                            "xen-igd-passthrough-isa-bridge");
> +
> +    /* Identify PCH card with its own real vendor/device ids.
> +     * Here that vendor id is always PCI_VENDOR_ID_INTEL.
> +     */
> +    if (dev) {
> +        device_id = (uint16_t)object_property_get_int(OBJECT(dev),
> +                                                      "device-id",
> +                                                      &local_err);

cast is not needed here.

> +        if ((!local_err) && (device_id != 0xffff)) {

too many ().

> +            pci_config_set_device_id(dev->config, device_id);
> +            return;
> +        }
> +    }
> +
> +    fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
> +}
> +
>  static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
>  {
>      PCIBus *bus;
> @@ -473,6 +499,7 @@ static void xen_igd_passthrough_pc_hvm_init(MachineState
> *machine)
>      bus = pci_find_primary_bus();
>      if (bus != NULL) {
>          pci_create_simple(bus, -1, "xen-platform");
> +        xen_igd_passthrough_isa_bridge_create(bus);
>      }
>  }
>  #endif
> 
> Tiejun
>

By the way, I wonder: would it make sense to add this code in xen_pt_initfn?

You would then do the following there:

	- look up GPU by device/vendor ID
	- if a legacy GPU is found, get the ID, translate
	  to host bridge ID, and create the compat bridge.

saves the hassle of using a distinct machine type for passthrough, no?
 
> >
> >>Thanks
> >>Tiejun
> >>
> >>
> >>>>
> >>>
> >>>If I understand this correctly, the only difference is instead of reading PCH DevID/RevID from the host hardware, QEMU inserts those values into PCH virtual device by looking at the reverse mapping table it maintains.
> >>>
> >>>I agree the downside of doing this is the reverse mapping table may be hard to maintain.
> >>>
> >>>What is the advantage of doing this instead of having QEMU reading it from the host?  Is it to test to make sure reverse mapping methods works before it is adopted in the new drivers?
> >>>
> >>>>Thanks
> >>>>Tiejun
> >>>>
> >>>>>
> >>>>>>>
> >>>>>>>the problem with these tables is they are hard to keep up to date
> >>>>>>
> >>>>>>Yeah. But I think currently we can just start from some modern CPU
> >>>>>>such as HSW and BDW, then things could be easy.
> >>>>>>
> >>>>>>Allen,
> >>>>>>
> >>>>>>Any idea to this suggestion?
> >>>>>>
> >>>>>>>as new hardware comes out, but as future hardware won't need these
> >>>>>>>hacks, we shall be fine.
> >>>>>>
> >>>>>>Yeah.
> >>>>>>
> >>>>>>Thanks
> >>>>>>Tiejun
> >>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>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
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>
> >>>>>
> >>>>>
> >>>
> >>>Allen
> >>>
> >>>
> >
Michael S. Tsirkin Oct. 7, 2014, 7:27 a.m. UTC | #22 
On Sun, Sep 28, 2014 at 01:38:43PM +0800, Chen, Tiejun wrote:
> >**errp)
> >{
> >     uint32_t value = 0;
> >     XenHostPCIDevice hdev;
> >     int r = 0, num;
> >
> >     r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
> >     if (!r) {
> >         value = hdev.device_id;
> >
> >         num = sizeof(xen_igd_combo_id_infos)/sizeof(uint16_t);
> 
> Sorry for this one typo:
> 
> sizeof(xen_igd_combo_id_infos)/sizeof(xen_igd_combo_id_infos[0]);
> 
> Tiejun


so don't opencode this, use ARRAY_SIZE.
Tiejun Chen Oct. 9, 2014, 2:16 a.m. UTC | #23 
On 2014/10/7 15:27, Michael S. Tsirkin wrote:
> On Sun, Sep 28, 2014 at 01:38:43PM +0800, Chen, Tiejun wrote:
>>> **errp)
>>> {
>>>      uint32_t value = 0;
>>>      XenHostPCIDevice hdev;
>>>      int r = 0, num;
>>>
>>>      r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
>>>      if (!r) {
>>>          value = hdev.device_id;
>>>
>>>          num = sizeof(xen_igd_combo_id_infos)/sizeof(uint16_t);
>>
>> Sorry for this one typo:
>>
>> sizeof(xen_igd_combo_id_infos)/sizeof(xen_igd_combo_id_infos[0]);
>>
>> Tiejun
>
>
> so don't opencode this, use ARRAY_SIZE.
>

Yes, I will refine this with ARRAY_SIZE.

Thanks
Tiejun
Tiejun Chen Oct. 9, 2014, 5:53 a.m. UTC | #24 
On 2014/10/7 15:26, Michael S. Tsirkin wrote:
> On Tue, Sep 30, 2014 at 10:43:09AM +0800, Chen, Tiejun wrote:
>> On 2014/9/29 18:01, Michael S. Tsirkin wrote:
>>> On Sun, Sep 28, 2014 at 10:59:05AM +0800, Chen, Tiejun wrote:
>>>> On 2014/9/3 9:40, Kay, Allen M wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Chen, Tiejun
>>>>>> Sent: Monday, September 01, 2014 12:50 AM
>>>>>> To: Michael S. Tsirkin
>>>>>> Cc: xen-devel@lists.xensource.com; Kay, Allen M; qemu-devel@nongnu.org;
>>>>>> Konrad Rzeszutek Wilk
>>>>>> Subject: Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen:i386:pc_piix: create
>>>>>> isa bridge specific to IGD passthrough
>>>>>>
>>>>>> On 2014/9/1 14:05, Michael S. Tsirkin wrote:
>>>>>>> On Mon, Sep 01, 2014 at 10:50:37AM +0800, Chen, Tiejun wrote:
>>>>>>>> On 2014/8/31 16:58, Michael S. Tsirkin wrote:
>>>>>>>>> On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2014/8/28 8:56, Chen, Tiejun wrote:
>>>>>>>>>>>>>>>>> +     */
>>>>>>>>>>>>>>>>> +    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,
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Ping.
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> Tiejun
>>>>>>>>>>>
>>>>>>>>>>>> Could you explain this exactly? Then I can try follow-up your
>>>>>>>>>>>> idea ASAP if this is necessary and possible.
>>>>>>>>>>
>>>>>>>>>> Michel,
>>>>>>>>>>
>>>>>>>>>> Could you give us some explanation for your "reverse logic" when
>>>>>>>>>> you're free?
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> Tiejun
>>>>>>>>>
>>>>>>>>> So future drivers will look at device ID for the card and figure out
>>>>>>>>> how things should work from there.
>>>>>>>>> Old drivers still poke at device id of the chipset for this, but
>>>>>>>>> maybe qemu can do what newer drivers do:
>>>>>>>>> look at the card and figure out what guest should do, then present
>>>>>>>>> the appropriate chipset id.
>>>>>>>>>
>>>>>>>>> This is based on what Jesse said:
>>>>>>>>> 	Practically speaking, we could probably assume specific GPU/PCH
>>>>>> combos,
>>>>>>>>> 	as I don't think they're generally mixed across generations, though
>>>>>> SNB
>>>>>>>>> 	and IVB did have compatible sockets, so there is the possibility of
>>>>>>>>> 	mixing CPT and PPT PCHs, but those are register identical as far as the
>>>>>>>>> 	graphics driver is concerned, so even that should be safe.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Michael,
>>>>>>>>
>>>>>>>> Thanks for your explanation.
>>>>>>>>
>>>>>>>>> so the idea is to have a reverse table mapping GPU back to PCH.
>>>>>>>>> Present to guest the ID that will let it assume the correct GPU.
>>>>>>>>
>>>>>>>> I guess you mean we should create to maintain such a table:
>>>>>>>>
>>>>>>>> [GPU Card: device_id(s), PCH: device_id]
>>>>>>>>
>>>>>>>> Then each time, instead of exposing that real PCH device id directly,
>>>>>>>> qemu first can get the real GPU device id to lookup this table to
>>>>>>>> present a appropriate PCH's device id to the guest.
>>>>>>>>
>>>>>>>> And looks here that appropriate PCH's device id is not always a that
>>>>>>>> real PCH's device id. Right? If I'm wrong please correct me.
>>>>>>>
>>>>>>> Exactly: we don't really care what the PCH ID is - we only need it for
>>>>>>> the guest driver to do the right thing.
>>>>>>
>>>>>> Agreed.
>>>>>>
>>>>>> I need to ask Allen to check if one given GPU card device id is always
>>>>>> corresponding to one given PCH on both HSW and BDW currently. If yes, I can
>>>>>> do this quickly. Otherwise I need some time to establish that sort
>>>>>> of connection.
>>>>
>>>> Michael,
>>>>
>>>> Sorry for this delay response but please keep in mind we really are in this
>>>> process.
>>>>
>>>> You know this involve different GPU components we have to take some time to
>>>> communicate or even discuss internal.
>>>>
>>>> Now I have a draft codes, could you take a look at this? I hope that comment
>>>> can help us to understand something, but if you have any question, we can
>>>> further respond inline.
>>>>
>>>> ---
>>>> typedef struct {
>>>>      uint16_t gpu_device_id;
>>>>      uint16_t pch_device_id;
>>>> } XenIGDDeviceIDInfo;
>>>>
>>>> /* In real world different GPU should have different PCH. But actually
>>>>   * the different PCH DIDs likely map to different PCH SKUs. We do the
>>>>   * same thing for the GPU. For PCH, the different SKUs are going to be
>>>>   * all the same silicon design and implementation, just different
>>>>   * features turn on and off with fuses. The SW interfaces should be
>>>>   * consistent across all SKUs in a given family (eg LPT). But just same
>>>>   * features may not be supported.
>>>>   *
>>>>   * Most of these different PCH features probably don't matter to the
>>>>   * Gfx driver, but obviously any difference in display port connections
>>>>   * will so it should be fine with any PCH in case of passthrough.
>>>>   *
>>>>   * So currently use one PCH version, 0x8c4e, to cover all HSW scenarios,
>>>>   * 0x9cc3 for BDW.
>>>
>>> Pls write Haswell and Broadwell in full in comment.
>>
>> Are you saying I should list all PCH device ids both of HSW and BDW here
>> like this format?
>>
>> HSWGT1D: 0xABCD
>> ...
>
> I am merely saying don't say "HSW, BDW" in comments. Say "Haswell,

Sorry I'm misunderstanding what you mean.

> Broadwell, or if you like HWS(Haswell), BDW(Broadwell).

Looks the latter is moderate.

+ * So currently use one PCH version, 0x8c4e, to cover all HSW(Haswell)
+ * scenarios, 0x9cc3 for BDW(Broadwell).
+ */


>
>>>
>>>>   */
>>>> static const XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
>>>>      /* HSW Classic */
>>>>      {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
>>>>      {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
>>>>      {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
>>>>      {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
>>>>      {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
>>>>      /* HSW ULT */
>>>>      {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
>>>>      {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
>>>>      {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
>>>>      {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
>>>>      {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
>>>>      {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
>>>>      /* HSW CRW */
>>>>      {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
>>>>      {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
>>>>      /* HSW Server */
>>>>      {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
>>>>      /* HSW SRVR */
>>>>      {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
>>>>      /* BSW */
>>>>      {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
>>>>      {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
>>>>      {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
>>>>      {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
>>>>      {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
>>>>      {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
>>>>      {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
>>>>      {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
>>>>      {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
>>>>      {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
>>>>      {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
>>>> };
>>>>
>>>> static void xen_igd_passthrough_pciisabridge_get_pci_device_id(Object *obj,
>>>>                                                                 Visitor *v,
>>>>                                                                 void *opaque,
>>>>                                                                 const char
>>>> *name,
>>>>                                                                 Error **errp)
>>>> {
>>>>      uint32_t value = 0;
>>>>      XenHostPCIDevice hdev;
>>>>      int r = 0, num;
>>>>
>>>>      r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
>>>>      if (!r) {
>>>>          value = hdev.device_id;
>>>>
>>>>          num = sizeof(xen_igd_combo_id_infos)/sizeof(uint16_t);
>>>>          for (r = 0; r < num; r++)
>>>>              if (value == xen_igd_combo_id_infos[r].gpu_device_id)
>>>>                  value = xen_igd_combo_id_infos[r].pch_device_id;
>>>
>>> that's messy, I would use different variables for ID
>>> of GPU and PCH.
>>
>>          for (r = 0; r < num; r++)
>>              if (gpu_id == xen_igd_combo_id_infos[r].gpu_device_id)
>>                  pch_id = xen_igd_combo_id_infos[r].pch_device_id;
>
> yea
>
>>>
>>>>      }
>>>>
>>>>      visit_type_uint32(v, &value, name, errp);
>>>
>>>
>>> what I was suggesting is to start with the GPU device ID
>>> (you can get it e.g. from the config array).
>>
>> I think current codes is doing this:
>>
>>      r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
>>      if (!r) {
>>          gpu_id = hdev.device_id;
>>
>> Here xen_host_pci_device_get() is a wrapper of reading pci sysfs.
>
>
> Do you want the device ID of the GPU?

Yes.

> So please don't poke at it directly in sysfs.
> Instead, find the GPU's PCIDevice and do
> 	gpu->config_read();
> or just get from gpu->config.

Good, but how can we get gpu as pci_dev instance here? Then we can call 
pci_dev->config_read as expect.

>
>>> Use that to look up the PCH ID in xen_igd_combo_id_infos.
>>> If there, override the PCH ID.
>>>
>>> If not there, this is a new device so its driver will
>>> not look at PCH at all, we can make it whatever or
>>> skip it completely.
>>
>> I think we should return one initialized value, 0xffff, since often this
>> represents an invalid PCI device.
>
> I'm not sure you really want an invalid PCI device,
> would be nicer not to have a PCH device there at all.

Yes, I means 0xffff indicates that there's no any PCI device.

>
>
>>>
>>> This seems to almost do this, however
>>> - Why are you looking at host PCH device ID at all?
>>> - Why don't you look at the GPU device ID?
>>
>> We still fix this bridge at 1f.0, and current our implementation can cover
>> our requirement and safe. I means this bridge should not be used for other
>> use cases, so if its still be accessed we mightn't take care of them, right?
>
> I was asking about the host bridge device, not guest.

Are you assuming what will happen to a new device?

>
>>>
>>>
>>>> }
>>>>
>>>> static void xen_igd_passthrough_isa_bridge_initfn(Object *obj)
>>>> {
>>>>      object_property_add(obj, "device-id", "int",
>>>>                          xen_igd_passthrough_pciisabridge_get_pci_device_id,
>>>>                          NULL, NULL, NULL, NULL);
>>>> }
>>>
>>> OK and what reads this property?
>>
>> In sequent patch I will do something like this,
>>
>> @@ -464,6 +464,32 @@ static void pc_xen_hvm_init(MachineState *machine)
>>       }
>>   }
>>
>> +static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
>> +{
>> +    struct PCIDevice *dev;
>> +    Error *local_err = NULL;
>> +    uint16_t device_id = 0xffff;
>> +
>> +    /* Currently IGD drivers always need to access PCH by 1f.0. */
>> +    dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
>> +                            "xen-igd-passthrough-isa-bridge");
>> +
>> +    /* Identify PCH card with its own real vendor/device ids.
>> +     * Here that vendor id is always PCI_VENDOR_ID_INTEL.
>> +     */
>> +    if (dev) {
>> +        device_id = (uint16_t)object_property_get_int(OBJECT(dev),
>> +                                                      "device-id",
>> +                                                      &local_err);
>
> cast is not needed here.
>
>> +        if ((!local_err) && (device_id != 0xffff)) {
>
> too many ().

+        device_id = object_property_get_int(OBJECT(dev), "device-id",
+                                            &local_err);
+        if (!local_err && device_id != 0xffff) {

>
>> +            pci_config_set_device_id(dev->config, device_id);
>> +            return;
>> +        }
>> +    }
>> +
>> +    fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
>> +}
>> +
>>   static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
>>   {
>>       PCIBus *bus;
>> @@ -473,6 +499,7 @@ static void xen_igd_passthrough_pc_hvm_init(MachineState
>> *machine)
>>       bus = pci_find_primary_bus();
>>       if (bus != NULL) {
>>           pci_create_simple(bus, -1, "xen-platform");
>> +        xen_igd_passthrough_isa_bridge_create(bus);
>>       }
>>   }
>>   #endif
>>
>> Tiejun
>>
>
> By the way, I wonder: would it make sense to add this code in xen_pt_initfn?
>
> You would then do the following there:
>
> 	- look up GPU by device/vendor ID
> 	- if a legacy GPU is found, get the ID, translate
> 	  to host bridge ID, and create the compat bridge.
>
> saves the hassle of using a distinct machine type for passthrough, no?

As I remember Paolo thought we'd better have a separate machine specific 
to IGD passthrough, so I'm not sure if its fine to him now.

Thanks
Tiejun

>
>>>
>>>> Thanks
>>>> Tiejun
>>>>
>>>>
>>>>>>
>>>>>
>>>>> If I understand this correctly, the only difference is instead of reading PCH DevID/RevID from the host hardware, QEMU inserts those values into PCH virtual device by looking at the reverse mapping table it maintains.
>>>>>
>>>>> I agree the downside of doing this is the reverse mapping table may be hard to maintain.
>>>>>
>>>>> What is the advantage of doing this instead of having QEMU reading it from the host?  Is it to test to make sure reverse mapping methods works before it is adopted in the new drivers?
>>>>>
>>>>>> Thanks
>>>>>> Tiejun
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>> the problem with these tables is they are hard to keep up to date
>>>>>>>>
>>>>>>>> Yeah. But I think currently we can just start from some modern CPU
>>>>>>>> such as HSW and BDW, then things could be easy.
>>>>>>>>
>>>>>>>> Allen,
>>>>>>>>
>>>>>>>> Any idea to this suggestion?
>>>>>>>>
>>>>>>>>> as new hardware comes out, but as future hardware won't need these
>>>>>>>>> hacks, we shall be fine.
>>>>>>>>
>>>>>>>> Yeah.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Tiejun
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> 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
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>> Allen
>>>>>
>>>>>
>>>
>
>
Michael S. Tsirkin Oct. 12, 2014, 9:50 a.m. UTC | #25 
On Thu, Oct 09, 2014 at 01:53:16PM +0800, Chen, Tiejun wrote:
> 
> 
> On 2014/10/7 15:26, Michael S. Tsirkin wrote:
> >On Tue, Sep 30, 2014 at 10:43:09AM +0800, Chen, Tiejun wrote:
> >>On 2014/9/29 18:01, Michael S. Tsirkin wrote:
> >>>On Sun, Sep 28, 2014 at 10:59:05AM +0800, Chen, Tiejun wrote:
> >>>>On 2014/9/3 9:40, Kay, Allen M wrote:
> >>>>>
> >>>>>
> >>>>>>-----Original Message-----
> >>>>>>From: Chen, Tiejun
> >>>>>>Sent: Monday, September 01, 2014 12:50 AM
> >>>>>>To: Michael S. Tsirkin
> >>>>>>Cc: xen-devel@lists.xensource.com; Kay, Allen M; qemu-devel@nongnu.org;
> >>>>>>Konrad Rzeszutek Wilk
> >>>>>>Subject: Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen:i386:pc_piix: create
> >>>>>>isa bridge specific to IGD passthrough
> >>>>>>
> >>>>>>On 2014/9/1 14:05, Michael S. Tsirkin wrote:
> >>>>>>>On Mon, Sep 01, 2014 at 10:50:37AM +0800, Chen, Tiejun wrote:
> >>>>>>>>On 2014/8/31 16:58, Michael S. Tsirkin wrote:
> >>>>>>>>>On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>On 2014/8/28 8:56, Chen, Tiejun wrote:
> >>>>>>>>>>>>>>>>>+     */
> >>>>>>>>>>>>>>>>>+    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,
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>Ping.
> >>>>>>>>>>>
> >>>>>>>>>>>Thanks
> >>>>>>>>>>>Tiejun
> >>>>>>>>>>>
> >>>>>>>>>>>>Could you explain this exactly? Then I can try follow-up your
> >>>>>>>>>>>>idea ASAP if this is necessary and possible.
> >>>>>>>>>>
> >>>>>>>>>>Michel,
> >>>>>>>>>>
> >>>>>>>>>>Could you give us some explanation for your "reverse logic" when
> >>>>>>>>>>you're free?
> >>>>>>>>>>
> >>>>>>>>>>Thanks
> >>>>>>>>>>Tiejun
> >>>>>>>>>
> >>>>>>>>>So future drivers will look at device ID for the card and figure out
> >>>>>>>>>how things should work from there.
> >>>>>>>>>Old drivers still poke at device id of the chipset for this, but
> >>>>>>>>>maybe qemu can do what newer drivers do:
> >>>>>>>>>look at the card and figure out what guest should do, then present
> >>>>>>>>>the appropriate chipset id.
> >>>>>>>>>
> >>>>>>>>>This is based on what Jesse said:
> >>>>>>>>>	Practically speaking, we could probably assume specific GPU/PCH
> >>>>>>combos,
> >>>>>>>>>	as I don't think they're generally mixed across generations, though
> >>>>>>SNB
> >>>>>>>>>	and IVB did have compatible sockets, so there is the possibility of
> >>>>>>>>>	mixing CPT and PPT PCHs, but those are register identical as far as the
> >>>>>>>>>	graphics driver is concerned, so even that should be safe.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>>Michael,
> >>>>>>>>
> >>>>>>>>Thanks for your explanation.
> >>>>>>>>
> >>>>>>>>>so the idea is to have a reverse table mapping GPU back to PCH.
> >>>>>>>>>Present to guest the ID that will let it assume the correct GPU.
> >>>>>>>>
> >>>>>>>>I guess you mean we should create to maintain such a table:
> >>>>>>>>
> >>>>>>>>[GPU Card: device_id(s), PCH: device_id]
> >>>>>>>>
> >>>>>>>>Then each time, instead of exposing that real PCH device id directly,
> >>>>>>>>qemu first can get the real GPU device id to lookup this table to
> >>>>>>>>present a appropriate PCH's device id to the guest.
> >>>>>>>>
> >>>>>>>>And looks here that appropriate PCH's device id is not always a that
> >>>>>>>>real PCH's device id. Right? If I'm wrong please correct me.
> >>>>>>>
> >>>>>>>Exactly: we don't really care what the PCH ID is - we only need it for
> >>>>>>>the guest driver to do the right thing.
> >>>>>>
> >>>>>>Agreed.
> >>>>>>
> >>>>>>I need to ask Allen to check if one given GPU card device id is always
> >>>>>>corresponding to one given PCH on both HSW and BDW currently. If yes, I can
> >>>>>>do this quickly. Otherwise I need some time to establish that sort
> >>>>>>of connection.
> >>>>
> >>>>Michael,
> >>>>
> >>>>Sorry for this delay response but please keep in mind we really are in this
> >>>>process.
> >>>>
> >>>>You know this involve different GPU components we have to take some time to
> >>>>communicate or even discuss internal.
> >>>>
> >>>>Now I have a draft codes, could you take a look at this? I hope that comment
> >>>>can help us to understand something, but if you have any question, we can
> >>>>further respond inline.
> >>>>
> >>>>---
> >>>>typedef struct {
> >>>>     uint16_t gpu_device_id;
> >>>>     uint16_t pch_device_id;
> >>>>} XenIGDDeviceIDInfo;
> >>>>
> >>>>/* In real world different GPU should have different PCH. But actually
> >>>>  * the different PCH DIDs likely map to different PCH SKUs. We do the
> >>>>  * same thing for the GPU. For PCH, the different SKUs are going to be
> >>>>  * all the same silicon design and implementation, just different
> >>>>  * features turn on and off with fuses. The SW interfaces should be
> >>>>  * consistent across all SKUs in a given family (eg LPT). But just same
> >>>>  * features may not be supported.
> >>>>  *
> >>>>  * Most of these different PCH features probably don't matter to the
> >>>>  * Gfx driver, but obviously any difference in display port connections
> >>>>  * will so it should be fine with any PCH in case of passthrough.
> >>>>  *
> >>>>  * So currently use one PCH version, 0x8c4e, to cover all HSW scenarios,
> >>>>  * 0x9cc3 for BDW.
> >>>
> >>>Pls write Haswell and Broadwell in full in comment.
> >>
> >>Are you saying I should list all PCH device ids both of HSW and BDW here
> >>like this format?
> >>
> >>HSWGT1D: 0xABCD
> >>...
> >
> >I am merely saying don't say "HSW, BDW" in comments. Say "Haswell,
> 
> Sorry I'm misunderstanding what you mean.
> 
> >Broadwell, or if you like HWS(Haswell), BDW(Broadwell).
> 
> Looks the latter is moderate.
> 
> + * So currently use one PCH version, 0x8c4e, to cover all HSW(Haswell)
> + * scenarios, 0x9cc3 for BDW(Broadwell).
> + */

Exactly.

> 
> >
> >>>
> >>>>  */
> >>>>static const XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
> >>>>     /* HSW Classic */
> >>>>     {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
> >>>>     {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
> >>>>     {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
> >>>>     {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
> >>>>     {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
> >>>>     /* HSW ULT */
> >>>>     {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
> >>>>     {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
> >>>>     {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
> >>>>     {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
> >>>>     {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
> >>>>     {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
> >>>>     /* HSW CRW */
> >>>>     {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
> >>>>     {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
> >>>>     /* HSW Server */
> >>>>     {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
> >>>>     /* HSW SRVR */
> >>>>     {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
> >>>>     /* BSW */
> >>>>     {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
> >>>>     {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
> >>>>     {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
> >>>>     {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
> >>>>     {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
> >>>>     {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
> >>>>     {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
> >>>>     {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
> >>>>     {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
> >>>>     {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
> >>>>     {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
> >>>>};
> >>>>
> >>>>static void xen_igd_passthrough_pciisabridge_get_pci_device_id(Object *obj,
> >>>>                                                                Visitor *v,
> >>>>                                                                void *opaque,
> >>>>                                                                const char
> >>>>*name,
> >>>>                                                                Error **errp)
> >>>>{
> >>>>     uint32_t value = 0;
> >>>>     XenHostPCIDevice hdev;
> >>>>     int r = 0, num;
> >>>>
> >>>>     r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
> >>>>     if (!r) {
> >>>>         value = hdev.device_id;
> >>>>
> >>>>         num = sizeof(xen_igd_combo_id_infos)/sizeof(uint16_t);
> >>>>         for (r = 0; r < num; r++)
> >>>>             if (value == xen_igd_combo_id_infos[r].gpu_device_id)
> >>>>                 value = xen_igd_combo_id_infos[r].pch_device_id;
> >>>
> >>>that's messy, I would use different variables for ID
> >>>of GPU and PCH.
> >>
> >>         for (r = 0; r < num; r++)
> >>             if (gpu_id == xen_igd_combo_id_infos[r].gpu_device_id)
> >>                 pch_id = xen_igd_combo_id_infos[r].pch_device_id;
> >
> >yea
> >
> >>>
> >>>>     }
> >>>>
> >>>>     visit_type_uint32(v, &value, name, errp);
> >>>
> >>>
> >>>what I was suggesting is to start with the GPU device ID
> >>>(you can get it e.g. from the config array).
> >>
> >>I think current codes is doing this:
> >>
> >>     r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
> >>     if (!r) {
> >>         gpu_id = hdev.device_id;
> >>
> >>Here xen_host_pci_device_get() is a wrapper of reading pci sysfs.
> >
> >
> >Do you want the device ID of the GPU?
> 
> Yes.
> 
> >So please don't poke at it directly in sysfs.
> >Instead, find the GPU's PCIDevice and do
> >	gpu->config_read();
> >or just get from gpu->config.
> 
> Good, but how can we get gpu as pci_dev instance here? Then we can call
> pci_dev->config_read as expect.

You can't do this at host init but you can do this when
gpu is added or at machine done.

> >
> >>>Use that to look up the PCH ID in xen_igd_combo_id_infos.
> >>>If there, override the PCH ID.
> >>>
> >>>If not there, this is a new device so its driver will
> >>>not look at PCH at all, we can make it whatever or
> >>>skip it completely.
> >>
> >>I think we should return one initialized value, 0xffff, since often this
> >>represents an invalid PCI device.
> >
> >I'm not sure you really want an invalid PCI device,
> >would be nicer not to have a PCH device there at all.
> 
> Yes, I means 0xffff indicates that there's no any PCI device.
> 
> >
> >
> >>>
> >>>This seems to almost do this, however
> >>>- Why are you looking at host PCH device ID at all?
> >>>- Why don't you look at the GPU device ID?
> >>
> >>We still fix this bridge at 1f.0, and current our implementation can cover
> >>our requirement and safe. I means this bridge should not be used for other
> >>use cases, so if its still be accessed we mightn't take care of them, right?
> >
> >I was asking about the host bridge device, not guest.
> 
> Are you assuming what will happen to a new device?

Yes, the assumption is a new devices will use a new driver which does
everything through BAR, so it will not need these hacks.


> >
> >>>
> >>>
> >>>>}
> >>>>
> >>>>static void xen_igd_passthrough_isa_bridge_initfn(Object *obj)
> >>>>{
> >>>>     object_property_add(obj, "device-id", "int",
> >>>>                         xen_igd_passthrough_pciisabridge_get_pci_device_id,
> >>>>                         NULL, NULL, NULL, NULL);
> >>>>}
> >>>
> >>>OK and what reads this property?
> >>
> >>In sequent patch I will do something like this,
> >>
> >>@@ -464,6 +464,32 @@ static void pc_xen_hvm_init(MachineState *machine)
> >>      }
> >>  }
> >>
> >>+static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
> >>+{
> >>+    struct PCIDevice *dev;
> >>+    Error *local_err = NULL;
> >>+    uint16_t device_id = 0xffff;
> >>+
> >>+    /* Currently IGD drivers always need to access PCH by 1f.0. */
> >>+    dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
> >>+                            "xen-igd-passthrough-isa-bridge");
> >>+
> >>+    /* Identify PCH card with its own real vendor/device ids.
> >>+     * Here that vendor id is always PCI_VENDOR_ID_INTEL.
> >>+     */
> >>+    if (dev) {
> >>+        device_id = (uint16_t)object_property_get_int(OBJECT(dev),
> >>+                                                      "device-id",
> >>+                                                      &local_err);
> >
> >cast is not needed here.
> >
> >>+        if ((!local_err) && (device_id != 0xffff)) {
> >
> >too many ().
> 
> +        device_id = object_property_get_int(OBJECT(dev), "device-id",
> +                                            &local_err);
> +        if (!local_err && device_id != 0xffff) {
> 
> >
> >>+            pci_config_set_device_id(dev->config, device_id);
> >>+            return;
> >>+        }
> >>+    }
> >>+
> >>+    fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
> >>+}
> >>+
> >>  static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
> >>  {
> >>      PCIBus *bus;
> >>@@ -473,6 +499,7 @@ static void xen_igd_passthrough_pc_hvm_init(MachineState
> >>*machine)
> >>      bus = pci_find_primary_bus();
> >>      if (bus != NULL) {
> >>          pci_create_simple(bus, -1, "xen-platform");
> >>+        xen_igd_passthrough_isa_bridge_create(bus);
> >>      }
> >>  }
> >>  #endif
> >>
> >>Tiejun
> >>
> >
> >By the way, I wonder: would it make sense to add this code in xen_pt_initfn?
> >
> >You would then do the following there:
> >
> >	- look up GPU by device/vendor ID
> >	- if a legacy GPU is found, get the ID, translate
> >	  to host bridge ID, and create the compat bridge.
> >
> >saves the hassle of using a distinct machine type for passthrough, no?
> 
> As I remember Paolo thought we'd better have a separate machine specific to
> IGD passthrough, so I'm not sure if its fine to him now.
> 
> Thanks
> Tiejun

I think the point was mostly to reserve 1f to prevent
devices from using it.
As we populate slots in order it doesn't seem to important ...
Paolo?



> >
> >>>
> >>>>Thanks
> >>>>Tiejun
> >>>>
> >>>>
> >>>>>>
> >>>>>
> >>>>>If I understand this correctly, the only difference is instead of reading PCH DevID/RevID from the host hardware, QEMU inserts those values into PCH virtual device by looking at the reverse mapping table it maintains.
> >>>>>
> >>>>>I agree the downside of doing this is the reverse mapping table may be hard to maintain.
> >>>>>
> >>>>>What is the advantage of doing this instead of having QEMU reading it from the host?  Is it to test to make sure reverse mapping methods works before it is adopted in the new drivers?
> >>>>>
> >>>>>>Thanks
> >>>>>>Tiejun
> >>>>>>
> >>>>>>>
> >>>>>>>>>
> >>>>>>>>>the problem with these tables is they are hard to keep up to date
> >>>>>>>>
> >>>>>>>>Yeah. But I think currently we can just start from some modern CPU
> >>>>>>>>such as HSW and BDW, then things could be easy.
> >>>>>>>>
> >>>>>>>>Allen,
> >>>>>>>>
> >>>>>>>>Any idea to this suggestion?
> >>>>>>>>
> >>>>>>>>>as new hardware comes out, but as future hardware won't need these
> >>>>>>>>>hacks, we shall be fine.
> >>>>>>>>
> >>>>>>>>Yeah.
> >>>>>>>>
> >>>>>>>>Thanks
> >>>>>>>>Tiejun
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>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
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>
> >>>>>Allen
> >>>>>
> >>>>>
> >>>
> >
> >
Tiejun Chen Oct. 24, 2014, 7:36 a.m. UTC | #26 
On 2014/10/12 17:50, Michael S. Tsirkin wrote:
> On Thu, Oct 09, 2014 at 01:53:16PM +0800, Chen, Tiejun wrote:
>>
>>
>> On 2014/10/7 15:26, Michael S. Tsirkin wrote:
>>> On Tue, Sep 30, 2014 at 10:43:09AM +0800, Chen, Tiejun wrote:
>>>> On 2014/9/29 18:01, Michael S. Tsirkin wrote:
>>>>> On Sun, Sep 28, 2014 at 10:59:05AM +0800, Chen, Tiejun wrote:
>>>>>> On 2014/9/3 9:40, Kay, Allen M wrote:
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Chen, Tiejun
>>>>>>>> Sent: Monday, September 01, 2014 12:50 AM
>>>>>>>> To: Michael S. Tsirkin
>>>>>>>> Cc: xen-devel@lists.xensource.com; Kay, Allen M; qemu-devel@nongnu.org;
>>>>>>>> Konrad Rzeszutek Wilk
>>>>>>>> Subject: Re: [Qemu-devel] [Xen-devel] [PATCH 2/2] xen:i386:pc_piix: create
>>>>>>>> isa bridge specific to IGD passthrough
>>>>>>>>
>>>>>>>> On 2014/9/1 14:05, Michael S. Tsirkin wrote:
>>>>>>>>> On Mon, Sep 01, 2014 at 10:50:37AM +0800, Chen, Tiejun wrote:
>>>>>>>>>> On 2014/8/31 16:58, Michael S. Tsirkin wrote:
>>>>>>>>>>> On Fri, Aug 29, 2014 at 09:28:50AM +0800, Chen, Tiejun wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 2014/8/28 8:56, Chen, Tiejun wrote:
>>>>>>>>>>>>>>>>>>> +     */
>>>>>>>>>>>>>>>>>>> +    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,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Ping.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>> Tiejun
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Could you explain this exactly? Then I can try follow-up your
>>>>>>>>>>>>>> idea ASAP if this is necessary and possible.
>>>>>>>>>>>>
>>>>>>>>>>>> Michel,
>>>>>>>>>>>>
>>>>>>>>>>>> Could you give us some explanation for your "reverse logic" when
>>>>>>>>>>>> you're free?
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks
>>>>>>>>>>>> Tiejun
>>>>>>>>>>>
>>>>>>>>>>> So future drivers will look at device ID for the card and figure out
>>>>>>>>>>> how things should work from there.
>>>>>>>>>>> Old drivers still poke at device id of the chipset for this, but
>>>>>>>>>>> maybe qemu can do what newer drivers do:
>>>>>>>>>>> look at the card and figure out what guest should do, then present
>>>>>>>>>>> the appropriate chipset id.
>>>>>>>>>>>
>>>>>>>>>>> This is based on what Jesse said:
>>>>>>>>>>> 	Practically speaking, we could probably assume specific GPU/PCH
>>>>>>>> combos,
>>>>>>>>>>> 	as I don't think they're generally mixed across generations, though
>>>>>>>> SNB
>>>>>>>>>>> 	and IVB did have compatible sockets, so there is the possibility of
>>>>>>>>>>> 	mixing CPT and PPT PCHs, but those are register identical as far as the
>>>>>>>>>>> 	graphics driver is concerned, so even that should be safe.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Michael,
>>>>>>>>>>
>>>>>>>>>> Thanks for your explanation.
>>>>>>>>>>
>>>>>>>>>>> so the idea is to have a reverse table mapping GPU back to PCH.
>>>>>>>>>>> Present to guest the ID that will let it assume the correct GPU.
>>>>>>>>>>
>>>>>>>>>> I guess you mean we should create to maintain such a table:
>>>>>>>>>>
>>>>>>>>>> [GPU Card: device_id(s), PCH: device_id]
>>>>>>>>>>
>>>>>>>>>> Then each time, instead of exposing that real PCH device id directly,
>>>>>>>>>> qemu first can get the real GPU device id to lookup this table to
>>>>>>>>>> present a appropriate PCH's device id to the guest.
>>>>>>>>>>
>>>>>>>>>> And looks here that appropriate PCH's device id is not always a that
>>>>>>>>>> real PCH's device id. Right? If I'm wrong please correct me.
>>>>>>>>>
>>>>>>>>> Exactly: we don't really care what the PCH ID is - we only need it for
>>>>>>>>> the guest driver to do the right thing.
>>>>>>>>
>>>>>>>> Agreed.
>>>>>>>>
>>>>>>>> I need to ask Allen to check if one given GPU card device id is always
>>>>>>>> corresponding to one given PCH on both HSW and BDW currently. If yes, I can
>>>>>>>> do this quickly. Otherwise I need some time to establish that sort
>>>>>>>> of connection.
>>>>>>
>>>>>> Michael,
>>>>>>
>>>>>> Sorry for this delay response but please keep in mind we really are in this
>>>>>> process.
>>>>>>
>>>>>> You know this involve different GPU components we have to take some time to
>>>>>> communicate or even discuss internal.
>>>>>>
>>>>>> Now I have a draft codes, could you take a look at this? I hope that comment
>>>>>> can help us to understand something, but if you have any question, we can
>>>>>> further respond inline.
>>>>>>
>>>>>> ---
>>>>>> typedef struct {
>>>>>>      uint16_t gpu_device_id;
>>>>>>      uint16_t pch_device_id;
>>>>>> } XenIGDDeviceIDInfo;
>>>>>>
>>>>>> /* In real world different GPU should have different PCH. But actually
>>>>>>   * the different PCH DIDs likely map to different PCH SKUs. We do the
>>>>>>   * same thing for the GPU. For PCH, the different SKUs are going to be
>>>>>>   * all the same silicon design and implementation, just different
>>>>>>   * features turn on and off with fuses. The SW interfaces should be
>>>>>>   * consistent across all SKUs in a given family (eg LPT). But just same
>>>>>>   * features may not be supported.
>>>>>>   *
>>>>>>   * Most of these different PCH features probably don't matter to the
>>>>>>   * Gfx driver, but obviously any difference in display port connections
>>>>>>   * will so it should be fine with any PCH in case of passthrough.
>>>>>>   *
>>>>>>   * So currently use one PCH version, 0x8c4e, to cover all HSW scenarios,
>>>>>>   * 0x9cc3 for BDW.
>>>>>
>>>>> Pls write Haswell and Broadwell in full in comment.
>>>>
>>>> Are you saying I should list all PCH device ids both of HSW and BDW here
>>>> like this format?
>>>>
>>>> HSWGT1D: 0xABCD
>>>> ...
>>>
>>> I am merely saying don't say "HSW, BDW" in comments. Say "Haswell,
>>
>> Sorry I'm misunderstanding what you mean.
>>
>>> Broadwell, or if you like HWS(Haswell), BDW(Broadwell).
>>
>> Looks the latter is moderate.
>>
>> + * So currently use one PCH version, 0x8c4e, to cover all HSW(Haswell)
>> + * scenarios, 0x9cc3 for BDW(Broadwell).
>> + */
>
> Exactly.
>
>>
>>>
>>>>>
>>>>>>   */
>>>>>> static const XenIGDDeviceIDInfo xen_igd_combo_id_infos[] = {
>>>>>>      /* HSW Classic */
>>>>>>      {0x0402, 0x8c4e}, /* HSWGT1D, HSWD_w7 */
>>>>>>      {0x0406, 0x8c4e}, /* HSWGT1M, HSWM_w7 */
>>>>>>      {0x0412, 0x8c4e}, /* HSWGT2D, HSWD_w7 */
>>>>>>      {0x0416, 0x8c4e}, /* HSWGT2M, HSWM_w7 */
>>>>>>      {0x041E, 0x8c4e}, /* HSWGT15D, HSWD_w7 */
>>>>>>      /* HSW ULT */
>>>>>>      {0x0A06, 0x8c4e}, /* HSWGT1UT, HSWM_w7 */
>>>>>>      {0x0A16, 0x8c4e}, /* HSWGT2UT, HSWM_w7 */
>>>>>>      {0x0A26, 0x8c4e}, /* HSWGT3UT, HSWM_w7 */
>>>>>>      {0x0A2E, 0x8c4e}, /* HSWGT3UT28W, HSWM_w7 */
>>>>>>      {0x0A1E, 0x8c4e}, /* HSWGT2UX, HSWM_w7 */
>>>>>>      {0x0A0E, 0x8c4e}, /* HSWGT1ULX, HSWM_w7 */
>>>>>>      /* HSW CRW */
>>>>>>      {0x0D26, 0x8c4e}, /* HSWGT3CW, HSWM_w7 */
>>>>>>      {0x0D22, 0x8c4e}, /* HSWGT3CWDT, HSWD_w7 */
>>>>>>      /* HSW Server */
>>>>>>      {0x041A, 0x8c4e}, /* HSWSVGT2, HSWD_w7 */
>>>>>>      /* HSW SRVR */
>>>>>>      {0x040A, 0x8c4e}, /* HSWSVGT1, HSWD_w7 */
>>>>>>      /* BSW */
>>>>>>      {0x1606, 0x9cc3}, /* BDWULTGT1, BDWM_w7 */
>>>>>>      {0x1616, 0x9cc3}, /* BDWULTGT2, BDWM_w7 */
>>>>>>      {0x1626, 0x9cc3}, /* BDWULTGT3, BDWM_w7 */
>>>>>>      {0x160E, 0x9cc3}, /* BDWULXGT1, BDWM_w7 */
>>>>>>      {0x161E, 0x9cc3}, /* BDWULXGT2, BDWM_w7 */
>>>>>>      {0x1602, 0x9cc3}, /* BDWHALOGT1, BDWM_w7 */
>>>>>>      {0x1612, 0x9cc3}, /* BDWHALOGT2, BDWM_w7 */
>>>>>>      {0x1622, 0x9cc3}, /* BDWHALOGT3, BDWM_w7 */
>>>>>>      {0x162B, 0x9cc3}, /* BDWHALO28W, BDWM_w7 */
>>>>>>      {0x162A, 0x9cc3}, /* BDWGT3WRKS, BDWM_w7 */
>>>>>>      {0x162D, 0x9cc3}, /* BDWGT3SRVR, BDWM_w7 */
>>>>>> };
>>>>>>
>>>>>> static void xen_igd_passthrough_pciisabridge_get_pci_device_id(Object *obj,
>>>>>>                                                                 Visitor *v,
>>>>>>                                                                 void *opaque,
>>>>>>                                                                 const char
>>>>>> *name,
>>>>>>                                                                 Error **errp)
>>>>>> {
>>>>>>      uint32_t value = 0;
>>>>>>      XenHostPCIDevice hdev;
>>>>>>      int r = 0, num;
>>>>>>
>>>>>>      r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
>>>>>>      if (!r) {
>>>>>>          value = hdev.device_id;
>>>>>>
>>>>>>          num = sizeof(xen_igd_combo_id_infos)/sizeof(uint16_t);
>>>>>>          for (r = 0; r < num; r++)
>>>>>>              if (value == xen_igd_combo_id_infos[r].gpu_device_id)
>>>>>>                  value = xen_igd_combo_id_infos[r].pch_device_id;
>>>>>
>>>>> that's messy, I would use different variables for ID
>>>>> of GPU and PCH.
>>>>
>>>>          for (r = 0; r < num; r++)
>>>>              if (gpu_id == xen_igd_combo_id_infos[r].gpu_device_id)
>>>>                  pch_id = xen_igd_combo_id_infos[r].pch_device_id;
>>>
>>> yea
>>>
>>>>>
>>>>>>      }
>>>>>>
>>>>>>      visit_type_uint32(v, &value, name, errp);
>>>>>
>>>>>
>>>>> what I was suggesting is to start with the GPU device ID
>>>>> (you can get it e.g. from the config array).
>>>>
>>>> I think current codes is doing this:
>>>>
>>>>      r = xen_host_pci_device_get(&hdev, 0, 0, 0x02, 0);
>>>>      if (!r) {
>>>>          gpu_id = hdev.device_id;
>>>>
>>>> Here xen_host_pci_device_get() is a wrapper of reading pci sysfs.
>>>
>>>
>>> Do you want the device ID of the GPU?
>>
>> Yes.
>>
>>> So please don't poke at it directly in sysfs.
>>> Instead, find the GPU's PCIDevice and do
>>> 	gpu->config_read();
>>> or just get from gpu->config.
>>
>> Good, but how can we get gpu as pci_dev instance here? Then we can call
>> pci_dev->config_read as expect.
>
> You can't do this at host init but you can do this when
> gpu is added or at machine done.
>
>>>
>>>>> Use that to look up the PCH ID in xen_igd_combo_id_infos.
>>>>> If there, override the PCH ID.
>>>>>
>>>>> If not there, this is a new device so its driver will
>>>>> not look at PCH at all, we can make it whatever or
>>>>> skip it completely.
>>>>
>>>> I think we should return one initialized value, 0xffff, since often this
>>>> represents an invalid PCI device.
>>>
>>> I'm not sure you really want an invalid PCI device,
>>> would be nicer not to have a PCH device there at all.
>>
>> Yes, I means 0xffff indicates that there's no any PCI device.
>>
>>>
>>>
>>>>>
>>>>> This seems to almost do this, however
>>>>> - Why are you looking at host PCH device ID at all?
>>>>> - Why don't you look at the GPU device ID?
>>>>
>>>> We still fix this bridge at 1f.0, and current our implementation can cover
>>>> our requirement and safe. I means this bridge should not be used for other
>>>> use cases, so if its still be accessed we mightn't take care of them, right?
>>>
>>> I was asking about the host bridge device, not guest.
>>
>> Are you assuming what will happen to a new device?
>
> Yes, the assumption is a new devices will use a new driver which does
> everything through BAR, so it will not need these hacks.
>
>
>>>
>>>>>
>>>>>
>>>>>> }
>>>>>>
>>>>>> static void xen_igd_passthrough_isa_bridge_initfn(Object *obj)
>>>>>> {
>>>>>>      object_property_add(obj, "device-id", "int",
>>>>>>                          xen_igd_passthrough_pciisabridge_get_pci_device_id,
>>>>>>                          NULL, NULL, NULL, NULL);
>>>>>> }
>>>>>
>>>>> OK and what reads this property?
>>>>
>>>> In sequent patch I will do something like this,
>>>>
>>>> @@ -464,6 +464,32 @@ static void pc_xen_hvm_init(MachineState *machine)
>>>>       }
>>>>   }
>>>>
>>>> +static void xen_igd_passthrough_isa_bridge_create(PCIBus *bus)
>>>> +{
>>>> +    struct PCIDevice *dev;
>>>> +    Error *local_err = NULL;
>>>> +    uint16_t device_id = 0xffff;
>>>> +
>>>> +    /* Currently IGD drivers always need to access PCH by 1f.0. */
>>>> +    dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
>>>> +                            "xen-igd-passthrough-isa-bridge");
>>>> +
>>>> +    /* Identify PCH card with its own real vendor/device ids.
>>>> +     * Here that vendor id is always PCI_VENDOR_ID_INTEL.
>>>> +     */
>>>> +    if (dev) {
>>>> +        device_id = (uint16_t)object_property_get_int(OBJECT(dev),
>>>> +                                                      "device-id",
>>>> +                                                      &local_err);
>>>
>>> cast is not needed here.
>>>
>>>> +        if ((!local_err) && (device_id != 0xffff)) {
>>>
>>> too many ().
>>
>> +        device_id = object_property_get_int(OBJECT(dev), "device-id",
>> +                                            &local_err);
>> +        if (!local_err && device_id != 0xffff) {
>>
>>>
>>>> +            pci_config_set_device_id(dev->config, device_id);
>>>> +            return;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
>>>> +}
>>>> +
>>>>   static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
>>>>   {
>>>>       PCIBus *bus;
>>>> @@ -473,6 +499,7 @@ static void xen_igd_passthrough_pc_hvm_init(MachineState
>>>> *machine)
>>>>       bus = pci_find_primary_bus();
>>>>       if (bus != NULL) {
>>>>           pci_create_simple(bus, -1, "xen-platform");
>>>> +        xen_igd_passthrough_isa_bridge_create(bus);
>>>>       }
>>>>   }
>>>>   #endif
>>>>
>>>> Tiejun
>>>>
>>>
>>> By the way, I wonder: would it make sense to add this code in xen_pt_initfn?
>>>
>>> You would then do the following there:
>>>
>>> 	- look up GPU by device/vendor ID
>>> 	- if a legacy GPU is found, get the ID, translate
>>> 	  to host bridge ID, and create the compat bridge.
>>>
>>> saves the hassle of using a distinct machine type for passthrough, no?
>>
>> As I remember Paolo thought we'd better have a separate machine specific to
>> IGD passthrough, so I'm not sure if its fine to him now.

CCed Paolo.

>>
>> Thanks
>> Tiejun
>
> I think the point was mostly to reserve 1f to prevent
> devices from using it.
> As we populate slots in order it doesn't seem to important ...

If we populate slot at !1f GFX driver can't find this ISA bridge.

> Paolo?

Thanks
Tiejun

>
>
>
>>>
>>>>>
>>>>>> Thanks
>>>>>> Tiejun
>>>>>>
>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> If I understand this correctly, the only difference is instead of reading PCH DevID/RevID from the host hardware, QEMU inserts those values into PCH virtual device by looking at the reverse mapping table it maintains.
>>>>>>>
>>>>>>> I agree the downside of doing this is the reverse mapping table may be hard to maintain.
>>>>>>>
>>>>>>> What is the advantage of doing this instead of having QEMU reading it from the host?  Is it to test to make sure reverse mapping methods works before it is adopted in the new drivers?
>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Tiejun
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> the problem with these tables is they are hard to keep up to date
>>>>>>>>>>
>>>>>>>>>> Yeah. But I think currently we can just start from some modern CPU
>>>>>>>>>> such as HSW and BDW, then things could be easy.
>>>>>>>>>>
>>>>>>>>>> Allen,
>>>>>>>>>>
>>>>>>>>>> Any idea to this suggestion?
>>>>>>>>>>
>>>>>>>>>>> as new hardware comes out, but as future hardware won't need these
>>>>>>>>>>> hacks, we shall be fine.
>>>>>>>>>>
>>>>>>>>>> Yeah.
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> Tiejun
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 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
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>> Allen
>>>>>>>
>>>>>>>
>>>>>
>>>
>>>
>
Michael S. Tsirkin Oct. 24, 2014, 1:47 p.m. UTC | #27 
On Fri, Oct 24, 2014 at 03:36:20PM +0800, Chen, Tiejun wrote:
> >I think the point was mostly to reserve 1f to prevent
> >devices from using it.
> >As we populate slots in order it doesn't seem to important ...
> 
> If we populate slot at !1f GFX driver can't find this ISA bridge.

Right, but I mean if no special options are used, 1f will typically
stay free without any effort on our side.
Tiejun Chen Oct. 30, 2014, 7:47 a.m. UTC | #28 
Sorry some urgent things always procrastinate on my response.

On 2014/10/24 21:47, Michael S. Tsirkin wrote:
> On Fri, Oct 24, 2014 at 03:36:20PM +0800, Chen, Tiejun wrote:
>>> I think the point was mostly to reserve 1f to prevent
>>> devices from using it.
>>> As we populate slots in order it doesn't seem to important ...
>>
>> If we populate slot at !1f GFX driver can't find this ISA bridge.
>
> Right, but I mean if no special options are used, 1f will typically
> stay free without any effort on our side.

Yeah.

Actually based on current info we know, seems 1f is just specific to our 
scenario :) So I always think we can occupy that. But Paolo and you can 
really determine this point.

Thanks
Tiejun
Tiejun Chen Nov. 3, 2014, 7:48 a.m. UTC | #29 
On 2014/10/30 15:47, Chen, Tiejun wrote:
> Sorry some urgent things always procrastinate on my response.
>
> On 2014/10/24 21:47, Michael S. Tsirkin wrote:
>> On Fri, Oct 24, 2014 at 03:36:20PM +0800, Chen, Tiejun wrote:
>>>> I think the point was mostly to reserve 1f to prevent
>>>> devices from using it.
>>>> As we populate slots in order it doesn't seem to important ...
>>>
>>> If we populate slot at !1f GFX driver can't find this ISA bridge.
>>
>> Right, but I mean if no special options are used, 1f will typically
>> stay free without any effort on our side.
>
> Yeah.
>
> Actually based on current info we know, seems 1f is just specific to our
> scenario :) So I always think we can occupy that. But Paolo and you can
> really determine this point.

Paolo,

What's your idea?

Thanks
Tiejun
Paolo Bonzini Nov. 3, 2014, 11:35 a.m. UTC | #30 
On 03/11/2014 08:48, Chen, Tiejun wrote:
>>>>> I think the point was mostly to reserve 1f to prevent
>>>>> devices from using it.
>>>>> As we populate slots in order it doesn't seem to important ...
>>>>
>>>> If we populate slot at !1f GFX driver can't find this ISA bridge.
>>>
>>> Right, but I mean if no special options are used, 1f will typically
>>> stay free without any effort on our side.
>>
>> Yeah.
>>
>> Actually based on current info we know, seems 1f is just specific to our
>> scenario :) So I always think we can occupy that. But Paolo and you can
>> really determine this point.
> 
> What's your idea?

I do not have any objection to always occupying 1f for Xen IGD passthrough.

Paolo
Tiejun Chen Nov. 3, 2014, 11:36 a.m. UTC | #31 
On 2014/11/3 19:35, Paolo Bonzini wrote:
> On 03/11/2014 08:48, Chen, Tiejun wrote:
>>>>>> I think the point was mostly to reserve 1f to prevent
>>>>>> devices from using it.
>>>>>> As we populate slots in order it doesn't seem to important ...
>>>>>
>>>>> If we populate slot at !1f GFX driver can't find this ISA bridge.
>>>>
>>>> Right, but I mean if no special options are used, 1f will typically
>>>> stay free without any effort on our side.
>>>
>>> Yeah.
>>>
>>> Actually based on current info we know, seems 1f is just specific to our
>>> scenario :) So I always think we can occupy that. But Paolo and you can
>>> really determine this point.
>>
>> What's your idea?
>
> I do not have any objection to always occupying 1f for Xen IGD passthrough.
>

Cool ;-)

Thanks
Tiejun
Tiejun Chen Nov. 3, 2014, 11:47 a.m. UTC | #32 
On 2014/11/3 19:36, Chen, Tiejun wrote:
> On 2014/11/3 19:35, Paolo Bonzini wrote:
>> On 03/11/2014 08:48, Chen, Tiejun wrote:
>>>>>>> I think the point was mostly to reserve 1f to prevent
>>>>>>> devices from using it.
>>>>>>> As we populate slots in order it doesn't seem to important ...
>>>>>>
>>>>>> If we populate slot at !1f GFX driver can't find this ISA bridge.
>>>>>
>>>>> Right, but I mean if no special options are used, 1f will typically
>>>>> stay free without any effort on our side.
>>>>
>>>> Yeah.
>>>>
>>>> Actually based on current info we know, seems 1f is just specific to
>>>> our
>>>> scenario :) So I always think we can occupy that. But Paolo and you can
>>>> really determine this point.
>>>
>>> What's your idea?
>>
>> I do not have any objection to always occupying 1f for Xen IGD
>> passthrough.

After I go back to look at this again, I hope you don't misunderstand 
what Michael mean now. He was saying we don't need to create a new 
separate machine specific to IGD passthrough. But that idea is just from 
you :)

Thanks
Tiejun
Paolo Bonzini Nov. 3, 2014, 12:01 p.m. UTC | #33 
On 03/11/2014 12:47, Chen, Tiejun wrote:
> On 2014/11/3 19:36, Chen, Tiejun wrote:
>> On 2014/11/3 19:35, Paolo Bonzini wrote:
>>> On 03/11/2014 08:48, Chen, Tiejun wrote:
>>>>>>>> I think the point was mostly to reserve 1f to prevent
>>>>>>>> devices from using it.
>>>>>>>> As we populate slots in order it doesn't seem to important ...
>>>>>>>
>>>>>>> If we populate slot at !1f GFX driver can't find this ISA bridge.
>>>>>>
>>>>>> Right, but I mean if no special options are used, 1f will typically
>>>>>> stay free without any effort on our side.
>>>>>
>>>>> Yeah.
>>>>>
>>>>> Actually based on current info we know, seems 1f is just specific to
>>>>> our
>>>>> scenario :) So I always think we can occupy that. But Paolo and you
>>>>> can
>>>>> really determine this point.
>>>>
>>>> What's your idea?
>>>
>>> I do not have any objection to always occupying 1f for Xen IGD
>>> passthrough.
> 
> After I go back to look at this again, I hope you don't misunderstand
> what Michael mean now. He was saying we don't need to create a new
> separate machine specific to IGD passthrough. But that idea is just from
> you :)

It's difficult for me to follow, because xen_igd_passthrough_pc_hvm_init
does not exist in the current tree.

The patches seem good to me; I was assuming that the new machine type
would call xen_igd_passthrough_pc_hvm_init, but apparently I'm wrong?
Paolo
Michael S. Tsirkin Nov. 3, 2014, 1:10 p.m. UTC | #34 
On Mon, Nov 03, 2014 at 01:01:03PM +0100, Paolo Bonzini wrote:
> 
> 
> On 03/11/2014 12:47, Chen, Tiejun wrote:
> > On 2014/11/3 19:36, Chen, Tiejun wrote:
> >> On 2014/11/3 19:35, Paolo Bonzini wrote:
> >>> On 03/11/2014 08:48, Chen, Tiejun wrote:
> >>>>>>>> I think the point was mostly to reserve 1f to prevent
> >>>>>>>> devices from using it.
> >>>>>>>> As we populate slots in order it doesn't seem to important ...
> >>>>>>>
> >>>>>>> If we populate slot at !1f GFX driver can't find this ISA bridge.
> >>>>>>
> >>>>>> Right, but I mean if no special options are used, 1f will typically
> >>>>>> stay free without any effort on our side.
> >>>>>
> >>>>> Yeah.
> >>>>>
> >>>>> Actually based on current info we know, seems 1f is just specific to
> >>>>> our
> >>>>> scenario :) So I always think we can occupy that. But Paolo and you
> >>>>> can
> >>>>> really determine this point.
> >>>>
> >>>> What's your idea?
> >>>
> >>> I do not have any objection to always occupying 1f for Xen IGD
> >>> passthrough.
> > 
> > After I go back to look at this again, I hope you don't misunderstand
> > what Michael mean now. He was saying we don't need to create a new
> > separate machine specific to IGD passthrough. But that idea is just from
> > you :)
> 
> It's difficult for me to follow, because xen_igd_passthrough_pc_hvm_init
> does not exist in the current tree.
> 
> The patches seem good to me; I was assuming that the new machine type
> would call xen_igd_passthrough_pc_hvm_init, but apparently I'm wrong?
> Paolo

Discussed on irc, Paolo said
<bonzini> so i don't really care how the ISA bridge is created
Tiejun Chen Nov. 5, 2014, 7:23 a.m. UTC | #35 
On 2014/11/3 21:10, Michael S. Tsirkin wrote:
> On Mon, Nov 03, 2014 at 01:01:03PM +0100, Paolo Bonzini wrote:
>>
>>
>> On 03/11/2014 12:47, Chen, Tiejun wrote:
>>> On 2014/11/3 19:36, Chen, Tiejun wrote:
>>>> On 2014/11/3 19:35, Paolo Bonzini wrote:
>>>>> On 03/11/2014 08:48, Chen, Tiejun wrote:
>>>>>>>>>> I think the point was mostly to reserve 1f to prevent
>>>>>>>>>> devices from using it.
>>>>>>>>>> As we populate slots in order it doesn't seem to important ...
>>>>>>>>>
>>>>>>>>> If we populate slot at !1f GFX driver can't find this ISA bridge.
>>>>>>>>
>>>>>>>> Right, but I mean if no special options are used, 1f will typically
>>>>>>>> stay free without any effort on our side.
>>>>>>>
>>>>>>> Yeah.
>>>>>>>
>>>>>>> Actually based on current info we know, seems 1f is just specific to
>>>>>>> our
>>>>>>> scenario :) So I always think we can occupy that. But Paolo and you
>>>>>>> can
>>>>>>> really determine this point.
>>>>>>
>>>>>> What's your idea?
>>>>>
>>>>> I do not have any objection to always occupying 1f for Xen IGD
>>>>> passthrough.
>>>
>>> After I go back to look at this again, I hope you don't misunderstand
>>> what Michael mean now. He was saying we don't need to create a new
>>> separate machine specific to IGD passthrough. But that idea is just from
>>> you :)
>>
>> It's difficult for me to follow, because xen_igd_passthrough_pc_hvm_init
>> does not exist in the current tree.
>>
>> The patches seem good to me; I was assuming that the new machine type
>> would call xen_igd_passthrough_pc_hvm_init, but apparently I'm wrong?
>> Paolo
>
> Discussed on irc, Paolo said
> <bonzini> so i don't really care how the ISA bridge is created
>

This means all those previous patches creating new separate machine 
should be gone.

I would rebase these two patches to resend again as RFC.

Thanks
Tiejun
diff mbox

Patch

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.
+     */
+    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);
+        } else
+            fprintf(stderr, "xen set xen-igd-passthrough-isa-bridge failed\n");
+    }
+}
+
 static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
 {
     PCIBus *bus;
@@ -472,6 +493,7 @@  static void xen_igd_passthrough_pc_hvm_init(MachineState *machine)
     bus = pci_find_primary_bus();
     if (bus != NULL) {
         pci_create_simple(bus, -1, "xen-platform");
+        xen_igd_passthrough_isa_bridge_create(bus);
     }
 }
 #endif