On Tue, Nov 03, 2020 at 06:14:53PM +0000, Nikos Nikoleris wrote:
> On 03/11/2020 17:36, Andrew Jones wrote:
> > On Tue, Nov 03, 2020 at 05:03:15PM +0000, Alexandru Elisei wrote:
> > > > +}
> > > > +
> > > > +static inline bool system_supports_granule(size_t granule)
> > > > +{
> > > > + u64 mmfr0 = get_id_aa64mmfr0_el1();
> > > > +
> > > > + return ((granule == SZ_4K && ((mmfr0 >> 28) & 0xf) == 0) ||
> > > > + (granule == SZ_64K && ((mmfr0 >> 24) & 0xf) == 0) ||
> > > > + (granule == SZ_16K && ((mmfr0 >> 20) & 0xf) == 1));
> > > > +}
> > >
> > > Or we can turn it into a switch statement and keep all the field defines. Either
> > > way looks good to me (funny how tgran16 stands out).
> > >
> >
> > Keeping the defines is probably a good idea. Whether the function uses
> > a switch or an expression like above doesn't matter to me much. Keeping
> > LOC down in the lib/ code is a goal of kvm-unit-tests, but so is
> > readabilty. If the switch looks better, then let's go that way.
> >
>
> I liked Drew's version in that it was very concise. The new version will be
> much longer. If you think it's more readable I'll use that instead.
>
> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
> index 02665b8..430ded3 100644
> --- a/lib/arm64/asm/processor.h
> +++ b/lib/arm64/asm/processor.h
> @@ -117,5 +117,38 @@ static inline u64 get_ctr(void)
>
> extern u32 dcache_line_size;
>
> +static inline unsigned long get_id_aa64mmfr0_el1(void)
> +{
> + return read_sysreg(id_aa64mmfr0_el1);
> +}
> +
> +#define ID_AA64MMFR0_TGRAN4_SHIFT 28
> +#define ID_AA64MMFR0_TGRAN64_SHIFT 24
> +#define ID_AA64MMFR0_TGRAN16_SHIFT 20
> +
> +#define ID_AA64MMFR0_TGRAN4_SUPPORTED 0x0
> +#define ID_AA64MMFR0_TGRAN64_SUPPORTED 0x0
> +#define ID_AA64MMFR0_TGRAN16_SUPPORTED 0x1
> +
> +static inline bool system_supports_granule(size_t granule)
> +{
> + u32 shift;
> + u32 val;
> + u64 mmfr0 = get_id_aa64mmfr0_el1();
> + if (granule == SZ_4K) {
> + shift = ID_AA64MMFR0_TGRAN4_SHIFT;
> + val = ID_AA64MMFR0_TGRAN4_SUPPORTED;
> + } else if (granule == SZ_16K) {
> + shift = ID_AA64MMFR0_TGRAN16_SHIFT;
> + val = ID_AA64MMFR0_TGRAN16_SUPPORTED;
> + } else {
> + assert(granule == SZ_64K);
> + shift = ID_AA64MMFR0_TGRAN64_SHIFT;
> + val = ID_AA64MMFR0_TGRAN64_SUPPORTED;
> + }
> +
> + return ((mmfr0 >> shift) & 0xf) == val;
> +}
> +
> #endif /* !__ASSEMBLY__ */
> #endif /* _ASMARM64_PROCESSOR_H_ */
>
I'm happy with it either way.
Thanks,
drew
