Re: [PATCH 03/22] target/arm: hide the 32bit version of PAR from gdbstub

[Alex Bennée]

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