[Qemu-devel] [PATCH] Revert "smbus: do not immediately complete commands"

[Qemu-devel] [PATCH] Revert "smbus: do not immediately complete commands"

[minyard]

From: Corey Minyard <cminyard@mvista.com>

This reverts commit 880b1ffe6ec2f0ae25cc4175716227ad275e8b8a.

The commit being reverted says:

    PIIX4 errata says that "immediate polling of the Host Status Register BUSY
    bit may indicate that the SMBus is NOT busy."
    Due to this, some code does the following steps:
    (a) set parameters
    (b) start command
    (c) check for smbus busy bit set (to know that command started)
    (d) check for smbus busy bit not set (to know that command finished)

    Let (c) happen, by immediately setting the busy bit, and really executing
    the command when status register has been read once.

    This fixes a problem with AMIBIOS, which can now properly initialize the
    PIIX4.

Emulating bad hardware so badly written software will work doesn't sound
like a good idea to me.  I have patches that add interrupt capability
to pm_smbus, but this change breaks that because the Linux driver
starts the transaction then waits for interrupts before reading the
status register.  That obviously won't work with these changes.

The right way to fix this in AMIBIOS is to ignore the host busy bit
and use the other bits in the host status register to tell if the
transaction has completed.  Using host busy is racy, anyway, if you
get interrupted or something while processing, you may miss step (c)
in your algorithm and fail.

Cc: Hervé Poussineau <hpoussin@reactos.org>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 hw/i2c/pm_smbus.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 0d26e0f..a044dd1 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -62,9 +62,6 @@ static void smb_transaction(PMSMBus *s)
     I2CBus *bus = s->smbus;
     int ret;
 
-    assert(s->smb_stat & STS_HOST_BUSY);
-    s->smb_stat &= ~STS_HOST_BUSY;
-
     SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot);
     /* Transaction isn't exec if STS_DEV_ERR bit set */
     if ((s->smb_stat & STS_DEV_ERR) != 0)  {
@@ -137,13 +134,6 @@ error:
 
 }
 
