linux-kernel - Re: [PATCH -next] 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: <5054a5cd-579f-3fe9-1884-5219c8d13531@huawei.com>
Date:   Thu, 22 Jul 2021 14:34:32 +0800
From:   Bixuan Cui <cuibixuan@...wei.com>
To:     Marc Zyngier <maz@...nel.org>, Robin Murphy <robin.murphy@....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>,
        <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] iommu/arm-smmu-v3: Add suspend and resume support



On 2021/7/21 23:01, Marc Zyngier wrote:
> On Wed, 21 Jul 2021 14:59:47 +0100,
> Robin Murphy <robin.murphy@....com> wrote:
>>
>> On 2021-07-21 14:12, Marc Zyngier wrote:
>>> On Wed, 21 Jul 2021 12:42:14 +0100,
>>> Robin Murphy <robin.murphy@....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@...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>
>>>>> ---
>>>>>
>>>>>    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.
Agree. If msg is saved to desc->msg in MSI core, the code here will not need.
During the initialization of the MSI interrupt of the SMMU, the desc->msg
is never used. So I save msg to desc->msg for resume use.


>>
>>>>> +
>>>>>    	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.
The code of arm_smmu_write_msi_msg() is multiplexed to restore the register. Therefore,
the parameter must be supplemented. Generally, desc is sufficient as an input parameter..
:)

Thanks,
Bixuan Cui
> 
> Thanks,
> 
> 	M.
>