Re: [PATCH] arm64: Introduce ISAR6 CPU ID register

From: Anshuman Khandual <anshuman.khandual@arm.com>
To: Mark Rutland <mark.rutland@arm.com>,
	Suzuki Kuruppassery Poulose <suzuki.poulose@arm.com>
Cc: linux-arm-kernel@lists.infradead.org,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>, Marc Zyngier <maz@kernel.org>,
	James Morse <james.morse@arm.com>,
	linux-kernel@vger.kernel.org, kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH] arm64: Introduce ISAR6 CPU ID register
Date: Wed, 18 Dec 2019 11:23:44 +0530	[thread overview]
Message-ID: <b815ef0b-4dee-fea1-a43a-182952035afb@arm.com> (raw)
In-Reply-To: <20191212163120.GH46910@lakrids.cambridge.arm.com>

On 12/12/2019 10:01 PM, Mark Rutland wrote:
> On Thu, Dec 12, 2019 at 03:22:13PM +0000, Suzuki Kuruppassery Poulose wrote:
>> On 12/12/2019 14:46, Mark Rutland wrote:
>>> On Thu, Dec 12, 2019 at 03:44:23PM +0530, Anshuman Khandual wrote:
>>>> +#define ID_ISAR6_JSCVT_SHIFT		0
>>>> +#define ID_ISAR6_DP_SHIFT		4
>>>> +#define ID_ISAR6_FHM_SHIFT		8
>>>> +#define ID_ISAR6_SB_SHIFT		12
>>>> +#define ID_ISAR6_SPECRES_SHIFT		16
>>>> +#define ID_ISAR6_BF16_SHIFT		20
>>>> +#define ID_ISAR6_I8MM_SHIFT		24
>>>
>>>> @@ -399,6 +399,7 @@ static const struct __ftr_reg_entry {
>>>>   	ARM64_FTR_REG(SYS_ID_ISAR4_EL1, ftr_generic_32bits),
>>>>   	ARM64_FTR_REG(SYS_ID_ISAR5_EL1, ftr_id_isar5),
>>>>   	ARM64_FTR_REG(SYS_ID_MMFR4_EL1, ftr_id_mmfr4),
>>>
>>>> +	ARM64_FTR_REG(SYS_ID_ISAR6_EL1, ftr_generic_32bits),
>>>
>>> Using ftr_generic_32bits exposes the lowest-common-denominator for all
>>> 4-bit fields in the register, and I don't think that's the right thing
>>> to do here, because:
>>>
>>> * We have no idea what ID_ISAR6 bits [31:28] may mean in future.
>>>
>>> * AFAICT, the instructions described by ID_ISAR6.SPECRES (from the
>>>    ARMv8.0-PredInv extension) operate on the local PE and are not
>>>    broadcast. To make those work as a guest expects, the host will need
>>>    to do additional things (e.g. to preserve that illusion when a vCPU is
>>>    migrated from one pCPU to another and back).
>>>
>>> Given that, think we should add an explicit ftr_id_isar6 which only
>>> exposes the fields that we're certain are safe to expose to a guest
>>> (i.e. without SPECRES).
>>
>> Agree. Thanks for pointing this out. I recommended the usage of
>> generic_32bits table without actually looking at the feature
>> definitions.
> 
> No worries; this is /really/ easy to miss!
> 
> Looking again, comparing to ARM DDI 0487E.a, there are a few other
> things we should probably sort out:
> 
> * ID_DFR0 fields need more thought; we should limit what we expose here.
>   I don't think it's valid for us to expose TraceFilt, and I suspect we

Sure, will go ahead and drop TraceFilt [28..31] from ID_DFR0 register.

>   need to add capping for debug features we currently emulate.

Could you please elaborate ?

> 
> * ID_ISAR0[31:28] are RES0 in ARMv8, Reserved/UNK in ARMv7.
>   We should probably ftr_id_isar0 so we can hide those bits.

Sure, will do.

> 
> * ID_ISAR5[23:10] are RES0
>   We handle this already! :)

I may be missing something here but some of these fields are already there.

#define ID_ISAR5_RDM_SHIFT              24
#define ID_ISAR5_CRC32_SHIFT            16
#define ID_ISAR5_SHA2_SHIFT             12
#define ID_ISAR5_SHA1_SHIFT             8
#define ID_ISAR5_AES_SHIFT              4
#define ID_ISAR5_SEVL_SHIFT             0

static const struct arm64_ftr_bits ftr_id_isar5[] = {
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR5_RDM_SHIFT, 4, 0),
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR5_CRC32_SHIFT, 4, 0),
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR5_SHA2_SHIFT, 4, 0),
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR5_SHA1_SHIFT, 4, 0),
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR5_AES_SHIFT, 4, 0),
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR5_SEVL_SHIFT, 4, 0),
        ARM64_FTR_END,
};

> 
> * ID_MMFR4.SpecSEI should be trated as higher safe.
>   We should update ftr_id_mmfr4 to handle this and other fields.

Sure but should we also export other fields as higher safe in there ?

> 
> * ID_PFR0 is missing DIT and CSV2
>   We should probably add these (but neither RAS not AMU).

Sure, will do.

> 
> * ID_PFR2 is missing
>   We should probably add this for SSBS and CSV3.

Sure but should we add corresponding ID_AA64PFR2_EL1 register as well ?

> 
> Thanks,
> Mark.
> 