Re: [flashrom] [PATCH] sbxxx: Handle active IMCs in AMD chipsets.

[Carl-Daniel Hailfinger]

Hi Stefan,

sorry for the late sending of this review (falling asleep a few times
due to the intense heat didn't really help me get this sent).
I believe the issues I raise are only cosmetic and the patch is
technically solid.

Am 18.07.2013 23:50 schrieb Stefan Tauner:
> Detect and temporarily disable the IMC while accessing the flash.
> Disable writes on default, but allow the user to enforce it.
>
> Signed-off-by: Rudolf Marek 
> Signed-off-by: Stefan Tauner

Thanks a lot for the patch! I believe it will fix pretty much every case
of IMC interference we saw.
> diff --git a/amd_imc.c b/amd_imc.c
> new file mode 100644
> index 0000000..b05390c
> --- /dev/null
> +++ b/amd_imc.c
> @@ -0,0 +1,159 @@
> [...]
> +static uint16_t get_sio_port(struct pci_dev *dev)
> +{
> uint16_t ec_port;
> if (!dev) {
> return 0;
> }
> ec_port = pci_read_word(dev, 0xa4);
> /* EcPortActive? */
> if (!(ec_port & 0x1))
> return 0;
> ec_port &= ~0x1;
> return ec_port;
>
> +}
>
> [...]
> +static uint16_t mbox_get_port(uint16_t sio_port)
> +{
> uint16_t mbox_port;
> enter_conf_mode_ec(sio_port);
> /* Go to LDN 9, mailbox */
> sio_write(sio_port, 7, 9);
> /* MBOX inactive? */
> if ((sio_read(sio_port, 0x30) & 1) == 0) {
> exit_conf_mode_ec(sio_port);
> return 0;
> }
> mbox_port = sio_read(sio_port, 0x60) << 8;
> mbox_port |= sio_read(sio_port, 0x61);
> exit_conf_mode_ec(sio_port);
> return mbox_port;
>
> +}
>
> +/* Returns negative values when IMC is inactive, positive values on errors */
> +static int imc_send_cmd(struct pci_dev *dev, uint8_t cmd)
> +{
> uint16_t sio_port;
> uint16_t mbox_port;
> /* IntegratedEcPresent? */
> if (!(pci_read_byte(dev, 0x40) & (1 << 7)))
> return -1;
> sio_port = get_sio_port(dev);
> if (!sio_port)
> return -1;
> msg_pdbg2("IMC SIO is at 0x%x.\n", sio_port);
> mbox_port = mbox_get_port(sio_port);
> if (!mbox_port)
> return -1;
> msg_pdbg2("IMC MBOX is at 0x%x.\n", mbox_port);
> sio_write(mbox_port, 0x82, 0x0);
> sio_write(mbox_port, 0x83, cmd);
> sio_write(mbox_port, 0x84, 0x0);
> /* trigger transfer 0x96 with subcommand cmd */
> sio_write(mbox_port, 0x80, 0x96);
> return mbox_wait_ack(mbox_port);
>
> +}
>
> [...]
> +int amd_imc_shutdown(struct pci_dev *dev)
> +{
> /* Try to put IMC to sleep */
> int ret = imc_send_cmd(dev, 0xb4);
> /* No IMC activity detectable, assume we are fine */
> if (ret < 0) {
> msg_pdbg2("No IMC found.\n");

The message above is printed after the message "Writes have been
disabled for safety reasons because the IMC is active" from
sb600_handle_imc(). AFAICS this mesage can only happen if the IMC is
marked as present but any of the SIO port or MBOX port are zero, i.e.
inaccessible. May I suggest the following message instead?
"IMC inactive or unreachable.\n"
> return 0;
> }
> if (ret != 0) {
> msg_perr("Shutting down IMC failed.\n");
> return ret;
> }
> msg_pdbg2("Shutting down IMC successful.\n");
> if (register_shutdown(imc_resume, dev))
> return 1;
> return ret;
>
> +}
>
> +#endif
> diff --git a/programmer.h b/programmer.h
> index db914cb..f03eac3 100644
> --- a/programmer.h
> +++ b/programmer.h
> @@ -572,6 +572,9 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
>    	 enum ich_chipset ich_generation);
>  int via_init_spi(struct pci_dev *dev, uint32_t mmio_base);
>
> +/* imc.c */

Should be amd_imc.c
> +int amd_imc_shutdown(struct pci_dev *dev);
>
> /* it85spi.c */
>  int it85xx_spi_init(struct superio s);
>
> diff --git a/sb600spi.c b/sb600spi.c
> index a5c00d8..e76c04a 100644
> --- a/sb600spi.c
> +++ b/sb600spi.c
> @@ -194,6 +195,39 @@ static int sb600_spi_send_command(struct flashctx *flash, unsigned int writecnt,
>    return 0;
>  }
>
> +static int sb600_handle_imc(struct pci_dev *dev, bool amd_imc_force)
> +{
> /* Handle IMC everywhere but sb600 which does not have one. */
> if (dev->device_id == 0x438d)
> return 0;
> /* TODO: we should not only look at IntegratedImcPresent (LPC Dev 20, Func 3, 40h) but also at
> * IMCEnable(Strap) and Override EcEnable(Strap) (sb8xx, sb9xx?, a50: Misc_Reg: 80h-87h;
> * sb7xx, sp5100: PM_Reg: B0h-B1h) etc. */
> uint8_t reg = pci_read_byte(dev, 0x40);
> if ((reg & (1 << 7)) == 0) {
> msg_pdbg("IMC is not active.\n");
> return 0;
> }

This is the point where we assume the IMC is active. Given that we only
check for the IntegratedImcPresent bit, would "IMC is not present" be a
better message? Or do we want to keep the message and eventually look at
the straps as well?
> if (!amd_imc_force)
> programmer_may_write = 0;
> msg_pinfo("Writes have been disabled for safety reasons because the IMC is active\n"

It's present, but does it have to be active?
>   "and it could interfere with accessing flash memory. Flashrom will try\n"
>   "to disable it temporarily but even then this might not be safe:\n"
>   "when it is reenabled and after a reboot it expects to find working code\n"
>   "in the flash and it is unpredictable what happens if there is none.\n"
>   "\n"
>   "To be safe make sure that there is a working IMC firmware at the right\n"
>   "location in the image you intend to write and do not attempt to erase.\n"
>   "\n"
>   "You can enforce write support with the amd_imc_force programmer option.\n");
> if (amd_imc_force)
> msg_pinfo("Continuing with write support because the user forced us to!\n");
> return amd_imc_shutdown(dev);

Call to shutdown the IMC, done after printing that the IMC is active,
but inside amd_imc_shutdown() we may print the message that no IMC was
found. This is a bit contradictory in some cases.
> +}
>
> static const struct spi_programmer spi_programmer_sb600 = {
>    .type = SPI_CONTROLLER_SB600,
>    .max_data_read = 8,

I just noticed the patch has been committed an hour ago, but throwing
away the review seemed a waste.

Regards,
Carl-Daniel