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


Alex Bennée (6):
  target/arm: relax permission checks for HWCAP_CPUID registers
  target/arm: expose CPUID registers to userspace
  target/arm: expose MPIDR_EL1 to userspace
  target/arm: expose remaining CPUID registers as RAZ
  linux-user/elfload: enable HWCAP_CPUID for AArch64
  tests/tcg/aarch64: userspace system register test

 linux-user/elfload.c              |   1 +
 target/arm/cpu.h                  |  36 +++++++
 target/arm/helper.c               | 106 ++++++++++++++++--
 tests/tcg/aarch64/Makefile.target |   4 +-
 tests/tcg/aarch64/sysregs.c       | 172 ++++++++++++++++++++++++++++++
 5 files changed, 310 insertions(+), 9 deletions(-)
 create mode 100644 tests/tcg/aarch64/sysregs.c

-- 
2.20.1

Alex Bennée <alex.bennee@linaro.org> writes:

> 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.

Ping (+ CC'd which I missed on the submission)

>
> 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.

We have a newer compiler so I can add more on the next re-spin, reviews
pending.

>
> 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
>
>
> Alex Bennée (6):
>   target/arm: relax permission checks for HWCAP_CPUID registers
>   target/arm: expose CPUID registers to userspace
>   target/arm: expose MPIDR_EL1 to userspace
>   target/arm: expose remaining CPUID registers as RAZ
>   linux-user/elfload: enable HWCAP_CPUID for AArch64
>   tests/tcg/aarch64: userspace system register test
>
>  linux-user/elfload.c              |   1 +
>  target/arm/cpu.h                  |  36 +++++++
>  target/arm/helper.c               | 106 ++++++++++++++++--
>  tests/tcg/aarch64/Makefile.target |   4 +-
>  tests/tcg/aarch64/sysregs.c       | 172 ++++++++++++++++++++++++++++++
>  5 files changed, 310 insertions(+), 9 deletions(-)
>  create mode 100644 tests/tcg/aarch64/sysregs.c


--
Alex Bennée

On Tue, 5 Feb 2019 at 20:21, Alex Bennée <alex.bennee@linaro.org> 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

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 5 Feb 2019 at 20:21, Alex Bennée <alex.bennee@linaro.org> 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.


Yeah I'm now on buster so have a newer gcc:

  aarch64-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0

But I'm surprised that the pauth test cases worked on your system.
However they should all work with the recent docker update which you
merged earlier this week.

> 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.)

Yeah the main reason I didn't push to hard to do it but I guess we
should for completeness sake. It's times like this I wish C's macros
weren't the pale imitations of what you can do with lisp (or rust ;-).

--
Alex Bennée