Thread: [Ocf-linux-users] crypto_newsession / crypto_freesession
                
                Brought to you by:
                
                    david-m
                    
                
            
            
        
        
        
    | 
      
      
      From: Pirasenna T. <pir...@gm...> - 2009-01-28 23:32:52
       | 
| 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. Piras | 
| 
      
      
      From: David M. <Dav...@se...> - 2009-01-28 23:58:33
       | 
| 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 | 
| 
      
      
      From: Pirasenna T. <pir...@gm...> - 2009-01-29 05:12:58
       | 
| 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). >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. Piras 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!! | 
| 
      
      
      From: David M. <Dav...@se...> - 2009-01-29 06:03:30
       | 
| 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 | 
| 
      
      
      From: Pirasenna T. <pir...@gm...> - 2009-01-29 18:40:53
       | 
| 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. 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. Thanks, Piras 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!! | 
| 
      
      
      From: David M. <Dav...@se...> - 2009-01-30 00:39:58
       | 
| 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 | 
| 
      
      
      From: Pirasenna T. <pir...@gm...> - 2009-01-30 02:47:21
       | 
| 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. 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. Any comments is definitely welcome. Thanks, Piras 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!! | 
| 
      
      
      From: David M. <Dav...@se...> - 2009-01-30 04:00:49
       | 
| 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 ? 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 ? 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 :-) > 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 | 
| 
      
      
      From: Pirasenna T. <pir...@gm...> - 2009-01-30 08:01:06
       | 
| diff -ruw /export/tmp/linux-2.6.17.10/crypto/ocf/crypto.c ./crypto.c
--- /export/tmp/linux-2.6.17.10/crypto/ocf/crypto.c	2009-01-29 10:10:36.000000000 -0800
+++ ./crypto.c	2009-01-29 18:54:59.000000000 -0800
@@ -389,17 +389,14 @@
 		/* Call the driver initialization routine. */
 		hid = cap - crypto_drivers;
 		lid = hid;		/* Pass the driver ID. */
-		cap->cc_sessions++;
 		CRYPTO_DRIVER_UNLOCK();
-		err = CRYPTODEV_NEWSESSION(cap->cc_dev, &lid, cri);
+		err = CRYPTODEV_NEWSESSION(cap->cc_dev, &sid[0], cri);
 		CRYPTO_DRIVER_LOCK();
 		if (err == 0) {
-			(*sid) = (cap->cc_flags & 0xff000000)
-			       | (hid & 0x00ffffff);
-			(*sid) <<= 32;
-			(*sid) |= (lid & 0xffffffff);
-		} else
-			cap->cc_sessions--;
+			cap->cc_sessions++;
+			(sid[1]) = ((cap->cc_flags & 0xff000000)
+			       | (hid & 0x00ffffff)) & 0xffffffff;
+		}
 	} else
 		err = EINVAL;
 	CRYPTO_DRIVER_UNLOCK();
@@ -419,7 +416,7 @@
  * driver).
  */
 int
-crypto_freesession(u_int64_t sid)
+crypto_freesession(u_int64_t *sid)
 {
 	struct cryptocap *cap;
 	u_int32_t hid;
@@ -447,7 +444,7 @@
 	if (cap->cc_dev) {
 		CRYPTO_DRIVER_UNLOCK();
 		/* Call the driver cleanup routine, if available, unlocked. */
-		err = CRYPTODEV_FREESESSION(cap->cc_dev, sid);
+		err = CRYPTODEV_FREESESSION(cap->cc_dev, CRYPTO_SESID2LID(sid));
 		CRYPTO_DRIVER_LOCK();
 	}
 
@@ -1014,7 +1011,7 @@
 #endif
 	if (cap->cc_flags & CRYPTOCAP_F_CLEANUP) {
 		struct cryptodesc *crd;
-		u_int64_t nid;
+		u_int64_t nid[2];
 
 		/*
 		 * Driver has unregistered; migrate the session and return
@@ -1030,8 +1027,10 @@
 
 		/* XXX propagate flags from initial session? */
 		if (crypto_newsession(&nid, &(crp->crp_desc->CRD_INI),
-		    CRYPTOCAP_F_HARDWARE | CRYPTOCAP_F_SOFTWARE) == 0)
-			crp->crp_sid = nid;
+				      CRYPTOCAP_F_HARDWARE | CRYPTOCAP_F_SOFTWARE) == 0) {
+			crp->crp_sid[0] = nid[0];
+			crp->crp_sid[1] = nid[1];
+		}
 
 		crp->crp_etype = EAGAIN;
 		crypto_done(crp);
