Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 21 Jul 2021 16:01:24 +0100
Message-ID: <877dhj3e4b.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Robin Murphy <robin.murphy@arm.com>
Cc:     Bixuan Cui <cuibixuan@huawei.com>,
        iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
        will@kernel.org, weiyongjun1@huawei.com, john.wanghui@huawei.com,
        dingtianhong@huawei.com, thunder.leizhen@huawei.com,
        guohanjun@huawei.com, joro@8bytes.org, jean-philippe@linaro.org,
        Jonathan.Cameron@huawei.com, song.bao.hua@hisilicon.com,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support
In-Reply-To: <848befb0-7a9a-0b2b-8be9-3dfa02919488@arm.com>
References: <20210721013350.17664-1-cuibixuan@huawei.com>
        <4e506481-5f6c-9c5e-eda3-300861581080@arm.com>
        <878s1z3j68.wl-maz@kernel.org>
        <848befb0-7a9a-0b2b-8be9-3dfa02919488@arm.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 21 Jul 2021 14:59:47 +0100,
Robin Murphy <robin.murphy@arm.com> wrote:
> 
> On 2021-07-21 14:12, Marc Zyngier wrote:
> > On Wed, 21 Jul 2021 12:42:14 +0100,
> > Robin Murphy <robin.murphy@arm.com> wrote:
> >> 
> >> [ +Marc for MSI bits ]
> >> 
> >> On 2021-07-21 02:33, Bixuan Cui 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@huawei.com>
> >>> Reviewed-by: Wei Yongjun <weiyongjun1@huawei.com>
> >>> Reviewed-by: Zhen Lei <thunder.leizhen@huawei.com>
> >>> Reviewed-by: Ding Tianhong <dingtianhong@huawei.com>
> >>> Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
> >>> ---
> >>> 
> >>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 72 ++++++++++++++++++---
> >>>    1 file changed, 64 insertions(+), 8 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..bf1163acbcb1 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;
> >>>    MODULE_PARM_DESC(disable_msipolling,
> >>>    	"Disable MSI-based polling for CMD_SYNC completion.");
> >>>    @@ -3129,11 +3130,37 @@ 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 == 0 && desc->msg.address_hi == 0) {
> >>> +		desc->msg.address_lo = msg->address_lo;
> >>> +		desc->msg.address_hi = msg->address_hi;
> >>> +		desc->msg.data = msg->data;
> >>> +	}
> >> 
> >> My gut feeling is that this is something a device driver maybe
> >> shouldn't be poking into, but I'm not entirely familiar with the area
> >> :/
> > 
> > Certainly not. If you rely on the message being stored into the
> > descriptors, then implement this in the core code, like we do for PCI.
> 
> Ah, so it would be an acceptable compromise to *read* desc->msg (and
> thus avoid having to store our own copy of the message) if the core
> was guaranteed to cache it? That's good to know, thanks.

Yeah, vfio, a couple of other weird drivers and (*surprise!*) ia64 are
using this kind of trick. I don't see a reason not to implement that
for platform-MSI (although level signalling may be interesting...), or
even to move it into the core MSI code.

> 
> >>> +
> >>>    	writeq_relaxed(doorbell, smmu->base + cfg[0]);
> >>>    	writel_relaxed(msg->data, smmu->base + cfg[1]);
> >>>    	writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
> >>>    }
> >>>    +static void arm_smmu_resume_msis(struct arm_smmu_device *smmu)
> >>> +{
> >>> +	struct msi_desc *desc;
> >>> +	struct device *dev = smmu->dev;
> >>> +
> >>> +	for_each_msi_entry(desc, dev) {
> >>> +		switch (desc->platform.msi_index) {
> >>> +		case EVTQ_MSI_INDEX:
> >>> +		case GERROR_MSI_INDEX:
> >>> +		case PRIQ_MSI_INDEX:
> >>> +			arm_smmu_write_msi_msg(desc, &(desc->msg));
> > 
> > Consider using get_cached_msi_msg() instead of using the internals of
> > the descriptor.
> 
> Oh, there's even a proper API for it, marvellous! I hadn't managed to
> dig that far myself :)

It is a bit odd in the sense that it takes a copy of the message
instead of returning a pointer, but at least this solves lifetime
issues.

Thanks,

	M.

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