Re: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

Re: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Konrad Rzeszutek Wilk]

On Wed, Jul 02, 2014 at 05:08:43PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jul 02, 2014 at 10:00:33AM -0400, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jul 02, 2014 at 01:33:09PM +0200, Paolo Bonzini wrote:
> > > Il 01/07/2014 19:39, Ross Philipson ha scritto:
> > > >
> > > >We do IGD pass-through in our project (XenClient). The patches
> > > >originally came from our project. We surface the same ISA bridge and
> > > >have never had activation issues on any version of Widows from XP to
> > > >Win8. We do not normally run server platforms so I can't say for sure
> > > >there.
> > > 
> > > The problem is not activation, the problem is that the patches are making
> > > assumptions on the driver and the firmware that might work today but are
> > > IMHO just not sane.
> > > 
> > > I would have no problem with a clean patchset that adds a new machine type
> > > and doesn't touch code in "-M pc", but it looks like mst disagrees.
> > > Ultimately, if a patchset is too hacky for upstream, you can include it in
> > > your downstream XenClient (and XenServer) QEMU branch.  It happens.
> > 
> > And then this discussion will come back again in a year when folks
> > rebase and ask: Why hasn't this been done upstream.
> > 
> > Then the discussion resumes ..
> > 
> > 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.
> 
> Before I answer could you clarify please:
> by Southbridge do you mean the PCH at slot 1f or the MCH at slot 0 or both?

MCH slot. We read/write from this (see intel_setup_mchbar) from couple of
registers (0x44 and 0x48 if gen >= 4, otherwise 0x54). It is hard-coded
in the i915_get_bridge_dev (see ec2a4c3fdc8e82fe82a25d800e85c1ea06b74372)
as 0:0.0 BDF.

The PCH (does not matter where it sits) we only use the model:vendor id
to figure out the pch_type (see intel_detect_pch).

I don't see why that model:vendor_id can't be exposed via checking the
type of device:vendor_id of the IGD itself. CC-ing some Intel i915 authors.

So for the discussion here, when I say Southbridge I mean MCH.
> 
> > 1). Fix IGD hardware to not use Southbridge magic addresses.
> >     We can moan and moan but I doubt it is going to change.
> > 
> > 2). Since we need the Southbridge magic addresses, we can expose
> >     an bridge. [I think everybody agrees that we need to do
> >     that since 1) is no go).
> > 
> > 3). What kind of bridge. We can do:
> > 
> >      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).
> > 
> >      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?)
> > 
> >            b2)  We would need to use the Q35.
> >                 b2a). If we need Q35, that needs to be exposed in
> >                       for Xen guests. That means exposing the 
> >                       MMCONFIG and restructing the E820 to fit that
> >                       in.
> >                       Problem:
> >                         - Migration is not working with Q35.
> >                           (But for v1 you wouldn't migrate, however
> >                            later hardware will surely have SR-IOV so
> >                            we will need to migrate).
> > 
> >                         - There are no developers who have an OK
> >                           from their management to focus on this.
> >                            (Potential solution: Poke Intel management to see
> >                             if they can get more developers on it)
> >                           
> > 
> > 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.
> > 
> > 
> > Is that about correct?
> > 
> > What are folks timezones and the best days next week to talk about
> > this on either Google Hangout or the phone?

Re: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Michael S. Tsirkin]

