From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerome Glisse Subject: Re: [PATCH 02/10] drm/radeon: UVD bringup v7 Date: Wed, 3 Apr 2013 13:10:54 -0400 Message-ID: <20130403171053.GB3244@gmail.com> References: <1364944719-5175-1-git-send-email-deathsimple@vodafone.de> <1364944719-5175-3-git-send-email-deathsimple@vodafone.de> <20130403145323.GC2010@gmail.com> <515C5093.9030709@vodafone.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-qc0-f173.google.com (mail-qc0-f173.google.com [209.85.216.173]) by gabe.freedesktop.org (Postfix) with ESMTP id 784FFE63F2 for ; Wed, 3 Apr 2013 10:14:13 -0700 (PDT) Received: by mail-qc0-f173.google.com with SMTP id b12so783229qca.4 for ; Wed, 03 Apr 2013 10:14:13 -0700 (PDT) Content-Disposition: inline In-Reply-To: <515C5093.9030709@vodafone.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Christian =?iso-8859-1?Q?K=F6nig?= Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Wed, Apr 03, 2013 at 05:53:55PM +0200, Christian K=F6nig wrote: > Am 03.04.2013 16:53, schrieb Jerome Glisse: > >On Wed, Apr 03, 2013 at 01:18:31AM +0200, Christian K=F6nig wrote: > >>[SNIP] > >> > >> /* hardcode those limit for now */ > >> #define RADEON_VA_IB_OFFSET (1 << 20) > >> #define RADEON_VA_RESERVED_SIZE (8 << 20) > >>@@ -357,8 +360,9 @@ struct radeon_bo_list { > >> struct ttm_validate_buffer tv; > >> struct radeon_bo *bo; > >> uint64_t gpu_offset; > >>- unsigned rdomain; > >>- unsigned wdomain; > >>+ bool written; > >>+ unsigned domain; > >>+ unsigned alt_domain; > >> u32 tiling_flags; > >> }; > >I think that the change to the rdomain/wdomain should be in a patch > >of its own. I think the change is fine but we had issue with change > >that touched that part previously, would make bisecting and > >understanding the change implication easier. > = > Agree, I actually planed to do so, but for the whole IP review stuff > we needed to maintain a more or less stable patch base. Long story, > but I'm going to change it. > = > >>@@ -826,7 +830,6 @@ struct radeon_cs_reloc { > >> struct radeon_bo *robj; > >> struct radeon_bo_list lobj; > >> uint32_t handle; > >>- uint32_t flags; > >> }; > >Why removing the flags ? iirc it's not really use right now but i > >remember plan to use it. > = > Ups, just a rebasing artifact. But when it's unused we should remove > it, probably just not in this patch. > = > >>[SNIP] > >> > >>+static int radeon_uvd_cs_reloc(struct radeon_cs_parser *p, int data0, = int data1) > >>+{ > >>+ struct radeon_cs_chunk *relocs_chunk; > >>+ struct radeon_cs_reloc *reloc; > >>+ unsigned idx, cmd; > >>+ uint64_t start, end; > >>+ > >>+ relocs_chunk =3D &p->chunks[p->chunk_relocs_idx]; > >>+ idx =3D radeon_get_ib_value(p, data1); > >>+ if (idx >=3D relocs_chunk->length_dw) { > >>+ DRM_ERROR("Relocs at %d after relocations chunk end %d !\n", > >>+ idx, relocs_chunk->length_dw); > >>+ return -EINVAL; > >>+ } > >>+ > >>+ reloc =3D p->relocs_ptr[(idx / 4)]; > >>+ start =3D reloc->lobj.gpu_offset; > >>+ end =3D start + radeon_bo_size(reloc->robj); > >>+ start +=3D radeon_get_ib_value(p, data0); > >I am assuming there is no way for you to know the size that the uvd engi= ne will write to ? > >You are not checking anything on uvd possibly overwritting after the bo = end. > = > Yeah that gave me headache for a quite long time, too. The problem > is to figure out how much is actually written you need to keep track > of the whole lot of informations including the UVD session, > create/decode/destroy messages and allot of fiddling with the codec > specific parameters. > = > And if I understand the UVD internals correctly even if we check > everything there is no guarantee that a special crafted bitstream > could not let UVD to write over the end of the buffer.... > = > Is it ok if we but a big TODO on it for the initial patch? > = > Cheers, > Christian. I think i only need one assurance and i think for uvd this will be the case. If UVD block write past bo end can you be sure that no matter what it will overwritte to address > start ie it could not overwritte to begining of VRA= M. I have big doubt on that given the 256M window, i fear that it might go back to writting to begining of memory where the page table is. Note that i think that now that we have cp dma pagetable entry update we can probably just move the pagetable to end of vram on 90% GPU with UVD this wi= ll be > 256M which seems like a zone where UVD can never write. If we can have such assurance i guess we can make uvd as an option and make a very explicit comment stating that UVD engine can be use as an exploit vector path. Cheers, Jerome