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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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