Re: [PATCH] Fix pvops Xen detection for arm machine

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Dec 20, 2021 at 5:26 PM Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx> wrote:
>
>
>
> On 2021/12/20 4:35 PM, Qi Zheng wrote:
> >
> >
> > On 2021/12/20 4:29 PM, lijiang wrote:
> >> Hi, Qi
> >> Thank you for the fix.
> >>
> >>> Date: Thu, 16 Dec 2021 15:09:49 +0800
> >>> From: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
> >>> To: k-hagio-ab@xxxxxxx, ptesarik@xxxxxxxx, lijiang@xxxxxxxxxx
> >>> Cc: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>, crash-utility@xxxxxxxxxx
> >>> Subject:  [PATCH] Fix pvops Xen detection for arm
> >>>          machine
> >>> Message-ID: <20211216070949.17668-1-zhengqi.arch@xxxxxxxxxxxxx>
> >>> Content-Type: text/plain; charset="US-ASCII"
> >>>
> >>> Since the xen_start_info on the arm/arm64 platform is static defined:
> >>>
> >>> ./arm/xen/enlighten.c:40:static struct start_info _xen_start_info;
> >>> ./arm/xen/enlighten.c:41:struct start_info *xen_start_info =
> >>> &_xen_start_info;
> >>> ./arm/xen/enlighten.c:42:EXPORT_SYMBOL(xen_start_info);
> >>>
> >> The above variable is statically defined in the kernel, but the
> >> 'xen_vcpu_info' is also a static variable, why does the former not
> >> work on arm machines, but the latter works well? Could you please
> >> explain more details?
> >> [linux]$ grep -nr "xen_vcpu_info" *
> >> arch/arm/xen/enlighten.c:51:static struct vcpu_info __percpu
> >> *xen_vcpu_info;
> >
> > Hi Lianbo,
> >
> > The 'xen_vcpu_info' is also a static variable on the arm/arm64 platform,
> > but different from 'xen_start_info', it is assigned during the
> > initialization process.
>
> Like the 'xen_start_info' on the x86 platform:
>
>         - definition:
>         struct start_info *xen_start_info;
>         EXPORT_SYMBOL_GPL(xen_start_info);
>
>         - initialization:
>         startup_xen
>         --> mov %rsi, xen_start_info
>
> The 'xen_vcpu_info' on the arm/arm64 platform:
>
>         - definition:
>         static struct vcpu_info __percpu *xen_vcpu_info;
>
>         - initialization:
>         xen_guest_init
>         --> xen_vcpu_info = alloc_percpu(struct vcpu_info);
>
> So I think that it will works well on the arm/arm64 platform. I have
> tested it on the arm64 platform(But I can not test it on the arm/arm64
> xen machine).
>

Thank you for the explanation, Qi.

The xen_start_info is only defined in x86 and arm platform, but
because the xen_start_info on the arm/arm64 platform points to a
static variable '_xen_start_info'(see its definition as below), which
makes that the address of xen_start_info will never be null. As a
result, the is_pvops_xen() in in commit 4badc6229c69 ("Fix pvops Xen
detection for kernels >= v4.20") always returns TRUE, the reason is
that it can always read out the non-null address of xen_start_info,
and finally caused the failure.

[linux]$ grep -nrw "xen_start_info" *
arch/arm/xen/enlighten.c:41:struct start_info *xen_start_info =
&_xen_start_info;
arch/arm/xen/enlighten.c:42:EXPORT_SYMBOL(xen_start_info);
...
arch/x86/xen/enlighten.c:49:struct start_info *xen_start_info;
arch/x86/xen/enlighten.c:50:EXPORT_SYMBOL_GPL(xen_start_info);

In view of this, it could be better to explicitly narrow the
'xen_start_info' check to x86 with the machine_type(), there is no
need to check it on other architectures.  What do you think?
For example:

diff --git a/kernel.c b/kernel.c
index f4598ea217a3..b17f3fb9e85e 100644
--- a/kernel.c
+++ b/kernel.c
@@ -10757,11 +10757,22 @@ is_pvops_xen(void)
             STREQ(sym, "paravirt_patch_default")))
                return TRUE;

