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

[Pirasenna T. <pir...@gm...>]

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?
Any comments?

Thanks,
Piras

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!!