<br><font size=2 face="sans-serif">we released a </font>
<br><font size=2><tt>https://sourceforge.net/projects/ibmehcad/ ehca2_0025</tt></font>
<br><font size=2 face="sans-serif">today which adresses most of these comments,
</font>
<br><font size=2 face="sans-serif">and which survives most of pallas</font>
<br>
<br><font size=2><tt>Roland Dreier <rolandd@cisco.com> wrote on 25.07.2005
22:42:16:<br>
<br>
> IBMEHCA> Hi, we've completed the first alpha code drop of the<br>
> IBMEHCA> Power5 IBM eHCA Device Driver for the for the gen2<br>
> IBMEHCA> openib.org stack.  We're running IPoIB and ibv userspace<br>
> IBMEHCA> programs successfully with this code in our lab setup.</tt></font>
<br><font size=2><tt>> <br>
> IBMEHCA> The source files can be downloaded from<br>
> IBMEHCA> https://sourceforge.net/projects/ibmehcad/ ehca2_0011e</tt></font>
<br><font size=2><tt>> <br>
> Thanks for posting this.  A few comments from a first read through.<br>
> These are in no particular order, with minor nitpicks mixed in with<br>
> more series problems.  Some more review is still needed before
this<br>
> code should go upstream.<br>
</tt></font>
<br><font size=2><tt>> ehca (kernel driver):<br>
</tt></font>
<br><font size=2><tt>> - Please use /* */ instead of // for comments<br>
</tt></font>
<br><font size=2><tt>done</tt></font>
<br>
<br><font size=2><tt>> - Can the debugging stuff be less ugly than EDEB_AV_EN()
etc?<br>
> Also the formatting of your goto labels is not very nice.  Instead<br>
> of something like</tt></font>
<br><font size=2><tt>> <br>
> EHCA_REG_SMR_EXIT0:<br>
> if (retcode == 0)</tt></font>
<br><font size=2><tt>> <br>
> I'd rather see<br>
</tt></font>
<br><font size=2><tt>> out:<br>
> if (retcode == 0)</tt></font>
<br>
<br><font size=2><tt>we changed the goto statements to lower case, but
didn't remove the numbering yet.</tt></font>
<br><font size=2><tt>The numbering actually is sort of a coding pattern
to detect cleanup bugs.</tt></font>
<br><font size=2><tt>The idea behind it is that resources must be freed
in the inverse order they where created,</tt></font>
<br><font size=2><tt>so a</tt></font>
<br><font size=2><tt>void * pd=malloc(4096)</tt></font>
<br><font size=2><tt>if (0 == pd) {</tt></font>
<br><font size=2><tt>   goto error_out_dealloc_pd</tt></font>
<br><font size=2><tt>}</tt></font>
<br><font size=2><tt>...</tt></font>
<br><font size=2><tt>void * cq=malloc(4096)</tt></font>
<br><font size=2><tt>if (0 == cq) {</tt></font>
<br><font size=2><tt>   goto error_out_dealloc_cq</tt></font>
<br><font size=2><tt>}</tt></font>
<br><font size=2><tt>....</tt></font>
<br><font size=2><tt>if (register_cq_fails) {</tt></font>
<br><font size=2><tt>   goto error_out_dealloc_cq</tt></font>
<br><font size=2><tt>}</tt></font>
<br><font size=2><tt>...</tt></font>
<br><font size=2><tt>void * qp=malloc(4096)</tt></font>
<br><font size=2><tt>if (0 == qp) {</tt></font>
<br><font size=2><tt>   goto error_out_dealloc_qp</tt></font>
<br><font size=2><tt>}</tt></font>
<br>
<br><font size=2><tt>goto out;</tt></font>
<br>
<br><font size=2><tt>error_out_dealloc_qp</tt></font>
<br><font size=2><tt>free(qp);</tt></font>
<br><font size=2><tt>error_out_dealloc_cq</tt></font>
<br><font size=2><tt>free(cq);</tt></font>
<br><font size=2><tt>error_out_dealloc_pd</tt></font>
<br><font size=2><tt>free(pd);</tt></font>
<br><font size=2><tt>out:</tt></font>
<br>
<br><font size=2><tt>return</tt></font>
<br>
<br>
<br><font size=2><tt>would be more error prone than using numering. The
basic idea is as soon as you allocate/register a resource (memory, HCA
resources, kobject...)</tt></font>
<br><font size=2><tt>increase the goto statement number by one</tt></font>
<br><font size=2><tt>free in opposite order as allocated,</tt></font>
<br><font size=2><tt>never skip a number during the initial checks</tt></font>
<br><font size=2><tt>...and if you see sth like</tt></font>
<br><font size=2><tt>if (...) goto out_0</tt></font>
<br><font size=2><tt>if (...) goto out_2</tt></font>
<br><font size=2><tt>if (...) goto out_1</tt></font>
<br><font size=2><tt>it's very obviously wrong.</tt></font>
<br>
<br>
<br><font size=2><tt>> <br>
> - In ehca_classes.c, it would be better to avoid using vmalloc() to<br>
> allocate your structures.</tt></font>
<br><font size=2><tt>done</tt></font>
<br><font size=2><tt>> <br>
> - In ehca_common.h, all the stuff copied from from hvcall.h should
be<br>
> deleted.  Also, there are some functions that seem to use ugly<br>
> return codes like H_Success for no good reason.</tt></font>
<br><font size=2><tt>done (mostly, some of these defines are IB specific)</tt></font>
<br><font size=2><tt>> <br>
> - The definitions of p_to_u64() etc. seem unnecesary and needlessly<br>
> obfuscated.  You can always do</tt></font>
<br><font size=2><tt>> <br>
> (unsigned long) ptr<br>
</tt></font>
<br><font size=2><tt>> to cast a void * to u64, and<br>
</tt></font>
<br><font size=2><tt>> (void *) (unsigned long) val<br>
done</tt></font>
<br>
<br><font size=2><tt>> - Similarly in ehca_qp.c,</tt></font>
<br><font size=2><tt>> #define QP_ATTR_IS_SET(mask,attr)   (((mask)&(attr))!=0)</tt></font>
<br><font size=2><tt>> is just needless obfuscation: just do the &
directly where you need it.<br>
done</tt></font>
<br><font size=2><tt>> - In ehca_qp.c, is there a way we can take modqp_statetrans_table[]<br>
> into common code and share with mthca_qp.c?</tt></font>
<br><font size=2><tt>> </tt></font>
<br>
<br><font size=2><tt>well, maybe, modify QP is very complicated to get
straight, but if somebody from mthca_qp.c thinks these are the right set
of transitions</tt></font>
<br><font size=2><tt>(this state machine base algorithm over the last 2
years) </tt></font>
<br><font size=2><tt><br>
> - It seems many source files include ehca_classes_pSeries.h<br>
> directly.  They should probably include ehca_classes.h.  Also<br>
> ehca_classes_zSeries.h seems to be missing.</tt></font>
<br>
<br><font size=2><tt>done</tt></font>
<br><font size=2><tt>we currently don't have a ehca_classes_zSeries.h </tt></font>
<br>
<br><font size=2><tt>> <br>
> Instead of defining P_SERIES on your command line, I think you<br>
> should just include <linux/config.h> and test CONFIG_PPC_PSERIES<br>
> and CONFIG_ARCH_S390 instead.</tt></font>
<br><font size=2><tt>> <br>
> - In ehca_main.c, ehca_hca_resources_show() violates the "one
value<br>
> per file" rule for sysfs.  Either put this in debugfs or
split it<br>
> up into separate attributes in a subdirectory.</tt></font>
<br>
<br><font size=2><tt>done</tt></font>
<br><font size=2><tt>> <br>
> - What is ehca_register_pci() doing?  Why are you changing the
main<br>
> kernel's pci_dma_ops??  Shouldn't you be creating a device on
your<br>
> own virtual bus with it's own dma ops?</tt></font>
<br><font size=2><tt>> </tt></font>
<br>
<br><font size=2><tt>done,</tt></font>
<br><font size=2><tt>see ebus</tt></font>
<br><font size=2><tt>we're currently trying to get that integrated into
base ppc64 kernel</tt></font>
<br><font size=2><tt><br>
> - It seems ehca_nopage() accesses ehca_idr without taking the<br>
> ehca_idr_sem, is this OK?</tt></font>
<br><font size=2><tt>> <br>
</tt></font>
<br><font size=2><tt>good, point!</tt></font>
<br><font size=2><tt>done</tt></font>
<br><font size=2><tt>> libehca:<br>
</tt></font>
<br>
<br>
<br><font size=2><tt>> - It would probably be a good idea to use GNU
autotools to build<br>
> libehca.  In particular you want to make the destination directory<br>
> configurable.</tt></font>
<br>
<br><font size=2><tt>done (partly)</tt></font>
<br><font size=2><tt>this is the initial version of autotools build,</tt></font>
<br><font size=2><tt>still learning how to use autotools in a better way...</tt></font>
<br><font size=2><tt>> <br>
> - I don't think you should hard code -m64 in your CFLAGS or put<br>
> /usr/local/lib64 anywhere -- I believe nearly everyone running<br>
> ppc64 is using 32-bit userspace.  Have you tested 32-bit userspace<br>
> on a 64-bit kernel?</tt></font>
<br>
<br><font size=2><tt>32 bit userspace will be supported in one of the next
versions.</tt></font>
<br><font size=2><tt>we're running mvapich as 64 bit library</tt></font>
<br>
<br><font size=2><tt>> <br>
> - I don't think the install should unconditionally delete<br>
> /dev/infiniband and then create the device nodes -- this should<br>
> really be handled by either udev or the distribution.</tt></font>
<br><font size=2><tt>> <br>
</tt></font>
<br><font size=2><tt>removed that from standard build, still trying to
figure out how to configure udev to automatically add the infiniband device
nodes</tt></font>
<br>
<br><font size=2><tt>> - In ehca_reqs_core.c, get rid of all the<br>
</tt></font>
<br><font size=2><tt>> #ifndef __KERNEL__<br>
> #define ib_recv_wr ibv_recv_wr<br>
> #define ib_send_wr ibv_send_wr<br>
> //#define ib_ah ibv_ah<br>
> #define ehca_av ehcau_av<br>
> #define unlikely(x) x<br>
> // ib_wr_opcode<br>
> #define IB_WR_SEND IBV_WR_SEND<br>
> #define IB_WR_SEND_WITH_IMM IBV_WR_SEND_WITH_IMM<br>
> #define IB_WR_RDMA_WRITE IBV_WR_RDMA_WRITE<br>
> #define IB_WR_RDMA_WRITE_WITH_IMM IBV_WR_RDMA_WRITE_WITH_IMM</tt></font>
<br><font size=2><tt>> <br>
> and so on.  It's not a good idea to try and use the same source
for<br>
> code that runs in the kernel and code that runs in userspace.<br>
</tt></font>
<br><font size=2><tt>technically the kernel version looks like it's a kernel
only module...</tt></font>
<br>
<br><font size=2><tt>> Splitting your source also gets rid of stuff
like</tt></font>
<br><font size=2><tt>> <br>
> #ifdef EHCA_USERDRIVER<br>
> u8 *data = (u8 *) sge->addr;</tt></font>
<br><font size=2><tt>> #else<br>
> u8 *data = (u8 *) abs_to_virt(sge->addr);</tt></font>
<br><font size=2><tt>> #endif<br>
</tt></font>
<br><font size=2><tt>EHCA_USERDRIVER is the rest of a userspace sanbox
to run the kernel code in userspace, as done for parts of the scsi stack
for example.</tt></font>
<br><font size=2><tt>we'll try to remove that flag on the next release</tt></font>
<br>
<br><font size=2><tt>> - In ehca_uinit.c, is the LIBEHCA_FILE environment
variable handling<br>
> safe?  It seems that a user could trick a setuid process into
doing<br>
> something bad here.</tt></font>
<br><font size=2><tt>> <br>
</tt></font>
<br><font size=2><tt>good point!</tt></font>
<br><font size=2><tt>now picking up the place where to trace in a config
file in /etc</tt></font>
<br>
<br><font size=2><tt>> - In ehca_umain.c there seems to be some weird
indentation:<br>
</tt></font>
<br><font size=2><tt>> my_cq->ehca_cq_core.galpas.kernel.fw_handle
= (u64)<br>
> mmap(NULL, 4096, PROT_WRITE, MAP_SHARED, context->cmd_fd,</tt></font>
<br><font size=2><tt>> ((u64) (my_cq->token) << 32) | 0x11000000);<br>
</tt></font>
<br><font size=2><tt>hmm, changed from bad to not completely good.</tt></font>
<br><font size=2><tt>Will fix in the next version. Hope we didn't add some
more of those.</tt></font>
<br>
<br><font size=2><tt>> - Does it make sense to encapsulate to use of
ppc64-specific assembly<br>
> stuff like:</tt></font>
<br><font size=2><tt>> <br>
> __asm__ __volatile("dcbz 0,%0"::"r"(adr));<br>
> __asm__ __volatile("dclst 0,%0"::"r"(adr));<br>
> __asm__ __volatile__("sync":::"memory"); // serialize
GAL register access</tt></font>
<br>
<br><font size=2><tt>done</tt></font>
<br><font size=2><tt>> <br>
> - In ehca_utools.h, ehca_swapbl32 and ehca_swapbl16 seem kind of useless.<br>
> Why not just use ntohl/ntohs directly?</tt></font>
<br>
<br><font size=2><tt>done</tt></font>
<br><font size=2><tt>> <br>
> Thanks,<br>
> Roland</tt></font>
<br>
<br><font size=2><tt>Thanks for your input, I'm pretty sure this only was
the start to get that code kernel ready.</tt></font>
<br><font size=2><tt>Christoph</tt></font>
<br>
<br>