[v3,for-2.11,1/3] tpm_emulator: Add a caching layer for the TPM Established flag

Message ID 1510696349-5151-2-git-send-email-stefanb@linux.vnet.ibm.com
State New
Headers show
Series
  • tpm: a few fixes
Related show

Commit Message

Stefan Berger Nov. 14, 2017, 9:52 p.m.
Add a caching layer for the TPM established flag so that we don't
need to go to the emulator every time the flag is read by accessing
the REG_ACCESS register.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>

v1->v2:
 - move the caching to the backend layer since detecting the
   TPM 1.2 TSC_ResetEstablishmentBit() command is easier to do
   here.
---
 hw/tpm/tpm_emulator.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Comments

Marc-André Lureau Nov. 14, 2017, 11:40 p.m. | #1
Hi

On Tue, Nov 14, 2017 at 10:52 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Add a caching layer for the TPM established flag so that we don't
> need to go to the emulator every time the flag is read by accessing
> the REG_ACCESS register.

What's the impact? Isn't this just a "small" optimization? Iotw, why
is this for-2.11?

> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>
> v1->v2:
>  - move the caching to the backend layer since detecting the
>    TPM 1.2 TSC_ResetEstablishmentBit() command is easier to do
>    here.
> ---
>  hw/tpm/tpm_emulator.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
> index e1a6810..b293db7 100644
> --- a/hw/tpm/tpm_emulator.c
> +++ b/hw/tpm/tpm_emulator.c
> @@ -73,6 +73,9 @@ typedef struct TPMEmulator {
>      Error *migration_blocker;
>
>      QemuMutex mutex;
> +
> +    unsigned int established_flag:1;
> +    unsigned int established_flag_cached:1;
>  } TPMEmulator;
>
>
> @@ -287,16 +290,22 @@ static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
>      TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>      ptm_est est;
>
> -    DPRINTF("%s", __func__);
> +    if (tpm_emu->established_flag_cached) {
> +        return tpm_emu->established_flag;
> +    }
> +
>      if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_TPMESTABLISHED, &est,
>                               0, sizeof(est)) < 0) {
>          error_report("tpm-emulator: Could not get the TPM established flag: %s",
>                       strerror(errno));
>          return false;
>      }
> -    DPRINTF("established flag: %0x", est.u.resp.bit);
> +    DPRINTF("got established flag: %0x", est.u.resp.bit);
> +
> +    tpm_emu->established_flag_cached = 1;
> +    tpm_emu->established_flag = (est.u.resp.bit != 0);
>
> -    return (est.u.resp.bit != 0);
> +    return tpm_emu->established_flag;
>  }
>
>  static int tpm_emulator_reset_tpm_established_flag(TPMBackend *tb,
> @@ -327,6 +336,8 @@ static int tpm_emulator_reset_tpm_established_flag(TPMBackend *tb,
>          return -1;
>      }
>
> +    tpm_emu->established_flag_cached = 0;
> +
>      return 0;
>  }
>
> --
> 2.5.5
>
Stefan Berger Nov. 15, 2017, 1:16 a.m. | #2
On 11/14/2017 06:40 PM, Marc-André Lureau wrote:
> Hi
>
> On Tue, Nov 14, 2017 at 10:52 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> Add a caching layer for the TPM established flag so that we don't
>> need to go to the emulator every time the flag is read by accessing
>> the REG_ACCESS register.
> What's the impact? Isn't this just a "small" optimization? Iotw, why
> is this for-2.11?

The TIS has a register that contains this flag and that's being polled 
quite frequently. So it generates a lot of traffic to the emulator. This 
caching layer gets rid of most of the traffic.

    Stefan

