From: Oleksii <oleksii.kurochko@gmail.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Julien Grall" <julien@xen.org>,
"Andrew Cooper" <andrew.cooper3@citrix.com>,
"Stefano Stabellini" <sstabellini@kernel.org>,
"Gianluca Guida" <gianluca@rivosinc.com>,
"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
xen-devel@lists.xenproject.org
Subject: Re: [PATCH v6 4/4] xen/x86: switch x86 to use generic implemetation of bug.h
Date: Wed, 08 Mar 2023 19:48:08 +0200 [thread overview]
Message-ID: <21bb8cfd68b9b48ab180b852d9b730300d3ad283.camel@gmail.com> (raw)
In-Reply-To: <aa8cf64e-e5b1-6c13-7b30-e409a0460e3c@suse.com>
On Wed, 2023-03-08 at 16:33 +0100, Jan Beulich wrote:
> On 07.03.2023 16:50, Oleksii Kurochko wrote:
> > @@ -1166,12 +1167,9 @@ void cpuid_hypervisor_leaves(const struct
> > vcpu *v, uint32_t leaf,
> >
> > void do_invalid_op(struct cpu_user_regs *regs)
> > {
> > - const struct bug_frame *bug = NULL;
> > u8 bug_insn[2];
> > - const char *prefix = "", *filename, *predicate, *eip = (char
> > *)regs->rip;
> > - unsigned long fixup;
> > - int id = -1, lineno;
> > - const struct virtual_region *region;
> > + void *eip = (void *)regs->rip;
>
> As said elsewhere already: "const" please whenever possible. The more
> that
> the variable was pointer-to-const before.
Sure, I will change it than to:
const void *eip = (const void *)regs->rip;
>
> > @@ -1185,83 +1183,20 @@ void do_invalid_op(struct cpu_user_regs
> > *regs)
> > memcmp(bug_insn, "\xf\xb", sizeof(bug_insn)) )
> > goto die;
> >
> > - region = find_text_region(regs->rip);
> > - if ( region )
> > - {
> > - for ( id = 0; id < BUGFRAME_NR; id++ )
> > - {
> > - const struct bug_frame *b;
> > - unsigned int i;
> > -
> > - for ( i = 0, b = region->frame[id].bugs;
> > - i < region->frame[id].n_bugs; b++, i++ )
> > - {
> > - if ( bug_loc(b) == eip )
> > - {
> > - bug = b;
> > - goto found;
> > - }
> > - }
> > - }
> > - }
> > -
> > - found:
> > - if ( !bug )
> > + id = do_bug_frame(regs, regs->rip);
> > + if ( id < 0 )
> > goto die;
> > - eip += sizeof(bug_insn);
> > - if ( id == BUGFRAME_run_fn )
> > - {
> > - void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
> > -
> > - fn(regs);
> > - fixup_exception_return(regs, (unsigned long)eip);
> > - return;
> > - }
> >
> > - /* WARN, BUG or ASSERT: decode the filename pointer and line
> > number. */
> > - filename = bug_ptr(bug);
> > - if ( !is_kernel(filename) && !is_patch(filename) )
> > - goto die;
> > - fixup = strlen(filename);
> > - if ( fixup > 50 )
> > - {
> > - filename += fixup - 47;
> > - prefix = "...";
> > - }
> > - lineno = bug_line(bug);
> > + eip = (unsigned char *)eip + sizeof(bug_insn);
>
> Why don't you keep the original
>
> eip += sizeof(bug_insn);
>
> ? As also said before we use the GNU extension of arithmetic on
> pointers
> to void pretty extensively elsewhere; there's no reason at all to
> introduce an unnecessary and questionable cast here.
Just missed that during rebase. ( I experimented with the branch and
received conflicts that were resolved incorrectly )
I will update that. Thanks
>
> With these adjustments and with the re-basing over the changes
> requested
> to patch 2
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Jan
~ Oleksii
prev parent reply other threads:[~2023-03-08 17:48 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-07 15:50 [PATCH v6 0/4] [PATCH v5 0/4] introduce generic implementation of macros from bug.h Oleksii Kurochko
2023-03-07 15:50 ` [PATCH v6 1/4] xen: introduce CONFIG_GENERIC_BUG_FRAME Oleksii Kurochko
2023-03-08 15:06 ` Jan Beulich
2023-03-08 17:18 ` Oleksii
2023-03-07 15:50 ` [PATCH v6 2/4] xen: change <asm/bug.h> to <xen/bug.h> Oleksii Kurochko
2023-03-08 15:27 ` Jan Beulich
2023-03-08 17:25 ` Oleksii
2023-03-09 9:49 ` Jan Beulich
2023-03-07 15:50 ` [PATCH v6 3/4] xen/arm: switch ARM to use generic implementation of bug.h Oleksii Kurochko
2023-03-07 15:50 ` [PATCH v6 4/4] xen/x86: switch x86 to use generic implemetation " Oleksii Kurochko
2023-03-08 15:33 ` Jan Beulich
2023-03-08 17:48 ` Oleksii [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=21bb8cfd68b9b48ab180b852d9b730300d3ad283.camel@gmail.com \
--to=oleksii.kurochko@gmail.com \
--cc=andrew.cooper3@citrix.com \
--cc=gianluca@rivosinc.com \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.