Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1558210

Re: [PATCH 1/1] iommu/arm-smmu: Fix for ThunderX erratum #27704

From Robin Murphy <robin.murphy@arm.com>
Newsgroups linux.kernel
Subject Re: [PATCH 1/1] iommu/arm-smmu: Fix for ThunderX erratum #27704
Date 2017-01-13 12:00 +0100
Message-ID <sZ7Kv-8gB-11@gated-at.bofh.it> (permalink)
References <sYpJn-6v8-13@gated-at.bofh.it> <sYq2K-6QF-19@gated-at.bofh.it> <sYHmV-FU-1@gated-at.bofh.it> <sZ7Kv-8gB-13@gated-at.bofh.it>
Organization linux.* mail to news gateway

Show all headers | View raw


On 13/01/17 10:43, Tomasz Nowicki wrote:
> On 12.01.2017 07:41, Tomasz Nowicki wrote:
>> On 11.01.2017 13:19, Robin Murphy wrote:
>>> On 11/01/17 11:51, Tomasz Nowicki wrote:
>>>> The goal of erratum #27704 workaround was to make sure that ASIDs and
>>>> VMIDs
>>>> are unique across all SMMU instances on affected Cavium systems.
>>>>
>>>> Currently, the workaround code partitions ASIDs and VMIDs by increasing
>>>> global cavium_smmu_context_count which in turn becomes the base ASID
>>>> and VMID
>>>> value for the given SMMU instance upon the context bank initialization.
>>>>
>>>> For systems with multiple SMMU instances this approach implies the risk
>>>> of crossing 8-bit ASID, like for CN88xx capable of 4 SMMUv2, 128
>>>> context bank each:
>>>> SMMU_0 (0-127 ASID RANGE)
>>>> SMMU_1 (127-255 ASID RANGE)
>>>> SMMU_2 (256-383 ASID RANGE) <--- crossing 8-bit ASID
>>>> SMMU_3 (384-511 ASID RANGE) <--- crossing 8-bit ASID
>>>
>>> I could swear that at some point in the original discussion it was said
>>> that the TLBs were only shared between pairs of SMMUs, so in fact 0/1
>>> and 2/3 are independent of each other
>>
>> Indeed TLBs are only shared between pairs of SMMUs but the workaround
>> makes sure ASIDs are unique across all SMMU instances so we do not have
>> to bother about SMMUs probe order.
>>
>>  - out of interest, have you
>>> managed to hit an actual problem in practice or is this patch just by
>>> inspection?
>>
>> Except SMMU0/1 devices all other devices under other SMMUs will fail on
>> guest power off/on. Since we try to invalidate tlb with 16bit ASID but
>> we actually have 8 bit zero padded 16 bit entry.
>>
>>>
>>> Of course, depending on the SMMUs to probe in the right order isn't
>>> particularly robust, so it's still probably a worthwhile change.
>>>
>>>> Since we use 8-bit ASID now we effectively misconfigure ASID[15:8]
>>>> bits for
>>>> SMMU_CBn_TTBRm register. Also, we still use non-zero ASID[15:8] bits
>>>> upon context invalidation. This patch adds 16-bit ASID support for
>>>> stage-1
>>>> AArch64 contexts for Cavium SMMUv2 model so that we use ASIDs
>>>> consistently.
>>>>
>>>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>>>> ---
>>>>  drivers/iommu/arm-smmu.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>>> index a60cded..ae8f059 100644
>>>> --- a/drivers/iommu/arm-smmu.c
>>>> +++ b/drivers/iommu/arm-smmu.c
>>>> @@ -260,6 +260,7 @@ enum arm_smmu_s2cr_privcfg {
>>>>
>>>>  #define TTBCR2_SEP_SHIFT        15
>>>>  #define TTBCR2_SEP_UPSTREAM        (0x7 << TTBCR2_SEP_SHIFT)
>>>> +#define TTBCR2_AS            (1 << 4)
>>>>
>>>>  #define TTBRn_ASID_SHIFT        48
>>>>
>>>> @@ -778,6 +779,9 @@ static void arm_smmu_init_context_bank(struct
>>>> arm_smmu_domain *smmu_domain,
>>>>              reg = pgtbl_cfg->arm_lpae_s1_cfg.tcr;
>>>>              reg2 = pgtbl_cfg->arm_lpae_s1_cfg.tcr >> 32;
>>>>              reg2 |= TTBCR2_SEP_UPSTREAM;
>>>> +            if (smmu->model == CAVIUM_SMMUV2 &&
>>>
>>> I'd be inclined to say "smmu->version == ARM_SMMU_V2" there, rather than
>>> make it Cavium-specific - we enable 16-bit VMID unconditionally where
>>> supported, so I don't see any reason not to handle 16-bit ASIDs in the
>>> same manner.
>>
>> I agree, I will enable 16-bit ASID for ARM_SMMU_V2.
>>
> 
> Actually, the ARM_SMMU_CTX_FMT_AARCH64 context check is all we need here:
> 
> +            if (cfg->fmt == ARM_SMMU_CTX_FMT_AARCH64)
> +                reg2 |= TTBCR2_AS;

Ah, clever! The horrible SMMUv1 64KB supplement supports AArch64
contexts without being SMMUv2, but of course doesn't have stage 1 :)

Robin.

> 
> Thanks,
> Tomasz

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread


Thread

[PATCH 1/1] iommu/arm-smmu: Fix for ThunderX erratum #27704 Tomasz Nowicki <tn@semihalf.com> - 2017-01-11 13:00 +0100
  Re: [PATCH 1/1] iommu/arm-smmu: Fix for ThunderX erratum #27704 Robin Murphy <robin.murphy@arm.com> - 2017-01-11 13:20 +0100
    Re: [PATCH 1/1] iommu/arm-smmu: Fix for ThunderX erratum #27704 Tomasz Nowicki <tn@semihalf.com> - 2017-01-12 07:50 +0100
      Re: [PATCH 1/1] iommu/arm-smmu: Fix for ThunderX erratum #27704 Robin Murphy <robin.murphy@arm.com> - 2017-01-13 12:00 +0100
      Re: [PATCH 1/1] iommu/arm-smmu: Fix for ThunderX erratum #27704 Tomasz Nowicki <tn@semihalf.com> - 2017-01-13 12:00 +0100

csiph-web