[Intel-gfx] [PATCH 1/2] drm/i915: Treat stolen memory as DMA addresses

[Chris Wilson]

On Fri, Jan 27, 2017 at 06:50:54PM -0200, Paulo Zanoni wrote:
> Em Sex, 2017-01-27 às 16:55 +0000, Chris Wilson escreveu:
> > The conversion of stolen to use phys_addr_t (from essentially u32)
> > sparked an interesting discussion. We treat stolen memory as only
> > accessible from the GPU (the DMA device) - an attempt to use it from
> > the
> > CPU will generate a MCE on gen6 onwards, although it is in theory a
> > physical address that can be dereferenced from the CPU as
> > demonstrated
> > by earlier generations. As such, using phys_addr_t has the wrong
> > connotations and as we pass the address into the DMA device via
> > dma_addr_t (through the scatterlists used to program the GTT
> > entries),
> > we should treat it as dma_addr_t throughout.
> 
> I'm not a specialist here, but from what I could learn/understand, this
> seems good. The patch seems to do what it says, so:
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> 
> But now that I looked some more at the code, I started to think about
> the following: shouldn't we convert our "stolen offset" variables from
> u32 to dma_addr_t or some other appropriate type? I mean, if we ever
> get 64 bit stolen pointers we may also get 64 bit stolen offsets... The
> i915_ggtt struct contains 4 of such u32 pointers.
> 
> For example, i915_ggtt->stolen_reserved_base is another pointer to
> stolen memory, it's generated directly from one of our drm_addr_t
> variables.
> 
> Of course, this could be a separate patch.

Oh yes, I advise you take gentle steps down the rabbit hole.

I thought the reserved_base was an offset, I may be wrong. Certainly the
limits and types are also good to review and be wary of for the future.
Something I've only really recently started trying to do is capture
these assumed limits in the code by checking for type overflows -
hopefully to catch the errors early. That would be a useful exercise
when reviewing the current code to annotate where we change types. Isn't
there a gcc flag for warning on type shrinkage?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre