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

[Alex Bennée]

Peter Maydell <address@hidden> writes:

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


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