On Sun, 8 May 2022 at 20:27, Chris Howard <cvz...@web.de> wrote: > Thanks for the explanation. What with this being “pretty easy” I’m attempting > my first ever patch! :-)
Thanks for having a go at this. You've got the right basic idea, but the process details of how to submit a patch aren't quite right. We document all that stuff here: https://www.qemu.org/docs/master/devel/submitting-a-patch.html It's a pretty long document, but we've tried to put the important stuff at the top. The really important bit is that patches must have a "Signed-off-by:" line, which is how you say "I wrote this code and it's OK for it to go into QEMU"; I can fix up most of the other minor stuff at my end but without that we can't take the patch at all. It's also best to send patches as new threads, not as replies to existing ones: the tooling (and some humans) assumes that. > This is a context diff with respect to the cloned git repository (Version > 7.0.50) Unified diffs are much easier to read than context ones. (If you create a proper git commit then "git show" and "git format-patch" and so on ought to default to unified.) > *** qemu/target/arm/helper.c 2022-05-08 20:41:48.000000000 +0200 > --- qemu-patch/target/arm/helper.c 2022-05-08 20:55:25.000000000 +0200 > *************** > *** 6551,6559 **** > define_one_arm_cp_reg(cpu, &dbgdidr); > } > > ! /* Note that all these register fields hold "number of Xs minus 1". */ > ! brps = arm_num_brps(cpu); > ! wrps = arm_num_wrps(cpu); > ctx_cmps = arm_num_ctx_cmps(cpu); > > assert(ctx_cmps <= brps); > --- 6551,6559 ---- > define_one_arm_cp_reg(cpu, &dbgdidr); > } > > ! /* Note that all these reg fields (in ID_AA64DFR0_EL1) hold "number of > Xs minus 1". */ > ! brps = arm_num_brps(cpu); /* returns actual number of > breakpoints */ > ! wrps = arm_num_wrps(cpu); /* returns actual number of > watchpoints */ > ctx_cmps = arm_num_ctx_cmps(cpu); > > assert(ctx_cmps <= brps); I agree that the comment here is wrong, but this is an unrelated change. We prefer to put separate changes in separate patches, even if they're in close areas of the code. I would just delete the existing comment line (as a separate patch) -- it's clear enough that arm_num_brps() returns the number of breakpoints, so we don't need to add extra comments saying so. (The old comment is an accidental leftover from before we defined those functions, when the code used to in-line extract values from the ID register fields. The definitions of arm_num_brps() etc in internals.h, where the ID-register reading now happens, already have comments remarking about the fields being "num bps - 1".) > *************** > *** 6565,6578 **** The rest of the change looks OK, assuming I'm reading the context-diff correctly. thanks -- PMM
