[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 <r.marek at assembler.cz>
> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>

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

-- 
http://www.hailfinger.org/