>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>
>> v1->v2:
>>   - move the caching to the backend layer since detecting the
>>     TPM 1.2 TSC_ResetEstablishmentBit() command is easier to do
>>     here.
>> ---
>>   hw/tpm/tpm_emulator.c | 17 ++++++++++++++---
>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
>> index e1a6810..b293db7 100644
>> --- a/hw/tpm/tpm_emulator.c
>> +++ b/hw/tpm/tpm_emulator.c
>> @@ -73,6 +73,9 @@ typedef struct TPMEmulator {
>>       Error *migration_blocker;
>>
>>       QemuMutex mutex;
>> +
>> +    unsigned int established_flag:1;
>> +    unsigned int established_flag_cached:1;
>>   } TPMEmulator;
>>
>>
>> @@ -287,16 +290,22 @@ static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
>>       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>>       ptm_est est;
>>
>> -    DPRINTF("%s", __func__);
>> +    if (tpm_emu->established_flag_cached) {
>> +        return tpm_emu->established_flag;
>> +    }
>> +
>>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_TPMESTABLISHED, &est,
>>                                0, sizeof(est)) < 0) {
>>           error_report("tpm-emulator: Could not get the TPM established flag: %s",
>>                        strerror(errno));
>>           return false;
>>       }
>> -    DPRINTF("established flag: %0x", est.u.resp.bit);
>> +    DPRINTF("got established flag: %0x", est.u.resp.bit);
>> +
>> +    tpm_emu->established_flag_cached = 1;
>> +    tpm_emu->established_flag = (est.u.resp.bit != 0);
>>
>> -    return (est.u.resp.bit != 0);
>> +    return tpm_emu->established_flag;
>>   }
>>
>>   static int tpm_emulator_reset_tpm_established_flag(TPMBackend *tb,
>> @@ -327,6 +336,8 @@ static int tpm_emulator_reset_tpm_established_flag(TPMBackend *tb,
>>           return -1;
>>       }
>>
>> +    tpm_emu->established_flag_cached = 0;
>> +
>>       return 0;
>>   }
>>
>> --
>> 2.5.5
>>
>
>
Marc-André Lureau Nov. 15, 2017, 3:47 a.m. | #3
Hi

On Wed, Nov 15, 2017 at 2:16 AM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> On 11/14/2017 06:40 PM, Marc-André Lureau wrote:
>>
>> Hi
>>
>> On Tue, Nov 14, 2017 at 10:52 PM, Stefan Berger
>> <stefanb@linux.vnet.ibm.com> wrote:
>>>
>>> Add a caching layer for the TPM established flag so that we don't
>>> need to go to the emulator every time the flag is read by accessing
>>> the REG_ACCESS register.
>>
>> What's the impact? Isn't this just a "small" optimization? Iotw, why
>> is this for-2.11?
>
>
> The TIS has a register that contains this flag and that's being polled quite
> frequently. So it generates a lot of traffic to the emulator. This caching
> layer gets rid of most of the traffic.

I didn't notice any problem when doing my tests, I guess Amarnath
niether. perhaps it's best to delay for after 2.11.

>    Stefan
>
>
>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>>
>>> v1->v2:
>>>   - move the caching to the backend layer since detecting the
>>>     TPM 1.2 TSC_ResetEstablishmentBit() command is easier to do
>>>     here.
>>> ---
>>>   hw/tpm/tpm_emulator.c | 17 ++++++++++++++---
>>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
>>> index e1a6810..b293db7 100644
>>> --- a/hw/tpm/tpm_emulator.c
>>> +++ b/hw/tpm/tpm_emulator.c
>>> @@ -73,6 +73,9 @@ typedef struct TPMEmulator {
>>>       Error *migration_blocker;
>>>
>>>       QemuMutex mutex;
>>> +
>>> +    unsigned int established_flag:1;
>>> +    unsigned int established_flag_cached:1;
>>>   } TPMEmulator;
>>>
>>>
>>> @@ -287,16 +290,22 @@ static bool
>>> tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
>>>       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>>>       ptm_est est;
>>>
>>> -    DPRINTF("%s", __func__);
>>> +    if (tpm_emu->established_flag_cached) {
>>> +        return tpm_emu->established_flag;
>>> +    }
>>> +
>>>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_TPMESTABLISHED, &est,
>>>                                0, sizeof(est)) < 0) {
>>>           error_report("tpm-emulator: Could not get the TPM established
>>> flag: %s",
>>>                        strerror(errno));
>>>           return false;
>>>       }
>>> -    DPRINTF("established flag: %0x", est.u.resp.bit);
>>> +    DPRINTF("got established flag: %0x", est.u.resp.bit);
>>> +
>>> +    tpm_emu->established_flag_cached = 1;
>>> +    tpm_emu->established_flag = (est.u.resp.bit != 0);
>>>
>>> -    return (est.u.resp.bit != 0);
>>> +    return tpm_emu->established_flag;
>>>   }
>>>
>>>   static int tpm_emulator_reset_tpm_established_flag(TPMBackend *tb,
>>> @@ -327,6 +336,8 @@ static int
>>> tpm_emulator_reset_tpm_established_flag(TPMBackend *tb,
>>>           return -1;
>>>       }
>>>
>>> +    tpm_emu->established_flag_cached = 0;
>>> +
>>>       return 0;
>>>   }
>>>
>>> --
>>> 2.5.5
>>>
>>
>>
>
Valluri, Amarnath Nov. 15, 2017, 9:05 a.m. | #4
On Wed, 2017-11-15 at 04:47 +0100, Marc-André Lureau wrote:
> Hi

