| Message ID | 20180103003355.oaie7tych5lmtxiq@cantor (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Le 03/01/18 à 01:33, Jerry Snitselaar a écrit : > On Wed Jan 03 18, Laurent Bigonville wrote: >> Le 17/11/17 à 14:16, Jarkko Sakkinen a écrit : >>> On Tue, Nov 14, 2017 at 07:17:11AM -0800, James Bottomley wrote: >>>> On Tue, 2017-11-14 at 16:59 +0200, Jarkko Sakkinen wrote: >>>>> On Sat, Nov 11, 2017 at 01:31:32PM -0700, Jerry Snitselaar wrote: >>>>>> On Sat Nov 11 17, Jason Gunthorpe wrote: >>>>>>> On Sat, Nov 11, 2017 at 12:12:57PM -0700, Jerry Snitselaar wrote: >>>>>>> >>>>>>>> Before the release_locality code would only actually release >>>>>>>> the locality if the request use bit was set. So after it >>>>>>>> grabbed the locality during probe it probably never released >>>>>>>> it. The idea with the new code was to release it when it was no >>>>>>>> longer needed so another requester would be able to take the >>>>>>>> tpm without having to wait for it to be released. >>>>>>> If I recall, this was so that system level things outside linux >>>>>>> could access the TPM properly?? >>>>>>> >>>>>> Yes, that is what drove this initially. I believe Jarkko was also >>>>>> thinking of the possibility in the future where something like a vm >>>>>> could request a locality as well, but that is just a hazy >>>>>> recollection of emails from back then. >>>>> This was something I recall discussing in LPC 2016 in the hallway at >>>>> least :-) A tidbit but it could make sense to tie it to VMM, not VM. >>>> I think we should be extremely wary of different localities before we >>>> have a cast iron definition of what they mean. All the TPM PC spec >>>> says is that locality 4 is reserved for firmware (meaning the kernel >>>> should have no access) and it implies there's a privilege hierarchy, >>>> making 4 the most privileged and 0 the least but leaves all the >>>> definition to the OS. Since we only have four other localities to >>>> play >>>> with, we need a global definition of what they mean in Linux (and who >>>> protects them) otherwise we'll get conflicting uses. What does >>>> Windows >>>> use them for? >>>> >>>> James >>> No idea. If I had to guess, they use only one locality for OS as this >>> what PTT/fTPM had when it didn't have localities. At least their >>> implementation works with only one locality. >>> >> >> No more idea then? :( > > Hi Laurent, > > Can you try the following debug patch (earlier idea of adding a sleep > to allow > tpm to complete state transition): > > --8<-- > > diff --git a/drivers/char/tpm/tpm_tis_core.c > b/drivers/char/tpm/tpm_tis_core.c > index fdde971bc810..6a9325b02059 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -80,6 +80,7 @@ static void release_locality(struct tpm_chip *chip, > int l) > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); > + tpm_msleep(200); > } > > static int request_locality(struct tpm_chip *chip, int l) Thanks, With that patch the device is showing up again in /dev and I tpm_sealdata is working as well
I don't remember if I replied to this, re-posting to be sure: Le 03/01/18 à 01:33, Jerry Snitselaar a écrit : > Hi Laurent, > > Can you try the following debug patch (earlier idea of adding a sleep > to allow > tpm to complete state transition): > > --8<-- > > diff --git a/drivers/char/tpm/tpm_tis_core.c > b/drivers/char/tpm/tpm_tis_core.c > index fdde971bc810..6a9325b02059 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -80,6 +80,7 @@ static void release_locality(struct tpm_chip *chip, > int l) > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); > + tpm_msleep(200); > } > > static int request_locality(struct tpm_chip *chip, int l) I tried the patch and this is working. Would that be a viable solution? Kind regards, Laurent Bigonville
On Fri, Feb 09, 2018 at 11:53:55AM +0100, Laurent Bigonville wrote: > I don't remember if I replied to this, re-posting to be sure: > > Le 03/01/18 à 01:33, Jerry Snitselaar a écrit : > > Hi Laurent, > > > > Can you try the following debug patch (earlier idea of adding a sleep to > > allow > > tpm to complete state transition): > > > > --8<-- > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c > > b/drivers/char/tpm/tpm_tis_core.c > > index fdde971bc810..6a9325b02059 100644 > > --- a/drivers/char/tpm/tpm_tis_core.c > > +++ b/drivers/char/tpm/tpm_tis_core.c > > @@ -80,6 +80,7 @@ static void release_locality(struct tpm_chip *chip, > > int l) > > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > > > tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); > > + tpm_msleep(200); > > } > > > > static int request_locality(struct tpm_chip *chip, int l) > > I tried the patch and this is working. > > Would that be a viable solution? > > Kind regards, > > Laurent Bigonville According to the PC client specification in the section 5.5.2.3: "For commands indicated as short or medium duration (i.e., those that do not cause key generation), the TPM SHALL respond to an abort within TIMEOUT_A. For commands indicated as lon g duration or those that cause key generation, the TPM SHALL respond to a request to abort the command within TIMEOUT B" The section 5.5.2.4 describing the access register does not give much more light to this i.e. what the expected duration maximum is when there is no command executing. /Jarkko
Le 14/02/18 à 12:44, Jarkko Sakkinen a écrit : > On Fri, Feb 09, 2018 at 11:53:55AM +0100, Laurent Bigonville wrote: >> I don't remember if I replied to this, re-posting to be sure: >> >> Le 03/01/18 à 01:33, Jerry Snitselaar a écrit : >>> Hi Laurent, >>> >>> Can you try the following debug patch (earlier idea of adding a sleep to >>> allow >>> tpm to complete state transition): >>> >>> --8<-- >>> >>> diff --git a/drivers/char/tpm/tpm_tis_core.c >>> b/drivers/char/tpm/tpm_tis_core.c >>> index fdde971bc810..6a9325b02059 100644 >>> --- a/drivers/char/tpm/tpm_tis_core.c >>> +++ b/drivers/char/tpm/tpm_tis_core.c >>> @@ -80,6 +80,7 @@ static void release_locality(struct tpm_chip *chip, >>> int l) >>> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >>> >>> tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); >>> + tpm_msleep(200); >>> } >>> >>> static int request_locality(struct tpm_chip *chip, int l) >> I tried the patch and this is working. >> >> Would that be a viable solution? >> >> Kind regards, >> >> Laurent Bigonville > According to the PC client specification in the section 5.5.2.3: > > "For commands indicated as short or medium duration (i.e., those that do > not cause key generation), the TPM SHALL respond to an abort within > TIMEOUT_A. For commands indicated as lon g duration or those that cause > key generation, the TPM SHALL respond to a request to abort the command > within TIMEOUT B" > > The section 5.5.2.4 describing the access register does not give much > more light to this i.e. what the expected duration maximum is when there > is no command executing. The duration that that was in your patch seems to work, cannot this be implemented? I'm quite surprised I'm the only one impacted by this...
On Fri, 2018-03-09 at 18:24 +0100, Laurent Bigonville wrote: > The duration that that was in your patch seems to work, cannot this be > implemented? > > I'm quite surprised I'm the only one impacted by this... Sorry it took so long time response but I had forgotten so too many details. After re-reading (PHEW!) the whole thread I'm thinking if we could use section 5.5.2.4 as a reference to select a timeout of TIMEOUT_A (albeit it is for request locality) and use Jerry's suggestion to put wait_for_tpm_stat() there? Opinions? /Jarkko
Le 15/03/18 à 17:24, Jarkko Sakkinen a écrit : > On Fri, 2018-03-09 at 18:24 +0100, Laurent Bigonville wrote: >> The duration that that was in your patch seems to work, cannot this be >> implemented? >> >> I'm quite surprised I'm the only one impacted by this... > Sorry it took so long time response but I had forgotten so too many > details. > > After re-reading (PHEW!) the whole thread I'm thinking if we could > use section 5.5.2.4 as a reference to select a timeout of TIMEOUT_A > (albeit it is for request locality) and use Jerry's suggestion to > put wait_for_tpm_stat() there? Opinions? Hey, Sorry to bother you again, but are there any progress on getting this mainlined? Kind regards, Laurent Bigonville
On Thu, May 03, 2018 at 01:38:17PM +0200, Laurent Bigonville wrote: > Le 15/03/18 à 17:24, Jarkko Sakkinen a écrit : > > On Fri, 2018-03-09 at 18:24 +0100, Laurent Bigonville wrote: > > > The duration that that was in your patch seems to work, cannot this be > > > implemented? > > > > > > I'm quite surprised I'm the only one impacted by this... > > Sorry it took so long time response but I had forgotten so too many > > details. > > > > After re-reading (PHEW!) the whole thread I'm thinking if we could > > use section 5.5.2.4 as a reference to select a timeout of TIMEOUT_A > > (albeit it is for request locality) and use Jerry's suggestion to > > put wait_for_tpm_stat() there? Opinions? > Hey, > > Sorry to bother you again, but are there any progress on getting this > mainlined? > > Kind regards, > > Laurent Bigonville Sorry for not doing anything. Jerry, do you want to send patch? /Jarkko
On Fri May 04 18, Jarkko Sakkinen wrote: >On Thu, May 03, 2018 at 01:38:17PM +0200, Laurent Bigonville wrote: >> Le 15/03/18 à 17:24, Jarkko Sakkinen a écrit : >> > On Fri, 2018-03-09 at 18:24 +0100, Laurent Bigonville wrote: >> > > The duration that that was in your patch seems to work, cannot this be >> > > implemented? >> > > >> > > I'm quite surprised I'm the only one impacted by this... >> > Sorry it took so long time response but I had forgotten so too many >> > details. >> > >> > After re-reading (PHEW!) the whole thread I'm thinking if we could >> > use section 5.5.2.4 as a reference to select a timeout of TIMEOUT_A >> > (albeit it is for request locality) and use Jerry's suggestion to >> > put wait_for_tpm_stat() there? Opinions? >> Hey, >> >> Sorry to bother you again, but are there any progress on getting this >> mainlined? >> >> Kind regards, >> >> Laurent Bigonville > >Sorry for not doing anything. Jerry, do you want to send patch? > >/Jarkko Yes. I'll send a patch today. Jerry
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index fdde971bc810..6a9325b02059 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -80,6 +80,7 @@ static void release_locality(struct tpm_chip *chip, int l) struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); + tpm_msleep(200); } static int request_locality(struct tpm_chip *chip, int l)