| Message ID | 1510696349-5151-2-git-send-email-stefanb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | tpm: a few fixes | expand |
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 >
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 >> > >
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 >>> >> >> >
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
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
On 11/15/2017 04:05 AM, Valluri, Amarnath wrote: > 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. Can you review it so we have it ready for 2.12? Thanks, Stefan > > - Amarnath
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. > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> looks good, Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.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 >
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; }
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(-)