diff -ruw /export/tmp/linux-2.6.17.10/crypto/ocf/cryptodev.c ./cryptodev.c
--- /export/tmp/linux-2.6.17.10/crypto/ocf/cryptodev.c	2009-01-29 10:10:36.000000000 -0800
+++ ./cryptodev.c	2009-01-29 23:39:00.000000000 -0800
@@ -82,7 +82,7 @@
 
 struct csession {
 	struct list_head	list;
-	u_int64_t	sid;
+	u_int64_t	sid[2];
 	u_int32_t	ses;
 
 	wait_queue_head_t waitq;
@@ -113,7 +113,7 @@
 static struct csession *csefind(struct fcrypt *, u_int);
 static int csedelete(struct fcrypt *, struct csession *);
 static struct csession *cseadd(struct fcrypt *, struct csession *);
-static struct csession *csecreate(struct fcrypt *, u_int64_t,
+static struct csession *csecreate(struct fcrypt *, u_int64_t *,
 		struct cryptoini *crie, struct cryptoini *cria, struct csession_info *);
 static int csefree(struct csession *);
 
@@ -256,7 +256,8 @@
 		       | (cop->flags & COP_F_BATCH);
 	crp->crp_buf = (caddr_t)&cse->uio;
 	crp->crp_callback = (int (*) (struct cryptop *)) cryptodev_cb;
-	crp->crp_sid = cse->sid;
+	crp->crp_sid[0] = cse->sid[0];
+	crp->crp_sid[1] = cse->sid[1];
 	crp->crp_opaque = (void *)cse;
 
 	if (cop->iv) {
@@ -325,7 +326,7 @@
 
 	if (crp->crp_etype != 0) {
 		error = crp->crp_etype;
-		dprintk("%s error in crp processing\n", __FUNCTION__);
+		dprintk("%s error in crp processing: error=%d\n", __FUNCTION__, error);
 		goto bail;
 	}
 
@@ -568,7 +569,7 @@
 }
 
 static struct csession *
-csecreate(struct fcrypt *fcr, u_int64_t sid, struct cryptoini *crie,
+csecreate(struct fcrypt *fcr, u_int64_t *sid, struct cryptoini *crie,
 	struct cryptoini *cria, struct csession_info *info)
 {
 	struct csession *cse;
@@ -586,7 +587,8 @@
 	cse->keylen = crie->cri_klen/8;
 	cse->mackey = cria->cri_key;
 	cse->mackeylen = cria->cri_klen/8;
-	cse->sid = sid;
+	cse->sid[0] = sid[0];
+	cse->sid[1] = sid[1];
 	cse->cipher = crie->cri_alg;
 	cse->mac = cria->cri_alg;
 	cse->info = *info;
@@ -600,7 +602,7 @@
 	int error;
 
 	dprintk("%s()\n", __FUNCTION__);
-	error = crypto_freesession(cse->sid);
+	error = crypto_freesession(&cse->sid[0]);
 	if (cse->key)
 		kfree(cse->key);
 	if (cse->mackey)
@@ -624,7 +626,7 @@
 	struct crypt_op cop;
 	struct crypt_kop kop;
 	struct crypt_find_op fop;
-	u_int64_t sid;
+	u_int64_t sid[2];
 	u_int32_t ses;
 	int feat, fd, error = 0, crid;
 	mm_segment_t fs;
@@ -822,15 +824,15 @@
 			/* allow either HW or SW to be used */
 			crid = CRYPTOCAP_F_HARDWARE | CRYPTOCAP_F_SOFTWARE;
 		}
-		error = crypto_newsession(&sid, (info.blocksize ? &crie : &cria), crid);
+		error = crypto_newsession(&sid[0], (info.blocksize ? &crie : &cria), crid);
 		if (error) {
 			dprintk("%s(%s) - newsession %d\n",__FUNCTION__,CIOCGSESSSTR,error);
 			goto bail;
 		}
 
-		cse = csecreate(fcr, sid, &crie, &cria, &info);
+		cse = csecreate(fcr, &sid[0], &crie, &cria, &info);
 		if (cse == NULL) {
-			crypto_freesession(sid);
+			crypto_freesession(&sid[0]);
 			error = EINVAL;
 			dprintk("%s(%s) - csecreate failed\n", __FUNCTION__, CIOCGSESSSTR);
 			goto bail;
diff -ruw /export/tmp/linux-2.6.17.10/crypto/ocf/cryptodev.h ./cryptodev.h
--- /export/tmp/linux-2.6.17.10/crypto/ocf/cryptodev.h	2009-01-29 10:10:36.000000000 -0800
+++ ./cryptodev.h	2009-01-29 23:42:33.000000000 -0800
@@ -345,7 +345,7 @@
 	struct list_head crp_next;
 	wait_queue_head_t crp_waitq;
 
-	u_int64_t	crp_sid;	/* Session ID */
+	u_int64_t	crp_sid[2];	/* Session ID */
 	int		crp_ilen;	/* Input data total length */
 	int		crp_olen;	/* Result total length */
 
@@ -415,12 +415,12 @@
  * a copy of the driver's capabilities that can be used by client code to
  * optimize operation.
  */
-#define CRYPTO_SESID2HID(_sid)	(((_sid) >> 32) & 0x00ffffff)
-#define CRYPTO_SESID2CAPS(_sid)	(((_sid) >> 32) & 0xff000000)
-#define CRYPTO_SESID2LID(_sid)	(((u_int32_t) (_sid)) & 0xffffffff)
+#define CRYPTO_SESID2HID(_sid)	((_sid[1]) & 0x00ffffffULL)
+#define CRYPTO_SESID2CAPS(_sid)	(((_sid[1]) >> 32) & 0xff000000ULL)
+#define CRYPTO_SESID2LID(_sid)	(_sid[0])
 
 extern	int crypto_newsession(u_int64_t *sid, struct cryptoini *cri, int hard);
-extern	int crypto_freesession(u_int64_t sid);
+extern	int crypto_freesession(u_int64_t *sid);
 #define CRYPTOCAP_F_HARDWARE	CRYPTO_FLAG_HARDWARE
 #define CRYPTOCAP_F_SOFTWARE	CRYPTO_FLAG_SOFTWARE
 #define CRYPTOCAP_F_SYNC	0x04000000	/* operates synchronously */
diff -ruw /export/tmp/linux-2.6.17.10/crypto/ocf/cryptosoft.c ./cryptosoft.c
--- /export/tmp/linux-2.6.17.10/crypto/ocf/cryptosoft.c	2009-01-29 10:10:36.000000000 -0800
+++ ./cryptosoft.c	2009-01-29 23:25:47.000000000 -0800
@@ -201,7 +201,7 @@
 static u_int32_t swcr_sesnum = 0;
 
 static	int swcr_process(device_t, struct cryptop *, int);
-static	int swcr_newsession(device_t, u_int32_t *, struct cryptoini *);
+static	int swcr_newsession(device_t, u_int64_t *, struct cryptoini *);
 static	int swcr_freesession(device_t, u_int64_t);
 
 static device_method_t swcr_methods = {
@@ -220,7 +220,7 @@
  * Generate a new software session.
  */
 static int
-swcr_newsession(device_t dev, u_int32_t *sid, struct cryptoini *cri)
+swcr_newsession(device_t dev, u_int64_t *sid, struct cryptoini *cri)
 {
 	struct swcr_data **swd;
 	u_int32_t i;
@@ -407,7 +407,7 @@
 swcr_freesession(device_t dev, u_int64_t tid)
 {
 	struct swcr_data *swd;
-	u_int32_t sid = CRYPTO_SESID2LID(tid);
+	u_int32_t sid = tid;
 
 	dprintk("%s()\n", __FUNCTION__);
 	if (sid > swcr_sesnum || swcr_sessions == NULL ||
@@ -466,7 +466,7 @@
 		goto done;
 	}
 
-	lid = crp->crp_sid & 0xffffffff;
+	lid = CRYPTO_SESID2LID(crp->crp_sid);
 	if (lid >= swcr_sesnum || lid == 0 || swcr_sessions == NULL ||
 			swcr_sessions[lid] == NULL) {
 		crp->crp_etype = ENOENT;
diff -ruw /export/tmp/linux-2.6.17.10/crypto/ocf/ocf-bench.c ./ocf-bench.c
--- /export/tmp/linux-2.6.17.10/crypto/ocf/ocf-bench.c	2009-01-29 10:10:36.000000000 -0800
+++ ./ocf-bench.c	2009-01-29 19:01:12.000000000 -0800
@@ -104,7 +104,7 @@
  * OCF benchmark routines
  */
 
-static uint64_t ocf_cryptoid;
+static uint64_t ocf_cryptoid[2];
 static int ocf_init(void);
 static int ocf_cb(struct cryptop *crp);
 static void ocf_request(void *arg);
@@ -134,7 +134,7 @@
 
 	crie.cri_next = &cria;
 
-	error = crypto_newsession(&ocf_cryptoid, &crie, 0);
+	error = crypto_newsession(&ocf_cryptoid[0], &crie, 0);
 	if (error) {
 		printk("crypto_newsession failed %d\n", error);
 		return -1;
@@ -203,7 +203,8 @@
 	crp->crp_flags = CRYPTO_F_CBIMM;
 	crp->crp_buf = (caddr_t) r->buffer;
 	crp->crp_callback = ocf_cb;
-	crp->crp_sid = ocf_cryptoid;
+	crp->crp_sid[0] = ocf_cryptoid[0];
+	crp->crp_sid[1] = ocf_cryptoid[1];
 	crp->crp_opaque = (caddr_t) r;
 	crypto_dispatch(crp);
 }
diff -ruw /export/tmp/linux-2.6.17.10/crypto/ocf/ocf-compat.h ./ocf-compat.h
--- /export/tmp/linux-2.6.17.10/crypto/ocf/ocf-compat.h	2009-01-29 10:10:36.000000000 -0800
+++ ./ocf-compat.h	2009-01-29 23:44:33.000000000 -0800
@@ -41,7 +41,7 @@
 typedef struct ocf_device *device_t;
 
 typedef struct {
-	int (*cryptodev_newsession)(device_t dev, u_int32_t *sidp, struct cryptoini *cri);
+	int (*cryptodev_newsession)(device_t dev, u_int64_t *sidp, struct cryptoini *cri);
 	int (*cryptodev_freesession)(device_t dev, u_int64_t tid);
 	int (*cryptodev_process)(device_t dev, struct cryptop *crp, int hint);
 	int (*cryptodev_kprocess)(device_t dev, struct cryptkop *krp, int hint);
diff -ruw /export/tmp/linux-2.6.17.10/crypto/ocf/ocfnull/ocfnull.c ./ocfnull/ocfnull.c
--- /export/tmp/linux-2.6.17.10/crypto/ocf/ocfnull/ocfnull.c	2009-01-29 10:10:36.000000000 -0800
+++ ./ocfnull/ocfnull.c	2009-01-29 19:17:59.000000000 -0800
@@ -52,7 +52,7 @@
 static u_int32_t		 null_sesnum = 0;
 
 static int null_process(device_t, struct cryptop *, int);
-static int null_newsession(device_t, u_int32_t *, struct cryptoini *);
+static int null_newsession(device_t, u_int64_t *, struct cryptoini *);
 static int null_freesession(device_t, u_int64_t);
 
 #define debug ocfnull_debug
@@ -79,7 +79,7 @@
  * Generate a new software session.
  */
 static int
-null_newsession(device_t arg, u_int32_t *sid, struct cryptoini *cri)
+null_newsession(device_t arg, u_int64_t *sid, struct cryptoini *cri)
 {
 	dprintk("%s()\n", __FUNCTION__);
 	if (sid == NULL || cri == NULL) {
@@ -100,7 +100,7 @@
 static int
 null_freesession(device_t arg, u_int64_t tid)
 {
-	u_int32_t sid = CRYPTO_SESID2LID(tid);
+	u_int32_t sid = tid;
 
 	dprintk("%s()\n", __FUNCTION__);
 	if (sid > null_sesnum) {
@@ -143,7 +143,7 @@
 	 * find the session we are using
 	 */
 
-	lid = crp->crp_sid & 0xffffffff;
+	lid = CRYPTO_SESID2LID(crp->crp_sid);
 	if (lid >= null_sesnum || lid == 0) {
 		crp->crp_etype = ENOENT;
 		dprintk("%s,%d: ENOENT\n", __FILE__, __LINE__);
 | 
| 
      
      
      From: Pirasenna T. <pir...@gm...> - 2009-02-03 02:15:18
       | 
| 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!! | 
| 
      
      
      From: David M. <Dav...@se...> - 2009-02-03 02:23:08
       | 
| 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 |