On Wed, Jul 02, 2014 at 12:05:27PM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Jul 02, 2014 at 05:08:43PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jul 02, 2014 at 10:00:33AM -0400, Konrad Rzeszutek Wilk wrote:
> > > On Wed, Jul 02, 2014 at 01:33:09PM +0200, Paolo Bonzini wrote:
> > > > Il 01/07/2014 19:39, Ross Philipson ha scritto:
> > > > >
> > > > >We do IGD pass-through in our project (XenClient). The patches
> > > > >originally came from our project. We surface the same ISA bridge and
> > > > >have never had activation issues on any version of Widows from XP to
> > > > >Win8. We do not normally run server platforms so I can't say for sure
> > > > >there.
> > > > 
> > > > The problem is not activation, the problem is that the patches are making
> > > > assumptions on the driver and the firmware that might work today but are
> > > > IMHO just not sane.
> > > > 
> > > > I would have no problem with a clean patchset that adds a new machine type
> > > > and doesn't touch code in "-M pc", but it looks like mst disagrees.
> > > > Ultimately, if a patchset is too hacky for upstream, you can include it in
> > > > your downstream XenClient (and XenServer) QEMU branch.  It happens.
> > > 
> > > And then this discussion will come back again in a year when folks
> > > rebase and ask: Why hasn't this been done upstream.
> > > 
> > > Then the discussion resumes ..
> > > 
> > > 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.
> > 
> > Before I answer could you clarify please:
> > by Southbridge do you mean the PCH at slot 1f or the MCH at slot 0 or both?
> 
> MCH slot. We read/write from this (see intel_setup_mchbar) from couple of
> registers (0x44 and 0x48 if gen >= 4, otherwise 0x54). It is hard-coded
> in the i915_get_bridge_dev (see ec2a4c3fdc8e82fe82a25d800e85c1ea06b74372)
> as 0:0.0 BDF.
> 
> The PCH (does not matter where it sits) we only use the model:vendor id
> to figure out the pch_type (see intel_detect_pch).
> 
> I don't see why that model:vendor_id can't be exposed via checking the
> type of device:vendor_id of the IGD itself. CC-ing some Intel i915 authors.
> 
> So for the discussion here, when I say Southbridge I mean MCH.

OK so PIIX spec says:

0x10-4F reserved.

So far so good, it is likely harmless to stick something at 0x44 and
0x48 most guests will very likely just keep ticking.

0x54-0x57 deal with RAM though.

Maybe we can just stick to emulating gen >= 4 for now:
detect it on host and fail assignment.
How old is gen 4?





> > 
> > > 1). Fix IGD hardware to not use Southbridge magic addresses.
> > >     We can moan and moan but I doubt it is going to change.
> > > 
> > > 2). Since we need the Southbridge magic addresses, we can expose
> > >     an bridge. [I think everybody agrees that we need to do
> > >     that since 1) is no go).
> > > 
> > > 3). What kind of bridge. We can do:
> > > 
> > >      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).
> > > 
> > >      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?)
> > > 
> > >            b2)  We would need to use the Q35.
> > >                 b2a). If we need Q35, that needs to be exposed in
> > >                       for Xen guests. That means exposing the 
> > >                       MMCONFIG and restructing the E820 to fit that
> > >                       in.
> > >                       Problem:
> > >                         - Migration is not working with Q35.
> > >                           (But for v1 you wouldn't migrate, however
> > >                            later hardware will surely have SR-IOV so
> > >                            we will need to migrate).
> > > 
> > >                         - There are no developers who have an OK
> > >                           from their management to focus on this.
> > >                            (Potential solution: Poke Intel management to see
> > >                             if they can get more developers on it)
> > >                           
> > > 
> > > 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.
> > > 
> > > 
> > > Is that about correct?
> > > 
> > > What are folks timezones and the best days next week to talk about
> > > this on either Google Hangout or the phone?

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Konrad Rzeszutek Wilk]

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");
> 
> >     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

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Paolo Bonzini]

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");
>> >

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Michael S. Tsirkin]

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

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Michael S. Tsirkin]

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

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Konrad Rzeszutek Wilk]

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.

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Jesse Barnes]

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+.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Michael S. Tsirkin]

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

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Konrad Rzeszutek Wilk]

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

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Chen, Tiejun]

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

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Kay, Allen M]

> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Konrad Rzeszutek Wilk]

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

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Kay, Allen M]

> 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

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Konrad Rzeszutek Wilk]

[-- Attachment #1: Type: text/plain, Size: 11779 bytes --]

On Sat, Jul 19, 2014 at 12:27:21AM +0000, 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!)
> 
> 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

[-- Attachment #2: 0002-i915-drm-xen-Use-subsystem_vendor-and-model-instead-.patch --]
[-- Type: text/plain, Size: 1601 bytes --]

>From d82292c9fde0bd5ad04816096be3c4337660d4c7 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 23 Jul 2014 16:38:27 -0400
Subject: [PATCH 2/3] i915/drm/xen: Use subsystem_vendor and model instead of
 bridge to figure type.

Instead of trying to figure out what type of i915 GPU it is
based on the bridge we will use the subsystem_device if it is
virtualized. The other benefit of this patch is that for
baremetal we can now remove the scanning of the bridge and
just use the GPU's PCI device to scan if we would like.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 404273b..11b6dc1 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -411,6 +411,9 @@ static int set_pch_type(struct pci_dev *pch, struct drm_i915_private *dev_priv)
 	if (pch->vendor != PCI_VENDOR_ID_INTEL)
 		return 0;
 
+	if (pch->subsystem_vendor == PCI_VENDOR_ID_XEN)
+		id = pch->subsystem_device & INTEL_PCH_DEVICE_ID_MASK;
+
 	dev_priv->pch_id = id;
 
 	if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
@@ -478,6 +481,14 @@ void intel_detect_pch(struct drm_device *dev)
 		if (ok)
 			break;
 	}
