Am 22.10.2024 um 11:40 hat Jamin Lin geschrieben:
> According to the design of ASPEED SPI controllers user mode, users write the
> data to flash, the SPI drivers set the Control Register(0x10) bit 0 and 1
> enter user mode. Then, SPI drivers send flash commands for writing data.
> Finally, SPI drivers set the Control Register (0x10) bit 2 to stop
> active control and restore bit 0 and 1.
>
> According to the design of ASPEED SMC model, firmware writes the
> Control Register and the "aspeed_smc_flash_update_ctrl" function is called.
> Then, this function verify Control Register(0x10) bit 0 and 1. If it set user
> mode, the value of s->snoop_index is SNOOP_START else SNOOP_OFF.
> If s->snoop_index is SNOOP_START, the "aspeed_smc_do_snoop" function verify
> the first incomming data is a new flash command and writes the corresponding
> dummy bytes if need.
>
> However, it did not check the current unselect status. If current unselect
> status is "false" and firmware set the IO MODE by Control Register bit 31:28,
> the value of s->snoop_index will be changed to SNOOP_START again and
> "aspeed_smc_do_snoop" misunderstand that the incomming data is the new flash
> command and it causes writing unexpected data into flash.
>
> Example:
> 1. Firmware set user mode by Control Register bit 0 and 1(0x03)
> 2. SMC model set s->snoop SNOOP_START
> 3. Firmware set Quad Page Program with 4-Byte Address command (0x34)
> 4. SMC model verify this flash command and it needs 4 dummy bytes.
> 5. Firmware send 4 bytes address.
> 6. SMC model receives 4 bytes address
> 7. Firmware set QPI IO MODE by Control Register bit 31. (0x80000003)
> 8. SMC model verify new user mode by Control Register bit 0 and 1.
> Then, set s->snoop SNOOP_START again. (It is the wrong behavior.)
> 9. Firmware send 0xebd8c134 data and it should be written into flash.
> However, SMC model misunderstand that the first incoming data, 0x34,
> is the new command because the value of s->snoop is changed to SNOOP_START.
> Finally, SMC sned the incorrect data to flash model.
>
> Introduce a new unselect attribute in AspeedSMCState to save the current
> unselect status for user mode and set it "true" by default.
> Update "aspeed_smc_flash_update_ctrl" function to check the previous unselect
> status. If both new unselect status and previous unselect status is different,
> update s->snoop_index value and call "aspeed_smc_flash_do_select".
>
> Increase VMStateDescription version.
>
> Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
> @@ -1261,12 +1276,13 @@ static void aspeed_smc_realize(DeviceState *dev,
> Error **errp)
>
> static const VMStateDescription vmstate_aspeed_smc = {
> .name = "aspeed.smc",
> - .version_id = 2,
> + .version_id = 3,
> .minimum_version_id = 2,
> .fields = (const VMStateField[]) {
> VMSTATE_UINT32_ARRAY(regs, AspeedSMCState, ASPEED_SMC_R_MAX),
> VMSTATE_UINT8(snoop_index, AspeedSMCState),
> VMSTATE_UINT8(snoop_dummies, AspeedSMCState),
> + VMSTATE_BOOL(unselect, AspeedSMCState),
> VMSTATE_END_OF_LIST()
> }
> };
I think this will break migration compatibility. In order to enable
at least forward migration, it should be:
VMSTATE_BOOL_V(unselect, AspeedSMCState, 3),
For allowing backwards migration, too, we should consider making it a
subsection instead that allows migration in the default case of an idle
device.
Kevin
