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

["Michael S. Tsirkin" <mst@redhat.com>]

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