Re: [Qemu-arm] [Qemu-devel] [PATCH v2 0/6] HWCAP_CPUID registers for aarch64

[Peter Maydell]

On Tue, 5 Feb 2019 at 20:21, Alex Bennée <address@hidden> wrote:
>
> Hi,
>
> I've re-spun the cpuid patches with the changes suggested by Peter's
> review. The biggest change is the squashing of bits is now all data
> driven with ARMCPRegUserSpaceInfo being used to control how bits are
> altered for userspace presentation. This includes using glob matching
> to set whole bunches to RAZ.
>
> The testcase isn't as comprehensive as it could be because you need a
> fairly new compiler (binutils) to emit all the various system register
> id's to test. I did look into upgrading debian-arm64-cross with Buster
> but I managed to find a bug in Debian's dependencies which rules out
> upgrading for now.
>
> checkpatch is complaining about the _m macro I used to group together
> words in the masks I defined. I'm not sure adding the spaces makes it
> as readable though.
>
> The following patches need review:
>  patch 0001/target arm relax permission checks for HWCAP_CPUI.patch
>  patch 0002/target arm expose CPUID registers to userspace.patch
>  patch 0003/target arm expose MPIDR_EL1 to userspace.patch
>  patch 0004/target arm expose remaining CPUID registers as RA.patch
>  patch 0006/tests tcg aarch64 userspace system register test.patch

I'll take 1-5 into target-arm.next, but 6 breaks 'make check-tcg'
for me:

  BUILD   aarch64 guest-tests with aarch64-linux-gnu-gcc
cc1: error: invalid feature modifier in ‘-march=armv8.1-a+sve’

I imagine you're assuming a newer binutils than Ubuntu ships.
You should probably go for just encoding the insns rather than using
their names -- binutils should accept "S<op0>_<op1>_<Cn>_<Cm>_<op2>"
as a register name for mrs and msr insn.

It would also be nice to test some of the sysreg cases which
go through your "glob the register name" code path -- I think
at the moment you are only testing the ones which use the
specific-match case ? (Ideally we'd loop through and test that
we could read everything in the defined-to-be-exposed encoding
range, though that would require some slightly tedious preprocessor
macro effort.)

thanks
-- PMM