linux-kernel - Re: [PATCH v8 3/3] iommu/arm-smmu: Add global/context fault implementation hooks

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:   Tue, 30 Jun 2020 13:42:52 +0100
From:   Jon Hunter <jonathanh@...dia.com>
To:     Robin Murphy <robin.murphy@....com>,
        Krishna Reddy <vdumpa@...dia.com>
CC:     <snikam@...dia.com>, <nicoleotsuka@...il.com>,
        <mperttunen@...dia.com>, <bhuntsman@...dia.com>, <will@...nel.org>,
        <linux-kernel@...r.kernel.org>, <praithatha@...dia.com>,
        <talho@...dia.com>, <iommu@...ts.linux-foundation.org>,
        <nicolinc@...dia.com>, <linux-tegra@...r.kernel.org>,
        <yhsu@...dia.com>, <treding@...dia.com>,
        <linux-arm-kernel@...ts.infradead.org>, <bbiswas@...dia.com>
Subject: Re: [PATCH v8 3/3] iommu/arm-smmu: Add global/context fault
 implementation hooks


On 30/06/2020 13:13, Robin Murphy wrote:
> On 2020-06-30 09:37, Jon Hunter wrote:
>>
>> On 30/06/2020 01:10, Krishna Reddy wrote:
>>> Add global/context fault hooks to allow NVIDIA SMMU implementation
>>> handle faults across multiple SMMUs.
>>
>> Nit ... this is not just for NVIDIA, but this allows anyone to add
>> custom global/context and fault hooks. So I think that the changelog
>> should be clear that this change permits custom fault hooks and that
>> custom fault hooks are needed for the Tegra194 SMMU. You may also want
>> to say why.
>>
>>>
>>> Signed-off-by: Krishna Reddy <vdumpa@...dia.com>
>>> ---
>>>   drivers/iommu/arm-smmu-nvidia.c | 98 +++++++++++++++++++++++++++++++++
>>>   drivers/iommu/arm-smmu.c        | 17 +++++-
>>>   drivers/iommu/arm-smmu.h        |  3 +
>>>   3 files changed, 116 insertions(+), 2 deletions(-)

...

>>> @@ -835,7 +836,13 @@ static int arm_smmu_init_domain_context(struct
>>> iommu_domain *domain,
>>>        * handler seeing a half-initialised domain state.
>>>        */
>>>       irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx];
>>> -    ret = devm_request_irq(smmu->dev, irq, arm_smmu_context_fault,
>>> +
>>> +    if (smmu->impl && smmu->impl->context_fault)
>>> +        context_fault = smmu->impl->context_fault;
>>> +    else
>>> +        context_fault = arm_smmu_context_fault;
>>
>> Why not see the default smmu->impl->context_fault to
>> arm_smmu_context_fault in arm_smmu_impl_init() and then allow the
>> various implementations to override as necessary? Then you can get rid
>> of this context_fault variable here and just use
>> smmu->impl->context_fault below.
> 
> Because the default smmu->impl is NULL. And as I've said before, NAK to
> forcing the common case to allocate a set of "quirks" purely to override
> the default IRQ handler with the default IRQ handler ;)


Ah OK, makes sense. Sorry I am a bit late to the review :-)

Jon

-- 
nvpublic