linux-kernel - Re: [PATCH -next v2] iommu/arm-smmu-v3: Add suspend and resume support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87czr3ewtq.wl-maz@kernel.org>
Date:   Tue, 27 Jul 2021 14:00:01 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Bixuan Cui <cuibixuan@...wei.com>
Cc:     <iommu@...ts.linux-foundation.org>, <linux-kernel@...r.kernel.org>,
        <will@...nel.org>, <weiyongjun1@...wei.com>,
        <john.wanghui@...wei.com>, <dingtianhong@...wei.com>,
        <thunder.leizhen@...wei.com>, <guohanjun@...wei.com>,
        <robin.murphy@....com>, <joro@...tes.org>,
        <jean-philippe@...aro.org>, <Jonathan.Cameron@...wei.com>,
        <song.bao.hua@...ilicon.com>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH -next v2] iommu/arm-smmu-v3: Add suspend and resume support

On Tue, 27 Jul 2021 13:14:08 +0100,
Bixuan Cui <cuibixuan@...wei.com> wrote:
> 
> Add suspend and resume support for arm-smmu-v3 by low-power mode.
> 
> When the smmu is suspended, it is powered off and the registers are
> cleared. So saves the msi_msg context during msi interrupt initialization
> of smmu. When resume happens it calls arm_smmu_device_reset() to restore
> the registers.
> 
> Signed-off-by: Bixuan Cui <cuibixuan@...wei.com>
> Reviewed-by: Wei Yongjun <weiyongjun1@...wei.com>
> Reviewed-by: Zhen Lei <thunder.leizhen@...wei.com>
> Reviewed-by: Ding Tianhong <dingtianhong@...wei.com>
> Reviewed-by: Hanjun Guo <guohanjun@...wei.com>
> ---
> Changes in v2:
> * Using get_cached_msi_msg() instead of the descriptor to resume msi_msg
>   in arm_smmu_resume_msis();
> 
> * Move arm_smmu_resume_msis() from arm_smmu_setup_unique_irqs() into
>   arm_smmu_setup_irqs() and rename it to arm_smmu_resume_unique_irqs();
> 
>   Call arm_smmu_setup_unique_irqs() to configure the IRQ during probe and
>   call arm_smmu_resume_unique_irqs() in resume mode to restore the IRQ
>   registers to make the code more reasonable.
> 
> * Call arm_smmu_device_disable() to disable smmu and clear CR0_SMMUEN on
>   suspend. Then the warning about CR0_SMMUEN being enabled can be cleared
>   on resume.
> 
> * Using SET_SYSTEM_SLEEP_PM_OPS();
> 
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 69 ++++++++++++++++++---
>  1 file changed, 62 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 235f9bdaeaf2..66f35d5c7a70 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -40,6 +40,7 @@ MODULE_PARM_DESC(disable_bypass,
>  
>  static bool disable_msipolling;
>  module_param(disable_msipolling, bool, 0444);
> +static bool bypass;

As outlined before, this is likely to be wrong if you can have
per-SMMU bypass control.

>  MODULE_PARM_DESC(disable_msipolling,
>  	"Disable MSI-based polling for CMD_SYNC completion.");
>  
> @@ -3129,11 +3130,38 @@ static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
>  	doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
>  	doorbell &= MSI_CFG0_ADDR_MASK;
>  
> +	/* Saves the msg context for resume if desc->msg is empty */
> +	if (desc->msg.address_lo == 0x0 && desc->msg.address_hi == 0x0) {
> +		desc->msg.address_lo = msg->address_lo;
> +		desc->msg.address_hi = msg->address_hi;
> +		desc->msg.data = msg->data;
> +	}

I thought I had made it clear that this approach is not acceptable.
Please fix the generic code to keep track of the latest message.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.