[Openib-windows] RE: User verbs
Leonid Keller
leonid at mellanox.co.il
Sun Mar 5 02:48:53 PST 2006
OK with me
> -----Original Message-----
> From: ftillier.sst at gmail.com [mailto:ftillier.sst at gmail.com]
> On Behalf Of Fabian Tillier
> Sent: Friday, March 03, 2006 4:05 AM
> To: Leonid Keller
> Cc: openib-windows at openib.org
> Subject: Re: [Openib-windows] RE: User verbs
>
> Hi Leonid,
>
> On 3/2/06, Leonid Keller <leonid at mellanox.co.il> wrote:
>
> > Index: S:/svn.wininf/trunk/hw/mt23108/kernel/hca_verbs.c
> > ===================================================================
> > --- S:/svn.wininf/trunk/hw/mt23108/kernel/hca_verbs.c
> (revision 401)
> > +++ S:/svn.wininf/trunk/hw/mt23108/kernel/hca_verbs.c
> (revision 1031)
> > @@ -582,6 +582,7 @@
> > {
> > /* Copy the dev info. */
> > p_um_ca->dev_info = *hca_ul_info;
> > + p_um_ca->hob_p = hob_p;
> > *ph_um_ca = (ib_ca_handle_t)p_um_ca;
> > (*(void** __ptr64)p_umv_buf->p_inout_buf) =
> > p_um_ca->p_mapped_addr;
> > p_umv_buf->status = IB_SUCCESS; @@ -644,7 +645,7 @@
> > OUT ib_pd_handle_t
> > *ph_pd,
> > IN OUT ci_umv_buf_t
> > *p_umv_buf )
> > {
> > - mlnx_hob_t *hob_p = (mlnx_hob_t
> > *)h_ca;
> > + mlnx_hob_t *hob_p;
> > mlnx_hobul_t *hobul_p;
> > HH_hca_dev_t *hca_ul_info;
> > HHUL_pd_hndl_t hhul_pd_hndl = 0;
> > @@ -655,6 +656,11 @@
> >
> > CL_ENTER(MLNX_DBG_TRACE, g_mlnx_dbg_lvl);
> >
> > + if( p_umv_buf && p_umv_buf->command )
> > + hob_p = ((mlnx_um_ca_t *)h_ca)->hob_p;
> > + else
> > + hob_p = (mlnx_hob_t *)h_ca;
>
> This won't work properly - when the CA is opened by a
> user-mode process without the kernel bypass provider (either
> it is missing, corrupted, or whatever other failure), the
> p_umv_buf->command is zero to mlnx_um_ca, in which case it
> returns NULL for the mlnx_um_ca_t object. This will cause a
> NULL dereference in the code above.
>
> There are a few ways of fixing this. First, we can keep
> checking the command, and in mlnx_um_open return the
> mlnx_hob_t if command is zero.
> This way h_um_ca is the same as h_ca.
>
> Another way is to always allocate the mlnx_um_ca_t object,
> even if p_umv_buf->command is zero, and remove the check for
> the command when resolving the HCA object.
>
> I'm leaning towards the first, since it's simplest. I'll
> implement that and send out a patch for approval.
>
> - Fab
>
> > hobul_p = mlnx_hobs_get_hobul(hob_p);
> > if (NULL == hobul_p) {
> > status = IB_INVALID_CA_HANDLE; @@ -1889,7
> +1895,7 @@ {
> > ib_api_status_t status;
> >
> > - mlnx_hob_t *hob_p = (mlnx_hob_t
> > *)h_ca;
> > + mlnx_hob_t *hob_p;
> > u_int32_t cq_idx;
> > u_int32_t cq_num;
> > u_int32_t cq_size = 0;
> > @@ -1901,6 +1907,11 @@
> >
> > CL_ENTER(MLNX_DBG_TRACE, g_mlnx_dbg_lvl);
> >
> > + if( p_umv_buf && p_umv_buf->command )
> > + hob_p = ((mlnx_um_ca_t *)h_ca)->hob_p;
> > + else
> > + hob_p = (mlnx_hob_t *)h_ca;
> > +
> > hobul_p = mlnx_hobs_get_hobul(hob_p);
> > if (NULL == hobul_p) {
> > status = IB_INVALID_CA_HANDLE;
> >
> >
> >
> > > -----Original Message-----
> > > From: Fab Tillier [mailto:ftillier at silverstorm.com]
> > > Sent: Wednesday, January 11, 2006 7:19 PM
> > > To: Leonid Keller
> > > Subject: RE: User verbs
> > >
> > > > From: Leonid Keller [mailto:leonid at mellanox.co.il]
> > > > Sent: Wednesday, January 11, 2006 8:45 AM
> > > >
> > > > > From: Fab Tillier [mailto:ftillier at silverstorm.com]
> > > > > Sent: Tuesday, January 10, 2006 8:05 PM
> > > > >
> > > > > Hi Leo,
> > > > >
> > > > > > From: Leonid Keller [mailto:leonid at mellanox.co.il]
> > > > > > Sent: Tuesday, January 10, 2006 8:45 AM
> > > > > >
> > > > > > Hi Fab !
> > > > > >
> > > > > > Working on porting of the user part of the MTHCA
> driver i came
> > > > > > accross the following question:
> > > > > > Why do you use h_ca (kernel HCA handle) and not h_um_ca
> > > (context,
> > > > > > returned by mlnx_um_open) in calls to call mlnx_allocate_pd
> > > > > > and mlnx_create_cq ?
> > > > >
> > > > > The problem comes from the fact that mlnx_um_open isn't
> > > called for
> > > > > all clients (not kernel ones), so passing h_um_ca to the
> > > verbs calls
> > > > > would break kernel clients.
> > > >
> > > > You didn't understand my suggestion, I guess.
> > >
> > > I had totally missed it!
> > >
> > > > It's quite natural, that user clients have their own
> _um_open call.
> > > > It's just an old good work paradigm: h = open(); verb(h);
> > > ...; close(h).
> > > > Pay attention, that h_ca and h_um_ca have the same type
> > > ib_ca_handle_t.
> > > > What I'm suggesting is to call alloc_pd and create_cq with h_ca
> > > > for kernel clients and with h_um_ca - for user ones. The verbs
> > > will choose
> > > > the right casting according to the value of p_umv_buf.
> > >
> > > But this creates an inconsistency with other verbs, which
> may also
> > > benefit from having the h_um_ca. ci_call is just an example of a
> > > call that would benefit, as it would allow the HCA driver
> to store
> > > any CR space mapping information in the h_um_ca
> structure, making CR
> > > space mapping into UM safe.
> > >
> > > So, I'm OK with changing all the verbs to check p_umv_buf
> and then
> > > doing the proper cast. However, in light of the
> direction I expect
> > > the verbs to go in (where the user multiplexing on open
> is done in
> > > the HCA driver and not IBAL), having all open calls
> result in some
> > > sort of user open call and then using that handle
> exclusively in all
> > > verb calls would align better.
> > >
> > > That is, rename um_open_ca to user_open_ca, and call it for all
> > > clients. For kernel clients, p_umv_buf would be NULL and the HCA
> > > driver would know not to try to map resources into
> user-mode. Then,
> > > every call changes to take the h_um_ca.
> > >
> > > Anyhow, whether we change every call to check p_umv_buf,
> or change
> > > um_open_ca and change the type of the structure for every
> other verb
> > > is pretty much equivalent. Plus it lets us merge the CR space
> > > mapping stuff in time for the OpenIB 1.0 release.
> > >
> > > What do you think?
> > >
> > > - Fab
> > >
> > >
> > _______________________________________________
> > openib-windows mailing list
> > openib-windows at openib.org
> > http://openib.org/mailman/listinfo/openib-windows
> >
>
>
>
More information about the ofw
mailing list