Re: [Intel-gfx] [RFC][PATCH] gpu:drm:i915:intel_detect_pch: back to check devfn instead of check class type
On 2014/6/25 15:55, Paolo Bonzini wrote: Il 25/06/2014 09:34, Chen, Tiejun ha scritto: On 2014/6/25 14:48, Paolo Bonzini wrote: Second problem. Your IGD passthrough code currently works with QEMU's PIIX4-based machine. But what happens if you try to extend it, so that Yes, current xen machine, xenpv, is based on pii4, and also I don't known if we will plan to migrate to q35 or others. So its hard to further say more now. it works with QEMU's ICH9-based machine? That's a more modern machine that has a PCIe chipset and hence has its ISA bridge at 00:1f.0. Now But even in this case, could we set the real vendor/device ids for that ISA bridge at 00:1f.0? If not, what's broken? The config space layout changes for different vendor/device ids, so the guest firmware only works if you have the right vendor/device id. Paolo, After I discuss internal, we think even we just set the real vendor/device ids to this ISA bridge at 00:1f.0, guest firmware should still work well with these pair of real vendor/device ids. So if you think something would conflict or be broken, could you tell us what's exactly that? Then we will double check. Thanks Tiejun It is only slightly better, but the right solution is to fix the driver. There is absolutely zero reason why a graphics driver should know about the vendor/device ids of the PCH. This means we have to fix this both on Linux and Windows but I'm not sure if this is feasible to us. You have to do it if you want this feature in QEMU in a future-proof way. You _can_ provide the ugly PIIX4-specific hack as a compatibility fallback (and this patch is okay to make the compatibility fallback less hacky). However, I don't think QEMU should accept the patch for IGD passthrough unless Intel is willing to make drivers virtualization-friendly. Once you assign the IGD, it is not that integrated anymore and the drivers must take that into account. It is worthwhile pointing out that neither AMD nor nVidia need any of this. The right way could be to make QEMU add a vendor-specific capability to the video device. The driver can probe for that capability before Do you mean we can pick two unused offsets in the configuration space of the video device as a vendor-specific capability to hold the vendor/device ids of the PCH? Yes, either that or add a new capability (which lets you choose the offsets more freely). If the IGD driver needs config space fields of the MCH, those fields could also be mirrored in the new capability. QEMU would forward them automatically. It could even be a new BAR, which gives even more freedom to allocate the fields. looking at the PCI bus. QEMU can add the capability to the list, it is easy because all accesses to the video device's configuration space trap to QEMU. Then you do not need to add fake devices to the machine. In fact, it would be nice if Intel added such a capability on the next generation of integrated graphics, too. On real hardware, ACPI or some Maybe, but even this would be implemented, shouldn't we need to be compatible with those old generations? Yes. - old generation / old driver: use 00:1f.0 hack, only guaranteed to work on PIIX4-based virtual guest - old generation / new driver: use 00:1f.0 hack on real hardware, use capability on 00:02.0 on virtual guest, can work on PCIe virtual guest - new generation / old driver: doesn't exist - new generation / new driver: always use capability on 00:02.0, can work on PCIe virtual guest. Paolo ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC][PATCH] gpu:drm:i915:intel_detect_pch: back to check devfn instead of check class type
On 2014/6/30 19:18, Michael S. Tsirkin wrote:
On Thu, Jun 19, 2014 at 05:53:51PM +0800, Tiejun Chen wrote:
Originally the reason to probe ISA bridge instead of Dev31:Fun0
is to make graphics device passthrough work easy for VMM, that
only need to expose ISA bridge to let driver know the real
hardware underneath. This is a requirement from virtualization
team. Especially in that virtualized environments, XEN, there
is irrelevant ISA bridge in the system with that legacy qemu
version specific to xen, qemu-xen-traditional. So to work
reliably, we should scan through all the ISA bridge devices
and check for the first match, instead of only checking the
first one.
But actually, qemu-xen-traditional, is always enumerated with
Dev31:Fun0, 00:1f.0 as follows:
hw/pt-graphics.c:
intel_pch_init()
|
+ pci_isa_bridge_init(bus, PCI_DEVFN(0x1f, 0), ...);
so this mean that isa bridge is still represented with Dev31:Func0
like the native OS. Furthermore, currently we're pushing VGA
passthrough support into qemu upstream, and with some discussion,
we wouldn't set the bridge class type and just expose this devfn.
So we just go back to check devfn to make life normal.
Signed-off-by: Tiejun Chen tiejun.c...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 19 +++
1 file changed, 3 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 651e65e..cb2526e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -417,18 +417,8 @@ void intel_detect_pch(struct drm_device *dev)
return;
}
- /*
-* The reason to probe ISA bridge instead of Dev31:Fun0 is to
-* make graphics device passthrough work easy for VMM, that only
-* need to expose ISA bridge to let driver know the real hardware
-* underneath. This is a requirement from virtualization team.
-*
-* In some virtualized environments (e.g. XEN), there is irrelevant
-* ISA bridge in the system. To work reliably, we should scan trhough
-* all the ISA bridge devices and check for the first match, instead
-* of only checking the first one.
-*/
- while ((pch = pci_get_class(PCI_CLASS_BRIDGE_ISA 8, pch))) {
+ pch = pci_get_bus_and_slot(0, PCI_DEVFN(0x1f, 0));
+ if (pch) {
Then if you want to use this slot for something else, what happens?
I think this slot is always occupied to be dedicated to this ISA bridge
in the platform.
So don't worry, the drivers in Linux and Windows can live with this.
Thanks
Tiejun
If you want to relax the PCI_CLASS_BRIDGE_ISA requirement when
running on top of a hypervisor, just scan all devices.
if (pch-vendor == PCI_VENDOR_ID_INTEL) {
unsigned short id = pch-device
INTEL_PCH_DEVICE_ID_MASK;
dev_priv-pch_id = id;
@@ -462,10 +452,7 @@ void intel_detect_pch(struct drm_device *dev)
DRM_DEBUG_KMS(Found LynxPoint LP PCH\n);
WARN_ON(!IS_HASWELL(dev));
WARN_ON(!IS_ULT(dev));
- } else
- continue;
-
- break;
+ }
}
}
if (!pch)
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC][PATCH] gpu:drm:i915:intel_detect_pch: back to check devfn instead of check class type
On 2014/7/2 14:21, Michael S. Tsirkin wrote:
On Thu, Jun 19, 2014 at 05:53:51PM +0800, Tiejun Chen wrote:
Originally the reason to probe ISA bridge instead of Dev31:Fun0
is to make graphics device passthrough work easy for VMM, that
only need to expose ISA bridge to let driver know the real
hardware underneath. This is a requirement from virtualization
team. Especially in that virtualized environments, XEN, there
is irrelevant ISA bridge in the system with that legacy qemu
version specific to xen, qemu-xen-traditional. So to work
reliably, we should scan through all the ISA bridge devices
and check for the first match, instead of only checking the
first one.
But actually, qemu-xen-traditional, is always enumerated with
Dev31:Fun0, 00:1f.0 as follows:
hw/pt-graphics.c:
intel_pch_init()
|
+ pci_isa_bridge_init(bus, PCI_DEVFN(0x1f, 0), ...);
so this mean that isa bridge is still represented with Dev31:Func0
like the native OS. Furthermore, currently we're pushing VGA
passthrough support into qemu upstream, and with some discussion,
we wouldn't set the bridge class type and just expose this devfn.
So we just go back to check devfn to make life normal.
Signed-off-by: Tiejun Chen tiejun.c...@intel.com
Making sure all relevant people are Cc'd.
Are you saying you and Paolo? Or others I'm missing?
I think we should wait for the hypervisor to settle
on what it wants to implement before making guest
changes. Otherwise we'll just add more guest configurations
that need to be supported.
Agreed. Actually this is why I just send this by RFC.
Thanks
Tiejun
---
drivers/gpu/drm/i915/i915_drv.c | 19 +++
1 file changed, 3 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 651e65e..cb2526e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -417,18 +417,8 @@ void intel_detect_pch(struct drm_device *dev)
return;
}
- /*
-* The reason to probe ISA bridge instead of Dev31:Fun0 is to
-* make graphics device passthrough work easy for VMM, that only
-* need to expose ISA bridge to let driver know the real hardware
-* underneath. This is a requirement from virtualization team.
-*
-* In some virtualized environments (e.g. XEN), there is irrelevant
-* ISA bridge in the system. To work reliably, we should scan trhough
-* all the ISA bridge devices and check for the first match, instead
-* of only checking the first one.
-*/
- while ((pch = pci_get_class(PCI_CLASS_BRIDGE_ISA 8, pch))) {
+ pch = pci_get_bus_and_slot(0, PCI_DEVFN(0x1f, 0));
+ if (pch) {
if (pch-vendor == PCI_VENDOR_ID_INTEL) {
unsigned short id = pch-device
INTEL_PCH_DEVICE_ID_MASK;
dev_priv-pch_id = id;
@@ -462,10 +452,7 @@ void intel_detect_pch(struct drm_device *dev)
DRM_DEBUG_KMS(Found LynxPoint LP PCH\n);
WARN_ON(!IS_HASWELL(dev));
WARN_ON(!IS_ULT(dev));
- } else
- continue;
-
- break;
+ }
}
}
if (!pch)
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support
On 2014/7/16 22:20, Konrad Rzeszutek Wilk wrote:
On Thu, Jul 03, 2014 at 11:27:40PM +0300, Michael S. Tsirkin wrote:
On Thu, Jul 03, 2014 at 12:09:28PM -0700, Jesse Barnes wrote:
On Thu, 3 Jul 2014 14:26:12 -0400
Konrad Rzeszutek Wilk konrad.w...@oracle.com wrote:
On Thu, Jul 03, 2014 at 10:32:12AM +0300, Michael S. Tsirkin wrote:
On Wed, Jul 02, 2014 at 12:23:37PM -0400, Konrad Rzeszutek Wilk wrote:
On Wed, Jul 02, 2014 at 04:50:15PM +0200, Paolo Bonzini wrote:
Il 02/07/2014 16:00, Konrad Rzeszutek Wilk ha scritto:
With this long thread I lost a bit context about the challenges
that exists. But let me try summarizing it here - which will hopefully
get some consensus.
1). Fix IGD hardware to not use Southbridge magic addresses.
We can moan and moan but I doubt it is going to change.
There are two problems:
- Northbridge (i.e. MCH i.e. PCI host bridge) configuration space addresses
Right. So in drivers/gpu/drm/i915/i915_dma.c:
1135 #define MCHBAR_I915 0x44
1136 #define MCHBAR_I965 0x48
1147 int reg = INTEL_INFO(dev)-gen = 4 ? MCHBAR_I965 : MCHBAR_I915;
1152 if (INTEL_INFO(dev)-gen = 4)
1153 pci_read_config_dword(dev_priv-bridge_dev, reg + 4,
temp_hi);
1154 pci_read_config_dword(dev_priv-bridge_dev, reg, temp_lo);
1155 mchbar_addr = ((u64)temp_hi 32) | temp_lo;
and
1139 #define DEVEN_REG 0x54
1193 int mchbar_reg = INTEL_INFO(dev)-gen = 4 ? MCHBAR_I965 :
MCHBAR_I915;
1202 if (IS_I915G(dev) || IS_I915GM(dev)) {
1203 pci_read_config_dword(dev_priv-bridge_dev, DEVEN_REG,
temp);
1204 enabled = !!(temp DEVEN_MCHBAR_EN);
1205 } else {
1206 pci_read_config_dword(dev_priv-bridge_dev, mchbar_reg,
temp);
1207 enabled = temp 1;
1208 }
- Southbridge (i.e. PCH i.e. ISA bridge) vendor/device ID; some versions of
the driver identify it by class, some versions identify it by slot (1f.0).
Right, So in drivers/gpu/drm/i915/i915_drv.c the giant intel_detect_pch
which sets the pch_type based on :
432 if (pch-vendor == PCI_VENDOR_ID_INTEL) {
433 unsigned short id = pch-device
INTEL_PCH_DEVICE_ID_MASK;
434 dev_priv-pch_id = id;
435
436 if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
It checks for 0x3b00, 0x1c00, 0x1e00, 0x8c00 and 0x9c00.
The INTEL_PCH_DEVICE_ID_MASK is 0xff00
To solve the first, make a new machine type, PIIX4-based, and pass through
the registers you need. The patch must document _exactly_ why the registers
are safe to pass. If they are not reserved on PIIX4, the patch must
document what the same offsets mean on PIIX4, and why it's sensible to
assume that firmware for virtual machine will not read/write them. Bonus
point for also documenting the same for Q35.
OK. They look to be related to setting up an MBAR , but I don't understand
why it is needed. Hopefully some of the i915 folks CC-ed here can answer.
In particular, I think setting up MBAR should move out of i915 and become
the bridge driver tweak on linux and windows.
That is an excellent idea.
However I am still curious to _why_ it has to be done in the first place.
The display part of the GPU is partly on the PCH, and it's possible to
mix match them a bit, so we have this detection code to figure things
out. In some cases, the PCH display portion may not even be present,
so we look for that too.
Practically speaking, we could probably assume specific CPU/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.
Beyond that, the other MCH data we need to look at is mirrored into the
GPU's MMIO space on current gens. On older gens, we do need to poke at
the memory controller a bit to get some info (see
intel_setup_mchbar()), but that's not true of newer stuff. Looks like
we only short circuit that on VLV though; we could probably do it on
SNB+.
That sounds great. Tiejun could you confirm that with
windows driver guys too?
ping?
Allen,
Could you reply this?
Thanks
Tiejun
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support
On 2014/7/24 4:54, Konrad Rzeszutek Wilk wrote: On Sat, Jul 19, 2014 at 12:27:21AM +, Kay, Allen M wrote: For the MCH PCI registers that do need to be read - can you tell us which ones those are? In qemu/hw/xen_pt_igd.c/igd_pci_read(), following MCH PCI config register reads are passthrough to the host HW. Some of the registers are needed by Ironlake GFX driver which we probably can remove. I did a trace recently on Broadwell, the number of register accessed are even smaller (0, 2, 2c, 2e, 50, 52, a0, a4). Given that we now have integrated MCH and GPU in the same socket, looks like driver can easily remove reads for offsets 0 - 0x2e. case 0x00:/* vendor id */ case 0x02:/* device id */ case 0x08:/* revision id */ case 0x2c:/* sybsystem vendor id */ case 0x2e:/* sybsystem id */ Right. We can fix the i915 to use the mechanism that Michael mentioned. (see attached RFC patches) case 0x50:/* SNB: processor graphics control register */ case 0x52:/* processor graphics control register */ case 0xa0:/* top of memory */ case 0xb0:/* ILK: BSM: should read from dev 2 offset 0x5c */ case 0x58:/* SNB: PAVPC Offset */ case 0xa4:/* SNB: graphics base of stolen memory */ case 0xa8:/* SNB: base of GTT stolen memory */ I dug in the intel-gtt.c (see ironlake_gtt_driver) to figure out this a bit more. As in, I speculated, what if we returned 0 (and not implement any support for reading from these registers). What would happen? 0x52 for Ironlake (g5): -- It looks like intel_gmch_probe is called when i915_gem_gtt_init starts (there is a lot of code that looks to be used between intel-gtt.c and i915.c). Anyhow the interesting parts are that i9xx_setup ends up calling ioremap the i915 BAR to setup some of these registers for older generations. Then i965_gtt_total_entries gets which reads 0x52, but it is only needed for v5 generation. For other (v4 and G33) it reads it from the GPU's 0x2020 register. If there is a mismatch, it writes to the GPU at 0x2020 to update the the size based on the bridge. And then it reads from 0x2020 and that is returned and stuck in intel_private.gtt_total_entries. So 0x52 in the emulated bridge could be populated with what the GPU has at 0x2020. And the writes go in the emulated area. 0x52 for v6 - v8: - We seem to go to gen6_gmch_probe which just figures out the the GTT based on the GPU's BAR sizes. The stolen values are read from 0x50 from the GPU. So no need to touch the bridge (see gen6_gmch_probe) OK, so no need to have 0x52 or 0x50 in the bridge. 0xA0: - Could not find any reference in the Linux code. Why would Windows driver need this? If we returned the _real_ TOM would it matter? Is it used to figure out the device should use 32-bit DMA operations or 40-bit? 0xb0 or 0x5c: - No mention of them in the Linux code. 0x58, 0xa4, 0xa8: - No usage of them in the Linux code. We seem to be using the 0x52 from the bridge and 0x2020 from the GPU for this functionality. So in theory*, if using Ironlake we need to have a proper value in 0x52 in the bridge. But for the later chipsets we do not need these values (I am assuming that intel_setup_mchbar can safely return as it does that for Ironlake and could very well for later generations). Can this be reflected in the Windows driver as well? P.S. *theory: That is assuming we modify the Linux i915_drv.c:intel_detect_pch to pick up the id as suggested earlier. See the RFC patches attached. (Not compile tested at all!) I take a look these patches, looks we still scan all PCI Bridge to walk all PCHs. So this means we still need to fake a PCI bridge, right? Or maybe you don't cover this problem this time. I prefer we should check dev slot to get that PCH like my previous patch, gpu:drm:i915:intel_detect_pch: back to check devfn instead of check class type. Because Windows always use this way, so I think this point should be same between Linux and Windows. Or we need anther better way to unify all OSs. Thanks Tiejun Allen -Original Message- From: Konrad Rzeszutek Wilk [mailto:konrad.w...@oracle.com] Sent: Friday, July 18, 2014 6:45 AM To: Kay, Allen M Cc: Michael S. Tsirkin; Jesse Barnes; peter.mayd...@linaro.org; xen-de...@lists.xensource.com; Ross Philipson; airl...@linux.ie; daniel.vet...@ffwll.ch; intel-gfx@lists.freedesktop.org; kelly.zyta...@amd.com; qemu-de...@nongnu.org; Anthony Perard; Stefano Stabellini; anth...@codemonkey.ws; Paolo Bonzini; Zhang, Yang Z; Chen, Tiejun Subject: Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support On Thu, Jul 17, 2014 at 05:37:12PM +, Kay, Allen M wrote
Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support
On 2014/7/26 1:01, Konrad Rzeszutek Wilk wrote: On Thu, Jul 24, 2014 at 09:44:41AM +0800, Chen, Tiejun wrote: On 2014/7/24 4:54, Konrad Rzeszutek Wilk wrote: On Sat, Jul 19, 2014 at 12:27:21AM +, Kay, Allen M wrote: For the MCH PCI registers that do need to be read - can you tell us which ones those are? In qemu/hw/xen_pt_igd.c/igd_pci_read(), following MCH PCI config register reads are passthrough to the host HW. Some of the registers are needed by Ironlake GFX driver which we probably can remove. I did a trace recently on Broadwell, the number of register accessed are even smaller (0, 2, 2c, 2e, 50, 52, a0, a4). Given that we now have integrated MCH and GPU in the same socket, looks like driver can easily remove reads for offsets 0 - 0x2e. case 0x00:/* vendor id */ case 0x02:/* device id */ case 0x08:/* revision id */ case 0x2c:/* sybsystem vendor id */ case 0x2e:/* sybsystem id */ Right. We can fix the i915 to use the mechanism that Michael mentioned. (see attached RFC patches) case 0x50:/* SNB: processor graphics control register */ case 0x52:/* processor graphics control register */ case 0xa0:/* top of memory */ case 0xb0:/* ILK: BSM: should read from dev 2 offset 0x5c */ case 0x58:/* SNB: PAVPC Offset */ case 0xa4:/* SNB: graphics base of stolen memory */ case 0xa8:/* SNB: base of GTT stolen memory */ I dug in the intel-gtt.c (see ironlake_gtt_driver) to figure out this a bit more. As in, I speculated, what if we returned 0 (and not implement any support for reading from these registers). What would happen? 0x52 for Ironlake (g5): -- It looks like intel_gmch_probe is called when i915_gem_gtt_init starts (there is a lot of code that looks to be used between intel-gtt.c and i915.c). Anyhow the interesting parts are that i9xx_setup ends up calling ioremap the i915 BAR to setup some of these registers for older generations. Then i965_gtt_total_entries gets which reads 0x52, but it is only needed for v5 generation. For other (v4 and G33) it reads it from the GPU's 0x2020 register. If there is a mismatch, it writes to the GPU at 0x2020 to update the the size based on the bridge. And then it reads from 0x2020 and that is returned and stuck in intel_private.gtt_total_entries. So 0x52 in the emulated bridge could be populated with what the GPU has at 0x2020. And the writes go in the emulated area. 0x52 for v6 - v8: - We seem to go to gen6_gmch_probe which just figures out the the GTT based on the GPU's BAR sizes. The stolen values are read from 0x50 from the GPU. So no need to touch the bridge (see gen6_gmch_probe) OK, so no need to have 0x52 or 0x50 in the bridge. 0xA0: - Could not find any reference in the Linux code. Why would Windows driver need this? If we returned the _real_ TOM would it matter? Is it used to figure out the device should use 32-bit DMA operations or 40-bit? 0xb0 or 0x5c: - No mention of them in the Linux code. 0x58, 0xa4, 0xa8: - No usage of them in the Linux code. We seem to be using the 0x52 from the bridge and 0x2020 from the GPU for this functionality. So in theory*, if using Ironlake we need to have a proper value in 0x52 in the bridge. But for the later chipsets we do not need these values (I am assuming that intel_setup_mchbar can safely return as it does that for Ironlake and could very well for later generations). Can this be reflected in the Windows driver as well? P.S. *theory: That is assuming we modify the Linux i915_drv.c:intel_detect_pch to pick up the id as suggested earlier. See the RFC patches attached. (Not compile tested at all!) I take a look these patches, looks we still scan all PCI Bridge to walk all PCHs. So this means we still need to fake a PCI bridge, right? Or maybe you don't cover this problem this time. I totally forgot. Feel free to fix that. Sorry for this delay response. I prefer we should check dev slot to get that PCH like my previous patch, Uh? Your patch was checking for 0:1f.0, not the slot of the device. Yes. (see https://lkml.org/lkml/2014/6/19/121) gpu:drm:i915:intel_detect_pch: back to check devfn instead of check class type. Because Windows always use this way, so I think this point should be same between Linux and Windows. Didn't we discuss that we did not want to pass in PCH at all if we can? I'm a bit confused since I guess I'm missing something important in this long discussion. I guess we just fake a simple PCIe device but just has PCI_VENDOR_ID_XEN/Real_PCH_Device_ID, and then well probe such a PCIe device inside intel_detect_pch(), right? And this means we will not support those existing drivers? And from
Re: [Intel-gfx] ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support
On 2014/7/29 16:32, Paolo Bonzini wrote: Il 29/07/2014 08:59, Chen, Tiejun ha scritto: (see https://lkml.org/lkml/2014/6/19/121) gpu:drm:i915:intel_detect_pch: back to check devfn instead of check class type. Because Windows always use this way, so I think this point should be same between Linux and Windows. Didn't we discuss that we did not want to pass in PCH at all if we can? I'm a bit confused since I guess I'm missing something important in this long discussion. I guess we just fake a simple PCIe device but just has PCI_VENDOR_ID_XEN/Real_PCH_Device_ID, and then well probe such a PCIe device inside intel_detect_pch(), right? Yes, for the special PIIX4 legacy machine type we want to do that and place the device at 00:1f.0. Got it. BTW, please review those patches implemented a separate machine, xenigd, firstly. Then I can step on this point. Thanks Tiejun ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] How to create PCH to support those existing driver
On 2014/8/18 16:21, Michael S. Tsirkin wrote:
On Mon, Aug 18, 2014 at 11:06:29AM +0800, Chen, Tiejun wrote:
On 2014/8/17 18:32, Michael S. Tsirkin wrote:
On Fri, Aug 15, 2014 at 09:58:40AM +0800, Chen, Tiejun wrote:
Michael and Paolo,
Please re-post discussion on list. These off list ones are just
wasting time since they invariably have to be repeated on list again.
Okay, now just reissue this discussion to all related guys. And do you think
we need to discuss in public, qemu and xen mail list?
Absolutely.
Now -CC qemu, xen and intel-gfx.
If I'm missing someone important please tell me as well.
After I created that new machine specific to IGD passthrough, xenigd, now I
will step next to register the PCH.
IIRC, our complete solution should be as follows:
#1 create a new machine based on piix, xenigd
This is done with Michael help.
#2 register ISA bridge
1 Its still fixed at 1f.0.
2 ISA bridge's vendor_id/device_id should be emulated but then
subsystem_vendor_id = PCI_VENDOR_ID_XEN;
subsystem_device_id = ISA Bridge's real device id
This mean we need to change driver to walk with this way.
For example, in
case of Linux native driver,
intel_detect_pch()
{
...
if (pch-subsystem_vendor == PCI_VENDOR_ID_XEN)
id = pch-subsystem_device INTEL_PCH_DEVICE_ID_MASK;
Then driver can get that real device id by 'subsystem_device', right?
This is fine now but how to support those existing drivers which are just
Here correct one point, we don't need to care about supporting the legacy
driver since the legacy driver still should work qemu-traditional. So we
just make sure the existing driver with this subsystem_id way can support
those existing and legacy platform.
Now this is clear to me.
dependent on checking real vendor_id/device_id directly,
if (pch-vendor == PCI_VENDOR_ID_INTEL) {
unsigned short id = pch-device INTEL_PCH_DEVICE_ID_MASK
Maybe I'm missing something, please hint me.
Thanks
Tiejun
The subsystem id was just one idea.
But from that email thread, RH / Intel Virtualization Engineering Meeting -
Minutes 7/10, I didn't see other idea we should prefer currently.
What was finally agreed for future drivers is that guests will get all
information they need from the video card, this ID hack was needed only
for very old legacy devices.
http://article.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/42258
So this is for newer guests, they will work without need
for hacks, like any other device.
Actually we had a meeting to discuss our future solution, but seems you were
on vacation at that moment :)
In that meeting we had an agreement between us and some upstream guys.
We will have such a PCI capability structure in this PCI device to represent
all information in the future. This make sens to Intel as well.
Maybe Allen or Paolo known more details.
But obviously this a long-term solution, so currently we will work with this
subsystem_id way temporarily. And this way is accepted by those guys in the
meeting.
I don't see the point really. If you are modifying the driver,
Yes, we need to modify something in the driver.
why not modify it to its final form.
What's your final form?
As I track that email thread, seems the follows is just a way you guys
achieve a better agreement.
why not set the subsys vid to the RH/Quamranet/Virtio VID, so it's
obvious for the use-match?
That's exactly the suggestion. Though upstream they might be using the
XenSource id since the patches were for Xen.
Paolo
Or I'm missing something?
Thanks
Tiejun
For existing drivers: Vendor ID is intel anyway.
Yes.
For device ID, override it through a property
or something. But I think poking at the real host from
qemu is a mistake though, host is not
protected by iommu.
Two possible suggestions were to reverse-detect
id of the device from the card that is assigned,
I guess you're saying pci_get_device(vendor/devices_ids), right?
or just make it a user property, and move the smarts
to management.
Sorry could you elaborate this way in detail?
Thanks
Tiejun
Will do but let's do it on the mailing list.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] How to create PCH to support those existing driver
On 2014/8/18 17:58, Michael S. Tsirkin wrote:
On Mon, Aug 18, 2014 at 05:01:25PM +0800, Chen, Tiejun wrote:
On 2014/8/18 16:21, Michael S. Tsirkin wrote:
On Mon, Aug 18, 2014 at 11:06:29AM +0800, Chen, Tiejun wrote:
On 2014/8/17 18:32, Michael S. Tsirkin wrote:
On Fri, Aug 15, 2014 at 09:58:40AM +0800, Chen, Tiejun wrote:
Michael and Paolo,
Please re-post discussion on list. These off list ones are just
wasting time since they invariably have to be repeated on list again.
Okay, now just reissue this discussion to all related guys. And do you think
we need to discuss in public, qemu and xen mail list?
Absolutely.
Now -CC qemu, xen and intel-gfx.
If I'm missing someone important please tell me as well.
After I created that new machine specific to IGD passthrough, xenigd, now I
will step next to register the PCH.
IIRC, our complete solution should be as follows:
#1 create a new machine based on piix, xenigd
This is done with Michael help.
#2 register ISA bridge
1 Its still fixed at 1f.0.
2 ISA bridge's vendor_id/device_id should be emulated but then
subsystem_vendor_id = PCI_VENDOR_ID_XEN;
subsystem_device_id = ISA Bridge's real device id
This mean we need to change driver to walk with this way.
For example, in
case of Linux native driver,
intel_detect_pch()
{
...
if (pch-subsystem_vendor == PCI_VENDOR_ID_XEN)
id = pch-subsystem_device INTEL_PCH_DEVICE_ID_MASK;
Then driver can get that real device id by 'subsystem_device', right?
This is fine now but how to support those existing drivers which are just
Here correct one point, we don't need to care about supporting the legacy
driver since the legacy driver still should work qemu-traditional. So we
just make sure the existing driver with this subsystem_id way can support
those existing and legacy platform.
Now this is clear to me.
dependent on checking real vendor_id/device_id directly,
if (pch-vendor == PCI_VENDOR_ID_INTEL) {
unsigned short id = pch-device INTEL_PCH_DEVICE_ID_MASK
Maybe I'm missing something, please hint me.
Thanks
Tiejun
The subsystem id was just one idea.
But from that email thread, RH / Intel Virtualization Engineering Meeting -
Minutes 7/10, I didn't see other idea we should prefer currently.
What was finally agreed for future drivers is that guests will get all
information they need from the video card, this ID hack was needed only
for very old legacy devices.
http://article.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/42258
So this is for newer guests, they will work without need
for hacks, like any other device.
Actually we had a meeting to discuss our future solution, but seems you were
on vacation at that moment :)
In that meeting we had an agreement between us and some upstream guys.
We will have such a PCI capability structure in this PCI device to represent
all information in the future. This make sens to Intel as well.
Maybe Allen or Paolo known more details.
But obviously this a long-term solution, so currently we will work with this
subsystem_id way temporarily. And this way is accepted by those guys in the
meeting.
I don't see the point really. If you are modifying the driver,
Yes, we need to modify something in the driver.
why not modify it to its final form.
What's your final form?
Avoid poking at other devices besides the passed through card.
Get everything from BAR and other registers of the card.
Allen,
Could you reply this?
As I track that email thread, seems the follows is just a way you guys
achieve a better agreement.
why not set the subsys vid to the RH/Quamranet/Virtio VID, so it's
obvious for the use-match?
That's exactly the suggestion. Though upstream they might be using the
XenSource id since the patches were for Xen.
Paolo
Or I'm missing something?
Thanks
Tiejun
I thought the point of this work is to make existing
linux/windows drivers work. Is it or isn't it?
Yes. We need change driver as I said previously like this,
2 ISA bridge's vendor_id/device_id should be emulated but then
subsystem_vendor_id = PCI_VENDOR_ID_XEN;
subsystem_device_id = ISA Bridge's real device id
This mean we need to change driver to walk with this way.
For example, in
case of Linux native driver,
intel_detect_pch()
{
...
if (pch-subsystem_vendor == PCI_VENDOR_ID_XEN)
id = pch-subsystem_device INTEL_PCH_DEVICE_ID_MASK;
This is a minimal change to driver as we discussed.
Wrt changing drivers, change them to behave sanely, like all other
drivers, and avoid poking at the chipset.
http://article.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/42258
Seems to suggest one way to do this.
Can what is suggested there work for existing devices?
Again, Allen,
Are you sure if this suggestion can work?
Thanks
Tiejun
For existing drivers: Vendor ID is intel anyway.
Yes.
For device ID
Re: [Intel-gfx] How to create PCH to support those existing driver
On 2014/8/20 5:51, Michael S. Tsirkin wrote: On Tue, Aug 19, 2014 at 09:24:03PM +, Kay, Allen M wrote: Allen, Could you reply this? Let me summarized what we have discussed and learned so far: 1) Future Windows/Linux IGD drivers will be modified to restrain from accessing MCH/PCH devices. We are prototyping this in Windows driver right now and will pass the same methodology to Linux driver once we have a workable solution. The goal is removing all MCH/PCH accesses in the IGD driver. 2) We want the same solution to work in both native and virtualization environments. Given most driver developers test their changes only in native environment, doing anything specific for virtualization in the driver will cause frequent breakage for virtualization use cases. 3) Back porting this new code to support previous generations of HW will be problematic if not impossible. Each Windows IGD driver release binary supports two generations of HW. For example, 15.36 driver supports Broadwell/Haswell, 15.33 driver supports Haswell/IvyBridge, 15.31 driver supports Ivybridge/Sandybridge, etc. Once the driver is product validated, there is little opportunity to go back and make high impact changes that might affect stability in native environment. 4) I agree there is little reason to do anything that requires driver changes at this point, unless it is the final desired solution. The question is whether/how to support IGD passthrough for previous generations of HW? 1) If we want to support SandyBridge/IvyBridge/Haswell/Broadwell, we will need most of the original QEMU patches. We might be able to do without igd_pci_write(). I have tested QEMU changes without igd_pci_write() on both Haswell/Broadwell and Windows booted without any problems. This will limit only read operations which should reduce a quite a bit of risk to the host platform. Excellent. I was thinking about changing host's driver to do the writes in a safe manner, but if don't need that, all the better. As a next step, we need to limit read operations to specific set of registers that we can validate. We can't just pass through reads blindly to host, pci reads have side-effects and host chipset isn't protected by the iommu. Since these are legacy devices and drivers, it should be possible to enumerate all registers that they need. 2) If we want the upstream QEMU only work with future driver version, then most of the IGD passthrough patch is probably not needed - with exception of opregion mapping handlers. The downside is products that depend on this feature will need to apply private patches separately to re-enable IGD passthrough capability. Any advice on how should Tiejun proceed from here? Allen I'm fine with either trying to make existing windows and linux drivers work, or waiting for future drivers. Michael and Allen, As I concern, now we may not take a consideration of supporting those existing drivers, just leave to qemu-traditional in Xen code repertory. I think what we should do here just focus on supporting all platforms including those legacy platforms. To me, quick hacks that need minor changes in driver but don't avoid poking at MCH/PCH don't seem to have value, So to me, that subsystem id way is more clear rather than others because I'm not sure its really possible not to poke MCH/PCH theoretically both Windows and Linux driver. Allen, I think Michael is saying this, http://article.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/42258 What about your opinion? I know I proposed some of these myself but this was before I realised a cleaner solution is possible. I think all we are fine if this really come true :) Tiejun ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC][PATCH] gpu:drm:i915:intel_detect_pch: back to check devfn instead of check class type
Just ping, any comments?
Thanks
Tiejun
On 2014/6/19 17:53, Tiejun Chen wrote:
Originally the reason to probe ISA bridge instead of Dev31:Fun0
is to make graphics device passthrough work easy for VMM, that
only need to expose ISA bridge to let driver know the real
hardware underneath. This is a requirement from virtualization
team. Especially in that virtualized environments, XEN, there
is irrelevant ISA bridge in the system with that legacy qemu
version specific to xen, qemu-xen-traditional. So to work
reliably, we should scan through all the ISA bridge devices
and check for the first match, instead of only checking the
first one.
But actually, qemu-xen-traditional, is always enumerated with
Dev31:Fun0, 00:1f.0 as follows:
hw/pt-graphics.c:
intel_pch_init()
|
+ pci_isa_bridge_init(bus, PCI_DEVFN(0x1f, 0), ...);
so this mean that isa bridge is still represented with Dev31:Func0
like the native OS. Furthermore, currently we're pushing VGA
passthrough support into qemu upstream, and with some discussion,
we wouldn't set the bridge class type and just expose this devfn.
So we just go back to check devfn to make life normal.
Signed-off-by: Tiejun Chen tiejun.c...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 19 +++
1 file changed, 3 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 651e65e..cb2526e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -417,18 +417,8 @@ void intel_detect_pch(struct drm_device *dev)
return;
}
- /*
-* The reason to probe ISA bridge instead of Dev31:Fun0 is to
-* make graphics device passthrough work easy for VMM, that only
-* need to expose ISA bridge to let driver know the real hardware
-* underneath. This is a requirement from virtualization team.
-*
-* In some virtualized environments (e.g. XEN), there is irrelevant
-* ISA bridge in the system. To work reliably, we should scan trhough
-* all the ISA bridge devices and check for the first match, instead
-* of only checking the first one.
-*/
- while ((pch = pci_get_class(PCI_CLASS_BRIDGE_ISA 8, pch))) {
+ pch = pci_get_bus_and_slot(0, PCI_DEVFN(0x1f, 0));
+ if (pch) {
if (pch-vendor == PCI_VENDOR_ID_INTEL) {
unsigned short id = pch-device
INTEL_PCH_DEVICE_ID_MASK;
dev_priv-pch_id = id;
@@ -462,10 +452,7 @@ void intel_detect_pch(struct drm_device *dev)
DRM_DEBUG_KMS(Found LynxPoint LP PCH\n);
WARN_ON(!IS_HASWELL(dev));
WARN_ON(!IS_ULT(dev));
- } else
- continue;
-
- break;
+ }
}
}
if (!pch)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC][PATCH] gpu:drm:i915:intel_detect_pch: back to check devfn instead of check class type
On 2014/6/20 20:32, Daniel Vetter wrote:
Well I have no clue about forwarding the intel gpu to virtualized
hosts and also no idea who could review this really. There's been a
bit a discussion around the iommu mapping forwarding and similar
No, this doesn't affect IOMMU mapping.
topics though. So I really wonder how well our driver works in this
use case ...
In native case the truth is intel_detect_pch() needs to probe the
00:15.0 to determine what type current pch is, then the i915 driver can
be initialized correctly. Actually the 00:15.0 is just a ISA bridge.
In virtualized case we thought this ISA bridge mayn't be represented
with 00:15.0 originally by qemu-xen-traditional. So instead of checking
devfn, the i915 driver check the class type to get this ISA bridge.
But with my investigation,qemu-xen-traditional always set 00:15.0
explicitly to represent this ISA bridge. And especially, we wouldn't
provide that ISA bridge with an explicit class type in qemu-upstream, so
we need to the i915 driver to probe pch by checking devfn.
This should work both on the native case and the virtualized case.
Thanks
Tiejun
-Daniel
On Fri, Jun 20, 2014 at 11:40 AM, Chen, Tiejun tiejun.c...@intel.com wrote:
Just ping, any comments?
Thanks
Tiejun
On 2014/6/19 17:53, Tiejun Chen wrote:
Originally the reason to probe ISA bridge instead of Dev31:Fun0
is to make graphics device passthrough work easy for VMM, that
only need to expose ISA bridge to let driver know the real
hardware underneath. This is a requirement from virtualization
team. Especially in that virtualized environments, XEN, there
is irrelevant ISA bridge in the system with that legacy qemu
version specific to xen, qemu-xen-traditional. So to work
reliably, we should scan through all the ISA bridge devices
and check for the first match, instead of only checking the
first one.
But actually, qemu-xen-traditional, is always enumerated with
Dev31:Fun0, 00:1f.0 as follows:
hw/pt-graphics.c:
intel_pch_init()
|
+ pci_isa_bridge_init(bus, PCI_DEVFN(0x1f, 0), ...);
so this mean that isa bridge is still represented with Dev31:Func0
like the native OS. Furthermore, currently we're pushing VGA
passthrough support into qemu upstream, and with some discussion,
we wouldn't set the bridge class type and just expose this devfn.
So we just go back to check devfn to make life normal.
Signed-off-by: Tiejun Chen tiejun.c...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 19 +++
1 file changed, 3 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c
b/drivers/gpu/drm/i915/i915_drv.c
index 651e65e..cb2526e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -417,18 +417,8 @@ void intel_detect_pch(struct drm_device *dev)
return;
}
- /*
-* The reason to probe ISA bridge instead of Dev31:Fun0 is to
-* make graphics device passthrough work easy for VMM, that only
-* need to expose ISA bridge to let driver know the real hardware
-* underneath. This is a requirement from virtualization team.
-*
-* In some virtualized environments (e.g. XEN), there is
irrelevant
-* ISA bridge in the system. To work reliably, we should scan
trhough
-* all the ISA bridge devices and check for the first match,
instead
-* of only checking the first one.
-*/
- while ((pch = pci_get_class(PCI_CLASS_BRIDGE_ISA 8, pch))) {
+ pch = pci_get_bus_and_slot(0, PCI_DEVFN(0x1f, 0));
+ if (pch) {
if (pch-vendor == PCI_VENDOR_ID_INTEL) {
unsigned short id = pch-device
INTEL_PCH_DEVICE_ID_MASK;
dev_priv-pch_id = id;
@@ -462,10 +452,7 @@ void intel_detect_pch(struct drm_device *dev)
DRM_DEBUG_KMS(Found LynxPoint LP PCH\n);
WARN_ON(!IS_HASWELL(dev));
WARN_ON(!IS_ULT(dev));
- } else
- continue;
-
- break;
+ }
}
}
if (!pch)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC][PATCH] gpu:drm:i915:intel_detect_pch: back to check devfn instead of check class type
On 2014/6/20 20:48, Paolo Bonzini wrote: Il 19/06/2014 11:53, Tiejun Chen ha scritto: so this mean that isa bridge is still represented with Dev31:Func0 like the native OS. Furthermore, currently we're pushing VGA passthrough support into qemu upstream, and with some discussion, we wouldn't set the bridge class type and just expose this devfn. Even this is not really optimal. It just happens to work just because QEMU's machine is currently a PCI machine with the ISA bridge on 00:01.0. As soon as you'll try doing integrated graphics passthrough on a PCIe machine type (such as QEMU's -M q35) things will break down just as badly. Sorry, I can't understand why this is related to the ISA bridge, 00:01.0 or even other PCIe machine type. In virtualized case we always need to create this ISA bridge as a devfn, 00:15.0, work for the i915 driver to support IGD passthrough. In qemu-xen-traditional, this ISA bridge is already created as follows: 1 We set this ISA type explicitly; 2 We register that as 00:15.0. In qemu-upstream, as you commented we can't create this as a ISA class type explicitly. So we compromise by faking this ISA bridge without ISA class type setting (as I recall you already said this way is slightly better). Maybe we will figure better way in the future. But anyway, this is always registered as 00:15.0, right? So I think the i915 driver can go back to probe the devfn like the native environment. If I'm wrong please correct me. Thanks Tiejun ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
