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

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

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@xxxxxxxxxx]
> > Sent: Friday, July 18, 2014 6:45 AM
> > To: Kay, Allen M
> > Cc: Michael S. Tsirkin; Jesse Barnes; peter.maydell@xxxxxxxxxx; 
> > xen-devel@xxxxxxxxxxxxxxxxxxx; Ross Philipson; airlied@xxxxxxxx; 
> > daniel.vetter@xxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Kelly.Zytaruk@xxxxxxx; 
> > qemu-devel@xxxxxxxxxx; Anthony Perard; Stefano Stabellini; 
> > anthony@xxxxxxxxxxxxx; 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@xxxxxxxxxxxxxxxxxxxxx] On
> > > Behalf Of Michael S. Tsirkin
> > > Sent: Thursday, July 03, 2014 1:28 PM
> > > To: Jesse Barnes
> > > Cc: peter.maydell@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxx; Ross
> > > Philipson; Konrad Rzeszutek Wilk; airlied@xxxxxxxx;
> > > daniel.vetter@xxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > > Kelly.Zytaruk@xxxxxxx; qemu-devel@xxxxxxxxxx; Anthony Perard; Stefano
> > > Stabellini; anthony@xxxxxxxxxxxxx; 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel