Re: [PATCH] crypto: sun4i-ss: prevent deadlock on emulated hardware

[Maxime Ripard]

On Thu, Jun 14, 2018 at 09:36:59PM +0200, Corentin Labbe wrote:
> Running a qemu emulated cubieboard with sun4i-ss driver enabled led to a never
> ending boot.
> This is due to sun4i-ss deadlocked and taking all cpu in an infinite loop.
> Since the crypto hardware is not implemented, all registers are read as 0.
> So sun4i-ss will never progress in any operations. (TX_CNT being always 0)
> 
> The first idea is to add a "TX_CNT always zero timeout" but this made cipher/hash loops
> more complex and prevent a case that never happen on real hardware.
> 
> The best way to fix is to check at probe time if we run on a virtual
> machine with hardware emulated but non-implemented and prevent
> sun4i-ss to be loaded in that case.
> Letting sun4i-ss to load is useless anyway since all crypto algorithm will be
> disabled since they will fail crypto selftests.
> 
> Tested-on: qemu-cubieboard
> Tested-on: cubieboard2
> 
> Signed-off-by: Corentin Labbe <clabbe.montjoie@xxxxxxxxx>
> ---
>  drivers/crypto/sunxi-ss/sun4i-ss-core.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/crypto/sunxi-ss/sun4i-ss-core.c b/drivers/crypto/sunxi-ss/sun4i-ss-core.c
> index a81d89b3b7d8..a178e80adcf3 100644
> --- a/drivers/crypto/sunxi-ss/sun4i-ss-core.c
> +++ b/drivers/crypto/sunxi-ss/sun4i-ss-core.c
> @@ -341,9 +341,18 @@ static int sun4i_ss_probe(struct platform_device *pdev)
>  	 * I expect to be a sort of Security System Revision number.
>  	 * Since the A80 seems to have an other version of SS
>  	 * this info could be useful
> +	 * Detect virtual machine with non-implemented hardware
> +	 * (qemu-cubieboard) by checking the register value after a write to it.
> +	 * On non-implemented hardware, all registers are read as 0.
> +	 * On real hardware we should have a value > 0.
>  	 */
>  	writel(SS_ENABLED, ss->base + SS_CTL);
>  	v = readl(ss->base + SS_CTL);
> +	if (!v) {
> +		dev_err(&pdev->dev, "Qemu with non-implemented SS detected.\n");
> +		err = -ENODEV;
> +		goto error_rst;
> +	}

This is wrong way to tackle the issue. There's multiple reason why
this could happen (for example the device not being clocked, or
maintained in reset). There's nothing specific about qemu here, and
the fundamental issue isn't that the device isn't functional in qemu,
it's that qemu lies about which hardware it can emulate in the DT it
passes to the kernel.

There's no way this can scale, alone from the fact that qemu should
patch the DT according to what it can do. Not trying to chase after
each and every device that is broken in qemu.

NAK.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: PGP signature