| Message ID | 20140702162337.GB32380@laptop.dumpdata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Il 02/07/2014 18:23, Konrad Rzeszutek Wilk ha scritto: > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 651e65e..03f2829 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -433,6 +433,8 @@ void intel_detect_pch(struct drm_device *dev) > unsigned short id = pch->device & INTEL_PCH_DEVICE_ID_MASK; > dev_priv->pch_id = id; > > + if (pch->subsystem_vendor == PCI_VENDOR_ID_XEN) > + id = pch->device & INTEL_PCH_DEVICE_ID_MASK; Actually you could look at *dev*'s subsystem IDs and skip the pch lookup completely. Paolo > if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) { > dev_priv->pch_type = PCH_IBX; > DRM_DEBUG_KMS("Found Ibex Peak PCH\n"); >> >
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. > > > > > Regarding the second, fixing IGD hardware to not rely on chipset magic is a > > no-go, I agree. I disagree that it's a no-go to define a "backdoor" that > > lets a hypervisor pass the right information to the driver without hacking > > the chipset device model. > > > > The hardware folks would have to give us a place for a pair of registers > > (something like data/address), and a bit somewhere else that would be always > > 0 on hardware and always 1 if the hypervisor is implementing the pair of > > registers. This is similar to CPUID, which has the HYPERVISOR bit + > > hypervisor-defined leaves at 0x40000000. > > > > The data/address pair could be in a BAR, in configuration space, in the low > > VGA ports at 0x3c0-0x3df, wherever. The hypervisor bit can be in the same > > place or somewhere else---again, whatever is convenient for the hardware > > folks. We just need *one bit* that is known-zero on all hardware, and 8 > > bytes in a reserved area. I don't think it's too hard to find this space, > > and I really, really would like Intel to follow up on a paravirtualized > > backdoor. > > > > That said, we have the problem of existing guests, so I agree something else > > is needed. > > > > > a) Two bridges - one 'passthrough' and the legacy ISA bridge > > > that QEMU emulates. Both Linux and Windows are OK with > > > two bridges (even thought it is pretty weird). > > > > This is pretty much the only solution for existing Linux guests that look up > > the southbridge by class. > > Right. > > > > The proposed solution here is to define a new "pci stub" device in QEMU that > > lets you define a do-nothing device with your desired vendor ID, device ID, > > class and optionally subsystem IDs. > > <nods> > > > > The new machine type (the one that instantiates the special > > IGD-passthrough-enabled northbridge) can then instantiate this stub device > > at 1f.0 with the desired vendor ID, device ID and class ID. > > Which is kind of neat because you can use a different type of device ID with > (say make it look like Ibex Peak) and pair it up with an IGD that is found > only on LynxPoint. Oh fun! > > > > If we cannot get the paravirtualized backdoor, it would also make sense to: > > > > - have drivers standardize on a single way to probe the southbridge > > > > - make this be neither by class (because the firmware wants to distinguish > > the actual ISA bridge from the stub, and it can do so by looking up the > > class), nor by slot (because this conflicts with the Q35 chipset model that > > has the southbridge at 1f.0). > > > > mst's proposal was to probe by subsystem id. I'm not sure I understood the > > details exactly, but I trust him. :) However, in case it wasn't clear I > > think a paravirtualized backdoor would still be better. > > OK, like this: > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 651e65e..03f2829 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -433,6 +433,8 @@ void intel_detect_pch(struct drm_device *dev) > unsigned short id = pch->device & INTEL_PCH_DEVICE_ID_MASK; > dev_priv->pch_id = id; > > + if (pch->subsystem_vendor == PCI_VENDOR_ID_XEN) > + id = pch->device & INTEL_PCH_DEVICE_ID_MASK; > if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) { > dev_priv->pch_type = PCH_IBX; > DRM_DEBUG_KMS("Found Ibex Peak PCH\n"); No! The point is to avoid looking at PCH at all, only look at the card itself. > > > > > b) One bridge - the one that QEMU emulates - and lets emulate > > > more of the registers (by emulate - I mean for some get the > > > data from the real hardware). > > > > > > b1). We can't use the legacy because the registers are > > > above 256 (is that correct? Did I miss something?) > > > > As I understand it, mst brought up Q35 because the northbridge configuration > > space layout might be more similar to what the driver expects than for > > PIIX4. But I don't think anyone really said whether this is true or false. > > > > I think Q35 is absolutely not a requirement for IGD passthrough, especially > > until this statement is either proved or disproved. > > OK, so lets drop that. > > > > >4). Code does a bit of sysfs that could use some refacturing with > > > the KVM code. > > > Problem: More time needed to do the code restructing. > > > > FWIW, I don't really care about code sharing with KVM. That's a separate > > problem and it's not necessary to bring it up and make waters even more > > muddy. > > > > OK, lets drop that for now. > > Paolo
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. It would then run on the priveledged host and we won't need to deal with it in QEMU. > > > > Regarding the second, fixing IGD hardware to not rely on chipset magic is a > > no-go, I agree. I disagree that it's a no-go to define a "backdoor" that > > lets a hypervisor pass the right information to the driver without hacking > > the chipset device model. > > > > The hardware folks would have to give us a place for a pair of registers > > (something like data/address), and a bit somewhere else that would be always > > 0 on hardware and always 1 if the hypervisor is implementing the pair of > > registers. This is similar to CPUID, which has the HYPERVISOR bit + > > hypervisor-defined leaves at 0x40000000. > > > > The data/address pair could be in a BAR, in configuration space, in the low > > VGA ports at 0x3c0-0x3df, wherever. The hypervisor bit can be in the same > > place or somewhere else---again, whatever is convenient for the hardware > > folks. We just need *one bit* that is known-zero on all hardware, and 8 > > bytes in a reserved area. I don't think it's too hard to find this space, > > and I really, really would like Intel to follow up on a paravirtualized > > backdoor. > > > > That said, we have the problem of existing guests, so I agree something else > > is needed. > > > > > a) Two bridges - one 'passthrough' and the legacy ISA bridge > > > that QEMU emulates. Both Linux and Windows are OK with > > > two bridges (even thought it is pretty weird). > > > > This is pretty much the only solution for existing Linux guests that look up > > the southbridge by class. > > Right. > > > > The proposed solution here is to define a new "pci stub" device in QEMU that > > lets you define a do-nothing device with your desired vendor ID, device ID, > > class and optionally subsystem IDs. > > <nods> > > > > The new machine type (the one that instantiates the special > > IGD-passthrough-enabled northbridge) can then instantiate this stub device > > at 1f.0 with the desired vendor ID, device ID and class ID. > > Which is kind of neat because you can use a different type of device ID with > (say make it look like Ibex Peak) and pair it up with an IGD that is found > only on LynxPoint. Oh fun! > > > > If we cannot get the paravirtualized backdoor, it would also make sense to: > > > > - have drivers standardize on a single way to probe the southbridge > > > > - make this be neither by class (because the firmware wants to distinguish > > the actual ISA bridge from the stub, and it can do so by looking up the > > class), nor by slot (because this conflicts with the Q35 chipset model that > > has the southbridge at 1f.0). > > > > mst's proposal was to probe by subsystem id. I'm not sure I understood the > > details exactly, but I trust him. :) However, in case it wasn't clear I > > think a paravirtualized backdoor would still be better. > > OK, like this: > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 651e65e..03f2829 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -433,6 +433,8 @@ void intel_detect_pch(struct drm_device *dev) > unsigned short id = pch->device & INTEL_PCH_DEVICE_ID_MASK; > dev_priv->pch_id = id; > > + if (pch->subsystem_vendor == PCI_VENDOR_ID_XEN) > + id = pch->device & INTEL_PCH_DEVICE_ID_MASK; > if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) { > dev_priv->pch_type = PCH_IBX; > DRM_DEBUG_KMS("Found Ibex Peak PCH\n"); > > > > > b) One bridge - the one that QEMU emulates - and lets emulate > > > more of the registers (by emulate - I mean for some get the > > > data from the real hardware). > > > > > > b1). We can't use the legacy because the registers are > > > above 256 (is that correct? Did I miss something?) > > > > As I understand it, mst brought up Q35 because the northbridge configuration > > space layout might be more similar to what the driver expects than for > > PIIX4. But I don't think anyone really said whether this is true or false. > > > > I think Q35 is absolutely not a requirement for IGD passthrough, especially > > until this statement is either proved or disproved. > > OK, so lets drop that. > > > > >4). Code does a bit of sysfs that could use some refacturing with > > > the KVM code. > > > Problem: More time needed to do the code restructing. > > > > FWIW, I don't really care about code sharing with KVM. That's a separate > > problem and it's not necessary to bring it up and make waters even more > > muddy. > > > > OK, lets drop that for now. > > Paolo
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. > It would then run on the priveledged host > and we won't need to deal with it in QEMU.
On Thu, 3 Jul 2014 14:26:12 -0400 Konrad Rzeszutek Wilk <konrad.wilk@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+.
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.wilk@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? > -- > Jesse Barnes, Intel Open Source Technology Center
Il 03/07/2014 21:09, Jesse Barnes ha scritto: > 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. I guess the driver could do that if it finds an unknown PCH device ID. But encoding it in the subsystem device ID could also work and it would be easy to do in the hypervisor. > Beyond that, the other MCH data we need to look at is mirrored into the > GPU's MMIO space on current gens. Heh, that's exactly the same as the paravirtualized solution we were suggesting. ;) Paolo > 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+.
On Fri, Jul 04, 2014 at 08:28:25AM +0200, Paolo Bonzini wrote: > Il 03/07/2014 21:09, Jesse Barnes ha scritto: > >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. > > I guess the driver could do that if it finds an unknown PCH device ID. I would say if possible, do this unconditionally. If this logic is strictly required then I would also check the subsystem vendor id and skip the PCH tricks if it matches Xen. > But > encoding it in the subsystem device ID could also work and it would be easy > to do in the hypervisor. Right, but that's custom code in the hypervisor as opposed to the generic one. If generic one can work, that's much better. > >Beyond that, the other MCH data we need to look at is mirrored into the > >GPU's MMIO space on current gens. > > Heh, that's exactly the same as the paravirtualized solution we were > suggesting. ;) > > Paolo > > >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+.
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.wilk@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? > > > -- > > Jesse Barnes, Intel Open Source Technology Center
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.wilk@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
> That sounds great. Tiejun could you confirm that with windows driver guys too? I believe windows driver can also assume specific CPU/PCH combos. I will discuss this with native Windows driver guys. Preferably, the same code path can be used for both native and virtualization cases to avoid frequent breakage as most developers and QA do not test new code changes in virtualization environment. I have verified that Windows driver do not need to write to any MCH PCI registers on HSW/BDW so the PCI write function can be removed. The MCH PCI registers that need to be read, we are working with HW team to get them mirrored in GPU MMIO registers in future HW. Allen -----Original Message----- From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Michael S. Tsirkin Sent: Thursday, July 03, 2014 1:28 PM To: Jesse Barnes Cc: peter.maydell@linaro.org; xen-devel@lists.xensource.com; Ross Philipson; Konrad Rzeszutek Wilk; airlied@linux.ie; daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Kelly.Zytaruk@amd.com; qemu-devel@nongnu.org; Anthony Perard; Stefano Stabellini; anthony@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 03, 2014 at 12:09:28PM -0700, Jesse Barnes wrote: > On Thu, 3 Jul 2014 14:26:12 -0400 > Konrad Rzeszutek Wilk <konrad.wilk@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? > -- > Jesse Barnes, Intel Open Source Technology Center
On Thu, Jul 17, 2014 at 05:37:12PM +0000, Kay, Allen M wrote: > > That sounds great. Tiejun could you confirm that with windows driver guys too? > > I believe windows driver can also assume specific CPU/PCH combos. I will discuss this with native Windows driver guys. Preferably, the same code path can be used for both native and virtualization cases to avoid frequent breakage as most developers and QA do not test new code changes in virtualization environment. > > I have verified that Windows driver do not need to write to any MCH PCI registers on HSW/BDW so the PCI write function can be removed. The MCH PCI registers that need to be read, we are working with HW team to get them mirrored in GPU MMIO registers in future HW. > For the MCH PCI registers that do need to be read - can you tell us which ones those are? Thank you! > Allen > > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Michael S. Tsirkin > Sent: Thursday, July 03, 2014 1:28 PM > To: Jesse Barnes > Cc: peter.maydell@linaro.org; xen-devel@lists.xensource.com; Ross Philipson; Konrad Rzeszutek Wilk; airlied@linux.ie; daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Kelly.Zytaruk@amd.com; qemu-devel@nongnu.org; Anthony Perard; Stefano Stabellini; anthony@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 03, 2014 at 12:09:28PM -0700, Jesse Barnes wrote: > > On Thu, 3 Jul 2014 14:26:12 -0400 > > Konrad Rzeszutek Wilk <konrad.wilk@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? > > > -- > > Jesse Barnes, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 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 */ 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 */ Allen -----Original Message----- From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com] Sent: Friday, July 18, 2014 6:45 AM To: Kay, Allen M Cc: Michael S. Tsirkin; Jesse Barnes; peter.maydell@linaro.org; xen-devel@lists.xensource.com; Ross Philipson; airlied@linux.ie; daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; Kelly.Zytaruk@amd.com; qemu-devel@nongnu.org; Anthony Perard; Stefano Stabellini; anthony@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 +0000, Kay, Allen M wrote: > > That sounds great. Tiejun could you confirm that with windows driver guys too? > > I believe windows driver can also assume specific CPU/PCH combos. I will discuss this with native Windows driver guys. Preferably, the same code path can be used for both native and virtualization cases to avoid frequent breakage as most developers and QA do not test new code changes in virtualization environment. > > I have verified that Windows driver do not need to write to any MCH PCI registers on HSW/BDW so the PCI write function can be removed. The MCH PCI registers that need to be read, we are working with HW team to get them mirrored in GPU MMIO registers in future HW. > For the MCH PCI registers that do need to be read - can you tell us which ones those are? Thank you! > Allen > > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On > Behalf Of Michael S. Tsirkin > Sent: Thursday, July 03, 2014 1:28 PM > To: Jesse Barnes > Cc: peter.maydell@linaro.org; xen-devel@lists.xensource.com; Ross > Philipson; Konrad Rzeszutek Wilk; airlied@linux.ie; > daniel.vetter@ffwll.ch; intel-gfx@lists.freedesktop.org; > Kelly.Zytaruk@amd.com; qemu-devel@nongnu.org; Anthony Perard; Stefano > Stabellini; anthony@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 03, 2014 at 12:09:28PM -0700, Jesse Barnes wrote: > > On Thu, 3 Jul 2014 14:26:12 -0400 > > Konrad Rzeszutek Wilk <konrad.wilk@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? > > > -- > > Jesse Barnes, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 651e65e..03f2829 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -433,6 +433,8 @@ void intel_detect_pch(struct drm_device *dev) unsigned short id = pch->device & INTEL_PCH_DEVICE_ID_MASK; dev_priv->pch_id = id; + if (pch->subsystem_vendor == PCI_VENDOR_ID_XEN) + id = pch->device & INTEL_PCH_DEVICE_ID_MASK; if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) { dev_priv->pch_type = PCH_IBX; DRM_DEBUG_KMS("Found Ibex Peak PCH\n");