linux-kernel - Re: [PATCH v3 1/2] iommu/arm-smmu-v3: fix unexpected CMD

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]

Date:   Thu, 16 Aug 2018 10:27:38 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Will Deacon <will.deacon@....com>,
        "Leizhen (ThunderTown)" <thunder.leizhen@...wei.com>
Cc:     Joerg Roedel <joro@...tes.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        iommu <iommu@...ts.linux-foundation.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        LinuxArm <linuxarm@...wei.com>,
        Hanjun Guo <guohanjun@...wei.com>,
        Libin <huawei.libin@...wei.com>,
        John Garry <john.garry@...wei.com>
Subject: Re: [PATCH v3 1/2] iommu/arm-smmu-v3: fix unexpected CMD_SYNC timeout

On 2018-08-16 10:18 AM, Will Deacon wrote:
> On Thu, Aug 16, 2018 at 04:21:17PM +0800, Leizhen (ThunderTown) wrote:
>> On 2018/8/15 20:26, Robin Murphy wrote:
>>> On 15/08/18 11:23, Zhen Lei wrote:
>>>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>>>> index 1d64710..3f5c236 100644
>>>> --- a/drivers/iommu/arm-smmu-v3.c
>>>> +++ b/drivers/iommu/arm-smmu-v3.c
>>>> @@ -566,7 +566,7 @@ struct arm_smmu_device {
>>>>
>>>>        int                gerr_irq;
>>>>        int                combined_irq;
>>>> -    atomic_t            sync_nr;
>>>> +    u32                sync_nr;
>>>>
>>>>        unsigned long            ias; /* IPA */
>>>>        unsigned long            oas; /* PA */
>>>> @@ -775,6 +775,11 @@ static int queue_remove_raw(struct arm_smmu_queue *q, u64 *ent)
>>>>        return 0;
>>>>    }
>>>>
>>>> +static inline void arm_smmu_cmdq_sync_set_msidata(u64 *cmd, u32 msidata)
>>>
>>> If we *are* going to go down this route then I think it would make sense
>>> to move the msiaddr and CMDQ_SYNC_0_CS_MSI logic here as well; i.e.
>>> arm_smmu_cmdq_build_cmd() always generates a "normal" SEV-based sync
>>> command, then calling this guy would convert it to an MSI-based one.
>>> As-is, having bits of mutually-dependent data handled across two
>>> separate places just seems too messy and error-prone.
>>
>> Yes, How about create a new function "arm_smmu_cmdq_build_sync_msi_cmd"?
>>
>> static inline
>> void arm_smmu_cmdq_build_sync_msi_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
>> {
>> 	cmd[0]  = FIELD_PREP(CMDQ_0_OP, ent->opcode);
>> 	cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_IRQ);
>> 	cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH);
>> 	cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB);
>> 	cmd[1]  = ent->sync.msiaddr & CMDQ_SYNC_1_MSIADDR_MASK;
>> }
> 
> None of this seems justified given the numbers from John, so please just do
> the simple thing and build the command with the lock held.

Agreed - sorry if my wording was unclear, but that suggestion was only 
for the possibility of it proving genuinely worthwhile to build the 
command outside the lock. Since that isn't the case, I definitely prefer 
the simpler approach too.

Robin.