-       if (symbol_exists("xen_start_info") &&
-           readmem(symbol_value("xen_start_info"), KVADDR, &addr,
-           sizeof(void *), "xen_start_info", RETURN_ON_ERROR) &&
-           addr != 0)
-               return TRUE;
+       if (machine_type("X86") || machine_type("X86_64")) {
+               if (symbol_exists("xen_start_info") &&
+                  readmem(symbol_value("xen_start_info"), KVADDR, &addr,
+                  sizeof(void *), "xen_start_info", RETURN_ON_ERROR) &&
+                  addr != 0)
+                       return TRUE;
+       }
+
+       if (machine_type("ARM") || machine_type("ARM64")) {
+               if (symbol_exists("xen_vcpu_info") &&
+                   readmem(symbol_value("xen_vcpu_info"), KVADDR, &addr,
+                   sizeof(void *), "xen_vcpu_info", RETURN_ON_ERROR) &&
+                   addr != 0)
+                       return TRUE;
+       }
+

        return FALSE;
 }

Although the above if(machine_type("ARM")) block has not been tested
on the arm Xen platform, it doesn't affect other architectures.

Thanks.
Lianbo

> Thanks,
> Qi
>
> >
> > Thanks,
> > Qi
> >
> >>
> >> Thanks.
> >> Lianbo
> >>
> >>> The is_pvops_xen() in commit 4badc6229c69f5cd9da7eb7bdf400a53ec6db01a
> >>> ("Fix pvops Xen detection for kernels >= v4.20") always return TRUE.
> >>> Then the following error will be reported because p2m_mid_missing
> >>> and xen_p2m_addr are not defined:
> >>>
> >>>          crash: cannot resolve "p2m_top"
> >>>
> >>> For the arm/arm64 platform, fix it by using xen_vcpu_info instead of
> >>> xen_start_info to detect Xen dumps.
> >>>
> >>> Signed-off-by: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
> >>> ---
> >>>   kernel.c | 15 +++++++++++----
> >>>   1 file changed, 11 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/kernel.c b/kernel.c
> >>> index f4598ea..4b4d789 100644
> >>> --- a/kernel.c
> >>> +++ b/kernel.c
> >>> @@ -10757,11 +10757,18 @@ is_pvops_xen(void)
> >>>               STREQ(sym, "paravirt_patch_default")))
> >>>                  return TRUE;
> >>>
> >>> -       if (symbol_exists("xen_start_info") &&
> >>> -           readmem(symbol_value("xen_start_info"), KVADDR, &addr,
> >>> -           sizeof(void *), "xen_start_info", RETURN_ON_ERROR) &&
> >>> -           addr != 0)
> >>> +       if (machine_type("ARM") || machine_type("ARM64")) {
> >>> +               if (symbol_exists("xen_vcpu_info") &&
> >>> +                   readmem(symbol_value("xen_vcpu_info"), KVADDR,
> >>> &addr,
> >>> +                   sizeof(void *), "xen_vcpu_info", RETURN_ON_ERROR) &&
> >>> +                   addr != 0)
> >>> +                       return TRUE;
> >>> +       } else if (symbol_exists("xen_start_info") &&
> >>> +                  readmem(symbol_value("xen_start_info"), KVADDR,
> >>> &addr,
> >>> +                  sizeof(void *), "xen_start_info", RETURN_ON_ERROR) &&
> >>> +                  addr != 0) {
> >>>                  return TRUE;
> >>> +       }
> >>>
> >>>          return FALSE;
> >>>   }
> >>> --
> >>> 2.11.0
> >>
> >
>
> --
> Thanks,
> Qi
>

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux