On Sun, 2019-08-25 at 16:08 -0400, Scott Bauer wrote:
> On Wed, Aug 21, 2019 at 01:10:51PM -0600, Revanth Rajashekar wrote:
>
> [snip]
>
> > The ioctl provides a private field with the intentiont to accommodate
> > any future expansions to the ioctl.
>
> spelling (intentiont)
>
> [snip]
>
> > + * IO_BUFFER_LENGTH = 2048
> > + * sizeof(header) = 56
> > + * No. of Token Bytes in the Response = 11
> > + * MAX size of data that can be carried in response buffer
> > + * at a time is : 2048 - (56 + 11) = 1981 = 0x7BD.
> > + */
> > +#define OPAL_MAX_READ_TABLE (0x7BD)
> > +
> > +static int read_table_data(struct opal_dev *dev, void *data)
> > +{
> > + dst = (u8 __user *)(uintptr_t)read_tbl->data;
> > + if (copy_to_user(dst + off, dev->prev_data, dev->prev_d_len)) {
> > + pr_debug("Error copying data to userspace\n");
> > + err = -EFAULT;
> > + break;
> > + }
>
> I'm with Jon on this one. Even though the spec says we have a max size, lets
> not put our trust in firmware engineers.
> A simple if check is easy to place before the CTU and will solve any future
> wtf debugging on a userland program.
>
>
I think we could do that as well as specify the
MaxResponseComPacketSize=IO_BUFFER_LENGTH in the command
https://trustedcomputinggroup.org/wp-content/uploads/TCG_Storage_Opal_SSC_Application_Note_1-00_1-00-Final.pdf
3.2.1.2.1 Host to TPer Properties invocation
>
>
>
> > +static int opal_generic_read_write_table(struct opal_dev *dev,
> > + struct opal_read_write_table
> > *rw_tbl)
> > +{
> > + const struct opal_step write_table_steps[] = {
> > + { start_admin1LSP_opal_session, &rw_tbl->key },
> > + { write_table_data, rw_tbl },
> > + { end_opal_session, }
> > + };
> > +
> > + const struct opal_step read_table_steps[] = {
> > + { start_admin1LSP_opal_session, &rw_tbl->key },
> > + { read_table_data, rw_tbl },
> > + { end_opal_session, }
> > + };
> > + int ret = 0;
> > +
> > + mutex_lock(&dev->dev_lock);
> > + setup_opal_dev(dev);
> > + if (rw_tbl->flags & OPAL_TABLE_READ) {
> > + if (rw_tbl->size > 0)
> > + ret = execute_steps(dev, read_table_steps,
> > + ARRAY_SIZE(read_table_steps));
> > + } else if (rw_tbl->flags & OPAL_TABLE_WRITE) {
> > + if (rw_tbl->size > 0)
> > + ret = execute_steps(dev, write_table_steps,
> > + ARRAY_SIZE(write_table_steps));
> > + } else {
> > + pr_debug("Invalid bit set in the flag.\n");
> > + ret = -EINVAL;
> > + }
> > + mutex_unlock(&dev->dev_lock);
> > +
> > + return ret;
> > +}
>
> Do we expect to add more flags in the future? I ask because this function can
> quickly get out
> of hand with regard to the else if chain and the function table list above.
> If we think we're going
> to add more flags in the future lets slap a switch statement in here to call
> opal_table_write() and
> opal_table_read(). We can deal with that in the future I guess, I just don't
> want a 3000 line function.
>
>
I had imagined potentially chaining ACS settings in the read/write
You could add a flag that says 'private' is another or multiple table
read/writes, and the private points to a descriptor equal to the ioctl
struct.
I'm ok with changing if/else to switch. Whichever looks better.
>
>
> > diff --git a/include/uapi/linux/sed-opal.h b/include/uapi/linux/sed-opal.h
> > index 59eed0bdffd3..a803ed0534da 100644
> > --- a/include/uapi/linux/sed-opal.h
> > +++ b/include/uapi/linux/sed-opal.h
> > +struct opal_read_write_table {
> > + struct opal_key key;
> > + const __u64 data;
> > + const __u8 table_uid[OPAL_UID_LENGTH];
> > + __u64 offset;
> > + __u64 size;
> > + #define OPAL_TABLE_READ (1 << 0)
> > + #define OPAL_TABLE_WRITE (1 << 1)
> > + __u64 flags;
> > + __u64 priv;
> > +};
>
> Two things, can you double check the pahole on this struct (Google it or ask
> Jon he knows).
I'll make sure we don't break padding and alignment for v2
> Second, can you lift those defines into Enumerations or out of the struct? Is
> there a reason
> they're in there?
Just seems to be common coding style for flags, ex fd.h
>