+	if (!ok) {
+		/*
+		 * Try the GPU in case it is virtualized and we want to use
+		 * the subsystem vendor id instead of the bridge to match.
+		 */
+		pch = dev->pdev;
+		ok = set_pch_type(pch, dev_priv);
+	}
 	if (!ok)
 		DRM_DEBUG_KMS("No PCH found.\n");
 }
-- 
1.9.3


[-- Attachment #3: 0003-i915-drm-xen-If-we-are-running-under-virtualized-env.patch --]
[-- Type: text/plain, Size: 2043 bytes --]

>From 26d593b3b3255a57536aabc5cdf0f57061b6d888 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 23 Jul 2014 16:41:47 -0400
Subject: [PATCH 3/3] i915/drm/xen: If we are running under virtualized env we
 don't need the bridge.

So lets check if the subsystem tells us that we are under Xen
and if so ignore the bridge.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 6c65639..70bd61f 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1123,6 +1123,11 @@ static int i915_set_status_page(struct drm_device *dev, void *data,
 static int i915_get_bridge_dev(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct pci_dev *pch = dev->pdev;
+
+	/* If we are virtualized we don't need the bridge. */
+	if (pch->subsystem_vendor == PCI_VENDOR_ID_XEN)
+		return 0;
 
 	dev_priv->bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0, 0));
 	if (!dev_priv->bridge_dev) {
@@ -1199,6 +1204,9 @@ intel_setup_mchbar(struct drm_device *dev)
 
 	dev_priv->mchbar_need_disable = false;
 
+	if (!dev_priv->bridge_dev)
+		return;
+
 	if (IS_I915G(dev) || IS_I915GM(dev)) {
 		pci_read_config_dword(dev_priv->bridge_dev, DEVEN_REG, &temp);
 		enabled = !!(temp & DEVEN_MCHBAR_EN);
@@ -1801,7 +1809,8 @@ out_regs:
 	intel_uncore_fini(dev);
 	pci_iounmap(dev->pdev, dev_priv->regs);
 put_bridge:
-	pci_dev_put(dev_priv->bridge_dev);
+	if (dev_priv->bridge_dev)
+		pci_dev_put(dev_priv->bridge_dev);
 free_priv:
 	if (dev_priv->slab)
 		kmem_cache_destroy(dev_priv->slab);
@@ -1903,7 +1912,8 @@ int i915_driver_unload(struct drm_device *dev)
 	if (dev_priv->slab)
 		kmem_cache_destroy(dev_priv->slab);
 
-	pci_dev_put(dev_priv->bridge_dev);
+	if (dev_priv->bridge_dev)
+		pci_dev_put(dev_priv->bridge_dev);
 	kfree(dev_priv);
 
 	return 0;
-- 
1.9.3


[-- Attachment #4: 0001-drm-i915-Move-figuring-out-of-the-PCH-type-to-its-ow.patch --]
[-- Type: text/plain, Size: 4524 bytes --]

>From d692d0458f60e3207d7dba615c8cbbcdeca07187 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 23 Jul 2014 16:31:12 -0400
Subject: [PATCH 1/3] drm/i915: Move figuring out of the PCH type to its own
 function and fix reference bug.

We move the figuring of the PCH type to its own function
and simplyfind the loop. Also we fix the 'pci_dev_put' which
previously was only called for the last bridge. Meaning
if the host had two bridges we would only put the last one, but
not the first one. With this we fix that.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 90 ++++++++++++++++++++++-------------------
 1 file changed, 49 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 651e65e..404273b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -404,11 +404,55 @@ static const struct pci_device_id pciidlist[] = {		/* aka */
 MODULE_DEVICE_TABLE(pci, pciidlist);
 #endif
 
+static int set_pch_type(struct pci_dev *pch, struct drm_i915_private *dev_priv)
+{
+	unsigned short id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
+
+	if (pch->vendor != PCI_VENDOR_ID_INTEL)
+		return 0;
+
+	dev_priv->pch_id = id;
+
+	if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
+		dev_priv->pch_type = PCH_IBX;
+		DRM_DEBUG_KMS("Found Ibex Peak PCH\n");
+		WARN_ON(!IS_GEN5(dev));
+	} else if (id == INTEL_PCH_CPT_DEVICE_ID_TYPE) {
+		dev_priv->pch_type = PCH_CPT;
+		DRM_DEBUG_KMS("Found CougarPoint PCH\n");
+		WARN_ON(!(IS_GEN6(dev) || IS_IVYBRIDGE(dev)));
+	} else if (id == INTEL_PCH_PPT_DEVICE_ID_TYPE) {
+		/* PantherPoint is CPT compatible */
+		dev_priv->pch_type = PCH_CPT;
+		DRM_DEBUG_KMS("Found PantherPoint PCH\n");
+		WARN_ON(!(IS_GEN6(dev) || IS_IVYBRIDGE(dev)));
+	} else if (id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
+		dev_priv->pch_type = PCH_LPT;
+		DRM_DEBUG_KMS("Found LynxPoint PCH\n");
+		WARN_ON(!IS_HASWELL(dev));
+		WARN_ON(IS_ULT(dev));
+	} else if (IS_BROADWELL(dev)) {
+		dev_priv->pch_type = PCH_LPT;
+		dev_priv->pch_id =
+			INTEL_PCH_LPT_LP_DEVICE_ID_TYPE;
+		DRM_DEBUG_KMS("This is Broadwell, assuming "
+			      "LynxPoint LP PCH\n");
+	} else if (id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
+		dev_priv->pch_type = PCH_LPT;
+		DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
+		WARN_ON(!IS_HASWELL(dev));
+		WARN_ON(!IS_ULT(dev));
+	} else
+		return 1;
+
+	return 1;
+}
+
 void intel_detect_pch(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct pci_dev *pch = NULL;
-
+	int ok = 0;
 	/* In all current cases, num_pipes is equivalent to the PCH_NOP setting
 	 * (which really amounts to a PCH but no South Display).
 	 */
@@ -429,49 +473,13 @@ void intel_detect_pch(struct drm_device *dev)
 	 * of only checking the first one.
 	 */
 	while ((pch = pci_get_class(PCI_CLASS_BRIDGE_ISA << 8, pch))) {
-		if (pch->vendor == PCI_VENDOR_ID_INTEL) {
-			unsigned short id = pch->device & INTEL_PCH_DEVICE_ID_MASK;
-			dev_priv->pch_id = id;
-
-			if (id == INTEL_PCH_IBX_DEVICE_ID_TYPE) {
-				dev_priv->pch_type = PCH_IBX;
-				DRM_DEBUG_KMS("Found Ibex Peak PCH\n");
-				WARN_ON(!IS_GEN5(dev));
-			} else if (id == INTEL_PCH_CPT_DEVICE_ID_TYPE) {
-				dev_priv->pch_type = PCH_CPT;
-				DRM_DEBUG_KMS("Found CougarPoint PCH\n");
-				WARN_ON(!(IS_GEN6(dev) || IS_IVYBRIDGE(dev)));
-			} else if (id == INTEL_PCH_PPT_DEVICE_ID_TYPE) {
-				/* PantherPoint is CPT compatible */
-				dev_priv->pch_type = PCH_CPT;
-				DRM_DEBUG_KMS("Found PantherPoint PCH\n");
-				WARN_ON(!(IS_GEN6(dev) || IS_IVYBRIDGE(dev)));
-			} else if (id == INTEL_PCH_LPT_DEVICE_ID_TYPE) {
-				dev_priv->pch_type = PCH_LPT;
-				DRM_DEBUG_KMS("Found LynxPoint PCH\n");
-				WARN_ON(!IS_HASWELL(dev));
-				WARN_ON(IS_ULT(dev));
-			} else if (IS_BROADWELL(dev)) {
-				dev_priv->pch_type = PCH_LPT;
-				dev_priv->pch_id =
-					INTEL_PCH_LPT_LP_DEVICE_ID_TYPE;
-				DRM_DEBUG_KMS("This is Broadwell, assuming "
-					      "LynxPoint LP PCH\n");
-			} else if (id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
-				dev_priv->pch_type = PCH_LPT;
-				DRM_DEBUG_KMS("Found LynxPoint LP PCH\n");
-				WARN_ON(!IS_HASWELL(dev));
-				WARN_ON(!IS_ULT(dev));
-			} else
-				continue;
-
+		ok = set_pch_type(pch, dev_priv);
+		pci_dev_put(pch);
+		if (ok)
 			break;
-		}
 	}
-	if (!pch)
+	if (!ok)
 		DRM_DEBUG_KMS("No PCH found.\n");
-
-	pci_dev_put(pch);
 }
 
 bool i915_semaphore_is_enabled(struct drm_device *dev)
-- 
1.9.3


[-- Attachment #5: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Chen, Tiejun]

On 2014/7/24 4:54, Konrad Rzeszutek Wilk wrote:
> On Sat, Jul 19, 2014 at 12:27:21AM +0000, 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.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

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Konrad Rzeszutek Wilk]

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 +0000, 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.
> 
> 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.

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

And from the observation I made above it seems that we can safely do it
under Linux. With Windows I don't know - but I presume the answer is yes too.


> 
> Or we need anther better way to unify all OSs.

Yes. The observation is that a lot of these registers are aliased in the GPU.
As such we can skip some of this bridge poking. Hm. I should have gotten
further and also done this on baremetal, but figured as an RFC it would
paint a picture of what we had in mind?

> 
> Thanks
> Tiejun
> 
> >>
> >>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

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Chen, Tiejun]

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 +0000, 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 the observation I made above it seems that we can safely do it
> under Linux. With Windows I don't know - but I presume the answer is yes too.
>
>
>>
>> Or we need anther better way to unify all OSs.
>
> Yes. The observation is that a lot of these registers are aliased in the GPU.
> As such we can skip some of this bridge poking. Hm. I should have gotten

Sounds reasonable but I'm not an Windows/Linux GFX driver developer, so 
I think we need to wait Allen to double check these points.

Thanks
Tiejun

> further and also done this on baremetal, but figured as an RFC it would
> paint a picture of what we had in mind?
>
>>
>> Thanks
>> Tiejun
>>
>>>>
>>>> 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
>

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Paolo Bonzini]

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.

Paolo

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Chen, Tiejun]

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

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Paolo Bonzini]

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+.

