| Message ID | 1482418735-23949-1-git-send-email-ian.jackson@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22/12/16 14:58, Ian Jackson wrote: > "x86emul: in_longmode() should not ignore ->read_msr() errors" aka > c/s 122dd9575c7a introduced a use of unlikely() in > xen/arch/x86/x86_emulate/x86_emulate.c. > > I think this is probably intentional and fine. However, there is no > definition of unlikely in the x86 emulator test harness, under tools. > > The result is this error: > x86_emulate/x86_emulate.c: In function 'in_longmode': > x86_emulate/x86_emulate.c:1300:10: error: implicit declaration of function 'unlikely' [-Werror=implicit-function-declaration] > unlikely(ops->read_msr(MSR_EFER, &efer, ctxt) != X86EMUL_OKAY) ) > ^~~~~~~~ > > Fix this by providing a boring definition of unlikely(). > > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> This was fixed by 3e84c8da7d2c5442a12789dae7163dca6c0e154f Part of that should backported to 4.8, where it is still broken. ~Andrew
Andrew Cooper writes ("Re: [PATCH] tools/tests/x86_emulate: #define unlikely in x86 emulator test harness"):
> On 22/12/16 14:58, Ian Jackson wrote:
> > "x86emul: in_longmode() should not ignore ->read_msr() errors" aka
> > c/s 122dd9575c7a introduced a use of unlikely() in
> > xen/arch/x86/x86_emulate/x86_emulate.c.
> >
> > I think this is probably intentional and fine. However, there is no
> > definition of unlikely in the x86 emulator test harness, under tools.
> >
> > The result is this error:
> > x86_emulate/x86_emulate.c: In function 'in_longmode':
> > x86_emulate/x86_emulate.c:1300:10: error: implicit declaration of function 'unlikely' [-Werror=implicit-function-declaration]
> > unlikely(ops->read_msr(MSR_EFER, &efer, ctxt) != X86EMUL_OKAY) )
> > ^~~~~~~~
> >
> > Fix this by providing a boring definition of unlikely().
> >
> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
>
> This was fixed by 3e84c8da7d2c5442a12789dae7163dca6c0e154f
I did not find this important build fix for a regression in 4.8.0
because:
* this commit contains a mixture of the build fix and other changes
* `git log -G unlikely' produced a lot of output: too much to read
the whole message for each commit through looking for this fix
* the commit message did not contain a copy of the error message
* once I had dug into the code myself and found 122dd9575c7a it
didn't occur to me to `git log --grep 122dd9'.
> Part of that should backported to 4.8, where it is still broken.
Backporting this is made more awkward by the decision to make the fix
a portmanteau. Do the x86 maintainers intend to provide a
ready-to-use backport or shall I try to prepare one ?
For now, is there any reason why I should not use my change
+#define unlikely(x) (x)
in an upload to Debian ?
Thanks,
Ian.
>>> On 22.12.16 at 16:10, <ian.jackson@eu.citrix.com> wrote: > I did not find this important build fix for a regression in 4.8.0 > because: I wonder why you consider this important - the harness doesn't get built by default, and is of little use for other than smoke testing a limited set of changes to the insn emulator. > Backporting this is made more awkward by the decision to make the fix > a portmanteau. Do the x86 maintainers intend to provide a > ready-to-use backport or shall I try to prepare one ? I've pushed one a minute ago to 4.8-staging. > For now, is there any reason why I should not use my change > +#define unlikely(x) (x) > in an upload to Debian ? I don't see anything speaking against that. Jan
Jan Beulich writes ("Re: [PATCH] tools/tests/x86_emulate: #define unlikely in x86 emulator test harness"):
> On 22.12.16 at 16:10, <ian.jackson@eu.citrix.com> wrote:
> > I did not find this important build fix for a regression in 4.8.0
> > because:
>
> I wonder why you consider this important - the harness doesn't
> get built by default, and is of little use for other than smoke
> testing a limited set of changes to the insn emulator.
I think this must be being enabled by something in the Debian package
build.
> > Backporting this is made more awkward by the decision to make the fix
> > a portmanteau. Do the x86 maintainers intend to provide a
> > ready-to-use backport or shall I try to prepare one ?
>
> I've pushed one a minute ago to 4.8-staging.
Ah, great, thanks.
> > For now, is there any reason why I should not use my change
> > +#define unlikely(x) (x)
> > in an upload to Debian ?
>
> I don't see anything speaking against that.
OK, thanks. I think now there's a fix in 4.8-staging I will cherry
pick that instead.
Ian.
diff --git a/tools/tests/x86_emulator/x86_emulate.c b/tools/tests/x86_emulator/x86_emulate.c index c46b7fc..e8f26fe 100644 --- a/tools/tests/x86_emulator/x86_emulate.c +++ b/tools/tests/x86_emulator/x86_emulate.c @@ -49,5 +49,6 @@ typedef bool bool_t; #define __init #define __maybe_unused __attribute__((__unused__)) +#define unlikely(x) (x) #include "x86_emulate/x86_emulate.c"
"x86emul: in_longmode() should not ignore ->read_msr() errors" aka c/s 122dd9575c7a introduced a use of unlikely() in xen/arch/x86/x86_emulate/x86_emulate.c. I think this is probably intentional and fine. However, there is no definition of unlikely in the x86 emulator test harness, under tools. The result is this error: x86_emulate/x86_emulate.c: In function 'in_longmode': x86_emulate/x86_emulate.c:1300:10: error: implicit declaration of function 'unlikely' [-Werror=implicit-function-declaration] unlikely(ops->read_msr(MSR_EFER, &efer, ctxt) != X86EMUL_OKAY) ) ^~~~~~~~ Fix this by providing a boring definition of unlikely(). Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Jan Beulich <jbeulich@suse.com> --- tools/tests/x86_emulator/x86_emulate.c | 1 + 1 file changed, 1 insertion(+)