Re: [PATCH v8 09/13] arm/smmu-v3: add suspend/resume handlers

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Mykola,

> 
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> b/xen/drivers/passthrough/arm/smmu-v3.c
> index bf153227db..7607ffc9ca 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -1814,8 +1814,7 @@ static int arm_smmu_write_reg_sync(struct 
> arm_smmu_device *smmu, u32 val,
> }
> 
> /* GBPA is "special" */
> -static int __init arm_smmu_update_gbpa(struct arm_smmu_device *smmu,
> -                                       u32 set, u32 clr)
> +static int arm_smmu_update_gbpa(struct arm_smmu_device *smmu, u32 set, u32 
> clr)
> {
> int ret;
> u32 reg, __iomem *gbpa = smmu->base + ARM_SMMU_GBPA;
> @@ -1995,10 +1994,29 @@ err_free_evtq_irq:
> return ret;
> }
> 
> +static int arm_smmu_enable_irqs(struct arm_smmu_device *smmu)
> +{
> + int ret;
> + u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
> +
> + if ( smmu->features & ARM_SMMU_FEAT_PRI )
> + irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
> +
> + /* Enable interrupt generation on the SMMU */
> + ret = arm_smmu_write_reg_sync(smmu, irqen_flags,
> +      ARM_SMMU_IRQ_CTRL, ARM_SMMU_IRQ_CTRLACK);
> + if ( ret )
> + {
> + dev_warn(smmu->dev, "failed to enable irqs\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int __init arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
> {
> int ret, irq;
> - u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
> 
> /* Disable IRQs first */
> ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
> @@ -2028,22 +2046,7 @@ static int __init arm_smmu_setup_irqs(struct 
> arm_smmu_device *smmu)
> }
> }
> 
> - if (smmu->features & ARM_SMMU_FEAT_PRI)
> - irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
> -
> - /* Enable interrupt generation on the SMMU */
> - ret = arm_smmu_write_reg_sync(smmu, irqen_flags,
> -      ARM_SMMU_IRQ_CTRL, ARM_SMMU_IRQ_CTRLACK);
> - if (ret) {
> - dev_warn(smmu->dev, "failed to enable irqs\n");
> - goto err_free_irqs;
> - }
> -
> return 0;
> -
> -err_free_irqs:
> - arm_smmu_free_irqs(smmu);
> - return ret;
> }
> 
> static int arm_smmu_device_disable(struct arm_smmu_device *smmu)
> @@ -2057,7 +2060,7 @@ static int arm_smmu_device_disable(struct 
> arm_smmu_device *smmu)
> return ret;
> }
> 
> -static int __init arm_smmu_device_reset(struct arm_smmu_device *smmu)
> +static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
> {
> int ret;
> u32 reg, enables;
> @@ -2163,17 +2166,9 @@ static int __init arm_smmu_device_reset(struct 
> arm_smmu_device *smmu)
> }
> }
> 
> - ret = arm_smmu_setup_irqs(smmu);
> - if (ret) {
> - dev_err(smmu->dev, "failed to setup irqs\n");

We are moving this one to the probe and .. 

> + ret = arm_smmu_enable_irqs(smmu);
> + if ( ret )

changing with this one, but arm_smmu_setup_irqs() also calls 
arm_smmu_setup_unique_irqs() which
calls arm_smmu_setup_msis(), are we sure that on resume we will get the same 
state?

> return ret;
> - }
> -
> - /* Initialize tasklets for threaded IRQs*/
> - tasklet_init(&smmu->evtq_irq_tasklet, arm_smmu_evtq_tasklet, smmu);
> - tasklet_init(&smmu->priq_irq_tasklet, arm_smmu_priq_tasklet, smmu);
> - tasklet_init(&smmu->combined_irq_tasklet, arm_smmu_combined_irq_tasklet,
> - smmu);
> 
> /* Enable the SMMU interface, or ensure bypass */
> if (disable_bypass) {
> @@ -2181,20 +2176,16 @@ static int __init arm_smmu_device_reset(struct 
> arm_smmu_device *smmu)
> } else {
> ret = arm_smmu_update_gbpa(smmu, 0, GBPA_ABORT);
> if (ret)
> - goto err_free_irqs;
> + return ret;
> }
> ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
>     ARM_SMMU_CR0ACK);
> if (ret) {
> dev_err(smmu->dev, "failed to enable SMMU interface\n");
> - goto err_free_irqs;
> + return ret;
> }
> 
> return 0;
> -
> -err_free_irqs:
> - arm_smmu_free_irqs(smmu);
> - return ret;
> }
> 
> static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
> @@ -2558,10 +2549,23 @@ static int __init arm_smmu_device_probe(struct 
> platform_device *pdev)
> if (ret)
> goto out_free;
> 
> + ret = arm_smmu_setup_irqs(smmu);
> + if ( ret )
> + {
> + dev_err(smmu->dev, "failed to setup irqs\n");
> + goto out_free;
> + }
> +
> + /* Initialize tasklets for threaded IRQs*/
> + tasklet_init(&smmu->evtq_irq_tasklet, arm_smmu_evtq_tasklet, smmu);
> + tasklet_init(&smmu->priq_irq_tasklet, arm_smmu_priq_tasklet, smmu);
> + tasklet_init(&smmu->combined_irq_tasklet, arm_smmu_combined_irq_tasklet,
> + smmu);
> +
> /* Reset the device */
> ret = arm_smmu_device_reset(smmu);
> if (ret)
> - goto out_free;
> + goto out_free_irqs;
> 
> /*
> * Keep a list of all probed devices. This will be used to query
> @@ -2575,6 +2579,8 @@ static int __init arm_smmu_device_probe(struct 
> platform_device *pdev)
> 
> return 0;
> 
> +out_free_irqs:
> + arm_smmu_free_irqs(smmu);
> 
> out_free:
> arm_smmu_free_structures(smmu);
> @@ -2855,6 +2861,96 @@ static void arm_smmu_iommu_xen_domain_teardown(struct 
> domain *d)
> xfree(xen_domain);
> }
> 
> +#ifdef CONFIG_SYSTEM_SUSPEND
> +
> +static void arm_smmu_reset_for_suspend_rollback(struct arm_smmu_device *smmu)
> +{
> + int ret = arm_smmu_device_reset(smmu);
> +
> + if ( ret )
> + dev_err(smmu->dev, "Failed to reset during suspend rollback: %d\n",
> + ret);
> +}
> +
> +static int arm_smmu_suspend(void)
> +{
> + struct arm_smmu_device *smmu;
> + int ret = 0;
> +
> + list_for_each_entry(smmu, &arm_smmu_devices, devices)
> + {
> + bool wfe = !!(smmu->features & ARM_SMMU_FEAT_SEV);
> +
> + /* Abort all transactions before disable to avoid spurious bypass */
> + ret = arm_smmu_update_gbpa(smmu, GBPA_ABORT, 0);
> + if ( ret )
> + goto fail;
> +
> + /* Disable the SMMU via CR0.EN and all queues except CMDQ */
> + ret = arm_smmu_write_reg_sync(smmu, CR0_CMDQEN, ARM_SMMU_CR0,
> + ARM_SMMU_CR0ACK);
> + if ( ret )
> + {
> + dev_err(smmu->dev, "Timed-out while disabling smmu\n");
> + goto fail;
> + }
> +
> + /*
> + * At this point the SMMU is completely disabled and won't access
> + * any translation/config structures, even speculative accesses
> + * aren't performed as per the IHI0070 spec (section 6.3.9.6).
> + */
> +
> + /* Wait for the CMDQs to be drained to flush any pending commands */
> + ret = queue_poll_cons(&smmu->cmdq.q, true, wfe);
> + if ( ret )
> + {
> + dev_err(smmu->dev, "Draining queues timed-out\n");
> + goto fail;
> + }

polling the queue doesn’t give you the assurance that all prior commands are 
complete,
I would use arm_smmu_cmdq_issue_sync for that instead of the above.

ret = arm_smmu_cmdq_issue_sync(smmu);
if ( ret )
   goto fail;

> +
> + /* Disable everything */
> + ret = arm_smmu_device_disable(smmu);
> + if ( ret )
> + goto fail;
> +
> + dev_dbg(smmu->dev, "Suspended smmu\n");
> + }
> +
> + return 0;
> +
> + fail:
> + /* Reset the device that failed as well as any already-suspended ones. */
> + arm_smmu_reset_for_suspend_rollback(smmu);
> +
> + list_for_each_entry_continue_reverse(smmu, &arm_smmu_devices, devices)
> + arm_smmu_reset_for_suspend_rollback(smmu);
> +
> + return ret;
> +}
> +

Cheers,
Luca