Richard Henderson <richard.henderson@linaro.org> writes:
> On 11/6/23 10:50, Alex Bennée wrote:
>> This is a slightly hacky way to avoid duplicate PAR's in the system
>> register XML we send to gdb which causes an alias. However the other
>> alternative would be to post process ARMCPRegInfo once all registers
>> have been defined looking for textual duplicates. And that seems like
>> overkill.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-Id: <20231103195956.1998255-4-alex.bennee@linaro.org>
>> ---
>> target/arm/helper.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 5dc0d20a84..104f9378b4 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -3727,7 +3727,7 @@ static const ARMCPRegInfo vapa_cp_reginfo[] = {
>> .access = PL1_RW, .resetvalue = 0,
>> .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.par_s),
>> offsetoflow32(CPUARMState, cp15.par_ns) },
>> - .writefn = par_write },
>> + .writefn = par_write, .type = ARM_CP_NO_GDB },
>> #ifndef CONFIG_USER_ONLY
>> /* This underdecoding is safe because the reginfo is NO_RAW. */
>> { .name = "ATS", .cp = 15, .crn = 7, .crm = 8, .opc1 = 0, .opc2 = CP_ANY,
>
> If the implementation includes LPAE, this is an alias of the full
> 64-bit register (the "other PAR", and so type should contain
> ARM_CP_ALIAS.
>
> If the implementation does not include LPAE, this should not be
> ARM_CP_NO_GDB because there is no 64-bit alternate.
I did discuss with Peter on IRC but its a lot of hoop jumping for very
little benefit. But I guess I can add some logic to do that.
[17:58:51] <pm215> no, those are different registers. the PAR in lpae_cp_reginfo is ARM_CP_64BIT, and the one in vapa_cp_reginfo is not
[17:59:51] > ah yes slightly different opcs
[17:59:54] > { .name = "PAR", .cp = 15, .crm = 7, .opc1 = 0,
[18:00:07] > vs
[18:00:09] > { .name = "PAR", .cp = 15, .crn = 7, .crm = 4, .opc1 = 0, .opc2 = 0,
[18:00:18] <pm215> the important bit is the ARM_CP_64BIT
[18:00:20] > pm215 should they have differnt names then?
[18:00:39] <pm215> historically we have said that the .name field is purely for debug purposes and not worried too much about overlap
[18:01:27] <pm215> architecturally these are considered two different access methods for getting at the same PAR register
[18:01:54] <pm215> (unlike the AArch64 PAR_EL1, which is considered a separate register whose bits are 'architecturally mapped' to the AArch32 PAR)
[18:02:51] > PAR_A32 and PAR_A64?
[18:03:19] <pm215> no
[18:03:20] > afterall these are extra registers we report to gdb so we can control the naming
[18:03:24] <pm215> the AArch64 register is PAR_EL1
[18:03:43] > or so PAR and PAR_EL1
[18:03:58] <pm215> the AArch32 register is PAR, and there are two different ways to read it, with the 32-bit MRC/MCR insns, or with the 64-bit MCRR/MRRC insns
[18:04:23] > oh we define PAR_EL1 as well
[18:04:26] > { .name = "PAR_EL1", .state = ARM_CP_STATE_AA64,
[18:04:26] > .type = ARM_CP_ALIAS,
[18:04:26] > .opc0 = 3, .opc1 = 0, .crn = 7, .crm = 4, .opc2 = 0,
[18:04:29] <pm215> yep
[18:05:08] > PAR, PAR_A64 and PAR_El1?
[18:05:20] <pm215> PAR_A64 implies AArch64, which it is not
[18:05:34] > PAR_64?
[18:05:49] <pm215> what are you trying to achieve here? that every cpreg has a distinct name string?
[18:06:22] > pm215 yes - as we expose them to gdb and hence are over writing one definition with the other
[18:06:40] <pm215> the correct way to expose them to the user would be to only present one PAR, which is 64 bits
[18:06:56] <pm215> i.e. if the 64-bit version is present, ignore the 32-bit version
[18:07:09] * stsquad2 has also fixed a duplicate q10 in arm-neon.xml
[18:07:39] <pm215> duplicate q10> I wonder if that's in gdb's copy too?
[18:09:14] > this is qemu-arm so
[18:09:36] <pm215> yes, qemu-arm will have both the 32-bit and 64-bit PAR
[18:09:40] <pm215> (depending on cpu type)
[18:10:20] > *sigh*
[18:10:33] > I suspect this will be messy to fix...
[18:11:02] <pm215> the gdb exposure of sysregs was always a bit of a "this basically works" hack
[18:11:36] <pm215> ideally for an AArch64 vcpu we would expose them with the correct-for-AArch64 names, but we don't in many cases where the regdef is shared with AArch32
[18:12:24] > so I could split vapa_cp_reginfo into two parts and do a check on if the 64 bit feature exits before adding it?
[18:13:00] <pm215> that won't really help you, because on a CPU with FEAT_LPAE we need both regdefs
[18:13:42] * stsquad2 does not want to add a .dont_expose_to_gdb field to ARMCPRegInfo
[18:14:21] <pm215> stsquad2: we already have ARM_CP_NO_GDB :-)
[18:17:19] > pm215 we have logic that applies r2->type |= ARM_CP_ALIAS | ARM_CP_NO_GDB;
[18:19:40] <pm215> stsquad2: yeah, that's for the CP_ANY wildcarding. But there's no inherent reason you couldn't mark a regdef with ARM_CP_NO_GDB by hand
[18:19:51] > pm215 looking at the commit that added it: "This bit could be enabled manually for any register we want to remove from the
[18:19:51] > dynamic XML description."
[18:20:32] <pm215> (ARM_CP_NO_RAW registers also don't get exposed to gdb)
[18:20:49] > pm215 so I'll just add it to the 32 bit register and wait for the 1 user in the future to complain they can't see it out of the 100s of sysregs we expose
[18:21:28] <pm215> that breaks reading PAR from gdbstub for any pre-v7VE CPU
[18:22:05] > pm215 we can argue the case on the review ;-)
[18:22:33] > in the current case its failing a linux-user test which is explicitly -cpu max anyway
[18:30:52] * stsquad2 looks at DBGDSAR next
>
>
> r~
--
Alex Bennée
Virtualisation Tech Lead @ Linaro