<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2//EN">
<HTML>
<HEAD>
<META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=US-ASCII">
<META NAME="Generator" CONTENT="MS Exchange Server version 5.5.2654.45">
<TITLE>RE: [Openib-windows] [PATCH] 2 new vendor calls</TITLE>
</HEAD>
<BODY>
<P><FONT SIZE=2>see below</FONT>
</P>
<P><FONT SIZE=2>> -----Original Message-----</FONT>
<BR><FONT SIZE=2>> From: Fab Tillier [<A HREF="mailto:ftillier@silverstorm.com">mailto:ftillier@silverstorm.com</A>]</FONT>
<BR><FONT SIZE=2>> Sent: Friday, September 23, 2005 9:22 AM</FONT>
<BR><FONT SIZE=2>> To: 'Leonid Keller'</FONT>
<BR><FONT SIZE=2>> Cc: openib-windows@openib.org</FONT>
<BR><FONT SIZE=2>> Subject: RE: [Openib-windows] [PATCH] 2 new vendor calls</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > From: Leonid Keller [<A HREF="mailto:leonid@mellanox.co.il">mailto:leonid@mellanox.co.il</A>]</FONT>
<BR><FONT SIZE=2>> > Sent: Thursday, September 22, 2005 1:09 AM</FONT>
<BR><FONT SIZE=2>> > </FONT>
<BR><FONT SIZE=2>> > Hi Fab,</FONT>
<BR><FONT SIZE=2>> > </FONT>
<BR><FONT SIZE=2>> > Thank you for your comments.</FONT>
<BR><FONT SIZE=2>> > </FONT>
<BR><FONT SIZE=2>> > First of all, the problem with unmapping is in need in </FONT>
<BR><FONT SIZE=2>> releasing of the MDL,</FONT>
<BR><FONT SIZE=2>> > not in unmapping itself.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Right, the problem is getting a pointer to the MDL, for which </FONT>
<BR><FONT SIZE=2>> we shouldn't trust</FONT>
<BR><FONT SIZE=2>> the user-mode program.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > You are right about security and resource tracking: i saw </FONT>
<BR><FONT SIZE=2>> these problems, but</FONT>
<BR><FONT SIZE=2>> > didn't find a simple solution.</FONT>
<BR><FONT SIZE=2>> > The right solution IMO would have been to add the context </FONT>
<BR><FONT SIZE=2>> of FW_MAP_CRSPACE</FONT>
<BR><FONT SIZE=2>> > operation to the process resource tracking information and </FONT>
<BR><FONT SIZE=2>> to perform</FONT>
<BR><FONT SIZE=2>> > FW_UNMAP_CRSPACE automatically upon process termination.</FONT>
<BR><FONT SIZE=2>> > </FONT>
<BR><FONT SIZE=2>> > Then we could remove the context from FW_MAP_CRSPACE and not provide</FONT>
<BR><FONT SIZE=2>> > FW_UNMAP_CRSPACE at all.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> You still want to allow FW_UNMAP_CRSPACE, rather than relying </FONT>
<BR><FONT SIZE=2>> on process</FONT>
<BR><FONT SIZE=2>> termination to drive that. </FONT>
</P>
<P><FONT SIZE=2>???</FONT>
<BR><FONT SIZE=2>Or my English is wrong or i'm missing here something.</FONT>
<BR><FONT SIZE=2>I thought, that i've suggested to perform the unmapping on process termination, which would allow us not to expose FW_UNMAP_CRSPACE at all.</FONT></P>
<P><FONT SIZE=2>Is it right thing to do - is another question.</FONT>
<BR><FONT SIZE=2>We have a lot of paired APIs like FW_OPEN_IF/FW_CLOSE_IF or create_qp/destroy_qp.</FONT>
<BR><FONT SIZE=2>Why shouldn't we provide here also a paired one ?</FONT>
</P>
<P><FONT SIZE=2>> The cleanup in case of abnormal process usage is</FONT>
<BR><FONT SIZE=2>> necessary but shouldn't be the norm.</FONT>
<BR><FONT SIZE=2>Yes, but, first, we has to do it there anyway, second, you suggest below to do it on CA cleanup anyway.</FONT>
</P>
<P><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > Your idea of putting the context into CA object seams to me </FONT>
<BR><FONT SIZE=2>> troublesome:</FONT>
<BR><FONT SIZE=2>> > - it prevents several applications to use </FONT>
<BR><FONT SIZE=2>> simultaneously this</FONT>
<BR><FONT SIZE=2>> > mechanism (on the same HCA);</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Do we want to allow multiple applications simultaneous access </FONT>
<BR><FONT SIZE=2>> to the FW? I</FONT>
<BR><FONT SIZE=2>> don't think so - it creates the possibility for two different </FONT>
<BR><FONT SIZE=2>> sessions trying to</FONT>
<BR><FONT SIZE=2>> update the FW simultaneously, which could result in a corrupt </FONT>
<BR><FONT SIZE=2>> FW image. </FONT>
<BR><FONT SIZE=2>It's - responsibility of the customers (see more reasons below).</FONT>
<BR><FONT SIZE=2>We have to warn them wrt that.</FONT>
<BR><FONT SIZE=2>[BTW, customer can burn into HCA a corrupt or wrong image - it's a normal and not too dangerous situation.</FONT>
<BR><FONT SIZE=2>He can then fix that by burning the card with one application and a right image]</FONT>
</P>
<BR>
<P><FONT SIZE=2>> The FW access should be serialized to a single client at a time.</FONT>
</P>
<P><FONT SIZE=2>A burning application is just one tool.</FONT>
<BR><FONT SIZE=2>There is DevMon, which is mainly used to look at CR space, but allows also to change it.</FONT>
<BR><FONT SIZE=2>There is also trace.exe, which reads debug prints of the FW from the card.</FONT>
<BR><FONT SIZE=2>These two applications are highly used by developers and sometimes simultaneously.</FONT>
<BR><FONT SIZE=2>There are or can be also another card management applications, some of them having additional equirements like mapping DDR or even UAR.</FONT></P>
<P><FONT SIZE=2>We are talking about tools for developers.</FONT>
<BR><FONT SIZE=2>Customer will get a small subset of them.</FONT>
</P>
<P><FONT SIZE=2>That's why I don't think, that we may restrict the access to one application at a time.</FONT>
</P>
<BR>
<P><FONT SIZE=2>> > - if an application was killed and restarted, there </FONT>
<BR><FONT SIZE=2>> will be a memory</FONT>
<BR><FONT SIZE=2>> > leak.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> No, when an application is killed, it's CA object is cleaned </FONT>
<BR><FONT SIZE=2>> up due to the</FONT>
<BR><FONT SIZE=2>> resource tracking. If that CA object tracks the FW CR space </FONT>
<BR><FONT SIZE=2>> mapping, then that</FONT>
<BR><FONT SIZE=2>> can get cleaned up automatically.</FONT>
</P>
<P><FONT SIZE=2>Nice, just we need to do it in the resource tracking code for not to impose the above restriction.</FONT>
</P>
<P><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > I don't know enough how the resource tracking mechaism </FONT>
<BR><FONT SIZE=2>> works today and whether</FONT>
<BR><FONT SIZE=2>> > there is a simple way to implement the above solution.</FONT>
<BR><FONT SIZE=2>> > </FONT>
<BR><FONT SIZE=2>> > Could you enlight me on this ?</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Currently, the user's CA object (the mlnx_um_ca_t) isn't </FONT>
<BR><FONT SIZE=2>> passed into the calls.</FONT>
<BR><FONT SIZE=2>> We probably need to change things so that any user's call to </FONT>
<BR><FONT SIZE=2>> ib_open_ca results</FONT>
<BR><FONT SIZE=2>> in some call to the HCA driver to establish a user context. </FONT>
<BR><FONT SIZE=2>> Currently, this</FONT>
<BR><FONT SIZE=2>> only happens for user-mode clients, </FONT>
</P>
<P><FONT SIZE=2>Are our tools applications not user-mode clients ?</FONT>
</P>
<P><FONT SIZE=2>> and that user-mode context isn't used for</FONT>
<BR><FONT SIZE=2>> any of the other verbs. If we can fix things to use a </FONT>
<BR><FONT SIZE=2>> per-user CA context in</FONT>
<BR><FONT SIZE=2>> all the verbs calls, then resource cleanup will be handled in </FONT>
<BR><FONT SIZE=2>> the proxy.</FONT>
</P>
<P><FONT SIZE=2>Can we pass mlnx_um_ca_t structure to the vendor call ?</FONT>
<BR><FONT SIZE=2>If it isn't too complicate, then i'd suggest the following "ultimate" solution,</FONT>
<BR><FONT SIZE=2>which takes in mind future extensions and various ideas, i got so far:</FONT>
</P>
<P><FONT SIZE=2> 1. User interface </FONT>
<BR> <FONT SIZE=2>- we provide both FW_MAP_CRSPACE and FW_UNMAP_CRSPACE calls;</FONT>
<BR> <FONT SIZE=2>- they will have the same parameter structure:</FONT>
<BR> <FONT SIZE=2>struct _FW_MAP_PARAMS {</FONT>
<BR> <FONT SIZE=2>PVOID __ptr64 va; /* mapped address; OUT for map, IN for UNMAP */</FONT>
<BR> <FONT SIZE=2>IN ULONG bar_num; /* 0 - CR, 1 - UAR, 2 - DDR */</FONT>
<BR> <FONT SIZE=2>IN ULONG offset; /* map/unmap from this offset */</FONT>
<BR> <FONT SIZE=2>IN ULONG size; /* map/unmap this size */</FONT>
<BR> <FONT SIZE=2>};</FONT>
<BR> <FONT SIZE=2>- return codes:</FONT>
<BR> <FONT SIZE=2>STATUS_NOT_IMPLEMENTED - for UAR and DDR space (for now); </FONT>
<BR> <FONT SIZE=2>STATUS_BUFFER_TOO_SMALL - not enough parameters; </FONT>
<BR> <FONT SIZE=2>STATUS_INVALID PARAMETERS - wrong parameters;</FONT>
<BR> <FONT SIZE=2>STATUS_NONE_MAPPED - unmap parameters do not match map ones; </FONT>
</P>
<BR>
<P><FONT SIZE=2> 2. Implementation</FONT>
<BR> <FONT SIZE=2>- we add necessary fielsd to mlnx_um_ca_t;</FONT>
<BR> <FONT SIZE=2>- the structure is passed to FW_MAP_CRSPACE/FW_UNMAP_CRSPACE; </FONT>
<BR> <FONT SIZE=2>- FW_MAP_CRSPACE store the context; FW_UNMAP_CRSPACE use it for unmapping;</FONT>
<BR> <FONT SIZE=2>- we add resource tracking code, that performs cleanup on appl. termination.</FONT>
</P>
<P><FONT SIZE=2>What do you think ?</FONT>
</P>
<P><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> - Fab</FONT>
<BR><FONT SIZE=2>> </FONT>
</P>
</BODY>
</HTML>