On Thu, Jan 25, 2024 at 11:08:04AM +0200, Ville Syrjälä wrote:
> On Fri, Jan 19, 2024 at 01:12:11AM +0200, Ville Syrjälä wrote:
> > On Wed, Jan 17, 2024 at 06:46:24PM +0100, Nirmoy Das wrote:
> > >
> > > On 1/17/2024 3:13 PM, Michał Winiarski wrote:
> > > > On Tue, Jan 16, 2024 at 09:56:25AM +0200, Ville Syrjala wrote:
> > > >> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > >>
> > > >> Now that the GGTT PTE updates go straight to GSMBASE (bypassing
> > > >> GTTMMADR) there should be no more risk of system hangs? So the
> > > >> "binder" (ie. update the PTEs via MI_UPDATE_GTT) is no longer
> > > >> necessary, disable it.
> > > >>
> > > >> My main worry with the MI_UPDATE_GTT are:
> > > >> - only used on this one platform so very limited testing coverage
> > > >> - async so more opprtunities to screw things up
> > > >> - what happens if the engine hangs while we're waiting for MI_UPDATE_GTT
> > > >> to finish?
> > > >> - requires working command submission, so even getting a working
> > > >> display now depends on a lot more extra components working correctly
> > > >>
> > > >> TODO: MI_UPDATE_GTT might be interesting as an optimization
> > > >> though, so perhaps someone should look into always using it
> > > >> (assuming the GPU is alive and well)?
> > > >>
> > > >> v2: Keep using MI_UPDATE_GTT on VM guests
> > > >>
> > > >> Cc: Paz Zcharya <pazz@xxxxxxxxxxxx>
> > > >> Cc: Nirmoy Das <nirmoy.das@xxxxxxxxx>
> > > >> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > >> ---
> > > >> drivers/gpu/drm/i915/gt/intel_gtt.c | 3 ++-
> > > >> 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c
> > > >> index 86f73fe558ca..e83dabc56a14 100644
> > > >> --- a/drivers/gpu/drm/i915/gt/intel_gtt.c
> > > >> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
> > > >> @@ -24,7 +24,8 @@
> > > >> bool i915_ggtt_require_binder(struct drm_i915_private *i915)
> > > >> {
> > > >> /* Wa_13010847436 & Wa_14019519902 */
> > > >> - return MEDIA_VER_FULL(i915) == IP_VER(13, 0);
> > > >> + return i915_run_as_guest() &&
> > > >> + MEDIA_VER_FULL(i915) == IP_VER(13, 0);
> > > > Note that i915_run_as_guest() is not the most reliable way to decide
> > > > whether to use MI_UPDATE_GTT or straight to GSMBASE, as it requires the
> > > > hypervisor to "opt-in" and set the X86_FEATURE_HYPERVISOR.
> > > > If it's not set - the driver will go into GSMBASE, which is not mapped
> > > > inside the guest.
> > > > Does the system firmware advertise whether GSMBASE is "open" or "closed"
> > > > to CPU access in any way?
> > >
> > > Had a chat with David from IVE team, David suggested to read 0x138914 to
> > > determine that. "GOP needs to qualify the WA by reading GFX MMIO offset
> > > 0x138914 and verify the value there is 0x1." -> as per the HSD-22018444074
> >
> > OK, so we can confirm the firmware is on board. I suppose no real harm
> > in doing so even though it would clearly be a rather weird if someone
> > would ship some ancient firmware that doesn't handle this.
> >
> > But that still won't help with the guest side handling because that
> > register will read the same in the guest.
>
> I guess we have two options here:
> 1) ignore non-standard vms that don't advertise themselves
> 2) try some other heuristics to detect them (eg. host/isa bridge PCI
> IDs/DMI/etc.)
>
> My preference is to just go with option 1, and if someone comes across
> a real world use case when the vm is hiding then we can think of some
> way to handle it. Trying to come up with heuristics for that without
> anything to test against would be 100% guesswork anyway.
>
> --
> Ville Syrjälä
> Intel
Option 1 can work, but there is a heuristic that should work for most
cases.
If we can assume that on bare-metal, e820 memory map excludes the stolen
region (it's marked as reserved), we should be able to do something that
looks roughly like this (warning - not tested, just a pseudo-code):
static int is_reserved(struct resource *res, void *arg)
{
return 1;
}
static bool _stolen_is_reserved(u64 addr)
{
int ret;
ret = walk_iomem_res_desc(IORES_DESC_RESERVED, IORESOURCE_MEM,
gsm, gsm + gsm_size, NULL, is_reserved)
if (ret != 1)
return false;
return true;
}
if (i915_run_as_guest() || !_stolen_is_reserved(gsm, gsm_size))
fallback_to_mi_ggtt()
Similar sanity check for stolen being reserved should probably also be
done in the regular stolen init path - currently we're creating a
resource named "Graphics Stolen Memory" somewhere in the middle of
System RAM when i915 runs inside VM with native device passthrough.
-Michał
