All of lore.kernel.org
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@infradead.org>
To: Scott Bauer <scott.bauer@intel.com>
Cc: hch@infradead.org, sagi@grimberg.me, axboe@fb.com,
	linux-nvme@lists.infradead.org, keith.busch@intel.com,
	Rafael.Antognolli@intel.com, linux-block@vger.kernel.org,
	jonathan.derrick@intel.com, j.naumann@fu-berlin.de
Subject: Re: [PATCH v1 0/7] SED OPAL Library
Date: Thu, 17 Nov 2016 05:12:51 -0800	[thread overview]
Message-ID: <20161117131251.GA15852@infradead.org> (raw)
In-Reply-To: <1479338252-8777-1-git-send-email-scott.bauer@intel.com>

Hi Scott,

I took a look at the code and here are some very high level comments:

 - we only call into block_device_operations.sec_ops from the ioctl
   handlers.  So instead of adding it to the block layer I'd rather
   structure the code so that the driver itself calls a new common
   blkdev_sed_ioctl handler implemented in lib/sed.c, which then gets
   callbacks passed directly from the calling, similar to how
   opal_unlock_from_suspend works.  And the callbacks might actually
   be condensed to one I think, given that all potential
   implementations would basically just dispatch to two
   different opcode but otherwise use the same implementation.
 - talking about lib/sed*.c - I'd move it to block/
 - there are a lot of levels of indirection in the code, I think
   we can condense them down a bit to basically just having the
   main blkdev_sed_ioctl entry point, which should check
   bdev_sec_capable first, and then dispatch to the security
   types, probably through a little method table.
 - what's so special about request_user_key that it can't be inline
   into the only caller but needs a separate file?
 - please don't use pointer indirections in your userspace ABI,
   struct sed_key will be a pain to handle for 32-bit userspace
   on 64-bit kernels.  I don't fully understand what the key_type
   is for anyway - it seems like exactly one type is supported
   per call anyway.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

WARNING: multiple messages have this Message-ID (diff)
From: hch@infradead.org (Christoph Hellwig)
Subject: [PATCH v1 0/7] SED OPAL Library
Date: Thu, 17 Nov 2016 05:12:51 -0800	[thread overview]
Message-ID: <20161117131251.GA15852@infradead.org> (raw)
In-Reply-To: <1479338252-8777-1-git-send-email-scott.bauer@intel.com>

Hi Scott,

I took a look at the code and here are some very high level comments:

 - we only call into block_device_operations.sec_ops from the ioctl
   handlers.  So instead of adding it to the block layer I'd rather
   structure the code so that the driver itself calls a new common
   blkdev_sed_ioctl handler implemented in lib/sed.c, which then gets
   callbacks passed directly from the calling, similar to how
   opal_unlock_from_suspend works.  And the callbacks might actually
   be condensed to one I think, given that all potential
   implementations would basically just dispatch to two
   different opcode but otherwise use the same implementation.
 - talking about lib/sed*.c - I'd move it to block/
 - there are a lot of levels of indirection in the code, I think
   we can condense them down a bit to basically just having the
   main blkdev_sed_ioctl entry point, which should check
   bdev_sec_capable first, and then dispatch to the security
   types, probably through a little method table.
 - what's so special about request_user_key that it can't be inline
   into the only caller but needs a separate file?
 - please don't use pointer indirections in your userspace ABI,
   struct sed_key will be a pain to handle for 32-bit userspace
   on 64-bit kernels.  I don't fully understand what the key_type
   is for anyway - it seems like exactly one type is supported
   per call anyway.

  parent reply	other threads:[~2016-11-17 13:12 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-16 23:17 [PATCH v1 0/7] SED OPAL Library Scott Bauer
2016-11-16 23:17 ` Scott Bauer
2016-11-16 23:17 ` [PATCH v1 1/7] Include: Add definitions for sed Scott Bauer
2016-11-16 23:17   ` Scott Bauer
2016-11-17 15:22   ` Christoph Hellwig
2016-11-17 15:22     ` Christoph Hellwig
2016-11-17 16:10     ` Scott Bauer
2016-11-17 16:10       ` Scott Bauer
2016-11-16 23:17 ` [PATCH v1 2/7] lib: Add Sed-opal library Scott Bauer
2016-11-16 23:17   ` Scott Bauer
2016-11-17  0:35   ` Keith Busch
2016-11-17  0:35     ` Keith Busch
2016-11-17 15:38   ` Christoph Hellwig
2016-11-17 15:38     ` Christoph Hellwig
2016-11-16 23:17 ` [PATCH v1 3/7] lib: Add Sed to Kconfig and Makefile Scott Bauer
2016-11-16 23:17   ` Scott Bauer
2016-11-16 23:17 ` [PATCH v1 4/7] include: Add sec_ops to block device operations Scott Bauer
2016-11-16 23:17   ` Scott Bauer
2016-11-16 23:17 ` [PATCH v1 5/7] nvme: Implement SED Security Operations Scott Bauer
2016-11-16 23:17   ` Scott Bauer
2016-11-17  0:09   ` Keith Busch
2016-11-17  0:09     ` Keith Busch
2016-11-16 23:17 ` [PATCH v1 6/7] nvme: Implement SED Unlock from suspend Scott Bauer
2016-11-16 23:17   ` Scott Bauer
2016-11-17 13:16   ` Christoph Hellwig
2016-11-17 13:16     ` Christoph Hellwig
2016-11-16 23:17 ` [PATCH v1 7/7] block: ioctl: Wire up Sed to block ioctls Scott Bauer
2016-11-16 23:17   ` Scott Bauer
2016-11-17 13:12 ` Christoph Hellwig [this message]
2016-11-17 13:12   ` [PATCH v1 0/7] SED OPAL Library Christoph Hellwig
2016-11-17 17:36   ` Scott Bauer
2016-11-17 17:36     ` Scott Bauer
2016-11-17 18:21     ` Rafael Antognolli
2016-11-17 18:21       ` Rafael Antognolli
2016-11-17 19:28     ` Christoph Hellwig
2016-11-17 19:28       ` Christoph Hellwig
2016-11-17 19:33       ` Scott Bauer
2016-11-17 19:33         ` Scott Bauer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161117131251.GA15852@infradead.org \
    --to=hch@infradead.org \
    --cc=Rafael.Antognolli@intel.com \
    --cc=axboe@fb.com \
    --cc=j.naumann@fu-berlin.de \
    --cc=jonathan.derrick@intel.com \
    --cc=keith.busch@intel.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=sagi@grimberg.me \
    --cc=scott.bauer@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.