[Qemu-devel] [PATCH v3 for-2.11 0/3] tpm: a few fixes

[Qemu-devel] [PATCH v3 for-2.11 0/3] tpm: a few fixes

[Stefan Berger]

From: Stefan Berger <Stefan Berger stefanb@linux.vnet.ibm.com>

The following patches fix a performance issue (patch 1) and an
error path issue (patches 2 and 3) for 2.11.

   Stefan

Stefan Berger (3):
  tpm_emulator: Add a caching layer for the TPM Established flag
  tpm_tis: Return TPM_VERSION_UNSPEC in case of BE failure
  tpm_tis: Return 0 for every register in case of failure mode

 hw/tpm/tpm_emulator.c | 17 ++++++++++++++---
 hw/tpm/tpm_tis.c      |  6 +++++-
 2 files changed, 19 insertions(+), 4 deletions(-)

-- 
2.5.5

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

[Stefan Berger]

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(-)

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

Re: [Qemu-devel] [PATCH v3 for-2.11 1/3] tpm_emulator: Add a caching layer for the TPM Established flag

[Marc-André Lureau]

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
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v3 for-2.11 1/3] tpm_emulator: Add a caching layer for the TPM Established flag

[Stefan Berger]

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
>>
>
>

Re: [Qemu-devel] [PATCH v3 for-2.11 1/3] tpm_emulator: Add a caching layer for the TPM Established flag

[Marc-André Lureau]

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
>>>
>>
>>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v3 for-2.11 1/3] tpm_emulator: Add a caching layer for the TPM Established flag

[Valluri, Amarnath]

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

Re: [Qemu-devel] [PATCH v3 for-2.11 1/3] tpm_emulator: Add a caching layer for the TPM Established flag

[Stefan Berger]

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

Re: [Qemu-devel] [PATCH v3 for-2.11 1/3] tpm_emulator: Add a caching layer for the TPM Established flag

[Stefan Berger]

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

Re: [Qemu-devel] [PATCH v3 for-2.11 1/3] tpm_emulator: Add a caching layer for the TPM Established flag

[Marc-André Lureau]

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
>



-- 
Marc-André Lureau

[Qemu-devel] [PATCH v3 for-2.11 2/3] tpm_tis: Return TPM_VERSION_UNSPEC in case of BE failure

[Stefan Berger]

In case the backend has a failure, such as the tpm_emulator's CMD_INIT
failing, the TIS goes into failure mode and does not respond to reads
or writes to MMIO registers. In this case we need to prevent the ACPI
table from being added and the straight-forward way is to indicate that
there's no known TPM version being used.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 hw/tpm/tpm_tis.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 7402528..fec2fc6 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -1008,6 +1008,10 @@ TPMVersion tpm_tis_get_tpm_version(Object *obj)
 {
     TPMState *s = TPM(obj);
 
+    if (tpm_backend_had_startup_error(s->be_driver)) {
+        return TPM_VERSION_UNSPEC;
+    }
+
     return tpm_backend_get_tpm_version(s->be_driver);
 }
 
-- 
2.5.5

Re: [Qemu-devel] [PATCH v3 for-2.11 2/3] tpm_tis: Return TPM_VERSION_UNSPEC in case of BE failure

[Marc-André Lureau]

Hi

On Tue, Nov 14, 2017 at 10:52 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> In case the backend has a failure, such as the tpm_emulator's CMD_INIT
> failing, the TIS goes into failure mode and does not respond to reads
> or writes to MMIO registers. In this case we need to prevent the ACPI
> table from being added and the straight-forward way is to indicate that
> there's no known TPM version being used.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

(we can probably iterate to improve the code around that later)

> ---
>  hw/tpm/tpm_tis.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 7402528..fec2fc6 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -1008,6 +1008,10 @@ TPMVersion tpm_tis_get_tpm_version(Object *obj)
>  {
>      TPMState *s = TPM(obj);
>
> +    if (tpm_backend_had_startup_error(s->be_driver)) {
> +        return TPM_VERSION_UNSPEC;
> +    }
> +
>      return tpm_backend_get_tpm_version(s->be_driver);
>  }
>
> --
> 2.5.5
>



-- 
Marc-André Lureau

[Qemu-devel] [PATCH v3 for-2.11 3/3] tpm_tis: Return 0 for every register in case of failure mode