Re: ResettRe: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Michael S. Tsirkin]

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+.

Re: [Xen-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support

[Konrad Rzeszutek Wilk]

On Wed, Jul 02, 2014 at 06:29:23PM +0200, Paolo Bonzini wrote:
> Il 02/07/2014 17:27, Michael S. Tsirkin ha scritto:
> > At some level, maybe Paolo is right.  Ignore existing drivers and ask
> > intel developers to update their drivers to do something sane on
> > hypervisors, even if they do ugly things on real hardware.
> > 
> > A simple proposal since what I wrote earlier though apparently wasn't
> > very clear:
> > 
> >   Detect Xen subsystem vendor id on vga card.
> >   If there, avoid poking at chipset. Instead
> > 	- use subsystem device # for card type
> 
> You mean for PCH type (aka PCH device id).
> 
> > 	- use second half of BAR0 of device
> > 	- instead of access to pci host
> > 
> > hypervisors will simply take BAR0 and double it in size,
> > make second part map to what would be the pci host.
> 
> Nice.  Detecting the backdoor via the subsystem vendor id
> is clever.
> 
> I'm not sure if it's possible to just double the size of BAR0 
> or not, but my laptop has:
> 
> 	Region 0: Memory at d0000000 (64-bit, non-prefetchable) [size=4M]
> 	Region 2: Memory at c0000000 (64-bit, prefetchable) [size=256M]
> 	Region 4: I/O ports at 5000 [size=64]
> 
> and I hope we can reserve a few KB for hypervisors within those
> 4M, or 8 bytes for an address/data pair (like cf8/cfc) within BAR4's
> 64 bytes (or grow BAR4 to 128 bytes, or something like that).
> 
> Xen can still add the hacky machine type if they want for existing 
> hosts, but this would be a nice way forward.

It would be good to understand first why i915 in the first place
needs to setup the bridge MBAR if it has not been set. As in, why
is this region needed? Is it needed to flush the pipeline (as in
you need to write there?) or .. 

Perhaps it is not needed anymore with the current hardware and
what can be done is put a stake in the ground saying that only
genX or later will be supported.

The commit ids allude to power managament and the earlier commits
did poke there - but I don't see it on the latest tree.
> 
> Paolo
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel