Re: [PATCH v2 2/7] iommu/riscv: Add RISC-V IOMMU platform device driver

From: Robin Murphy <robin.murphy@arm.com>
To: Tomasz Jeznach <tjeznach@rivosinc.com>
Cc: Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Anup Patel <apatel@ventanamicro.com>,
	Sunil V L <sunilvl@ventanamicro.com>,
	Nick Kossifidis <mick@ics.forth.gr>,
	Sebastien Boeuf <seb@rivosinc.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	devicetree@vger.kernel.org, iommu@lists.linux.dev,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux@rivosinc.com
Subject: Re: [PATCH v2 2/7] iommu/riscv: Add RISC-V IOMMU platform device driver
Date: Thu, 25 Apr 2024 12:23:10 +0100	[thread overview]
Message-ID: <059ae516-aed4-4836-a2ca-aff150ff428d@arm.com> (raw)
In-Reply-To: <CAH2o1u6FpDWNZsQCADQQKauPYzxDE73DRHHQKw6LjNb27ePeXw@mail.gmail.com>

On 24/04/2024 10:59 pm, Tomasz Jeznach wrote:
[...]
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 2657f9eae84c..051599c76585 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -18972,6 +18972,12 @@ L:   iommu@lists.linux.dev
>>>    L:  linux-riscv@lists.infradead.org
>>>    S:  Maintained
>>>    F:  Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
>>> +F:   drivers/iommu/riscv/Kconfig
>>> +F:   drivers/iommu/riscv/Makefile
>>> +F:   drivers/iommu/riscv/iommu-bits.h
>>> +F:   drivers/iommu/riscv/iommu-platform.c
>>> +F:   drivers/iommu/riscv/iommu.c
>>> +F:   drivers/iommu/riscv/iommu.h
>>
>> I'm pretty sure a single "F: drivers/iommu/riscv/" pattern will suffice.
>>
> 
> Correct. But will required a workaround for pretty naive MAINTAINERS update
> check in scripts/checkpatch.pl:3014 in next patch.

As long as what you're doing is clearly reasonable to humans, the 
correct workaround for any checkpatch complaint is to ignore checkpatch.

[...]
>>> +static int riscv_iommu_init_check(struct riscv_iommu_device *iommu)
>>> +{
>>> +     u64 ddtp;
>>> +
>>> +     /* Hardware must be configured in OFF | BARE mode at system initialization. */
>>> +     riscv_iommu_readq_timeout(iommu, RISCV_IOMMU_REG_DDTP,
>>> +                               ddtp, !(ddtp & RISCV_IOMMU_DDTP_BUSY),
>>> +                               10, RISCV_IOMMU_DDTP_TIMEOUT);
>>> +     if (FIELD_GET(RISCV_IOMMU_DDTP_MODE, ddtp) > RISCV_IOMMU_DDTP_MODE_BARE)
>>> +             return -EBUSY;
>>
>> It looks like RISC-V already supports kdump, so you probably want to be
>> prepared to find the IOMMU with its pants down and deal with it from day
>> one.
>>
> 
> This is the simplest check/fail for the kexec and/or boot loaders
> leaving IOMMU translations active.
> I've been already looking into kexec path to quiesce all devices and
> IOMMU in shutdown path.
> I'm not convinced it's ready for the prime time on RISC-V, will
> address this in follow up patches.

Yeah, for regular kexec you definitely want an orderly shutdown of the 
IOMMU, although there's still a bit of an open question about whether 
it's better to actively block any remaining traffic from devices whose 
drivers haven't cleanly stopped them. It's in the kdump crash kernel 
case that you can't have any expectations and need to be able to recover 
the IOMMU into a usable state, since it's likely to be in the way of 
devices which the crash kernel wants to take over and use.

Thanks,
Robin.