From: Jens Wiklander <jens.wiklander@linaro.org>
To: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Daniel Thompson <daniel.thompson@linaro.org>,
Sumit Garg <sumit.garg@linaro.org>,
Jerome Forissier <jerome@forissier.org>,
Maxim Uvarov <maxim.uvarov@linaro.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
Arnd Bergmann <arnd@linaro.org>,
"tee-dev @ lists . linaro . org" <tee-dev@lists.linaro.org>,
Jason Gunthorpe <jgg@ziepe.ca>,
linux-integrity@vger.kernel.org, peterhuewe@gmx.de
Subject: Re: [Tee-dev] [PATCHv8 1/3] optee: use uuid for sysfs driver entry
Date: Mon, 29 Jun 2020 09:03:02 +0200 [thread overview]
Message-ID: <20200629070302.GA210870@jade> (raw)
In-Reply-To: <1593184281.7381.16.camel@HansenPartnership.com>
On Fri, Jun 26, 2020 at 08:11:21AM -0700, James Bottomley wrote:
> On Fri, 2020-06-26 at 12:29 +0100, Daniel Thompson wrote:
> > On Fri, Jun 26, 2020 at 10:40:41AM +0530, Sumit Garg wrote:
> > > On Fri, 26 Jun 2020 at 05:01, James Bottomley
> > > <James.Bottomley@hansenpartnership.com> wrote:
> > > >
> > > > On Thu, 2020-06-25 at 19:54 +0530, Sumit Garg wrote:
> > > > > On Wed, 24 Jun 2020 at 20:51, James Bottomley
> > > > > <James.Bottomley@hansenpartnership.com> wrote:
> > > > > >
> > > > > > On Wed, 2020-06-24 at 16:17 +0530, Sumit Garg wrote:
> > > > > > > Apologies for delay in my reply as I was busy with some
> > > > > > > other stuff.
> > > > > > >
> > > > > > > On Fri, 19 Jun 2020 at 20:30, James Bottomley
> > > > > > > <James.Bottomley@hansenpartnership.com> wrote:
> > > > > >
> > > > > > [...]
> > > > > > > > it's about consistency with what the kernel types
> > > > > > > > mean. When some checker detects your using little endian
> > > > > > > > operations on a big endian structure (like in the prink
> > > > > > > > for instance) they're going to keep emailing you about
> > > > > > > > it.
> > > > > > >
> > > > > > > As mentioned above, using different terminology is meant to
> > > > > > > cause more confusion than just difference in endianness
> > > > > > > which is manageable inside TEE.
> > > > > > >
> > > > > > > And I think it's safe to say that the kernel implements
> > > > > > > UUID in big endian format and thus uses %pUb whereas OP-TEE
> > > > > > > implements UUID in little endian format and thus uses %pUl.
> > > > > >
> > > > > > So what I think you're saying is that if we still had uuid_be
> > > > > > and uuid_le you'd use uuid_le, because that's exactly the
> > > > > > structure described in the docs. But because we renamed
> > > > > >
> > > > > > uuid_be -> uuid_t
> > > > > > uuid_le -> guid_t
> > > > > >
> > > > > > You can't use guid_t as a kernel type because it has the
> > > > > > wrong name?
> > > > >
> > > > > Isn't the rename commit description [1] pretty clear about
> > > > > which is the true UUID type from Linux point of view?
> > > >
> > > > I don't think the kernel code takes a position on eternal verity,
> > > > just on logical or arithmetic truth. We just have to deal with
> > > > both LE and BE UUIDs so we have appropriate types for them and
> > > > the LE type is now named guid_t. They're both equally correct to
> > > > use provided the use case matches the designed one. So does the
> > > > name really matter?
> > >
> > > Yes it does. I guess I have provided enough reasoning for that.
> > > Also, the rename commit itself illustrates its importance and
> > > clarifies the use case for which they are meant to be used.
> > >
> > > > If we did
> > > >
> > > > #define uuid_le_t guid_t
> > > >
> > > > would you be happy? (not that the kernel cares about karmic
> > > > emotional states either ...)
> > >
> > > It's not about me being happy but more about confusion and
> > > inconsistency it will bring.
> > >
> > > IMO, either kernel should be opinionated about UUID endianness like
> > > currently it is:
> > >
> > > uuid_t and its corresponding helpers (eg. UUID_INIT) follows BE
> > > format.
> > >
> > > or support both endianness for UUID (no common type: uuid_t) like
> > > we had earlier prior to rename commit:
> > >
> > > uuid_be_t and its corresponding helpers (eg. UUID_BE_INIT) follow
> > > BE format. uuid_le_t and its corresponding helpers (eg.
> > > UUID_LE_INIT) follow LE format.
> > >
> > > But even if we consider later case as well, I am still not sure if
> > > we can switch to uuid_le_t as it's been part of TEE core ABI
> > > (open_session) where UUID is passed in BE format (see LE to BE
> > > conversion in TEE client [1] and vice-versa in OP-TEE OS [2]) and
> > > won't be a backwards compatible change.
> > >
> > > [1] https://github.com/OP-TEE/optee_client/blob/master/libteec/src/
> > > tee_client_api.c#L595
> > > [2] https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/ke
> > > rnel/ree_fs_ta.c#L92
> >
> > I'm struck that all references here are to code that does not run in
> > kernel space. Frankly on LKML I don't know if we should even *care*
> > what format UUIDs are stored in other address spaces.
> >
> > We care about is the endianness of the UUID on the interface
> > boundaries into and out of the kernel[1] and we care that we use the
> > correct kernel type to describe it.
> >
> > I understood from Jerome's post that the UUID that the kernel
> > manipulates are, in fact, big endian and that they should be called
> > uuid_t.
> >
> > Is there more going on here or is that it?
>
> As you say, a UUID to the kernel is a binary blob except for input,
> which to the kernel is INIT_UUID or INIT_GUID and output, which is
> either printk %Ub for uuid_t or %Ul for guid_t.
>
> The bit I objected to was doing a %Ul on a uuid_t because it's going to
> trip the static checkers. That shows me there's a confusion in the
> code between little and big endian UUID types, but I haven't looked
> further than that.
Thanks for bringing our attention to this, it was educating for me at
least. It seems the only problem was just the %Ul that should have been
a %Ub. The OP-TEE driver is only dealing with BE UUIDs internallay and
in the ABIs to user space and secure world.
I agree with Daniel, what happens in user space and in secure world
isn't of interest here as long as the ABIs are OK.
Cheers,
Jens
next prev parent reply other threads:[~2020-06-29 20:38 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-04 17:58 [PATCHv8 0/3] optee: register drivers on optee bus Maxim Uvarov
2020-06-04 17:58 ` [PATCHv8 1/3] optee: use uuid for sysfs driver entry Maxim Uvarov
2020-06-16 20:50 ` Jarkko Sakkinen
2020-06-17 6:07 ` Sumit Garg
2020-06-17 6:45 ` Maxim Uvarov
2020-06-17 13:58 ` Sumit Garg
2020-06-17 14:12 ` Maxim Uvarov
2020-06-17 15:16 ` [Tee-dev] " Jerome Forissier
2020-06-17 19:52 ` Maxim Uvarov
2020-06-17 20:45 ` Jerome Forissier
2020-06-18 4:59 ` Sumit Garg
2020-06-18 5:12 ` Sumit Garg
2020-06-18 19:18 ` James Bottomley
2020-06-19 8:12 ` Sumit Garg
2020-06-19 15:00 ` James Bottomley
2020-06-24 9:50 ` Jens Wiklander
2020-06-24 10:47 ` Sumit Garg
2020-06-24 15:21 ` James Bottomley
2020-06-24 15:44 ` Jerome Forissier
2020-06-25 12:51 ` Maxim Uvarov
2020-06-26 5:13 ` Sumit Garg
2020-06-26 8:47 ` Andy Shevchenko
2020-06-26 5:26 ` Sumit Garg
2020-06-25 14:24 ` Sumit Garg
2020-06-25 23:31 ` James Bottomley
2020-06-26 5:10 ` Sumit Garg
2020-06-26 7:26 ` Sumit Garg
2020-06-26 11:29 ` Daniel Thompson
2020-06-26 15:11 ` James Bottomley
2020-06-29 6:55 ` Sumit Garg
2020-06-29 7:03 ` Jens Wiklander [this message]
2020-06-18 6:57 ` Jerome Forissier
2020-06-18 7:44 ` Maxim Uvarov
2020-06-04 17:58 ` [PATCHv8 2/3] optee: enable support for multi-stage bus enumeration Maxim Uvarov
2020-06-16 20:56 ` Jarkko Sakkinen
2020-06-04 17:58 ` [PATCHv8 3/3] tpm_ftpm_tee: register driver on TEE bus Maxim Uvarov
2020-06-16 20:57 ` Jarkko Sakkinen
2020-06-15 14:32 ` [PATCHv8 0/3] optee: register drivers on optee bus Maxim Uvarov
2020-06-16 8:29 ` Jens Wiklander
2020-06-17 14:26 ` Maxim Uvarov
2020-06-18 8:00 ` Jens Wiklander
2020-06-17 23:37 ` Jarkko Sakkinen
2020-06-18 7:56 ` Jens Wiklander
2020-06-23 0:50 ` Jarkko Sakkinen
2020-06-16 20:49 ` Jarkko Sakkinen
2020-06-16 20:54 ` Jarkko Sakkinen
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=20200629070302.GA210870@jade \
--to=jens.wiklander@linaro.org \
--cc=James.Bottomley@HansenPartnership.com \
--cc=arnd@linaro.org \
--cc=daniel.thompson@linaro.org \
--cc=gregkh@linuxfoundation.org \
--cc=jarkko.sakkinen@linux.intel.com \
--cc=jerome@forissier.org \
--cc=jgg@ziepe.ca \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maxim.uvarov@linaro.org \
--cc=peterhuewe@gmx.de \
--cc=sumit.garg@linaro.org \
--cc=tee-dev@lists.linaro.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).