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.
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
