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

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

[Stefan Tauner]

On Fri, 26 Jul 2013 02:26:34 +0200
Carl-Daniel Hailfinger  wrote:
> Am 18.07.2013 23:50 schrieb Stefan Tauner:
> > [...]
> > +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"

ack in general, but the wording inactive contradicts exactly the
wording you have cited... "Writes have been disabled for safety reasons
because the IMC is *active*"... so this might be worse. Also, this is
just a debug2 level message.
> > 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

yes, fixed in tested_stuff 19
> > +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?

What does active, present, enabled mean to us? What does the respective
wording in the documentation specify exactly? We have essentially no
idea (at least I dont). So I think discussing the wording is rather
pointless :) Also, this is a debugging level message.
> > 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?

Similar to above but here the wording is more important. I am not sure
if it is better, but this might make you more happy?
    msg_pinfo("Writes have been disabled for safety reasons because the presence of the IMC\n"
    	  "was detected and it could interfere with accessing flash memory. Flashrom will\n"
    	  "try to disable it temporarily but even then this might not be safe:\n"

The reason why I am not sure if that is better: the longer and more
eloquent the message the less it is read and/or understood. We tend to
explain facts very thoroughly and I personally like it when apps do
that, but I think most ppl dont. :)
> >   "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.

see above
> > +}
> >
> > 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.

Yes indeed, thanks for the feedback. I am very happy that you have not
found more severe problems (yet) :)