Hi Vinayak,
Thanks for the patch.
Some queries below:
On Thu, 25 Mar 2021 at 20:08, Vinayak Menon <vinayakm.list@xxxxxxxxx> wrote:
>
> Raw ramdumps without vmcoreinfo does not work currently
> with pointer authentication or memory tagging enabled.
> The arm capability array can be queried but that creates
> a dependency on the bits identifying the capabilities.
> Add a machdep option instead to ignore the tags when any
> feature using the tag bits is enabled.
Hmm.. I am still trying to understand why we need to ignore the tags.
Can you please explain the issue at hand and also share the steps you
used to reproduce it.
I have an arm64 hardware with memory tagging support available, I can
try to reproduce it at my end.
I suspect we need to enable reading capabilities instead. Do you
foresee any issues with the same?
> Signed-off-by: Vinayak Menon <vinayakm.list@xxxxxxxxx>
> ---
> arm64.c | 26 +++++++++++++++++++-------
> crash.8 | 1 +
> defs.h | 1 +
> help.c | 1 +
> 4 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/arm64.c b/arm64.c
> index 37aed07..3ed6795 100644
> --- a/arm64.c
> +++ b/arm64.c
> @@ -745,7 +745,8 @@ arm64_parse_machdep_arg_l(char *argstring, char *param, ulong *value)
> char *p;
>
> len = strlen(param);
> - if (!STRNEQ(argstring, param) || (argstring[len] != '='))
> + if (!STRNEQ(argstring, param) || (!STRNEQ(argstring, "tag_ignore") &&
> + (argstring[len] != '=')))
> return FALSE;
>
> if ((LASTCHAR(argstring) == 'm') ||
> @@ -763,6 +764,8 @@ arm64_parse_machdep_arg_l(char *argstring, char *param, ulong *value)
> *value = dtol(p, flags, &err);
> } else if (STRNEQ(argstring, "vabits_actual")) {
> *value = dtol(p, flags, &err);
> + } else if (STRNEQ(argstring, "tag_ignore")) {
> + *value = 1;
> } else if (megabytes) {
> *value = dtol(p, flags, &err);
> if (!err)
> @@ -797,12 +800,6 @@ arm64_parse_cmdline_args(void)
> if (!machdep->cmdline_args[index])
> break;
>
> - if (!strstr(machdep->cmdline_args[index], "=")) {
> - error(WARNING, "ignoring --machdep option: %x\n",
> - machdep->cmdline_args[index]);
> - continue;
> - }
> -
> strcpy(buf, machdep->cmdline_args[index]);
>
> for (p = buf; *p; p++) {
> @@ -838,6 +835,11 @@ arm64_parse_cmdline_args(void)
> "setting vabits_actual to: %ld\n\n",
> machdep->machspec->VA_BITS_ACTUAL);
> continue;
> + } else if (arm64_parse_machdep_arg_l(arglist[i], "tag_ignore",
> + &machdep->machspec->tag_ignore)) {
> + error(NOTE,
> + "setting tag_ignore\n\n");
> + continue;
> }
>
> error(WARNING, "ignoring --machdep option: %s\n",
> @@ -4122,6 +4124,9 @@ arm64_swp_offset(ulong pte)
> return pte;
> }
>
> +#define __GENMASK_ULL(h, l) \
> + (((~0ULL) - (1ULL << (l)) + 1) & \
> + (~0ULL >> (64 - 1 - (h))))
Please add it to defs.h instead of a .c file. Also since this is
picked up from Linux kernel, please add a comment about this being
borrowed from the Linux kernel (something like: Borrowed from /*
include/linux/bits.h */). Also add the relevant Linux comment which is
very useful:
/*
* Create a contiguous bitmask starting at bit position @l and ending at
* position @h. For example
* GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
*/
> static void arm64_calc_KERNELPACMASK(void)
> {
> ulong value;
> @@ -4133,6 +4138,13 @@ static void arm64_calc_KERNELPACMASK(void)
> machdep->machspec->CONFIG_ARM64_KERNELPACMASK = value;
> if (CRASHDEBUG(1))
> fprintf(fp, "CONFIG_ARM64_KERNELPACMASK: %lx\n", value);
> + } else if (machdep->machspec->VA_BITS_ACTUAL &&
> + machdep->machspec->tag_ignore) {
> + machdep->machspec->CONFIG_ARM64_KERNELPACMASK =
> + __GENMASK_ULL(63, machdep->machspec->VA_BITS_ACTUAL);
> + if (CRASHDEBUG(1))
> + fprintf(fp, "CONFIG_ARM64_KERNELPACMASK: %lx\n",
> + machdep->machspec->CONFIG_ARM64_KERNELPACMASK);
We can make this fprintf common for the if and else-if legs, right?
Just take it out and calculate value in both the cases and print it.
Also why do we set machdep->machspec->CONFIG_ARM64_KERNELPACMASK =
__GENMASK_ULL(63, machdep->machspec->VA_BITS_ACTUAL)
here, can you add some comments to this le, since the kernel passed a
0 value for KERNELPACMASK in vmcoreinfo.
Thanks,
Bhupesh
> }
> }
>
> diff --git a/crash.8 b/crash.8
> index 5020ce1..91f01fc 100644
> --- a/crash.8
> +++ b/crash.8
> @@ -289,6 +289,7 @@ ARM64:
> kimage_voffset=<kimage_voffset-value>
> max_physmem_bits=<value>
> vabits_actual=<value>
> + tag_ignore
> X86:
> page_offset=<CONFIG_PAGE_OFFSET-value>
> .fi
> diff --git a/defs.h b/defs.h
> index 35b983a..cd3bcf9 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -3331,6 +3331,7 @@ struct machine_specific {
> ulong VA_START;
> ulong CONFIG_ARM64_KERNELPACMASK;
> ulong physvirt_offset;
> + ulong tag_ignore;
> };
>
> struct arm64_stackframe {
> diff --git a/help.c b/help.c
> index 531f50a..d66125b 100644
> --- a/help.c
> +++ b/help.c
> @@ -182,6 +182,7 @@ char *program_usage_info[] = {
> " kimage_voffset=<kimage_voffset-value>",
> " max_physmem_bits=<value>",
> " vabits_actual=<value>",
> + " tag_ignore",
> " X86:",
> " page_offset=<CONFIG_PAGE_OFFSET-value>",
> "",
> --
>
> --
> Crash-utility mailing list
> Crash-utility@xxxxxxxxxx
> https://listman.redhat.com/mailman/listinfo/crash-utility
>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
