From: José F. <j_r...@ya...> - 2002-05-01 08:59:23
|
Frank just commited a set of changes to the mach64-0-0-3-dma-branch. Is in own words: "Most of the first cut of the DMA code. It's got most of the dispatch architecture in place (Lacks actual DMA submission (The easy part, really...)) so it's not done yet, but I promised people that done or not, I'd do a check-in of this..." The part which is missing is more or less what we have in the mach64-0-0-4-branch, except that the state update is still being made with MMIO. So either we add the remaining parts to mach64-0-0-3-branch or we bring Frank's changes to mach64-0-0-4-branch. Personally I'm more in favor of the later, since it will avoid redundant work of merging back and forward, and will also enable the PowerPC architecture to participate in testing. So before we starting the merge, it's needed to include the state update in the buffers. Although I still have some concerns about security, the fact is that the only security problem we've seen so far is that a malicious client can lock the card (by setting a 0 texture address offset) and is not clear that we can recover from that too. The DMA submission does create some obstacules indiscriminate register access, as Frank commented in his code: "Through some pretty thorough testing, it has been found that the RagePRO engine will pretty much ignore any "commands" sent via the gui-master pathway that aren't gui operations (the register gets set, but the actions that are normally associated with the setting of those said registers doesn't happen.). So, it's safe to send us buffers of gui commands from userspace (altering the buffer in mid-execution will at worst scribble all over the screen and pushing bogus commands will have no apparent effect...)" But above all, is much easier to put the state update in the client for now - while the security isn't completely worked out - so that we can move forward with the DMA support, and at any time if we do come to the conclusion that this model isn't secure we can easily switch back. José Fonseca |
From: Leif D. <lde...@re...> - 2002-05-01 15:18:35
|
On Wed, 1 May 2002, Jos=E9 Fonseca wrote: > Frank just commited a set of changes to the mach64-0-0-3-dma-branch. Is= in=20 > own words: >=20 > "Most of the first cut of the DMA code. It's got most of the=20 > dispatch > architecture in place (Lacks actual DMA submission (The easy part,=20 > really...)) > so it's not done yet, but I promised people that done or not, I'd do a=20 > check-in > of this..." >=20 > The part which is missing is more or less what we have in the=20 > mach64-0-0-4-branch, except that the state update is still being made w= ith=20 > MMIO. So either we add the remaining parts to mach64-0-0-3-branch or we= =20 > bring Frank's changes to mach64-0-0-4-branch. Personally I'm more in fa= vor=20 > of the later, since it will avoid redundant work of merging back and=20 > forward, and will also enable the PowerPC architecture to participate i= n=20 > testing. I agree. Frank is using basically the same setup for the descriptor tabl= e as I have, so that should merge well. We can add the necessary list=20 initialization to _dma_init and keep the bus master test and setup from=20 the mach64-0-0-4-branch, I think. One thing I realized concerning blits:= =20 the utah driver uses the host_data[0-15] registers to do blits which=20 treats blits as a GUI-master operation. This means it works with=20 pseudo-DMA. I think the better way to do it is to use the system bus=20 master method and interrupt handling that Frank has set up. However, if=20 we want to keep an MMIO mode, we'll have to have an alternate method for=20 blits. =20 > So before we starting the merge, it's needed to include the state updat= e=20 > in the buffers. Although I still have some concerns about security, the= =20 > fact is that the only security problem we've seen so far is that a=20 > malicious client can lock the card (by setting a 0 texture address offs= et)=20 > and is not clear that we can recover from that too. The DMA submission=20 > does create some obstacules indiscriminate register access, as Frank=20 > commented in his code: >=20 > "Through some pretty thorough testing, it has been found that the=20 > RagePRO engine will pretty much ignore any "commands" sent via the=20 > gui-master pathway that aren't gui operations (the register gets set, b= ut=20 > the actions that are normally associated with the setting of those said= =20 > registers doesn't happen.). So, it's safe to send us buffers of gui=20 > commands from userspace (altering the buffer in mid-execution will at=20 > worst scribble all over the screen and pushing bogus commands will have= no=20 > apparent effect...)" >=20 > But above all, is much easier to put the state update in the client for= =20 > now - while the security isn't completely worked out - so that we can m= ove=20 > forward with the DMA support, and at any time if we do come to the=20 > conclusion that this model isn't secure we can easily switch back. This would make the driver work more like the utah setup, so it might mak= e it easier to reuse some of the code, and it would be easier to get the proper order of state and vertex data in the buffers. On the security issue, I think we need to be careful about how we handle blits. Frank's code for _dma_buffers allows sending buffers to the DRM, does that mean this takes the place of _dma_vertex? I assume you still need to call thi= s=20 to allocate the buffers before filling them. --=20 Leif Delgass=20 http://www.retinalburn.net |
From: José F. <j_r...@ya...> - 2002-05-01 17:10:04
|
On 2002.05.01 16:18 Leif Delgass wrote: > .... One thing I realized concerning blits: > the utah driver uses the host_data[0-15] registers to do blits which > treats blits as a GUI-master operation. This means it works with > pseudo-DMA. I think the better way to do it is to use the system bus > master method and interrupt handling that Frank has set up. However, if > we want to keep an MMIO mode, we'll have to have an alternate method for > blits. I suppose we can do as we have been doing so far: using the MMIO frame buffer. Anyway, the MMIO support is interesting as a debugging aid, since theoretically every hardware should work with DMA. > > So before we starting the merge, it's needed to include the state > > update in the buffers. ... > > This would make the driver work more like the utah setup, so it might > make > it easier to reuse some of the code, and it would be easier to get the > proper order of state and vertex data in the buffers. Yes. I think that the gamma driver does this as well. > On the security > issue, I think we need to be careful about how we handle blits. Frank's > code for _dma_buffers allows sending buffers to the DRM, does that mean > this takes the place of _dma_vertex? I assume you still need to call > this to allocate the buffers before filling them. > The gamma driver also uses a general buffer submission DMA IOCTL instead of _dma_vertex, but I think that we could just use the current _dma_vertex interface for the time being, since they are semantically identical. José Fonseca |
From: Frank C. E. <fe...@ai...> - 2002-05-01 18:41:34
|
On Wednesday 01 May 2002 03:58 am, JosX Fonseca wrote: > Frank just commited a set of changes to the mach64-0-0-3-dma-branch. Is in > own words: > > "Most of the first cut of the DMA code. It's got most of the > dispatch > architecture in place (Lacks actual DMA submission (The easy part, > really...)) > so it's not done yet, but I promised people that done or not, I'd do a > check-in > of this..." Yeah, if you'll remember, I said that I'd complete the code/test it barring fate handing me another full plate. Well, I probably shouldn't have made that comment, because I didn't get much of any coding done last week. I won't bore you w/the details, suffice it to say that I had to scramble to try to fix my financial situation due to my so-called employer's not paying me consistently. > The part which is missing is more or less what we have in the > mach64-0-0-4-branch, except that the state update is still being made with > MMIO. So either we add the remaining parts to mach64-0-0-3-branch or we > bring Frank's changes to mach64-0-0-4-branch. Personally I'm more in favor > of the later, since it will avoid redundant work of merging back and > forward, and will also enable the PowerPC architecture to participate in > testing. I'm for it, I was going to actually consider doing that very thing but the fun on my end of things just kept me distracted enough that I couldn't sit down and just code the whole thing. For the record, NEVER stay around at a company if they don't do a consistent and regular payroll run at the appointed times per their policies- any inconsistencies show in that situation, you should leave it right then and there. > So before we starting the merge, it's needed to include the state update > in the buffers. Although I still have some concerns about security, the > fact is that the only security problem we've seen so far is that a > malicious client can lock the card (by setting a 0 texture address offset) > and is not clear that we can recover from that too. I was able to make it recover from the "locked" state with DMA, etc. with the XAA FIFO size change by using the reset code in the mach64-0-0-3-dma-branch. Something to check would be to see if a 0 texture address offset would hang things with the interrupt (another reason for having it...) handler checking to see if we're locked up solid. I couldn't get the X server to do it's usual lockup behavior (if you ran X after doing the DMA test with the little testbed driver, it'd lock up tight...) when I inserted a forced reset like the one in my submitted code- and the DMA code worked just fine after startup (Of course, it reset the FIFO size...). -- Frank Earl |
From: Leif D. <lde...@re...> - 2002-05-01 22:09:41
Attachments:
mach64-dma-merge.diff.gz
|
On Wed, 1 May 2002, Frank C. Earl wrote: > > The part which is missing is more or less what we have in the > > mach64-0-0-4-branch, except that the state update is still being made with > > MMIO. So either we add the remaining parts to mach64-0-0-3-branch or we > > bring Frank's changes to mach64-0-0-4-branch. Personally I'm more in favor > > of the later, since it will avoid redundant work of merging back and > > forward, and will also enable the PowerPC architecture to participate in > > testing. > > I'm for it, I was going to actually consider doing that very thing but the > fun on my end of things just kept me distracted enough that I couldn't sit > down and just code the whole thing. For the record, NEVER stay around at a > company if they don't do a consistent and regular payroll run at the > appointed times per their policies- any inconsistencies show in that > situation, you should leave it right then and there. Frank, I merged your changes by hand into my 0-0-4 local tree and it compiles and runs (of course not using the interrupt path yet). Since we don't call the ioctl to install the interrupt handler yet, that path isn't used. A couple of changes I made were omitting some of the includes that drmP.h already includes and changing DMA(dma_immediate_bh) to mach64_dma_immediate_bh (which is the same thing). There were some changes already made in 0-0-4 that I ommitted, but there weren't many conflicts. I'm attaching the patch against 0-0-4 so you can take a look at it. If it looks ok as a starting point, I can check it in. -- Leif Delgass http://www.retinalburn.net |
From: José F. <j_r...@ya...> - 2002-05-03 07:22:00
|
On 2002.05.01 19:41 Frank C. Earl wrote: > On Wednesday 01 May 2002 03:58 am, JosX Fonseca wrote: > > > Frank just commited a set of changes to the mach64-0-0-3-dma-branch. Is > in > > own words: > > > > "Most of the first cut of the DMA code. It's got most of the > > dispatch > > architecture in place (Lacks actual DMA submission (The easy part, > > really...)) > > so it's not done yet, but I promised people that done or not, I'd do a > > check-in > > of this..." > > Yeah, if you'll remember, I said that I'd complete the code/test it > barring > fate handing me another full plate. Well, I probably shouldn't have made > that comment, because I didn't get much of any coding done last week. I > won't bore you w/the details, suffice it to say that I had to scramble to > try > to fix my financial situation due to my so-called employer's not paying > me consistently. Of course I remember and, as I said before, you don't need to justify yourself before us. Anyway, it's a great deal of complicated work you pulled together already. > > > ... or we > > bring Frank's changes to mach64-0-0-4-branch. Personally I'm more in > favor > > of the later, since it will avoid redundant work of merging back and > > forward, and will also enable the PowerPC architecture to participate > > in testing. > > I'm for it, ... Great. So we are all in agreement in this matter. José Fonseca |
From: Leif D. <lde...@re...> - 2002-05-03 08:01:54
|
On Wed, 1 May 2002, Jos=E9 Fonseca wrote: > >=20 > > > ... or we > > > bring Frank's changes to mach64-0-0-4-branch. Personally I'm more i= n > > favor > > > of the later, since it will avoid redundant work of merging back an= d > > > forward, and will also enable the PowerPC architecture to participa= te > > > in testing. > >=20 > > I'm for it, ... >=20 > Great. So we are all in agreement in this matter. >=20 I wanted to give you an update on my progress since I sent the first patc= h of my merge. None of this is checked in yet, btw. I added the necessary initialization to atidri.c, based on the glint/gamma driver, and got the interrupt service routine successfully installed and handling VBLANK interrupts. That was just a matter of getting the IRQ number through a DRM function and calling the control ioctl as part of the DRI screen init= . =20 However, when exiting the X server, I get a hard lockup. I haven't found the source of the problem yet. I filled in the DRIVER_[PRE,POST,UN]INSTALL macros in mach64.h to disable/enable interrupts in CRTC_INT_CNTL, and the PRE/POST work, but the UNINSTALL isn't even reached before the lockup as far as I can tell. These macros are called from the template code in _irq_install/_irq_uninstall. There seem to be some discrepencies in the docs concerning FIFO and HOST error interrupts. First off, I don't see any bits to enable these interrupts listed in BUS_CNTL. Second, the sample code uses 0x00a00000 i= n the engine reset function to clear/acknowledge these error conditions (this is in our current rest function as well). However, these bits are listed in the docs as something unrelated. The lower bit -- which is use= d in Frank's code to ack a host error (the comment says this is per Vernon Chiang at ATI) -- is always set by default on my card. That meant that the first time I got the code running, it was resetting the engine on every VBLANK! This needs some more investigation and maybe another email= =20 to ATI. I started filling in the dma_dispatch from the code already in=20 _dispatch_vertex, and realized that we'll need to factor in special=20 handling for dispatching blits, since these work differently from=20 GUI-master operations. We could use the HOST_DATA registers and treat=20 blits as GUI masters as the utah driver does, but then you don't get an=20 interrupt on completion. The last thing is that I also started looking at how to initialize the freelist and hand out buffers. I haven't done a thorough comparison, but I was wondering if we could use the freelist/waitlist/queue code in the DRM templates. They include spinlocks for manipulating list pointers and other details I haven't had the time to fully understand yet, but I think gamma and i830 use them. Now, it's time for sleep... --=20 Leif Delgass=20 http://www.retinalburn.net |
From: Felix <fx...@gm...> - 2002-05-03 08:44:26
|
On Fri, 3 May 2002 04:01:42 -0400 (EDT) Leif Delgass <lde...@re...> wrote: > I wanted to give you an update on my progress since I sent the first patch > of my merge. None of this is checked in yet, btw. I added the necessary > initialization to atidri.c, based on the glint/gamma driver, and got the > interrupt service routine successfully installed and handling VBLANK > interrupts. That was just a matter of getting the IRQ number through a > DRM function and calling the control ioctl as part of the DRI screen init. > However, when exiting the X server, I get a hard lockup. I haven't found Maybe this is related: whenever the X-server exits I get the following messages at the end of XFree86.1.log: Fatal server error: Caught signal 11. Server aborting So far it hasn't caused any real trouble so I didn't bother reporting it. But if this happens before or during the card is reset for text mode this could cause a lockup. Regards, Felix __\|/__ ___ ___ ___ __Tschüß_______\_6 6_/___/__ \___/__ \___/___\___You can do anything,___ _____Felix_______\Ä/\ \_____\ \_____\ \______U___just not everything____ fx...@gm... >o<__/ \___/ \___/ at the same time! |
From: Leif D. <lde...@re...> - 2002-05-05 20:04:47
|
On Fri, 3 May 2002, Felix K=FChling wrote: > > However, when exiting the X server, I get a hard lockup. I haven't f= ound >=20 > Maybe this is related: whenever the X-server exits I get the following > messages at the end of XFree86.1.log: >=20 > Fatal server error: > Caught signal 11. Server aborting >=20 > So far it hasn't caused any real trouble so I didn't bother reporting > it. But if this happens before or during the card is reset for text mod= e > this could cause a lockup. >=20 I've narrowed this down a bit. The problem happens in the=20 pScreen->CloseScreen callback in ATICloseScreen (atiscreen.c), which is=20 after the kernel cleanup and ATIDRICloseScreen (atidri.c). I've haven't=20 figured out what function the callback points to yet. Does anyone know=20 where to look for this? --=20 Leif Delgass=20 http://www.retinalburn.net |
From: José F. <j_r...@ya...> - 2002-05-03 14:50:35
|
As I was studying the specs and code to be able to understand and reply to Leif's previous post (which I haven't completed yet..), I noticed at the same time a bug and a feature which could mean that blind client buffering could be insecure after all. The bug is that we should be using MACH64_BM_GUI_TABLE and not MACH64_BM_GUI_TABLE_CMD when setting up the GUI master operation. The difference between the two is that the later is queued in the FIFO and the former not, and we really don't want this as it could get in the way later. Only commands which are on block 0 of MMIO region can be streamed into a GUI master operation, as said in the BM_DATA register spec (8-11). The MACH64_BM_GUI_TABLE_CMD is an alias in this block exactly for this purpose, i.e., to be streamed trhough the GUI command FIFO, as said in its spec (8-12). Doesn't this means that we can initiate further GUI master operations from a command buffer since, once the first GUI master operation is setup, it's only necessary to set MACH64_BM_GUI_TABLE_CMD and MACH64_DST_HEIGHT_WIDTH to fire it up - both accessible from GUI FIFO. Although firing up a stream of arbitrary commands shouldn't be a reason for concern since the commands are only innocent(?) GUI operations, this gives the ability of setting up any descriptor table. One consequence is that if this table is invalid the whole DMA engine is unnoperational until a cold reset. Another is that they can access to any register... I plan to build a test case for this, but I would like to hear preliminary opinions about this, in case I'm missing something. Frank, have you tested this before? José Fonseca |
From: Leif D. <lde...@re...> - 2002-05-03 16:08:48
|
On Fri, 3 May 2002, Jos=E9 Fonseca wrote: > As I was studying the specs and code to be able to understand and reply= to=20 > Leif's previous post (which I haven't completed yet..), I noticed at th= e=20 > same time a bug and a feature which could mean that blind client buffer= ing=20 > could be insecure after all. >=20 > The bug is that we should be using MACH64_BM_GUI_TABLE and not=20 > MACH64_BM_GUI_TABLE_CMD when setting up the GUI master operation. The=20 > difference between the two is that the later is queued in the FIFO and = the=20 > former not, and we really don't want this as it could get in the way la= ter. I think the reason for the alias is that the card increments the GUI_TABLE_ADDR @ BM_GUI_TABLE as it consumes descriptors, so writing to BM_GUI_TABLE could disrupt a DMA pass in progress. Using the alias ensures that the commands already in the FIFO/command buffers are processed before changing the table address. However, as you say below,=20 it could potentially be used inside a command buffer as well. > Only commands which are on block 0 of MMIO region can be streamed into = a=20 > GUI master operation, as said in the BM_DATA register spec (8-11).=20 This can't be right since the setup engine registers are in block 1 and w= e=20 are using them in a GUI master op. That's what BUS_EXT_REG_EN @ BUS_CNTL= =20 is for. > The MACH64_BM_GUI_TABLE_CMD is an alias in this block exactly for this > purpose, i.e., to be streamed trhough the GUI command FIFO, as said in > its spec (8-12). Doesn't this means that we can initiate further GUI > master operations from a command buffer since, once the first GUI maste= r > operation is setup, it's only necessary to set MACH64_BM_GUI_TABLE_CMD > and MACH64_DST_HEIGHT_WIDTH to fire it up - both accessible from GUI > FIFO. We definitely don't want clients changing the descriptor table head from inside a dma buffer. > Although firing up a stream of arbitrary commands shouldn't be a reason= =20 > for concern since the commands are only innocent(?) GUI operations, thi= s=20 > gives the ability of setting up any descriptor table. One consequence i= s=20 > that if this table is invalid the whole DMA engine is unnoperational un= til=20 > a cold reset. Another is that they can access to any register... If it's really possible to do this, you might be able to set up a blit=20 as well. btw, I also noticed that HOST_CNTL has a bit for big-endian translation o= f host data. At 15 and 16bpp, it swaps bytes within each word and at 32bpp it reverses the order of the 4 bytes within each dword. This might fix=20 Peter's texture problem. > I plan to build a test case for this, but I would like to hear prelimin= ary=20 > opinions about this, in case I'm missing something. Frank, have you tes= ted=20 > this before? Yeah, the first thing is to determine if it's really possible to kick off= =20 a second DMA pass from within a DMA buffer. --=20 Leif Delgass=20 http://www.retinalburn.net |
From: José F. <j_r...@ya...> - 2002-05-03 18:41:18
|
On 2002.05.03 17:08 Leif Delgass wrote: > On Fri, 3 May 2002, José Fonseca wrote: > > > As I was studying the specs and code to be able to understand and reply > to > > Leif's previous post (which I haven't completed yet..), I noticed at > the > > same time a bug and a feature which could mean that blind client > buffering > > could be insecure after all. > > > > The bug is that we should be using MACH64_BM_GUI_TABLE and not > > MACH64_BM_GUI_TABLE_CMD when setting up the GUI master operation. The > > difference between the two is that the later is queued in the FIFO and > the > > former not, and we really don't want this as it could get in the way > later. > > I think the reason for the alias is that the card increments the > GUI_TABLE_ADDR @ BM_GUI_TABLE as it consumes descriptors, so writing to > BM_GUI_TABLE could disrupt a DMA pass in progress. Using the alias > ensures that the commands already in the FIFO/command buffers are > processed before changing the table address. So you think that it's less prone to error use the BM_GUI_TABLE_CMD alias to setup the GUI master operation. > However, as you say below, > it could potentially be used inside a command buffer as well. > > > Only commands which are on block 0 of MMIO region can be streamed into > a > > GUI master operation, as said in the BM_DATA register spec (8-11). > > This can't be right since the setup engine registers are in block 1 and > we > are using them in a GUI master op. That's what BUS_EXT_REG_EN @ BUS_CNTL > is for. Good point. Although it's not documented anywhere that BUS_EXT_REG_EN @ BUS_CNTL has any relation with the GUI master operation (especially because during a GUI master operation the registers are not written trhough the apperture but internally through BM_ADDR/BM_DATA). > ... > > btw, I also noticed that HOST_CNTL has a bit for big-endian translation > of > host data. At 15 and 16bpp, it swaps bytes within each word and at 32bpp > it reverses the order of the 4 bytes within each dword. This might fix > Peter's texture problem. It's still not clear that this will solve Peter's problem, because, assuming the textures are 16bit, the words are swaped, but _not_ the bytes. The question is where did the bytes got swapped? I think Mesa probably stores pixels in little endian format, or doesn't even try to change the pixel format. If so, how did the other drivers addressed this problem. (Note these are retorical question as I just didn't had the time to look the answers in the code - I'm still a little more concerned with the MMIO on PowerPC architecture). > ... > > Yeah, the first thing is to determine if it's really possible to kick off > a second DMA pass from within a DMA buffer. > I'll report when I get some results. José Fonseca |
From: Michel <mi...@da...> - 2002-05-03 19:28:27
|
On Fri, 2002-05-03 at 18:08, Leif Delgass wrote: > On Fri, 3 May 2002, Jos=E9 Fonseca wrote: >=20 > btw, I also noticed that HOST_CNTL has a bit for big-endian translation o= f > host data. At 15 and 16bpp, it swaps bytes within each word and at 32bpp > it reverses the order of the 4 bytes within each dword. This might fix=20 > Peter's texture problem. Yes, if it allows to freely choose the three ways to swap. That's how I'm going to solve this in the radeon driver (and probably could have in r128 if I had noticed this then :). --=20 Earthling Michel D=E4nzer (MrCooper)/ Debian GNU/Linux (powerpc) developer XFree86 and DRI project member / CS student, Free Software enthusiast |
From: Frank C. E. <fe...@ai...> - 2002-05-05 18:24:54
|
On Friday 03 May 2002 11:08 am, you wrote: > I think the reason for the alias is that the card increments the > GUI_TABLE_ADDR @ BM_GUI_TABLE as it consumes descriptors, so writing to > BM_GUI_TABLE could disrupt a DMA pass in progress. Using the alias > ensures that the commands already in the FIFO/command buffers are > processed before changing the table address. However, as you say below, > it could potentially be used inside a command buffer as well. I tested that in my tests as well. What appears to happen is as follows: The alias is queued up in the FIFO and is synced against the stream. The other is immediately executed and if you're busy, changing it doesn't seem to do anything other than change it after the fact. Attempts to alter the head of the descriptor table or my position in the table didn't do anything and the descriptor table that was specified seemed to execute correctly. Now, for longer chains, it may be that it is a problem- I only tested for one 4K page of data. > > Only commands which are on block 0 of MMIO region can be streamed into a > > GUI master operation, as said in the BM_DATA register spec (8-11). > > This can't be right since the setup engine registers are in block 1 and we > are using them in a GUI master op. That's what BUS_EXT_REG_EN @ BUS_CNTL > is for. I think that it's more of something like the engine only works with GUI engine commands, of which most of them are in Block 0. I've found that the documentation's a good starting point, but it's not consistent or completely accurate. > We definitely don't want clients changing the descriptor table head from > inside a dma buffer. It currently appears that the chip won't let you do that, thankfully. > If it's really possible to do this, you might be able to set up a blit > as well. This doesn't constitute a GUI operation, and it doesn't seem to let you do anything other than issue stuff to the GUI registers with any effect. I tried every one of the possible stunts to get the engine to do something untoward and couldn't get it to do anything other than rendering commands. A BLIT, while we think of it as a GUI operation, isn't one as far as the chip is concerned, it's a DMA operation- which doesn't get acted upon until the current pass is completed. > btw, I also noticed that HOST_CNTL has a bit for big-endian translation of > host data. At 15 and 16bpp, it swaps bytes within each word and at 32bpp > it reverses the order of the 4 bytes within each dword. This might fix > Peter's texture problem. I do believe that might be what that is there for... > Yeah, the first thing is to determine if it's really possible to kick off > a second DMA pass from within a DMA buffer. Already done, but if you want to reverify (just in case I may have missed something, i.e. since I didn't test past one descriptor entry, it might be that under some cases there's a loophole that allows someone to arbitrarily fire off their own DMA operations...)- go right on ahead. I want this to be the best performing solution for the chip, but also the most secure possible. -- Frank Earl |
From: Frank C. E. <fe...@ai...> - 2002-05-05 18:41:27
|
On Friday 03 May 2002 09:49 am, you wrote: > As I was studying the specs and code to be able to understand and reply to > Leif's previous post (which I haven't completed yet..), I noticed at the > same time a bug and a feature which could mean that blind client buffering > could be insecure after all. After testing, it doesn't appear to be. > Only commands which are on block 0 of MMIO region can be streamed into a > GUI master operation, as said in the BM_DATA register spec (8-11). The > MACH64_BM_GUI_TABLE_CMD is an alias in this block exactly for this > purpose, i.e., to be streamed trhough the GUI command FIFO, as said in its > spec (8-12). Doesn't this means that we can initiate further GUI master > operations from a command buffer since, once the first GUI master > operation is setup, it's only necessary to set MACH64_BM_GUI_TABLE_CMD and > MACH64_DST_HEIGHT_WIDTH to fire it up - both accessible from GUI FIFO. Since, upon examination, many of the pieces of documentation we have gotten for the display chips are either incomplete, testing a theory is the best way of knowing whether or not something will do what you expect. I thought that it might be that way, so I tested to see if I could I could re-submit new GUI-master operations to the engine. It appears from the tests that I ran that the GUI-master operation only works with GUI setup and GUI comand registers. Setting up a new GUI-master or BLIT operation did nothing and seemed to just leave the registers set with those values (this was for one descriptor entry's worth of data- we may need to set up a test with MUCH larger spaces with the code in place to be certain...) no actions were carried out (probably because they're not FIFOed and the engine WAS busy...). > Although firing up a stream of arbitrary commands shouldn't be a reason > for concern since the commands are only innocent(?) GUI operations, this > gives the ability of setting up any descriptor table. One consequence is > that if this table is invalid the whole DMA engine is unnoperational until > a cold reset. Another is that they can access to any register... Yes, and they can set it to any value, but the engine doesn't seem to act on any command settings done this way. It is one of the reasons why I said we ought to push a few register reset operations on some of the critical registers (BUS_CNTL, for example) so that if something DOES initiate a command immedately afterward that expects the settings that were there prior to a DMA pass they won't be broken. You can set up a new descriptor table, but there is no direct way to fire it off after the pass is over with and you can't do it in the middle of a pass (Because your GUI commands needed to fire off a pass are NOT put in the FIFO, they're executed directly from the bus-mastering engine...)- so, if you reset the bus-master control registers at the end of a pass, there will be nothing a malicious client could do to initiate their own passes, bogus or otherwise. > I plan to build a test case for this, but I would like to hear preliminary > opinions about this, in case I'm missing something. Frank, have you tested > this before? Yes, pretty extensively, but I didn't have time to set up tests for spanning multiple pages- we ought to do that one last one before commiting to the path we're now looking at. -- Frank Earl |
From: José F. <j_r...@ya...> - 2002-05-12 11:49:49
Attachments:
mach64-gui-table-cmd.txt
|
On 2002.05.05 19:41 Frank C. Earl wrote: > ... > > > I plan to build a test case for this, but I would like to hear > preliminary > > opinions about this, in case I'm missing something. Frank, have you > tested > > this before? > > Yes, pretty extensively, but I didn't have time to set up tests for > spanning > multiple pages- we ought to do that one last one before commiting to the > path we're now looking at. Ok. I've made a test to see if this is possible and it failed. It's best that Leif and Frank made a quick review of the test I made (attached), to see if is there any mistake I made, before we put a stone on this issue. Basically I changed mach64_bm_dma_test to allocate a 2nd descriptor table and 2 more data buffers. The first buffer attempts to override MACH64_BM_GUI_TABLE to read the 2nd table (which points to a 3rd buffer which fille the vertex registers with different values). The 2nd buffer is the continuation of the 1st and has the regular cleanup. Now I plan to reproduce the hang that we had when trying to draw a multitextured triangle without the texture offset specified to see if the engine can recover or not from the lock. Frank, on IRC I got the impression that you were gonna try this. Did you? José Fonseca |