On 05/01/17 14:47, Will Deacon wrote: > On Thu, Jan 05, 2017 at 02:07:31PM +0000, Mark Rutland wrote: >> On Thu, Jan 05, 2017 at 02:00:05PM +0000, Will Deacon wrote: >>> On Thu, Jan 05, 2017 at 12:08:57PM +0000, Mark Rutland wrote: >>>> On Thu, Jan 05, 2017 at 11:55:29AM +0000, Will Deacon wrote: >>>>> On Tue, Jan 03, 2017 at 04:30:54PM -0500, Rob Clark wrote: >>>>>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt >>>>>> index ef465b0..5f405a6 100644 >>>>>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt >>>>>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt >>>>>> @@ -68,6 +68,9 @@ conditions. >>>>>> aliases of secure registers have to be used during >>>>>> SMMU configuration. >>>>>> >>>>>> +- arm,smmu-enable-stall : Enable stall mode to stall memory transactions >>>>>> + and resume after fault is handled >>>> >>>> The wording here seems to describe a policy rather than a property. >>>> >>>> Can you elaborate on when/why this is required/preferred/valid? >>> >>> It's not a policy, it's a hardware capability. There are some non-probeable >>> reasons why stalling mode is unsafe or unusable: >>> >>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-December/474530.html >> >> Ok. My point was that the wording above is an imperative -- it tells >> the kernel to enable stall mode, not if/why it is safe to do so (i.e. it >> is a policy, not a property). >> >> It sounds like that's just a wording issue. Something like >> "arm,stalling-is-usable" (along with a descrition of when that >> can/should be in the DT) would be vastly better. > > Why does it need a vendor prefix? I'm not down on the convention there. > "stalling-safe" or "stalling-supported" are alternative strings. > >>>>>> static irqreturn_t arm_smmu_global_fault(int irq, void *dev) >>>>>> @@ -824,6 +852,8 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain, >>>>>> >>>>>> /* SCTLR */ >>>>>> reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M; >>>>>> + if (smmu->options & ARM_SMMU_OPT_ENABLE_STALL) >>>>>> + reg |= SCTLR_CFCFG; >>>>> >>>>> I wonder if this should also be predicated on the compatible string, so >>>>> that the "arm,smmu-enable-stall" property is ignored (with a warning) if >>>>> the compatible string isn't specific enough to identify an implementation >>>>> with the required SS behaviour? On the other hand, it feels pretty >>>>> redundant and a single "stalling works" property is all we need. >>>> >>>> Can you elaborate on what "stalling works" entails? Is that just the SS >>>> bit behaviour? are there integration or endpoint-specific things that we >>>> need to care about? >>> >>> See above. The "stalling works" property (arm,smmu-enable-stall) would >>> indicate that both the implementation *and* the integration are such >>> that stalling is usable for demand paging. I suspect there are endpoints >>> that can't deal with stalls (e.g. they might timeout and signal a RAS >>> event), but in that case their respective device drivers should ensure >>> that any DMA buffers are pinned and/or register a fault handler to >>> request termination of the faulting transaction. >> >> Ok. It would be good to elaborate on what "stalling is useable" means in >> the property description. i.e. what specificallty the implementation and >> integration need to ensure. > > We can describe some of those guarantees in the property description, but > it's difficult to enumerate them exhaustively. For example, you wouldn't > want stalling to lead to data corruption, denial of service, or for the > thing to catch fire, but having those as explicit requirements is a bit > daft. It's also impossible to check that you thought of everything. > > Aside from renaming the option, I'm really after an opinion on whether > it's better to have one property or combine it with the compatible > string, because I can see benefits of both and don't much care either > way. The SMMU implementation side of the decision (i.e. independence of IRQ assertion vs. SS) seems like exactly the sort of stuff the compatible string already has covered. The integration side I'm less confident can be described this way at all - the "this device definitely won't go wrong if stalled for an indefinite length of time" is inherently a per-master thing, so a single property on the SMMU implying that for every device connected to it seems a bit optimistic, and breaks down as soon as you have one device in the system for which that isn't true (a PCI root complex, say), even if that guy's traffic never crosses paths with whichever few devices you actually care about using stalls with. I think this needs to be some kind of "arm,smmu-stall-safe" property placed on individual master device nodes (mad idea: or even an extra cell of flags in the IOMMU specifier) to encapsulate both that the given device itself is OK with being stalled, and that it's integrated in such a way that its stalled transactions cannot disrupt any *other* device (e.g. it has a TBU all to itself). Then upon initialising a context bank on a suitable SMMU implementation, we set CFCFG based on whatever device is being attached to the corresponding domain, and refuse any subsequent attempts to attach a non-stallable device to a stalling domain (and possibly even vice-versa). Robin. > > Will > _______________________________________________ > iommu mailing list > iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx > https://lists.linuxfoundation.org/mailman/listinfo/iommu > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
