Re: [PATCH] arm64/io: Don't use WZR in writel

[Robin Murphy <robin.murphy@arm.com>]

On 03/05/2019 12:36, Marc Gonzalez wrote:
> On 02/05/2019 18:50, Marc Gonzalez wrote:
> 
>> Are you saying that when writing to any of
>>
>> 	gr0_base + ARM_SMMU_GR0_TLBIALLH
>> 	gr0_base + ARM_SMMU_GR0_TLBIALLNSNH
>> 	base + ARM_SMMU_GR0_sTLBGSYNC
>>
>> the actual value written does not matter? Is it ignored by the HW?
>>
>> We could write 0xdeadbeef?
> 
> 
> http://infocenter.arm.com/help/topic/com.arm.doc.ihi0062d.c/IHI0062D_c_system_mmu_architecture_specification.pdf
> 
>> 0x068	SMMU_TLBIALLNSNH	WO	32	TLB Invalidate All Non-secure Non-Hyp
>> 0x06C	SMMU_TLBIALLH		WO	32	TLB Invalidate All Hyp
>> 0x070	SMMU_sTLBGSYNC		WO	32	Global Synchronize TLB Invalidate
>>
>> The SMMU_TLBIALLNSNH	bit assignments are reserved.
>> The SMMU_TLBIALLH	bit assignments are reserved.
>> The SMMU_sTLBGSYNC	bit assignments are reserved.
>>
>>
>> Reserved:
>>
>> Unless otherwise stated in the architecture or product documentation, reserved:
>> o Instruction and 32-bit system control register encodings are UNPREDICTABLE.
>> o 64-bit system control register encodings are UNDEFINED.
>> o Register bit fields are UNK/SBZP.
>>
>>
>> UNK/SBZP:
>>
>> Hardware must implement the field as Read-As-Zero, and must ignore writes to the field.

That "Hardware [...] must ignore writes to the field" is the crux of the 
matter here.
>> Software must not rely on the field reading as all 0s, and except for writing back to the register
>> it must treat the value as if it is UNKNOWN. Software must use an SBZP policy to write to the field.

In practice, this is a forwards-compatibility provision - the 
architecture is effectively making a promise that if a 
previously-reserved field becomes meaningful in a future version, 0 will 
remain a "safe" value which does not enable any new and unexpected 
behaviour that current software won't understand.

In this case, however, there is zero chance of fields in these 
particular registers ever being defined, so I'm happy to take advantage 
of assumptions about hardware's end of the bargain. Note that even the 
architecture spec itself provides this example:

MOV	R0,#SMMU_CBn_TLBSYNC
STR	R0,[R0] ; Initiate TLB SYNC

>> This description can apply to a single bit that should be written as its preserved value or as 0,
>> or to a field that should be written as its preserved value or as all 0s.
>>
>> See also Read-As-Zero (RAZ), Should-Be-Zero-or-Preserved (SBZP), and UNKNOWN.
>>
>>
>> UNKNOWN:
>>
>> An UNKNOWN value does not contain valid data, and can vary from moment to moment,
>> instruction to instruction, and implementation to implementation.
>>
>> An UNKNOWN value must not be a security hole. An UNKNOWN value must not be documented
>> or promoted as having a defined value or effect.
>>
>> When UNKNOWN appears in body text, it is always in SMALL CAPITALS.
>>
>>
>> Should-Be-Zero-or-Preserved (SBZP)
>>
>>  From the introduction of ARMv7 Large Physical Address Extension, the definition of SBZP is modified
>> for register bits that are SBZP in some but not all contexts. The generic definition of SBZP given
>> here applies only to bits that are not affected by this modification.
>>
>> Hardware must ignore writes to the field.
>>
>> If software has read the field since the PE implementing the field was last reset and initialized,
>> it must preserve the value of the field by writing the value that it previously read from the field.
>> Otherwise, it must write the field as all 0s.
>>
>> If software writes a value to the field that is not a value previously read for the field and is
>> not all 0s, it must expect an UNPREDICTABLE result.
> 
> 
> Considering the above, instead of writing any random value, what do
> you think of writing back the current value, as in:

They're defined as write-only registers...

Even if the bits *within* a register nominally behave as RAZ/WI, I don't 
think that gives you any guarantee that the register itself will 
actually be readable (for starters, consider that the register as a 
whole does *not* ignore writes, because its fundamental reason for 
existing is to trigger an operation when written to).

Anyway, I'll clean up my patch and post it properly - thanks to you and 
Bjorn for testing.

Robin.

> 
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 930c07635956..fe27908d5295 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -417,13 +417,18 @@ static void __arm_smmu_free_bitmap(unsigned long *map, int idx)
>   	clear_bit(idx, map);
>   }
>   
> +static inline void sbzp_writel(void __iomem *reg)
> +{
> +	writel_relaxed(readl_relaxed(reg), reg);
> +}
> +
>   /* Wait for any pending TLB invalidations to complete */
>   static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu,
>   				void __iomem *sync, void __iomem *status)
>   {
>   	unsigned int spin_cnt, delay;
>   
> -	writel_relaxed(0, sync);
> +	sbzp_writel(sync);
>   	for (delay = 1; delay < TLB_LOOP_TIMEOUT; delay *= 2) {
>   		for (spin_cnt = TLB_SPIN_COUNT; spin_cnt > 0; spin_cnt--) {
>   			if (!(readl_relaxed(status) & sTLBGSTATUS_GSACTIVE))
> @@ -1761,8 +1766,8 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
>   	}
>   
>   	/* Invalidate the TLB, just in case */
> -	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLH);
> -	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_TLBIALLNSNH);
> +	sbzp_writel(gr0_base + ARM_SMMU_GR0_TLBIALLH);
> +	sbzp_writel(gr0_base + ARM_SMMU_GR0_TLBIALLNSNH);
>   
>   	reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
>   
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel