From: Marc Gonzalez <marc.w.gonzalez@free.fr>
To: Will Deacon <will.deacon@arm.com>
Cc: Robin Murphy <robin.murphy@arm.com>,
Joerg Roedel <joro@8bytes.org>,
MSM <linux-arm-msm@vger.kernel.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
iommu <iommu@lists.linux-foundation.org>,
AngeloGioacchino Del Regno <kholk11@gmail.com>,
Jeffrey Hugo <jeffrey.l.hugo@gmail.com>,
Andy Gross <agross@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>
Subject: Re: [PATCH v2] iommu/arm-smmu: Avoid constant zero in TLBI writes
Date: Wed, 29 May 2019 16:31:06 +0200 [thread overview]
Message-ID: <84791515-e0ae-0322-78aa-02ca0b40d157@free.fr> (raw)
In-Reply-To: <20190529130559.GB11023@fuggles.cambridge.arm.com>
On 29/05/2019 15:05, Will Deacon wrote:
> On Wed, May 29, 2019 at 01:55:48PM +0200, Marc Gonzalez wrote:
>
>> From: Robin Murphy <robin.murphy@arm.com>
>>
>> Apparently, some Qualcomm arm64 platforms which appear to expose their
>> SMMU global register space are still, in fact, using a hypervisor to
>> mediate it by trapping and emulating register accesses. Sadly, some
>> deployed versions of said trapping code have bugs wherein they go
>> horribly wrong for stores using r31 (i.e. XZR/WZR) as the source
>> register.
>
> ^^^
> This should be in the comment instead of "qcom bug".
As you wish. I wasn't sure how much was too much.
>> While this can be mitigated for GCC today by tweaking the constraints
>> for the implementation of writel_relaxed(), to avoid any potential
>> arms race with future compilers more aggressively optimising register
>> allocation, the simple way is to just remove all the problematic
>> constant zeros. For the write-only TLB operations, the actual value is
>> irrelevant anyway and any old nearby variable will provide a suitable
>> GPR to encode. The one point at which we really do need a zero to clear
>> a context bank happens before any of the TLB maintenance where crashes
>> have been reported, so is apparently not a problem... :/
>
> Hmm. It would be nice to understand this a little better. In which cases
> does XZR appear to work?
There are 4 occurrences of writel_relaxed(0 in the driver.
The following do not crash. Perhaps they run natively from NS EL1.
[ SMMU + 008000] = 00000000
[ SMMU + 009000] = 00000000
[ SMMU + 00a000] = 00000000
[ SMMU + 00b000] = 00000000
[ SMMU + 00c000] = 00000000
[ SMMU + 00d000] = 00000000
The following do crash. They trap to some evil place.
[ SMMU + 00006c] = 00000000
[ SMMU + 000068] = 00000000
[ SMMU + 000070] = 11190070
NB: with Robin's patch, we end up writing 0 anyway.
It would be "fun" if the emulation puked at !0
Unlikely since it worked for +70
> Any reason not to make these obviously dummy values e.g.:
>
> /*
> * Text from the commit message about broken hypervisor
> */
> #define QCOM_DUMMY_VAL_NOT_XZR ~0U
>
> That makes the callsites much easier to understand and I doubt there's a
> performance impact from allocating an extra register here.
Robin, what sayeth thee? Should I spin a v3?
Regards.
WARNING: multiple messages have this Message-ID (diff)
From: Marc Gonzalez <marc.w.gonzalez@free.fr>
To: Will Deacon <will.deacon@arm.com>
Cc: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>,
MSM <linux-arm-msm@vger.kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
iommu <iommu@lists.linux-foundation.org>,
Andy Gross <agross@kernel.org>,
AngeloGioacchino Del Regno <kholk11@gmail.com>,
Robin Murphy <robin.murphy@arm.com>,
Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2] iommu/arm-smmu: Avoid constant zero in TLBI writes
Date: Wed, 29 May 2019 16:31:06 +0200 [thread overview]
Message-ID: <84791515-e0ae-0322-78aa-02ca0b40d157@free.fr> (raw)
In-Reply-To: <20190529130559.GB11023@fuggles.cambridge.arm.com>
On 29/05/2019 15:05, Will Deacon wrote:
> On Wed, May 29, 2019 at 01:55:48PM +0200, Marc Gonzalez wrote:
>
>> From: Robin Murphy <robin.murphy@arm.com>
>>
>> Apparently, some Qualcomm arm64 platforms which appear to expose their
>> SMMU global register space are still, in fact, using a hypervisor to
>> mediate it by trapping and emulating register accesses. Sadly, some
>> deployed versions of said trapping code have bugs wherein they go
>> horribly wrong for stores using r31 (i.e. XZR/WZR) as the source
>> register.
>
> ^^^
> This should be in the comment instead of "qcom bug".
As you wish. I wasn't sure how much was too much.
>> While this can be mitigated for GCC today by tweaking the constraints
>> for the implementation of writel_relaxed(), to avoid any potential
>> arms race with future compilers more aggressively optimising register
>> allocation, the simple way is to just remove all the problematic
>> constant zeros. For the write-only TLB operations, the actual value is
>> irrelevant anyway and any old nearby variable will provide a suitable
>> GPR to encode. The one point at which we really do need a zero to clear
>> a context bank happens before any of the TLB maintenance where crashes
>> have been reported, so is apparently not a problem... :/
>
> Hmm. It would be nice to understand this a little better. In which cases
> does XZR appear to work?
There are 4 occurrences of writel_relaxed(0 in the driver.
The following do not crash. Perhaps they run natively from NS EL1.
[ SMMU + 008000] = 00000000
[ SMMU + 009000] = 00000000
[ SMMU + 00a000] = 00000000
[ SMMU + 00b000] = 00000000
[ SMMU + 00c000] = 00000000
[ SMMU + 00d000] = 00000000
The following do crash. They trap to some evil place.
[ SMMU + 00006c] = 00000000
[ SMMU + 000068] = 00000000
[ SMMU + 000070] = 11190070
NB: with Robin's patch, we end up writing 0 anyway.
It would be "fun" if the emulation puked at !0
Unlikely since it worked for +70
> Any reason not to make these obviously dummy values e.g.:
>
> /*
> * Text from the commit message about broken hypervisor
> */
> #define QCOM_DUMMY_VAL_NOT_XZR ~0U
>
> That makes the callsites much easier to understand and I doubt there's a
> performance impact from allocating an extra register here.
Robin, what sayeth thee? Should I spin a v3?
Regards.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
WARNING: multiple messages have this Message-ID (diff)
From: Marc Gonzalez <marc.w.gonzalez@free.fr>
To: Will Deacon <will.deacon@arm.com>
Cc: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>,
MSM <linux-arm-msm@vger.kernel.org>,
Joerg Roedel <joro@8bytes.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
iommu <iommu@lists.linux-foundation.org>,
Andy Gross <agross@kernel.org>,
AngeloGioacchino Del Regno <kholk11@gmail.com>,
Robin Murphy <robin.murphy@arm.com>,
Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2] iommu/arm-smmu: Avoid constant zero in TLBI writes
Date: Wed, 29 May 2019 16:31:06 +0200 [thread overview]
Message-ID: <84791515-e0ae-0322-78aa-02ca0b40d157@free.fr> (raw)
In-Reply-To: <20190529130559.GB11023@fuggles.cambridge.arm.com>
On 29/05/2019 15:05, Will Deacon wrote:
> On Wed, May 29, 2019 at 01:55:48PM +0200, Marc Gonzalez wrote:
>
>> From: Robin Murphy <robin.murphy@arm.com>
>>
>> Apparently, some Qualcomm arm64 platforms which appear to expose their
>> SMMU global register space are still, in fact, using a hypervisor to
>> mediate it by trapping and emulating register accesses. Sadly, some
>> deployed versions of said trapping code have bugs wherein they go
>> horribly wrong for stores using r31 (i.e. XZR/WZR) as the source
>> register.
>
> ^^^
> This should be in the comment instead of "qcom bug".
As you wish. I wasn't sure how much was too much.
>> While this can be mitigated for GCC today by tweaking the constraints
>> for the implementation of writel_relaxed(), to avoid any potential
>> arms race with future compilers more aggressively optimising register
>> allocation, the simple way is to just remove all the problematic
>> constant zeros. For the write-only TLB operations, the actual value is
>> irrelevant anyway and any old nearby variable will provide a suitable
>> GPR to encode. The one point at which we really do need a zero to clear
>> a context bank happens before any of the TLB maintenance where crashes
>> have been reported, so is apparently not a problem... :/
>
> Hmm. It would be nice to understand this a little better. In which cases
> does XZR appear to work?
There are 4 occurrences of writel_relaxed(0 in the driver.
The following do not crash. Perhaps they run natively from NS EL1.
[ SMMU + 008000] = 00000000
[ SMMU + 009000] = 00000000
[ SMMU + 00a000] = 00000000
[ SMMU + 00b000] = 00000000
[ SMMU + 00c000] = 00000000
[ SMMU + 00d000] = 00000000
The following do crash. They trap to some evil place.
[ SMMU + 00006c] = 00000000
[ SMMU + 000068] = 00000000
[ SMMU + 000070] = 11190070
NB: with Robin's patch, we end up writing 0 anyway.
It would be "fun" if the emulation puked at !0
Unlikely since it worked for +70
> Any reason not to make these obviously dummy values e.g.:
>
> /*
> * Text from the commit message about broken hypervisor
> */
> #define QCOM_DUMMY_VAL_NOT_XZR ~0U
>
> That makes the callsites much easier to understand and I doubt there's a
> performance impact from allocating an extra register here.
Robin, what sayeth thee? Should I spin a v3?
Regards.
_______________________________________________
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-05-29 14:31 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-29 11:55 [PATCH v2] iommu/arm-smmu: Avoid constant zero in TLBI writes Marc Gonzalez
2019-05-29 11:55 ` Marc Gonzalez
2019-05-29 11:55 ` Marc Gonzalez
2019-05-29 13:05 ` Will Deacon
2019-05-29 13:05 ` Will Deacon
2019-05-29 13:05 ` Will Deacon
2019-05-29 14:31 ` Marc Gonzalez [this message]
2019-05-29 14:31 ` Marc Gonzalez
2019-05-29 14:31 ` Marc Gonzalez
2019-06-03 12:15 ` [PATCH v3] " Marc Gonzalez
2019-06-03 12:15 ` Marc Gonzalez
2019-06-03 12:15 ` Marc Gonzalez
2019-06-05 12:19 ` Will Deacon
2019-06-05 12:19 ` Will Deacon
2019-06-05 12:19 ` Will Deacon
2019-06-07 10:40 ` Marc Gonzalez
2019-06-07 10:40 ` Marc Gonzalez
2019-06-07 10:40 ` Marc Gonzalez
2019-06-12 8:10 ` Joerg Roedel
2019-06-12 8:10 ` Joerg Roedel
2019-06-12 8:10 ` Joerg Roedel
2019-06-14 11:24 ` Marc Gonzalez
2019-06-14 11:24 ` Marc Gonzalez
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=84791515-e0ae-0322-78aa-02ca0b40d157@free.fr \
--to=marc.w.gonzalez@free.fr \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=iommu@lists.linux-foundation.org \
--cc=jeffrey.l.hugo@gmail.com \
--cc=joro@8bytes.org \
--cc=kholk11@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=robin.murphy@arm.com \
--cc=will.deacon@arm.com \
/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: link
Be 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.