-static void smb_transaction_start(PMSMBus *s)
-{
-    /* Do not execute immediately the command ; it will be
-     * executed when guest will read SMB_STAT register */
-    s->smb_stat |= STS_HOST_BUSY;
-}
-
 static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
                               unsigned width)
 {
@@ -159,7 +149,7 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
     case SMBHSTCNT:
         s->smb_ctl = val;
         if (val & 0x40)
-            smb_transaction_start(s);
+            smb_transaction(s);
         break;
     case SMBHSTCMD:
         s->smb_cmd = val;
@@ -191,10 +181,6 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
     switch(addr) {
     case SMBHSTSTS:
         val = s->smb_stat;
-        if (s->smb_stat & STS_HOST_BUSY) {
-            /* execute command now */
-            smb_transaction(s);
-        }
         break;
     case SMBHSTCNT:
         s->smb_index = 0;
-- 
2.7.4

Re: [Qemu-devel] [PATCH] Revert "smbus: do not immediately complete commands"

[Michael S. Tsirkin]

On Thu, Jan 18, 2018 at 07:55:41PM -0600, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> This reverts commit 880b1ffe6ec2f0ae25cc4175716227ad275e8b8a.
> 
> The commit being reverted says:
> 
>     PIIX4 errata says that "immediate polling of the Host Status Register BUSY
>     bit may indicate that the SMBus is NOT busy."
>     Due to this, some code does the following steps:
>     (a) set parameters
>     (b) start command
>     (c) check for smbus busy bit set (to know that command started)
>     (d) check for smbus busy bit not set (to know that command finished)
> 
>     Let (c) happen, by immediately setting the busy bit, and really executing
>     the command when status register has been read once.
> 
>     This fixes a problem with AMIBIOS, which can now properly initialize the
>     PIIX4.
> 
> Emulating bad hardware so badly written software will work doesn't sound
> like a good idea to me.  I have patches that add interrupt capability
> to pm_smbus, but this change breaks that because the Linux driver
> starts the transaction then waits for interrupts before reading the
> status register.  That obviously won't work with these changes.
> 
> The right way to fix this in AMIBIOS is to ignore the host busy bit
> and use the other bits in the host status register to tell if the
> transaction has completed.  Using host busy is racy, anyway, if you
> get interrupted or something while processing, you may miss step (c)
> in your algorithm and fail.
> 
> Cc: Hervé Poussineau <hpoussin@reactos.org>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Corey Minyard <cminyard@mvista.com>

Would it be possible to limit the change to when guest uses
interrupts?

> ---
>  hw/i2c/pm_smbus.c | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 0d26e0f..a044dd1 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -62,9 +62,6 @@ static void smb_transaction(PMSMBus *s)
>      I2CBus *bus = s->smbus;
>      int ret;
>  
> -    assert(s->smb_stat & STS_HOST_BUSY);
> -    s->smb_stat &= ~STS_HOST_BUSY;
> -
>      SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot);
>      /* Transaction isn't exec if STS_DEV_ERR bit set */
>      if ((s->smb_stat & STS_DEV_ERR) != 0)  {
> @@ -137,13 +134,6 @@ error:
>  
>  }
>  
> -static void smb_transaction_start(PMSMBus *s)
> -{
> -    /* Do not execute immediately the command ; it will be
> -     * executed when guest will read SMB_STAT register */
> -    s->smb_stat |= STS_HOST_BUSY;
> -}
> -
>  static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
>                                unsigned width)
>  {
> @@ -159,7 +149,7 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
>      case SMBHSTCNT:
>          s->smb_ctl = val;
>          if (val & 0x40)
> -            smb_transaction_start(s);
> +            smb_transaction(s);
>          break;
>      case SMBHSTCMD:
>          s->smb_cmd = val;
> @@ -191,10 +181,6 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
>      switch(addr) {
>      case SMBHSTSTS:
>          val = s->smb_stat;
> -        if (s->smb_stat & STS_HOST_BUSY) {
> -            /* execute command now */
> -            smb_transaction(s);
> -        }
>          break;
>      case SMBHSTCNT:
>          s->smb_index = 0;
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH] Revert "smbus: do not immediately complete commands"

[Corey Minyard]

On 01/18/2018 09:17 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 18, 2018 at 07:55:41PM -0600, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> This reverts commit 880b1ffe6ec2f0ae25cc4175716227ad275e8b8a.
>>
>> The commit being reverted says:
>>
>>      PIIX4 errata says that "immediate polling of the Host Status Register BUSY
>>      bit may indicate that the SMBus is NOT busy."
>>      Due to this, some code does the following steps:
>>      (a) set parameters
>>      (b) start command
>>      (c) check for smbus busy bit set (to know that command started)
>>      (d) check for smbus busy bit not set (to know that command finished)
>>
>>      Let (c) happen, by immediately setting the busy bit, and really executing
>>      the command when status register has been read once.
>>
>>      This fixes a problem with AMIBIOS, which can now properly initialize the
>>      PIIX4.
>>
>> Emulating bad hardware so badly written software will work doesn't sound
>> like a good idea to me.  I have patches that add interrupt capability
>> to pm_smbus, but this change breaks that because the Linux driver
>> starts the transaction then waits for interrupts before reading the
>> status register.  That obviously won't work with these changes.
>>
>> The right way to fix this in AMIBIOS is to ignore the host busy bit
>> and use the other bits in the host status register to tell if the
>> transaction has completed.  Using host busy is racy, anyway, if you
>> get interrupted or something while processing, you may miss step (c)
>> in your algorithm and fail.
>>
>> Cc: Hervé Poussineau <hpoussin@reactos.org>
>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Would it be possible to limit the change to when guest uses
> interrupts?

I did think about that, but it seems rather frail.  What if another 
piece of software
does this but has the interrupt enable bit set?  And AMIBIOS is still 
broken doing
that algorithm on real hardware.  If you get a bus collision, for 
instance, that will
be almost instantaneous and the firmware is likely to miss it.

The 82801 documentation is pretty clear that you should use the INTR and 
error
bits in the status register to know if a transaction is complete.

If you really want to emulate real hardware, I guess the right way to do 
this
would be to add a delay between the start bit being set and the transaction
being done.  I'm not sure how timers work with vmstate, I'd have to look at
that.

IMHO it's best to revert this change and fix AMIBIOS.  If that is
impossible, then adding the delay or doing the interrupt enable
thing you suggest (assuming AMIBIOS doesn't have interrupts
enabled), and fixing that assert would be best.  I can submit
a patch either way, depending on what you want.

-corey

>> ---
>>   hw/i2c/pm_smbus.c | 16 +---------------
>>   1 file changed, 1 insertion(+), 15 deletions(-)
>>
>> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
>> index 0d26e0f..a044dd1 100644
>> --- a/hw/i2c/pm_smbus.c
>> +++ b/hw/i2c/pm_smbus.c
>> @@ -62,9 +62,6 @@ static void smb_transaction(PMSMBus *s)
>>       I2CBus *bus = s->smbus;
>>       int ret;
>>   
>> -    assert(s->smb_stat & STS_HOST_BUSY);
>> -    s->smb_stat &= ~STS_HOST_BUSY;
>> -
>>       SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot);
>>       /* Transaction isn't exec if STS_DEV_ERR bit set */
>>       if ((s->smb_stat & STS_DEV_ERR) != 0)  {
>> @@ -137,13 +134,6 @@ error:
>>   
>>   }
>>   
>> -static void smb_transaction_start(PMSMBus *s)
>> -{
>> -    /* Do not execute immediately the command ; it will be
>> -     * executed when guest will read SMB_STAT register */
>> -    s->smb_stat |= STS_HOST_BUSY;
>> -}
>> -
>>   static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
>>                                 unsigned width)
>>   {
>> @@ -159,7 +149,7 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
>>       case SMBHSTCNT:
>>           s->smb_ctl = val;
>>           if (val & 0x40)
>> -            smb_transaction_start(s);
>> +            smb_transaction(s);
>>           break;
>>       case SMBHSTCMD:
>>           s->smb_cmd = val;
>> @@ -191,10 +181,6 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
>>       switch(addr) {
>>       case SMBHSTSTS:
>>           val = s->smb_stat;
>> -        if (s->smb_stat & STS_HOST_BUSY) {
>> -            /* execute command now */
>> -            smb_transaction(s);
>> -        }
>>           break;
>>       case SMBHSTCNT:
>>           s->smb_index = 0;
>> -- 
>> 2.7.4

Re: [Qemu-devel] [PATCH] Revert "smbus: do not immediately complete commands"

[Corey Minyard]

On 01/19/2018 08:07 AM, Corey Minyard wrote:
> On 01/18/2018 09:17 PM, Michael S. Tsirkin wrote:
>> On Thu, Jan 18, 2018 at 07:55:41PM -0600, minyard@acm.org wrote:
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> This reverts commit 880b1ffe6ec2f0ae25cc4175716227ad275e8b8a.
>>>
>>> The commit being reverted says:
>>>
>>>      PIIX4 errata says that "immediate polling of the Host Status 
>>> Register BUSY
>>>      bit may indicate that the SMBus is NOT busy."
>>>      Due to this, some code does the following steps:
>>>      (a) set parameters
>>>      (b) start command
>>>      (c) check for smbus busy bit set (to know that command started)
>>>      (d) check for smbus busy bit not set (to know that command 
>>> finished)
>>>
>>>      Let (c) happen, by immediately setting the busy bit, and really 
>>> executing
>>>      the command when status register has been read once.
>>>
>>>      This fixes a problem with AMIBIOS, which can now properly 
>>> initialize the
>>>      PIIX4.
>>>
>>> Emulating bad hardware so badly written software will work doesn't 
>>> sound
>>> like a good idea to me.  I have patches that add interrupt capability
>>> to pm_smbus, but this change breaks that because the Linux driver
>>> starts the transaction then waits for interrupts before reading the
>>> status register.  That obviously won't work with these changes.
>>>
>>> The right way to fix this in AMIBIOS is to ignore the host busy bit
>>> and use the other bits in the host status register to tell if the
>>> transaction has completed.  Using host busy is racy, anyway, if you
>>> get interrupted or something while processing, you may miss step (c)
>>> in your algorithm and fail.
>>>
>>> Cc: Hervé Poussineau <hpoussin@reactos.org>
>>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Would it be possible to limit the change to when guest uses
>> interrupts?
>
> I did think about that, but it seems rather frail.  What if another 
> piece of software
> does this but has the interrupt enable bit set?  And AMIBIOS is still 
> broken doing
> that algorithm on real hardware.  If you get a bus collision, for 
> instance, that will
> be almost instantaneous and the firmware is likely to miss it.
>
> The 82801 documentation is pretty clear that you should use the INTR 
> and error
> bits in the status register to know if a transaction is complete.
>
> If you really want to emulate real hardware, I guess the right way to 
> do this
> would be to add a delay between the start bit being set and the 
> transaction
> being done.  I'm not sure how timers work with vmstate, I'd have to 
> look at
> that.

I realized that the timer is not going to be able to correctly work 
around the
AMIBIOS.  It would probably work most of the time, but if qemu got switched
out, then switched back and the timer went off before the guest was allowed
to run, then you would have the same issue.

Also, looking at a more complete implementation of the pm_smbus device,
using the host busy bit to know when to start the transaction won't work,
that bit also does other things when doing byte at a time block transfers.
So a separate bool is needed to know when to do this.

-corey

>
> IMHO it's best to revert this change and fix AMIBIOS.  If that is
> impossible, then adding the delay or doing the interrupt enable
> thing you suggest (assuming AMIBIOS doesn't have interrupts
> enabled), and fixing that assert would be best.  I can submit
> a patch either way, depending on what you want.
>
> -corey
>
>>> ---
>>>   hw/i2c/pm_smbus.c | 16 +---------------
>>>   1 file changed, 1 insertion(+), 15 deletions(-)
>>>
>>> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
>>> index 0d26e0f..a044dd1 100644
>>> --- a/hw/i2c/pm_smbus.c
>>> +++ b/hw/i2c/pm_smbus.c
>>> @@ -62,9 +62,6 @@ static void smb_transaction(PMSMBus *s)
>>>       I2CBus *bus = s->smbus;
>>>       int ret;
>>>   -    assert(s->smb_stat & STS_HOST_BUSY);
>>> -    s->smb_stat &= ~STS_HOST_BUSY;
>>> -
>>>       SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, 
>>> prot);
>>>       /* Transaction isn't exec if STS_DEV_ERR bit set */
>>>       if ((s->smb_stat & STS_DEV_ERR) != 0)  {
>>> @@ -137,13 +134,6 @@ error:
>>>     }
>>>   -static void smb_transaction_start(PMSMBus *s)
>>> -{
>>> -    /* Do not execute immediately the command ; it will be
>>> -     * executed when guest will read SMB_STAT register */
>>> -    s->smb_stat |= STS_HOST_BUSY;
>>> -}
>>> -
>>>   static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t 
>>> val,
>>>                                 unsigned width)
>>>   {
>>> @@ -159,7 +149,7 @@ static void smb_ioport_writeb(void *opaque, 
>>> hwaddr addr, uint64_t val,
>>>       case SMBHSTCNT:
>>>           s->smb_ctl = val;
>>>           if (val & 0x40)
>>> -            smb_transaction_start(s);
>>> +            smb_transaction(s);
>>>           break;
>>>       case SMBHSTCMD:
>>>           s->smb_cmd = val;
>>> @@ -191,10 +181,6 @@ static uint64_t smb_ioport_readb(void *opaque, 
>>> hwaddr addr, unsigned width)
>>>       switch(addr) {
>>>       case SMBHSTSTS:
>>>           val = s->smb_stat;
>>> -        if (s->smb_stat & STS_HOST_BUSY) {
>>> -            /* execute command now */
>>> -            smb_transaction(s);
>>> -        }
>>>           break;
>>>       case SMBHSTCNT:
>>>           s->smb_index = 0;
>>> -- 
>>> 2.7.4
>
>

Re: [Qemu-devel] [PATCH] Revert "smbus: do not immediately complete commands"

[Hervé Poussineau]

[-- Attachment #1: Type: text/plain, Size: 3870 bytes --]

Le 19/01/2018 à 22:15, Corey Minyard a écrit :
> On 01/19/2018 08:07 AM, Corey Minyard wrote:
>> On 01/18/2018 09:17 PM, Michael S. Tsirkin wrote:
>>> On Thu, Jan 18, 2018 at 07:55:41PM -0600, minyard@acm.org wrote:
>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>
>>>> This reverts commit 880b1ffe6ec2f0ae25cc4175716227ad275e8b8a.
>>>>
>>>> The commit being reverted says:
>>>>
>>>>      PIIX4 errata says that "immediate polling of the Host Status Register BUSY
>>>>      bit may indicate that the SMBus is NOT busy."
>>>>      Due to this, some code does the following steps:
>>>>      (a) set parameters
>>>>      (b) start command
>>>>      (c) check for smbus busy bit set (to know that command started)
>>>>      (d) check for smbus busy bit not set (to know that command finished)
>>>>
>>>>      Let (c) happen, by immediately setting the busy bit, and really executing
>>>>      the command when status register has been read once.
>>>>
>>>>      This fixes a problem with AMIBIOS, which can now properly initialize the
>>>>      PIIX4.
>>>>
>>>> Emulating bad hardware so badly written software will work doesn't sound
>>>> like a good idea to me.  I have patches that add interrupt capability
>>>> to pm_smbus, but this change breaks that because the Linux driver
>>>> starts the transaction then waits for interrupts before reading the
>>>> status register.  That obviously won't work with these changes.
>>>>
>>>> The right way to fix this in AMIBIOS is to ignore the host busy bit
>>>> and use the other bits in the host status register to tell if the
>>>> transaction has completed.  Using host busy is racy, anyway, if you
>>>> get interrupted or something while processing, you may miss step (c)
>>>> in your algorithm and fail.
>>>>
>>>> Cc: Hervé Poussineau <hpoussin@reactos.org>
>>>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>> Would it be possible to limit the change to when guest uses
>>> interrupts?
>>
>> I did think about that, but it seems rather frail.  What if another piece of software
>> does this but has the interrupt enable bit set?  And AMIBIOS is still broken doing
>> that algorithm on real hardware.  If you get a bus collision, for instance, that will
>> be almost instantaneous and the firmware is likely to miss it.
>>
>> The 82801 documentation is pretty clear that you should use the INTR and error
>> bits in the status register to know if a transaction is complete.
>>
>> If you really want to emulate real hardware, I guess the right way to do this
>> would be to add a delay between the start bit being set and the transaction
>> being done.  I'm not sure how timers work with vmstate, I'd have to look at
>> that.
> 
> I realized that the timer is not going to be able to correctly work around the
> AMIBIOS.  It would probably work most of the time, but if qemu got switched
> out, then switched back and the timer went off before the guest was allowed
> to run, then you would have the same issue.
> 
> Also, looking at a more complete implementation of the pm_smbus device,
> using the host busy bit to know when to start the transaction won't work,
> that bit also does other things when doing byte at a time block transfers.
> So a separate bool is needed to know when to do this.

AMIBIOS can't be fixed to do the right thing.
My first implementation of this patch was using a timer, and it was working quite well.
I don't think that smbus is very latency-sensitive, so I think a timer is a valid
solution to this problem. If a timer also works for your use case, I'll be happy with it.

See my patch proposal to use a timer instead. Does it fit your needs?

An improvement might be to execute the command either on the timer or when the guest
reads the host status register.

Regards,

Hervé

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-smbus-replace-transaction-execution-at-first-registe.patch --]
[-- Type: text/x-patch; name="0001-smbus-replace-transaction-execution-at-first-registe.patch", Size: 3186 bytes --]

>From a5e13ceebd77b9f813fe1987b73fc8a3498cf7ca Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Herv=C3=A9=20Poussineau?= <hpoussin@reactos.org>
Date: Sun, 21 Jan 2018 14:17:27 +0100
Subject: [PATCH] smbus: replace transaction execution at first register read
 by a constant delay
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This execution at first register read was added in 880b1ffe6ec2f0ae25cc4175716227ad275e8b8a
While this is not very correct, it was enough to fix some use cases.

However, this break further developpements of the smbus emulation, especially
related to smbus interrupt (STS_INTR): the OS reads the host status register
only when an interrupt will be raised.
If QEMU executes the command (and raises the interrupt) only when the OS reads
the host status register, we have a dead-lock...

Change emulation to execute the command after a constant delay.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/i2c/pm_smbus.c         | 15 ++++++++-------
 include/hw/i2c/pm_smbus.h |  1 +
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 0d26e0f6b5..e3ff47ec15 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -21,6 +21,7 @@
 #include "hw/hw.h"
 #include "hw/i2c/pm_smbus.h"
 #include "hw/i2c/smbus.h"
+#include "qemu/timer.h"
 
 /* no save/load? */
 
@@ -53,8 +54,9 @@
 #endif
 
 
-static void smb_transaction(PMSMBus *s)
+static void smb_transaction(void *opaque)
 {
+    PMSMBus *s = opaque;
     uint8_t prot = (s->smb_ctl >> 2) & 0x07;
     uint8_t read = s->smb_addr & 0x01;
     uint8_t cmd = s->smb_cmd;
@@ -139,9 +141,10 @@ error:
 
 static void smb_transaction_start(PMSMBus *s)
 {
-    /* Do not execute immediately the command ; it will be
-     * executed when guest will read SMB_STAT register */
+    /* Do not execute immediately the command */
     s->smb_stat |= STS_HOST_BUSY;
+    timer_mod(s->result_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)
+              + (NANOSECONDS_PER_SECOND / 1000));
 }
 
 static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
@@ -191,10 +194,6 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
     switch(addr) {
     case SMBHSTSTS:
         val = s->smb_stat;
-        if (s->smb_stat & STS_HOST_BUSY) {
-            /* execute command now */
-            smb_transaction(s);
-        }
         break;
     case SMBHSTCNT:
         s->smb_index = 0;
@@ -238,4 +237,6 @@ void pm_smbus_init(DeviceState *parent, PMSMBus *smb)
     smb->smbus = i2c_init_bus(parent, "i2c");
     memory_region_init_io(&smb->io, OBJECT(parent), &pm_smbus_ops, smb,
                           "pm-smbus", 64);
+    smb->result_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                     smb_transaction, smb);
 }
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index 2a837afdcb..02adeefcbf 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -4,6 +4,7 @@
 typedef struct PMSMBus {
     I2CBus *smbus;
     MemoryRegion io;
+    QEMUTimer *result_timer;
 
     uint8_t smb_stat;
     uint8_t smb_ctl;
-- 
2.11.0

Re: [Qemu-devel] [PATCH] Revert "smbus: do not immediately complete commands"

[Corey Minyard]

On 01/21/2018 11:36 AM, Hervé Poussineau wrote:
> Le 19/01/2018 à 22:15, Corey Minyard a écrit :
>> On 01/19/2018 08:07 AM, Corey Minyard wrote:
>>> On 01/18/2018 09:17 PM, Michael S. Tsirkin wrote:
>>>> On Thu, Jan 18, 2018 at 07:55:41PM -0600, minyard@acm.org wrote:
>>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>>
>>>>> This reverts commit 880b1ffe6ec2f0ae25cc4175716227ad275e8b8a.
>>>>>
>>>>> The commit being reverted says:
>>>>>
>>>>>      PIIX4 errata says that "immediate polling of the Host Status 
>>>>> Register BUSY
>>>>>      bit may indicate that the SMBus is NOT busy."
>>>>>      Due to this, some code does the following steps:
>>>>>      (a) set parameters
>>>>>      (b) start command
>>>>>      (c) check for smbus busy bit set (to know that command started)
>>>>>      (d) check for smbus busy bit not set (to know that command 
>>>>> finished)
>>>>>
>>>>>      Let (c) happen, by immediately setting the busy bit, and 
>>>>> really executing
>>>>>      the command when status register has been read once.
>>>>>
>>>>>      This fixes a problem with AMIBIOS, which can now properly 
>>>>> initialize the
>>>>>      PIIX4.
>>>>>
>>>>> Emulating bad hardware so badly written software will work doesn't 
>>>>> sound
>>>>> like a good idea to me.  I have patches that add interrupt capability
>>>>> to pm_smbus, but this change breaks that because the Linux driver
>>>>> starts the transaction then waits for interrupts before reading the
>>>>> status register.  That obviously won't work with these changes.
>>>>>
>>>>> The right way to fix this in AMIBIOS is to ignore the host busy bit
>>>>> and use the other bits in the host status register to tell if the
>>>>> transaction has completed.  Using host busy is racy, anyway, if you
>>>>> get interrupted or something while processing, you may miss step (c)
>>>>> in your algorithm and fail.
>>>>>
>>>>> Cc: Hervé Poussineau <hpoussin@reactos.org>
>>>>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>> Would it be possible to limit the change to when guest uses
>>>> interrupts?
>>>
>>> I did think about that, but it seems rather frail.  What if another 
>>> piece of software
>>> does this but has the interrupt enable bit set?  And AMIBIOS is 
>>> still broken doing
>>> that algorithm on real hardware.  If you get a bus collision, for 
>>> instance, that will
>>> be almost instantaneous and the firmware is likely to miss it.
>>>
>>> The 82801 documentation is pretty clear that you should use the INTR 
>>> and error
>>> bits in the status register to know if a transaction is complete.
>>>
>>> If you really want to emulate real hardware, I guess the right way 
>>> to do this
>>> would be to add a delay between the start bit being set and the 
>>> transaction
>>> being done.  I'm not sure how timers work with vmstate, I'd have to 
>>> look at
>>> that.
>>
>> I realized that the timer is not going to be able to correctly work 
>> around the
>> AMIBIOS.  It would probably work most of the time, but if qemu got 
>> switched
>> out, then switched back and the timer went off before the guest was 
>> allowed
>> to run, then you would have the same issue.
>>
>> Also, looking at a more complete implementation of the pm_smbus device,
>> using the host busy bit to know when to start the transaction won't 
>> work,
>> that bit also does other things when doing byte at a time block 
>> transfers.
>> So a separate bool is needed to know when to do this.
>
> AMIBIOS can't be fixed to do the right thing.

That's a real bummer.

> My first implementation of this patch was using a timer, and it was 
> working quite well.
> I don't think that smbus is very latency-sensitive, so I think a timer 
> is a valid
> solution to this problem. If a timer also works for your use case, 
> I'll be happy with it.

A timer is not a 100% guaranteed solution.  It should work most of the 
time, but if you
get a situation like I describe above, it is possible for the BIOS to 
miss the setting of the
busy bit.

I've modified my code that adds interrupts to use your solution only 
when interrupts are
disabled  Unfortunately, the current pm_smbus.c code is a fairly 
incomplete implementation
of the device, so I can't do a patch to the main tree for that change.

-corey

>
> See my patch proposal to use a timer instead. Does it fit your needs?
>
> An improvement might be to execute the command either on the timer or 
> when the guest
> reads the host status register.
>
> Regards,
>
> Hervé