> 

> On Wed, Nov 15, 2017 at 2:16 AM, Stefan Berger

> <stefanb@linux.vnet.ibm.com> wrote:

> > 

> > On 11/14/2017 06:40 PM, Marc-André Lureau wrote:

> > > 

> > > 

> > > Hi

> > > 

> > > On Tue, Nov 14, 2017 at 10:52 PM, Stefan Berger

> > > <stefanb@linux.vnet.ibm.com> wrote:

> > > > 

> > > > 

> > > > Add a caching layer for the TPM established flag so that we

> > > > don't

> > > > need to go to the emulator every time the flag is read by

> > > > accessing

> > > > the REG_ACCESS register.

> > > What's the impact? Isn't this just a "small" optimization? Iotw,

> > > why

> > > is this for-2.11?

> > 

> > The TIS has a register that contains this flag and that's being

> > polled quite

> > frequently. So it generates a lot of traffic to the emulator. This

> > caching

> > layer gets rid of most of the traffic.

> I didn't notice any problem when doing my tests, I guess Amarnath

> niether. perhaps it's best to delay for after 2.11.


Yes, I could see there is little frequent(21 calls to backend) to get
establish flag, and this patch will help to avoid them. But i still
agree with Marc and tag this as "small" optimization.

- Amarnath
Stefan Berger Nov. 15, 2017, 12:18 p.m. | #5
On 11/14/2017 10:47 PM, Marc-André Lureau wrote:
> Hi
>
> On Wed, Nov 15, 2017 at 2:16 AM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> On 11/14/2017 06:40 PM, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Tue, Nov 14, 2017 at 10:52 PM, Stefan Berger
>>> <stefanb@linux.vnet.ibm.com> wrote:
>>>> Add a caching layer for the TPM established flag so that we don't
>>>> need to go to the emulator every time the flag is read by accessing
>>>> the REG_ACCESS register.
>>> What's the impact? Isn't this just a "small" optimization? Iotw, why
>>> is this for-2.11?
>>
>> The TIS has a register that contains this flag and that's being polled quite
>> frequently. So it generates a lot of traffic to the emulator. This caching
>> layer gets rid of most of the traffic.
> I didn't notice any problem when doing my tests, I guess Amarnath
> niether. perhaps it's best to delay for after 2.11.

So then let's delay it. Following a Reviewed-by I'd put it in the 
tpm-next queue then.

    Stefan

Patch

diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index e1a6810..b293db7 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -73,6 +73,9 @@  typedef struct TPMEmulator {
     Error *migration_blocker;
 
     QemuMutex mutex;
+
+    unsigned int established_flag:1;
+    unsigned int established_flag_cached:1;
 } TPMEmulator;
 
 
@@ -287,16 +290,22 @@  static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
     TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
     ptm_est est;
 
-    DPRINTF("%s", __func__);
+    if (tpm_emu->established_flag_cached) {
+        return tpm_emu->established_flag;
+    }
+
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_TPMESTABLISHED, &est,
                              0, sizeof(est)) < 0) {
         error_report("tpm-emulator: Could not get the TPM established flag: %s",
                      strerror(errno));
         return false;
     }
-    DPRINTF("established flag: %0x", est.u.resp.bit);
+    DPRINTF("got established flag: %0x", est.u.resp.bit);
+
+    tpm_emu->established_flag_cached = 1;
+    tpm_emu->established_flag = (est.u.resp.bit != 0);
 
-    return (est.u.resp.bit != 0);
+    return tpm_emu->established_flag;
 }
 
 static int tpm_emulator_reset_tpm_established_flag(TPMBackend *tb,
@@ -327,6 +336,8 @@  static int tpm_emulator_reset_tpm_established_flag(TPMBackend *tb,
         return -1;
     }
 
+    tpm_emu->established_flag_cached = 0;
+
     return 0;
 }