Re: [Qemu-arm] [Qemu-devel] [PATCH 18/19] aspeed/smc: inject errors in DMA checksum

[Philippe Mathieu-Daudé]

Cc'ing Markus & Marc-André

On 6/14/19 2:02 PM, Cédric Le Goater wrote:
> On 13/06/2019 16:31, Philippe Mathieu-Daudé wrote:
>> Hi Cédric,
>>
>> On 5/25/19 5:12 PM, Cédric Le Goater wrote:
>>> Emulate read errors in the DMA Checksum Register for high frequencies
>>> and optimistic settings of the Read Timing Compensation Register. This
>>> will help in tuning the SPI timing calibration algorithm.
>>>
>>> The values below are those to expect from the first flash device of
>>> the FMC controller of a palmetto-bmc machine.
>>>
>>> Signed-off-by: Cédric Le Goater <address@hidden>
>>> ---
>>>  hw/ssi/aspeed_smc.c | 29 +++++++++++++++++++++++++++++
>>>  1 file changed, 29 insertions(+)
>>>
>>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>>> index 406c30c60b3f..4c162912cf62 100644
>>> --- a/hw/ssi/aspeed_smc.c
>>> +++ b/hw/ssi/aspeed_smc.c
>>> @@ -866,6 +866,30 @@ static void aspeed_smc_dma_calibration(AspeedSMCState 
>>> *s)
>>>      s->regs[s->r_ctrl0 + cs] |= CE_CTRL_CLOCK_FREQ(hclk_div);
>>>  }
>>>  
>>
>> Can you add a comment (like the patch description) here?
> 
> yes. done.
>  
>>> +static bool aspeed_smc_inject_read_failure(AspeedSMCState *s)
>>> +{
>>> +    uint8_t delay =
>>> +        (s->regs[R_DMA_CTRL] >> DMA_CTRL_DELAY_SHIFT) & 
>>> DMA_CTRL_DELAY_MASK;
>>> +    uint8_t hclk_mask =
>>> +        (s->regs[R_DMA_CTRL] >> DMA_CTRL_FREQ_SHIFT) & DMA_CTRL_FREQ_MASK;
>>> +
>>> +    /*
>>> +     * Typical values of a palmetto-bmc machine.
>>> +     */
>>> +    switch (aspeed_smc_hclk_divisor(hclk_mask)) {
>>> +    case 4 ... 16:
>>> +        return false;
>>> +    case 3: /* at least one HCLK cycle delay */
>>> +        return (delay & 0x7) < 1;
>>> +    case 2: /* at least two HCLK cycle delay */
>>> +        return (delay & 0x7) < 2;
>>> +    case 1: /* (> 100MHz) is above the max freq of the controller */
>>> +        return true;
>>> +    default:
>>> +        g_assert_not_reached();
>>> +    }
>>> +}
>>> +
>>>  /*
>>>   * Accumulate the result of the reads to provide a checksum that will
>>>   * be used to validate the read timing settings.
>>> @@ -903,6 +927,11 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
>>>          s->regs[R_DMA_FLASH_ADDR] += 4;
>>>          s->regs[R_DMA_LEN] -= 4;
>>>      }
>>> +
>>> +    if (aspeed_smc_inject_read_failure(s)) {
>>
>> So this model real world where noise eventually triggers errors. Don't
>> we want this to be enable by the user (or a QMP command)?
> 
> I can add a property at the device model level to trigger this behavior.
> Such as :
> 
>    -global driver=aspeed.smc,property=timing,value=true
> 
> timing if defined would provide the maximum clock and delay settings.

I was thinking of a simpler:

    -global driver=aspeed.smc,property=inject_failure,value=true

Then

    if (s->inject_failure && aspeed_smc_inject_read_failure(s)) {
        s->regs[R_DMA_CHECKSUM] = 0xbadc0de;
    }

> 
> Are there any other examples in QEMU ?

I think most of them are hidden in the codebase...

> 
> Thanks,
> 
> C.
> 
>>
>>> +        s->regs[R_DMA_CHECKSUM] = 0xbadc0de;
>>> +    }
>>> +
>>>  }
>>>  
>>>  static void aspeed_smc_dma_rw(AspeedSMCState *s)
>>>
>