morello: print capabilities in a CPU trace #286
Conversation
target/arm/cpu.c
Outdated
| CAP_cc(get_perms)(capreg) & CAP_CC(PERM_STORE_CAP) ? "W" : "", | ||
| cap_get_base(capreg), | ||
| cap_get_top(capreg), | ||
| (!capreg->cr_tag || CAP_cc(is_cap_sealed)(capreg)) ? " (" : "", |
There was a problem hiding this comment.
You could maybe add a local variable for this expression:
bool have_attributes = (!capreg->cr_tag || CAP_cc(is_cap_sealed)(capreg));
It's also a shame you can't use some sort of string builder. If it is ok to make multiple calls to qemu_fprintf() I would suggest splitting up this logic using a bool comma (and perhaps not even needing the helper I suggested above), so something like:
bool comma;
...
comma = false;
#define PRINT_ATTR(name) do { \
qemu_fprintf(f, "%s%s", comma ? ", ", "("); \
comma = true; \
} while (0)
if (!capreg->cr_tag)
PRINT_ATTR("invalid");
if (CAP_cc(is_cap_sealed(capreg))
PRINT_ATTR(cap_get_otype_unsigned(capreg) == CAP_CC(OTYPE_SENTRY) ?
"sentry" : "sealed");
if (comma)
qemu_fprintf(f, ")");
#undef PRINT_ATTR
qemu_fprintf(f, "\n");
Can maybe simplify printing of permissions similarly by splitting up into multiple calls, but those don't have the same duplicated logic that the attribute handling code has.
There was a problem hiding this comment.
Thanks for encouraging to improve this!
I've changed this code not to use the CAP_cc() interface but the cap_*() API internal to QEMU.
Given that comma approach would need a conditional qemu_fprintf() call for ")" and that multiple qemu_fprintf() calls would depend on have_attributes, I've decided not to use the comma approach but use additional if statements instead.
Regarding the permissions, I think having a variable for permissions (perms) and CAP_* macros makes the qemu_fprintf() call simple enough.
What do you think?
kwitaszczyk
force-pushed
the
log-cpu-capabilities
branch
from
37fc13b to
485e9b3
Compare
Print capability registers in the same format as GDB does. Discussed with: @bsdjhb
kwitaszczyk
force-pushed
the
log-cpu-capabilities
branch
from
485e9b3 to
886204e
Compare
| qemu_fprintf(f, " ("); | ||
| if (!capreg->cr_tag) { | ||
| qemu_fprintf(f, "invalid"); | ||
| if (!cap_is_unsealed(capreg)) { |
There was a problem hiding this comment.
The comma approach is to avoid duplicate checks like this. Note that you check !capreg->cr_tag twice and !cap_is_unsealed() three times currently. If we ever added an additional attribute you would have to add checks for that attribute in all these places as well along with its own check. For only two effective attributes that's probably ok, but it doesn't really scale well if we added more in the future. The comma approach is more about the readability/future maintainability as it only requires checking attribute condition once, and adding a new attribute in the future only requires adding code for that attribute without having to update the various other attribute handlers. It is also a bit unusual to conditionally add a trailing comma vs conditionally adding a leading comma (or pipe when doing bitmasks). You also still end up with a call to qemu_fprintf() for the closing ) character still. This is probably fine, but I would probably rework it if we ever had to change it.
Print capability registers in the same format as GDB does.
Example output: