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

Re: [PATCH v8 5/6] xen/riscv: implement do_bug_frame()


  • To: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 14 Aug 2023 14:59:12 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=GL5M+67szkAI50psuf7sHfXI6Vjlas0Sktn2Wb5LWqo=; b=UpvLM/KCHRPFtK9t1bY9UJWHYekqphbertpEd5HPTp0hzKm7lVg0vETvTZH3DpHSPpI8tKaLa6gwQcWAx9ycA5KxJONerbPTy8x727F4Nac98xA2hTx9AfBuKDp7SWdfH1/o88fiUhf1N+4GDPfvhNVQlH1zS0V1ONPmAiFECVr6/NctAM514EKB/h8PKEnR4va8OOPea/qVWfnrW+Y6QZ4kV+NyrW4ZGiZMdRNF7FYF/VKDzrkHfGZUNRM3+IcKc0T1QPmgkvLZRmD+OoTQq9f9Z1WtNKJcD1JEf8GIeJCONm1lGMfyS9DaTvmZ5Thmpy2mRebruHOh/W+gHUjzMw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=HgWPFEoalhEfdTZpN6iFvYQsX+gbp4rMGa+HXy7KUyfzvXZLzVZ+mgW64SBjqxg57NKtiyG01iWW6Izj6HvIMwwGPF/ddT7IcW3u8M+tvJsoNb7bqyK80io99nO3Ae0gl8G0TH3YwajX54F66ZfJNghYbOpUExJF/T/XT0d+UjQh9Jrd7RizzzXfnvMdpOZPn28jh8dO8DMZkgnAIEEPhH0TiJPSwIU1FvPOLi+j2HfkC3dJHEA1d0YzdaBv94hZIhJDQDjcQS/DiYSwjM6SVjb8Lw/PcfG92lRv+I4o2miIAyvNLnRAA6eZ45w5urD3qy6+U7cjbU3pyhQbqe7Cgw==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Bob Eshleman <bobbyeshleman@xxxxxxxxx>, Alistair Francis <alistair.francis@xxxxxxx>, Connor Davis <connojdavis@xxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Mon, 14 Aug 2023 12:59:24 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 11.08.2023 16:32, Oleksii Kurochko wrote:
> The patch introduces the temporary implementation of do_bug_frame().
> When common code is merged, CONFIG_GENERIC_BUG_FRAME should
> be used instead.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in V8:
>   - remove Pointless initializer of id.
>   - make bug_frames[] array constant.
>   - remove cast_to_bug_frame(addr).
>   - rename is_valig_bugaddr to is_valid_bug_insn().
>   - add check that read_instr is used only on xen code
>   - update the commit message.

Looks largely okay to me now, yet still a couple of remarks / suggestions:

> +int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
> +{
> +    const struct bug_frame *start, *end;
> +    const struct bug_frame *bug = NULL;
> +    unsigned int id;
> +    const char *filename, *predicate;
> +    int lineno;
> +
> +    static const struct bug_frame * const bug_frames[] = {
> +        &__start_bug_frames[0],
> +        &__stop_bug_frames_0[0],
> +        &__stop_bug_frames_1[0],
> +        &__stop_bug_frames_2[0],
> +        &__stop_bug_frames_3[0],
> +    };
> +
> +    for ( id = 0; id < BUGFRAME_NR; id++ )
> +    {
> +        start = bug_frames[id];
> +        end   = bug_frames[id + 1];
> +
> +        while ( start != end )
> +        {
> +            if ( (vaddr_t)bug_loc(start) == pc )
> +            {
> +                bug = start;
> +                goto found;
> +            }
> +
> +            start++;
> +        }
> +    }
> +
> + found:
> +    if ( bug == NULL )
> +        return -ENOENT;
> +
> +    if ( id == BUGFRAME_run_fn )
> +    {
> +        void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> +
> +        fn(regs);
> +
> +        goto end;

I'd like to suggest that you avoid "goto" when at the label there's just
a single, simple statement (like return in the case here).

> +    }
> +
> +    /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
> +    filename = bug_ptr(bug);
> +    lineno = bug_line(bug);
> +
> +    switch ( id )
> +    {
> +    case BUGFRAME_warn:
> +        printk("Xen WARN at %s:%d\n", filename, lineno);
> +
> +        show_execution_state(regs);
> +
> +        goto end;
> +
> +    case BUGFRAME_bug:
> +        printk("Xen BUG at %s:%d\n", filename, lineno);
> +
> +        show_execution_state(regs);
> +
> +        printk("change wait_for_interrupt to panic() when common is 
> available\n");
> +        die();

The log message here ...

> +    case BUGFRAME_assert:
> +        /* ASSERT: decode the predicate string pointer. */
> +        predicate = bug_msg(bug);
> +
> +        printk("Assertion %s failed at %s:%d\n", predicate, filename, 
> lineno);
> +
> +        show_execution_state(regs);
> +
> +        printk("change wait_for_interrupt to panic() when common is 
> available\n");
> +        die();

... and here doesn't really fit with the following code. There's no
wait_for_interrupt() in sight.

> +    }
> +
> +    return -EINVAL;
> +
> + end:
> +    return 0;
> +}
> +
> +static bool is_valid_bug_insn(uint32_t insn)
> +{
> +    return insn == BUG_INSN_32 ||
> +           (insn & COMPRESSED_INSN_MASK) == BUG_INSN_16;
> +}
> +
> +/* There are no guests for now so  the func has to return alwasys false */
> +static inline bool guest_mode(void)
> +{
> +    return false;
> +}

I think this makes little sense to have, and ...

> +/* Should be used only on Xen code */

... just the comment is good enough. First and foremost I don't think
a function with name and purpose of the above can get away without
any parameters. Furthermore, as said before, ...

> +static uint32_t read_instr(unsigned long pc)
> +{
> +    uint16_t instr16 = *(uint16_t *)pc;

... such casting can't be legitimate for guest addresses anyway; you
would need to map guest memory instead. What you might do ...

> +    if ( guest_mode() )
> +    {
> +        printk("%s should be called only on Xen code\n", __func__);
> +        die();
> +    }

... instead of this, as some minimalistic checking, is

    ASSERT(is_kernel_text(pc + 1) || is_kernel_inittext(pc + 1));

> +    if ( GET_INSN_LENGTH(instr16) == 2 )
> +        return instr16;
> +    else
> +        return *(uint32_t *)pc;

and then

    if ( GET_INSN_LENGTH(instr16) == 2 )
        return instr16;

    ASSERT(is_kernel_text(pc + 3) || is_kernel_inittext(pc + 3));

    return *(uint32_t *)pc;

Yet then again asserting in exception handlers is of limited use
only, as then you risk recursive exceptions.

>  void do_trap(struct cpu_user_regs *cpu_regs)
>  {
> +    register_t pc = cpu_regs->sepc;
> +    uint32_t instr = read_instr(pc);
> +
> +    if ( is_valid_bug_insn(instr) )
> +    {
> +        if ( !do_bug_frame(cpu_regs, pc) )

Two if()-s like these typically want folding into one.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.