From: Robin Murphy <robin.murphy@arm.com> To: Will Deacon <will@kernel.org> Cc: iommu@lists.linux-foundation.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling Date: Tue, 20 Aug 2019 19:41:52 +0100 [thread overview] Message-ID: <8cc47f43-ad74-b4e2-e977-6c78780abc91@arm.com> (raw) In-Reply-To: <20190820160700.6ircxomwuo5bksqz@willie-the-truck> On 20/08/2019 17:07, Will Deacon wrote: > On Tue, Aug 20, 2019 at 04:25:56PM +0100, Robin Murphy wrote: >> On 20/08/2019 11:31, Will Deacon wrote: >>> On Mon, Aug 19, 2019 at 07:19:30PM +0100, Robin Murphy wrote: >>>> Although it's conceptually nice for the io_pgtable_cfg to provide a >>>> standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU >>>> looks exactly like an Arm CPU, and they all have various other TCR >>>> controls which io-pgtable can't be expected to understand. Thus since >>>> there is an expectation that drivers will have to add to the given TCR >>>> value anyway, let's strip it down to just the essentials that are >>>> directly relevant to io-pgatble's inner workings - namely the address >>>> sizes, walk attributes, and where appropriate, format selection. >>>> >>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >>>> --- >>>> drivers/iommu/arm-smmu-v3.c | 7 +------ >>>> drivers/iommu/arm-smmu.c | 1 + >>>> drivers/iommu/arm-smmu.h | 2 ++ >>>> drivers/iommu/io-pgtable-arm-v7s.c | 6 ++---- >>>> drivers/iommu/io-pgtable-arm.c | 4 ---- >>>> drivers/iommu/qcom_iommu.c | 2 +- >>>> 6 files changed, 7 insertions(+), 15 deletions(-) >>> >>> Hmm, so I'm a bit nervous about this one since I think we really should >>> be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying >>> on the driver to do this worries me. See my comments on the next patch. >> >> The whole idea is that we already know we can't provide a *complete* TCR >> value (not least because anything above bit 31 is the wild west), thus >> there's really no point in io-pgtable trying to provide anything other than >> the parts it definitely controls. It makes sense to provide this partial TCR >> value "as if" for TTBR0, since that's the most common case, but ultimately >> io-pgatble doesn't know (or need to) which TTBR the caller intends to >> actually use for this table. Even if the caller *is* allocating it for >> TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1 >> already, so it still wouldn't be in a position to make the EPD1 call either >> way. > > Ok, but the driver can happily rewrite/ignore what it gets back. I suppose > an alternative would be scrapped the 'u64 tcr' and instead having a bunch > of named bitfields for the stuff we're actually providing, although I'd > still like EPDx to be in there. I like the bitfield idea; it would certainly emphasise the "you have to do something more with this" angle that I'm pushing towards here, but still leave things framed in TCR terms without having to go to some more general abstraction. It really doesn't play into your EPD argument though - such a config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x. For a driver to understand that and do the right thing with it is even more involved than for the driver to just set EPD1 by itself anyway. >> Ultimately, it's the IOMMU drivers who decide what they put in which TTBR, >> so it's the IOMMU drivers which have to take responsibility for EPD*. Sure >> you can worry about it, but you can equally worry about them them >> misprogramming the ASID or anything else... > > I find the EPDx bits particularly dangerous because: > > - They're easily overlooked > - Clobbering TTBR1 with 0x0 doesn't disable walks via TTBR1 as you might > reasonably expect (FWIW I'm not sure that that is a reasonable expectation, at least for anyone savvy enough to be programming an MMU in the first place. There are plenty of systems with RAM at 0x0) > - If you do the above without EPD, the breakage will be subtle > > and given that I don't see any real downsides to us providing a default TCR > value with EPD set appropriately, then I think we should do that. I'd be > happy to revisit the decision later on if it's getting the way of a real > use-case, but it feels like we're throwing the baby out with the bathwater > at the moment and I'd rather do this incrementally based on actual need. The downside is maintaining extra complexity for the sake of a theoretical concern which hasn't been borne out in practice, with a promise of yet more complexity down the line. Moving the 3 babies which anyone acknowledges into their own private baths takes a whole -8 lines of code to implement. That said, if we don't go down the bitfield route, then I *can* leave the default TCR having EPD1 set if that makes you feel warm and safe, but it will still be resoundingly ignored. If only LPAE had created these bits as enables rather than disables then things would be logical and we could all be happy, but here we are... Robin. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
WARNING: multiple messages have this Message-ID (diff)
From: Robin Murphy <robin.murphy@arm.com> To: Will Deacon <will@kernel.org> Cc: robdclark@gmail.com, joro@8bytes.org, jcrouse@codeaurora.org, iommu@lists.linux-foundation.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling Date: Tue, 20 Aug 2019 19:41:52 +0100 [thread overview] Message-ID: <8cc47f43-ad74-b4e2-e977-6c78780abc91@arm.com> (raw) In-Reply-To: <20190820160700.6ircxomwuo5bksqz@willie-the-truck> On 20/08/2019 17:07, Will Deacon wrote: > On Tue, Aug 20, 2019 at 04:25:56PM +0100, Robin Murphy wrote: >> On 20/08/2019 11:31, Will Deacon wrote: >>> On Mon, Aug 19, 2019 at 07:19:30PM +0100, Robin Murphy wrote: >>>> Although it's conceptually nice for the io_pgtable_cfg to provide a >>>> standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU >>>> looks exactly like an Arm CPU, and they all have various other TCR >>>> controls which io-pgtable can't be expected to understand. Thus since >>>> there is an expectation that drivers will have to add to the given TCR >>>> value anyway, let's strip it down to just the essentials that are >>>> directly relevant to io-pgatble's inner workings - namely the address >>>> sizes, walk attributes, and where appropriate, format selection. >>>> >>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >>>> --- >>>> drivers/iommu/arm-smmu-v3.c | 7 +------ >>>> drivers/iommu/arm-smmu.c | 1 + >>>> drivers/iommu/arm-smmu.h | 2 ++ >>>> drivers/iommu/io-pgtable-arm-v7s.c | 6 ++---- >>>> drivers/iommu/io-pgtable-arm.c | 4 ---- >>>> drivers/iommu/qcom_iommu.c | 2 +- >>>> 6 files changed, 7 insertions(+), 15 deletions(-) >>> >>> Hmm, so I'm a bit nervous about this one since I think we really should >>> be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying >>> on the driver to do this worries me. See my comments on the next patch. >> >> The whole idea is that we already know we can't provide a *complete* TCR >> value (not least because anything above bit 31 is the wild west), thus >> there's really no point in io-pgtable trying to provide anything other than >> the parts it definitely controls. It makes sense to provide this partial TCR >> value "as if" for TTBR0, since that's the most common case, but ultimately >> io-pgatble doesn't know (or need to) which TTBR the caller intends to >> actually use for this table. Even if the caller *is* allocating it for >> TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1 >> already, so it still wouldn't be in a position to make the EPD1 call either >> way. > > Ok, but the driver can happily rewrite/ignore what it gets back. I suppose > an alternative would be scrapped the 'u64 tcr' and instead having a bunch > of named bitfields for the stuff we're actually providing, although I'd > still like EPDx to be in there. I like the bitfield idea; it would certainly emphasise the "you have to do something more with this" angle that I'm pushing towards here, but still leave things framed in TCR terms without having to go to some more general abstraction. It really doesn't play into your EPD argument though - such a config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x. For a driver to understand that and do the right thing with it is even more involved than for the driver to just set EPD1 by itself anyway. >> Ultimately, it's the IOMMU drivers who decide what they put in which TTBR, >> so it's the IOMMU drivers which have to take responsibility for EPD*. Sure >> you can worry about it, but you can equally worry about them them >> misprogramming the ASID or anything else... > > I find the EPDx bits particularly dangerous because: > > - They're easily overlooked > - Clobbering TTBR1 with 0x0 doesn't disable walks via TTBR1 as you might > reasonably expect (FWIW I'm not sure that that is a reasonable expectation, at least for anyone savvy enough to be programming an MMU in the first place. There are plenty of systems with RAM at 0x0) > - If you do the above without EPD, the breakage will be subtle > > and given that I don't see any real downsides to us providing a default TCR > value with EPD set appropriately, then I think we should do that. I'd be > happy to revisit the decision later on if it's getting the way of a real > use-case, but it feels like we're throwing the baby out with the bathwater > at the moment and I'd rather do this incrementally based on actual need. The downside is maintaining extra complexity for the sake of a theoretical concern which hasn't been borne out in practice, with a promise of yet more complexity down the line. Moving the 3 babies which anyone acknowledges into their own private baths takes a whole -8 lines of code to implement. That said, if we don't go down the bitfield route, then I *can* leave the default TCR having EPD1 set if that makes you feel warm and safe, but it will still be resoundingly ignored. If only LPAE had created these bits as enables rather than disables then things would be logical and we could all be happy, but here we are... Robin. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-08-20 18:42 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-19 18:19 [PATCH 0/4] iommu/io-pgtable: Cleanup and prep for split tables Robin Murphy 2019-08-19 18:19 ` Robin Murphy 2019-08-19 18:19 ` [PATCH 1/4] iommu/io-pgtable-arm: Rationalise MAIR handling Robin Murphy 2019-08-19 18:19 ` Robin Murphy 2019-08-19 18:19 ` [PATCH 2/4] iommu/io-pgtable-arm: Rationalise TTBRn handling Robin Murphy 2019-08-19 18:19 ` Robin Murphy 2019-08-20 10:19 ` Will Deacon 2019-08-20 10:19 ` Will Deacon 2019-08-20 14:17 ` Robin Murphy 2019-08-20 14:17 ` Robin Murphy 2019-08-20 15:50 ` Will Deacon 2019-08-20 15:50 ` Will Deacon 2019-08-19 18:19 ` [PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling Robin Murphy 2019-08-19 18:19 ` Robin Murphy 2019-08-20 10:31 ` Will Deacon 2019-08-20 10:31 ` Will Deacon 2019-08-20 15:25 ` Robin Murphy 2019-08-20 15:25 ` Robin Murphy 2019-08-20 16:07 ` Will Deacon 2019-08-20 16:07 ` Will Deacon 2019-08-20 18:41 ` Robin Murphy [this message] 2019-08-20 18:41 ` Robin Murphy 2019-08-21 12:11 ` Will Deacon 2019-08-21 12:11 ` Will Deacon 2019-08-21 12:56 ` Robin Murphy 2019-08-21 12:56 ` Robin Murphy 2019-10-03 17:33 ` Jordan Crouse 2019-10-03 17:33 ` Jordan Crouse 2019-10-24 10:51 ` Will Deacon 2019-10-24 10:51 ` Will Deacon 2019-10-24 11:23 ` Robin Murphy 2019-10-24 11:23 ` Robin Murphy 2019-10-24 11:40 ` Will Deacon 2019-10-24 11:40 ` Will Deacon 2019-08-20 16:23 ` Jordan Crouse 2019-08-20 16:23 ` Jordan Crouse 2019-08-19 18:19 ` [PATCH 4/4] iommu/io-pgtable-arm: Prepare for TTBR1 usage Robin Murphy 2019-08-19 18:19 ` Robin Murphy 2019-08-19 22:34 ` Jordan Crouse 2019-08-19 22:34 ` Jordan Crouse 2019-08-20 13:51 ` Robin Murphy 2019-08-20 13:51 ` Robin Murphy 2019-08-20 10:30 ` Will Deacon 2019-08-20 10:30 ` Will Deacon 2019-08-20 14:51 ` Robin Murphy 2019-08-20 14:51 ` Robin Murphy 2019-08-20 15:58 ` Will Deacon 2019-08-20 15:58 ` Will Deacon
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=8cc47f43-ad74-b4e2-e977-6c78780abc91@arm.com \ --to=robin.murphy@arm.com \ --cc=iommu@lists.linux-foundation.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.