[Stefan Berger]

Rather than returning ~0, return 0 for every register in case of
failure mode. The '0' is better to indicate that there's no device
there.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 hw/tpm/tpm_tis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index fec2fc6..42d647d 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -545,7 +545,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
     uint8_t v;
 
     if (tpm_backend_had_startup_error(s->be_driver)) {
-        return val;
+        return 0;
     }
 
     switch (offset) {
-- 
2.5.5

Re: [Qemu-devel] [PATCH v3 for-2.11 3/3] tpm_tis: Return 0 for every register in case of failure mode

[Marc-André Lureau]

Hi

On Tue, Nov 14, 2017 at 10:52 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Rather than returning ~0, return 0 for every register in case of
> failure mode. The '0' is better to indicate that there's no device
> there.

For most registers, 0 makes more sense. However, I wonder if we
shouldn't just fail to start qemu in this case...

Not convincing me this is 2.11 material either. Does this fix a specific bug?

> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  hw/tpm/tpm_tis.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index fec2fc6..42d647d 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -545,7 +545,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
>      uint8_t v;
>
>      if (tpm_backend_had_startup_error(s->be_driver)) {
> -        return val;
> +        return 0;
>      }
>
>      switch (offset) {
> --
> 2.5.5
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v3 for-2.11 3/3] tpm_tis: Return 0 for every register in case of failure mode

[Stefan Berger]

On 11/14/2017 06:47 PM, Marc-André Lureau wrote:
> Hi
>
> On Tue, Nov 14, 2017 at 10:52 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> Rather than returning ~0, return 0 for every register in case of
>> failure mode. The '0' is better to indicate that there's no device
>> there.
> For most registers, 0 makes more sense. However, I wonder if we
> shouldn't just fail to start qemu in this case...
>
> Not convincing me this is 2.11 material either. Does this fix a specific bug?

Yes, SeaBIOS detects the ~0 when it probes and thinks there's a device 
there. It then hangs trying to set flags and read registers to be able 
to use the device.

    Stefan

>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   hw/tpm/tpm_tis.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
>> index fec2fc6..42d647d 100644
>> --- a/hw/tpm/tpm_tis.c
>> +++ b/hw/tpm/tpm_tis.c
>> @@ -545,7 +545,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque, hwaddr addr,
>>       uint8_t v;
>>
>>       if (tpm_backend_had_startup_error(s->be_driver)) {
>> -        return val;
>> +        return 0;
>>       }
>>
>>       switch (offset) {
>> --
>> 2.5.5
>>
>
>

Re: [Qemu-devel] [PATCH v3 for-2.11 3/3] tpm_tis: Return 0 for every register in case of failure mode

[Marc-André Lureau]

Hi

On Wed, Nov 15, 2017 at 2:18 AM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> On 11/14/2017 06:47 PM, Marc-André Lureau wrote:
>>
>> Hi
>>
>> On Tue, Nov 14, 2017 at 10:52 PM, Stefan Berger
>> <stefanb@linux.vnet.ibm.com> wrote:
>>>
>>> Rather than returning ~0, return 0 for every register in case of
>>> failure mode. The '0' is better to indicate that there's no device
>>> there.
>>
>> For most registers, 0 makes more sense. However, I wonder if we
>> shouldn't just fail to start qemu in this case...
>>
>> Not convincing me this is 2.11 material either. Does this fix a specific
>> bug?
>
>
> Yes, SeaBIOS detects the ~0 when it probes and thinks there's a device
> there. It then hangs trying to set flags and read registers to be able to
> use the device.
>

Please update the commit message with that added,
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


>    Stefan
>
>
>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>> ---
>>>   hw/tpm/tpm_tis.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
>>> index fec2fc6..42d647d 100644
>>> --- a/hw/tpm/tpm_tis.c
>>> +++ b/hw/tpm/tpm_tis.c
>>> @@ -545,7 +545,7 @@ static uint64_t tpm_tis_mmio_read(void *opaque,
>>> hwaddr addr,
>>>       uint8_t v;
>>>
>>>       if (tpm_backend_had_startup_error(s->be_driver)) {
>>> -        return val;
>>> +        return 0;
>>>       }
>>>
>>>       switch (offset) {
>>> --
>>> 2.5.5
>>>
>>
>>
>



-- 
Marc-André Lureau