| Message ID | 20231106185112.2755262-4-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Maintainer updates for 8.2 (gdbstub, tests, plugins) pre-PR | expand |
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. r~
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~
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,
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(-)