Re: [Ocf-linux-users] crypto_newsession / crypto_freesession

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

Jivin Pirasenna Thiyagarajan lays it down ...
> Hi David:
> I got an email saying the patch may not have been posted.   But, the website
> shows the patch has been posted to the list.
> Did you see my earlier email with the patch?

Yes,  I got the patch

> Any comments?

Sorry,  just haven't had a chance yet to look at it.  Will try and get
there today,

Cheers,
Davidm

> On Fri, Jan 30, 2009 at 12:01 AM, Pirasenna Thiyagarajan <
> pir...@gm...> wrote:
> 
> > The crypto hardware can access the entire host memory subsystem.  The
> > session and outstanding operations are maintained by the driver.  Due to
> > many families of crypto engines that the driver supports, the notification
> > of completion operation is abstracted by the driver and translated to
> > operation instance that is complete.  The driver also provides a queue
> > mechanism very similar to the OCF to enqueue operations, which the driver
> > submits to the crypto hardware asynchronously.
> > I've answered your specific questions below.  I've also attached a patch on
> > top of September 17th, 2008 sources.  It has the changes that enables driver
> > its own 64-bit sid and uses the next 64-bit to put all the driver related
> > parameters.  I tested the cryptosoft with this driver and it worked without
> > any issues.  More importantly, the patch will give you a general idea of my
> > suggestions.
> >
> >
> > Thanks,
> > Piras
> >
> >
> > On Thu, Jan 29, 2009 at 8:00 PM, David McCullough <
> > Dav...@se...> wrote:
> >
> >>
> >> Jivin Pirasenna Thiyagarajan lays it down ...
> >> > David:
> >> > Here is what I'm trying achieve.  In my driver I'm adding an abstract
> >> layer
> >> > to interface with OCF.  There is an opaque structure that hosts the
> >> crypto
> >> > fields, which in OCF-speak is the session.  So, in the kernel space,
> >> instead
> >> > of creating a table to maintain the mapping between session id and the
> >> > appropriate crypto operation, I'm trying to return the address.
> >> >
> >> > I explored your idea of using offset address as session id.  That would
> >> work
> >> > in Linux 32-bit mode since the addressable space in kernel is less than
> >> a
> >> > gigabyte.  Whereas, in 64-bit mode, I cannot make an assumption of
> >> having
> >> > memory obtained from the OS to be within a 32-bit offset.
> >>
> >> Ok,  then it is not "on crypto chip" memory but host system memory
> >> obtained through one of the kernel allocator functions.
> >>
> >> It sounds like the HW doesn't have a classic "divided up" ring buffer but
> >> just chains arbitary descriptors together  ?
> >>
> >> > I'm expecting to use the session id as an opaque handler to the OCF
> >> > interface and the driver may use it to dereference the operation however
> >> it
> >> > may see fit.
> >> >
> >> > In a multi-processor scenario, it would eliminate lock to access the
> >> session
> >> > table structure safely.
> >>
> >> Are you doing this to make finding the "session" info easy,  and
> >> everything you need to know about a session is contained within the memory
> >> pointed to by that address ?
> >>
> > Yes.  The session id is a sort of crypto handle I'll de-reference and use
> > it to extract keys and other details.
> >
> >
> >>
> >>
> >> That I can understand.  Especially if you want unlimited sessions.
> >> That way when I ask to do something on session N,  you just dereference N
> >> inside your driver to find out context for that session.
> >>
> >> Do you have infomation that is not stored in the memory at the "address" ?
> >> How does the HW find out about a new session ?  Do you just link it into
> >> a list of sessions ?  Does the HW have a session limit ?
> >
> > The driver supports different engines of the same family and the limits
> > vary.  The driver will determine if the engine cannot take any more requests
> > and respond with EAGAIN.  From looking at the OCF code, I might have to
> > return ERESTART, which is fine.
> >
> >>
> >>
> >> If the address is only used by the driver,  it sounds reasonable, if you
> >> want
> >> to use the address outside of your driver for anything else then it sounds
> >> quite risky :-)
> >
> > No.  The id is kept opaque in all other contexts.
> >
> >
> > Thanks,
> > Piras
> >
> >
> >
> >>
> >> > Any comments is definitely welcome.
> >>
> >> Looking at an OCF driver (forget cryptodev, thats different),  newsession
> >> returns a 32 bit session id,  but freesession takes a 64 bit id,  that
> >> doesn't make much sense in any way :-)
> >>
> >> I just checked the current FreeBSD source and the API hasn't changed in
> >> that respect.  I wouldn't like to deviate from that too much, but
> >> I can't say I am completely tied to it either.
> >>
> >> Having to recompile doesn't bother me much either as this is a source
> >> only thing anyway.
> >>
> >> Cheers,
> >> Davidm
> >>
> >> > On Thu, Jan 29, 2009 at 4:39 PM, David McCullough <
> >> > Dav...@se...> wrote:
> >> >
> >> > >
> >> > > Jivin Pirasenna Thiyagarajan lays it down ...
> >> > > > Here is one option, that does not require interface change and
> >> requires
> >> > > no
> >> > > > modification in ocf-compat.h.  It would require recompiling existing
> >> > > drivers
> >> > > > though.
> >> > > >
> >> > > > Since sid is always passed as u_int64_t * for create_newsession,
> >> > > internally
> >> > > > in cryptop structure crp_sid may be setup as a 2 entry 64 byte
> >> array. The
> >> > > > nid defintion in crypto.c:1017 may be defined the same way.  The
> >> first
> >> > > > 64-bytes are left for driver to put whatever value they want.  The
> >> second
> >> > > > 64-byte is left for OCF to store flags and anything else that is
> >> required
> >> > > > for administration.
> >> > >
> >> > > Ok,  I think I am missing something here.  Are you trying to pass the
> >> > > address back to userspace by any chance ?
> >> >
> >> >
> >> > >
> >> > > > crypto_newsession in crytpodev.c would be modified to store the
> >> flags in
> >> > > the
> >> > > > second 64-byte field.
> >> > > > The macros CRYPTO_SESID2HID, CRYPTO_SESID2CAPS and CRYPTO_SESID2LID
> >> will
> >> > > be
> >> > > > modified to harvest the information from correct index.
> >> > > > crypto_freesession sends only the first 64-byte data as the argument
> >> > > value
> >> > > > to the function.  So, driver can use it as it is.
> >> > > >
> >> > > > Let me know if you think there are any loose ends or
> >> incompatibilities.
> >> > >  I
> >> > > > can send you a patch with these changes after testing it out with
> >> > > software
> >> > > > driver using cryptotest.
> >> > >
> >> > > IMO it is sounding a little messy at this point,  but that may be
> >> > > because I don't quite understand what you are trying to do and how it
> >> > > will be used.
> >> > >
> >> > > Did you consider returning an offset rather than a full pointer  as I
> >> > > suggested below ?  Is it even possible in your context ?
> >> > >
> >> > > If you think the patch is simple enough to do perhaps it will help me
> >> > > understand what you need better ;-)
> >> > >
> >> > > Cheers,
> >> > > Davidm
> >> > >
> >> > >
> >> > > > On Wed, Jan 28, 2009 at 10:03 PM, David McCullough <
> >> > > > Dav...@se...> wrote:
> >> > > >
> >> > > > >
> >> > > > > Jivin Pirasenna Thiyagarajan lays it down ...
> >> > > > > > I'm writing a driver and attempting it to make it OCF
> >> compatible.  I
> >> > > was
> >> > > > > > intending on passing the virtual memory address I allocate for
> >> driver
> >> > > > > data
> >> > > > > > structures for a given operation as the session id.  To avoid
> >> > > 32/64bit
> >> > > > > > issues, having the crypto_newsession pass in u_int64_t * would
> >> make
> >> > > it
> >> > > > > > convenient.  Otherwise, I'll have to keep a mapping table and
> >> look up
> >> > > > > > routines appropriately (unnecessary computation cycles).
> >> > > > >
> >> > > > > Hmm,  sounds fair,  only I think that crypto.c messes with the 64
> >> bit
> >> > > > > version a fair bit before handing it to the driver.
> >> > > > >
> >> > > > > Check crypto.c:crypto_newsession to see this in action.
> >> > > > >
> >> > > > > The 64bit version has the driver id + the drivers session id and
> >> the
> >> > > > > drivers capability flags all merged into it.
> >> > > > >
> >> > > > > It's probably unlikely your card with have more than 4GB of
> >> memory, or
> >> > > > > maybe it does.  You could just return the "offset" into it's
> >> memory for
> >> > > > > that session.  Saves a lookup table (baseaddr + offset = address).
> >> > > > >
> >> > > > > > From the looks of it, it seems just like an interface definition
> >> > > issue,
> >> > > > > > since as I pointed out, from within the ioctl, it does use
> >> u_int64_t
> >> > > > > > internally.
> >> > > > >
> >> > > > > Unfortunately I think it's not that simple, happy to consider
> >> something
> >> > > > > though if you have ideas.
> >> > > > >
> >> > > > > Are you planning to do a FreeBSD driver as well ?  If so it
> >> wouldn't
> >> > > hurt
> >> > > > > to
> >> > > > > check if it's still the same.  Otherwise I'll get around to
> >> checking in
> >> > > > > time ;-)
> >> > > > >
> >> > > > > Cheers,
> >> > > > > Davidm
> >> > > > >
> >> > > > >
> >> > > > > > On Wed, Jan 28, 2009 at 3:58 PM, David McCullough <
> >> > > > > > Dav...@se...> wrote:
> >> > > > > >
> >> > > > > > >
> >> > > > > > > Jivin Pirasenna Thiyagarajan lays it down ...
> >> > > > > > > > I've used OCF patch and attempting to use the cryptodev
> >> > > interface.
> >> > > > >  In
> >> > > > > > > > ocf-compat.h, I see crypto_newsession's session ID is passed
> >> in
> >> > > as a
> >> > > > > > > > u_int32_t *, whereas, the crypto_freesession takes the
> >> session id
> >> > > as
> >> > > > > > > > u_int64_t.  It seems like this is in error.  Looks like sid
> >> data
> >> > > type
> >> > > > > > > should
> >> > > > > > > > be u_int64_t *sidp.  In cryptodev.c where it is used, the
> >> sid is
> >> > > > > defined
> >> > > > > > > as
> >> > > > > > > > u_int64_t in cryptodev_ioctl.
> >> > > > > > >
> >> > > > > > > I had to look twice at that too now that you point it out.
> >> > > > > > >
> >> > > > > > > The trick is that device_method_t structure and resulting
> >> > > > > > > CRYPTODEV_NEWSESSION and cryptodev_newsession used for it are
> >> > > > > > > part of the interface for OCF drivers.  This is how OCF
> >> > > communicates
> >> > > > > > > with it's crypto drivers (ie., safe, hifn7751, cryptosoft
> >> etc).
> >> > > > > > >
> >> > > > > > > cryptodev,  the user interface driver,  doesn't talk to the
> >> crypto
> >> > > > > > > drivers,  it uses the OCF API's:
> >> > > > > > >
> >> > > > > > >        crypto_newsession
> >> > > > > > >        crypto_freesession
> >> > > > > > >
> >> > > > > > > and they are both u_int64_t.
> >> > > > > > >
> >> > > > > > > Are you having a problem with cryptodev or did you just notice
> >> this
> >> > > > > oddity
> >> > > > > > > in the code ?
> >> > > > > > >
> >> > > > > > > I admit the naming is confusing,  but we inherited that with
> >> that
> >> > > last
> >> > > > > > > round of BSD updates.  Unless you are writing a driver,
> >>  ignore the
> >> > > > > section
> >> > > > > > > at the top of ocf-compat.h,  in fact,  ignore as much of
> >> > > ocf-compat.h
> >> > > > > > > as you can every chance you get :-)  It's where we hide the
> >> older
> >> > > OS
> >> > > > > > > support and some BSD compat things to try and keep the driver
> >> > > source as
> >> > > > > > > close to the original as practical.
> >> > > > > > >
> >> > > > > > > Cheers,
> >> > > > > > > Davidm
> >> > > > > > >
> >> > > > > > > --
> >> > > > > > > David McCullough,  dav...@se...,
> >> Ph:+61
> >> > > > > > > 734352815
> >> > > > > > > Secure Computing - SnapGear  http://www.uCdot.org
> >> > > > > > > http://www.snapgear.com
> >> > > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > >
> >> > > > > > --
> >> > > > > > Online Gallery: http://www.deptons.com
> >> > > > > > You comments and ratings are very welcome!!
> >> > > > >
> >> > > > > --
> >> > > > > David McCullough,  dav...@se...,   Ph:+61
> >> > > > > 734352815
> >> > > > > Secure Computing - SnapGear  http://www.uCdot.org
> >> > > > > http://www.snapgear.com
> >> > > > >
> >> > > >
> >> > > >
> >> > > >
> >> > > > --
> >> > > > Online Gallery: http://www.deptons.com
> >> > > > You comments and ratings are very welcome!!
> >> > >
> >> > > --
> >> > > David McCullough,  dav...@se...,   Ph:+61
> >> > > 734352815
> >> > > Secure Computing - SnapGear  http://www.uCdot.org
> >> > > http://www.snapgear.com
> >> > >
> >> >
> >> >
> >> >
> >> > --
> >> > Online Gallery: http://www.deptons.com
> >> > You comments and ratings are very welcome!!
> >>
> >> --
> >> David McCullough,  dav...@se...,   Ph:+61
> >> 734352815
> >> Secure Computing - SnapGear  http://www.uCdot.org
> >> http://www.snapgear.com
> >>
> >
> >
> >
> > --
> > Online Gallery: http://www.deptons.com
> > You comments and ratings are very welcome!!
> >
> 
> 
> 
> -- 
> Online Gallery: http://www.deptons.com
> You comments and ratings are very welcome!!

-- 
David McCullough,  dav...@se...,  Ph:+61 734352815
McAfee - SnapGear  http://www.snapgear.com                http://www.uCdot.org