Re: [PATCH] drm/qxl: remove variable count
On Sat, Apr 8, 2023 at 9:50 AM Tom Rix wrote:
>
> clang with W=1 reports
> drivers/gpu/drm/qxl/qxl_cmd.c:424:6: error: variable
> 'count' set but not used [-Werror,-Wunused-but-set-variable]
> int count = 0;
> ^
> This variable is not used so remove it.
Thanks for the patch!
Fixes: 64122c1f6ad ("drm: add new QXL driver. (v1.4)")
Reviewed-by: Nick Desaulniers
>
> Signed-off-by: Tom Rix
> ---
> drivers/gpu/drm/qxl/qxl_cmd.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
> index 281edab518cd..d6ea01f3797b 100644
> --- a/drivers/gpu/drm/qxl/qxl_cmd.c
> +++ b/drivers/gpu/drm/qxl/qxl_cmd.c
> @@ -421,7 +421,6 @@ int qxl_surface_id_alloc(struct qxl_device *qdev,
> {
> uint32_t handle;
> int idr_ret;
> - int count = 0;
> again:
> idr_preload(GFP_ATOMIC);
> spin_lock(&qdev->surf_id_idr_lock);
> @@ -433,7 +432,6 @@ int qxl_surface_id_alloc(struct qxl_device *qdev,
> handle = idr_ret;
>
> if (handle >= qdev->rom->n_surfaces) {
> - count++;
> spin_lock(&qdev->surf_id_idr_lock);
> idr_remove(&qdev->surf_id_idr, handle);
> spin_unlock(&qdev->surf_id_idr_lock);
> --
> 2.27.0
>
--
Thanks,
~Nick Desaulniers
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] drm/qxl: remove unused num_relocs variable
On Fri, Mar 31, 2023 at 10:24 AM Tom Rix wrote:
>
> clang with W=1 reports
> drivers/gpu/drm/qxl/qxl_ioctl.c:149:14: error: variable
> 'num_relocs' set but not used [-Werror,-Wunused-but-set-variable]
> int i, ret, num_relocs;
> ^
> This variable is not used so remove it.
>
> Signed-off-by: Tom Rix
Thanks for the patch!
Fixes: 74d9a6335dce ("drm/qxl: Simplify cleaning qxl processing command")
Reviewed-by: Nick Desaulniers
That commit should also have removed the label out_free_bos IMO since
having two labels for the same statement is a code smell. Tom, do you
mind sending a v2 with that folded in?
> ---
> drivers/gpu/drm/qxl/qxl_ioctl.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index 30f58b21372a..3422206d59d4 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
> @@ -146,7 +146,7 @@ static int qxl_process_single_command(struct qxl_device
> *qdev,
> struct qxl_release *release;
> struct qxl_bo *cmd_bo;
> void *fb_cmd;
> - int i, ret, num_relocs;
> + int i, ret;
> int unwritten;
>
> switch (cmd->type) {
> @@ -201,7 +201,6 @@ static int qxl_process_single_command(struct qxl_device
> *qdev,
> }
>
> /* fill out reloc info structs */
> - num_relocs = 0;
> for (i = 0; i < cmd->relocs_num; ++i) {
> struct drm_qxl_reloc reloc;
> struct drm_qxl_reloc __user *u = u64_to_user_ptr(cmd->relocs);
> @@ -231,7 +230,6 @@ static int qxl_process_single_command(struct qxl_device
> *qdev,
> reloc_info[i].dst_bo = cmd_bo;
> reloc_info[i].dst_offset = reloc.dst_offset +
> release->release_offset;
> }
> - num_relocs++;
>
> /* reserve and validate the reloc dst bo */
> if (reloc.reloc_type == QXL_RELOC_TYPE_BO ||
> reloc.src_handle) {
> --
> 2.27.0
>
--
Thanks,
~Nick Desaulniers
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: vdpa: fix snprintf size argument in snet_vdpa driver
On Tue, Dec 20, 2022 at 11:20 PM Alvaro Karsz
wrote:
>
> The buffer size and the size passed to snprintf don't match, causing
> clang warnings.
>
> This patch increases a little bit the size of the buffer, and uses
> sizeof() to get the buffer size.
>
> This patch should be applied on top of the following patch:
>
> virtio: vdpa: new SolidNET DPU driver,
> by Alvaro Karsz alvaro.ka...@solid-run.com
>
> Reported-by: Nathan Chancellor
> Signed-off-by: Alvaro Karsz
Thanks for the fix!
Reviewed-by: Nick Desaulniers
> ---
> drivers/vdpa/solidrun/snet_main.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vdpa/solidrun/snet_main.c
> b/drivers/vdpa/solidrun/snet_main.c
> index d438a89b359..9ceacf96de0 100644
> --- a/drivers/vdpa/solidrun/snet_main.c
> +++ b/drivers/vdpa/solidrun/snet_main.c
> @@ -540,7 +540,7 @@ static const struct vdpa_config_ops snet_config_ops = {
>
> static int psnet_open_pf_bar(struct pci_dev *pdev, struct psnet *psnet)
> {
> - char name[25];
> + char name[50];
> int ret, i, mask = 0;
> /* We don't know which BAR will be used to communicate..
> * We will map every bar with len > 0.
> @@ -558,7 +558,7 @@ static int psnet_open_pf_bar(struct pci_dev *pdev, struct
> psnet *psnet)
> return -ENODEV;
> }
>
> - snprintf(name, SNET_NAME_SIZE, "psnet[%s]-bars", pci_name(pdev));
> + snprintf(name, sizeof(name), "psnet[%s]-bars", pci_name(pdev));
> ret = pcim_iomap_regions(pdev, mask, name);
> if (ret) {
> SNET_ERR(pdev, "Failed to request and map PCI BARs\n");
> @@ -575,10 +575,10 @@ static int psnet_open_pf_bar(struct pci_dev *pdev,
> struct psnet *psnet)
>
> static int snet_open_vf_bar(struct pci_dev *pdev, struct snet *snet)
> {
> - char name[20];
> + char name[50];
> int ret;
>
> - snprintf(name, SNET_NAME_SIZE, "snet[%s]-bar", pci_name(pdev));
> + snprintf(name, sizeof(name), "snet[%s]-bar", pci_name(pdev));
> /* Request and map BAR */
> ret = pcim_iomap_regions(pdev, BIT(snet->psnet->cfg.vf_bar), name);
> if (ret) {
> --
> 2.32.0
>
--
Thanks,
~Nick Desaulniers
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] x86/paravirt: add extra clobbers with ZERO_CALL_USED_REGS enabled
On Sun, Sep 4, 2022 at 11:02 PM Bill Wendling wrote:
>
> On Sat, Sep 3, 2022 at 12:18 AM Kees Cook wrote:
> >
> > On Fri, Sep 02, 2022 at 09:37:50PM +, Bill Wendling wrote:
> > > [...]
> > > callq *pv_ops+536(%rip)
> >
> > Do you know which pv_ops function is this? I can't figure out where
> > pte_offset_kernel() gets converted into a pv_ops call
> >
> This one is _paravirt_ident_64, I believe. I think that the original
> issue Nathan was seeing was with another seemingly innocuous function.
_paravirt_ident_64 is marked noinstr, which makes me suspect that it
really needs to not be touched at all by the compiler for
these...special features.
Maybe the definition of noinstr in include/linux/compiler_types.h
should be adding __attribute__((zero_call_used_regs(skip)))?
Untested:
```
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 4f2a819fd60a..a51ab77e2da8 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -226,10 +226,17 @@ struct ftrace_likely_data {
#define __no_sanitize_or_inline __always_inline
#endif
+#ifdef CONFIG_ZERO_CALL_USED_REGS
+#define __no_zero_call_used_regs __attribute__((__zero_call_used_reg__(skip)))
+#else
+#define __no_zero_call_used_regs
+#endif
+
/* Section for code which can't be instrumented at all */
#define noinstr
\
noinline notrace __attribute((__section__(".noinstr.text")))\
- __no_kcsan __no_sanitize_address __no_profile __no_sanitize_coverage
+ __no_kcsan __no_sanitize_address __no_profile \
+ __no_sanitize_coverage __no_zero_call_used_regs
#endif /* __KERNEL__ */
```
Or use __has_attribute in include/linux/compiler_attributes.h.
--
Thanks,
~Nick Desaulniers
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, Nov 25, 2020 at 8:24 AM Jakub Kicinski wrote: > > Applying a real patch set and then getting a few follow ups the next day > for trivial coding things like fallthrough missing or static missing, > just because I didn't have the full range of compilers to check with > before applying makes me feel pretty shitty, like I'm not doing a good > job. YMMV. I understand. Everyone feels that way, except maybe Bond villains and robots. My advice in that case is don't take it personally. We're working with a language that's more error prone relative to others. While one would like to believe they are flawless, over time they can't beat the aggregate statistics. A balance between Imposter Syndrome and Dunning Kruger is walked by all software developers, and the fear of making mistakes in public is one of the number one reasons folks don't take the plunge contributing to open source software or even the kernel. My advice to them is "don't sweat the small stuff." -- Thanks, ~Nick Desaulniers ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, Nov 25, 2020 at 1:33 PM Finn Thain wrote: > > Or do you think that a codebase can somehow satisfy multiple checkers and > their divergent interpretations of the language spec? Have we found any cases yet that are divergent? I don't think so. It sounds to me like GCC's cases it warns for is a subset of Clang's. Having additional coverage with Clang then should ensure coverage for both. > > This is not a shiny new warning; it's already on for GCC and has existed > > in both compilers for multiple releases. > > > > Perhaps you're referring to the compiler feature that lead to the > ill-fated, tree-wide /* fallthrough */ patch series. > > When the ink dries on the C23 language spec and the implementations figure > out how to interpret it then sure, enforce the warning for new code -- the > cost/benefit analysis is straight forward. However, the case for patching > existing mature code is another story. I don't think we need to wait for the ink to dry on the C23 language spec to understand that implicit fallthrough is an obvious defect of the C language. While the kernel is a mature codebase, it's not immune to bugs. And its maturity has yet to slow its rapid pace of development. -- Thanks, ~Nick Desaulniers ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Tue, Nov 24, 2020 at 11:05 PM James Bottomley
wrote:
>
> On Tue, 2020-11-24 at 13:32 -0800, Kees Cook wrote:
> > We already enable -Wimplicit-fallthrough globally, so that's not the
> > discussion. The issue is that Clang is (correctly) even more strict
> > than GCC for this, so these are the remaining ones to fix for full
> > Clang coverage too.
> >
> > People have spent more time debating this already than it would have
> > taken to apply the patches. :)
>
> You mean we've already spent 90% of the effort to come this far so we
> might as well go the remaining 10% because then at least we get some
> return? It's certainly a clinching argument in defence procurement ...
So developers and distributions using Clang can't have
-Wimplicit-fallthrough enabled because GCC is less strict (which has
been shown in this thread to lead to bugs)? We'd like to have nice
things too, you know.
I even agree that most of the churn comes from
case 0:
++x;
default:
break;
which I have a patch for: https://reviews.llvm.org/D91895. I agree
that can never lead to bugs. But that's not the sole case of this
series, just most of them.
Though, note how the reviewer (C++ spec editor and clang front end
owner) in https://reviews.llvm.org/D91895 even asks in that review how
maybe a new flag would be more appropriate for a watered
down/stylistic variant of the existing behavior. And if the current
wording of Documentation/process/deprecated.rst around "fallthrough"
is a straightforward rule of thumb, I kind of agree with him.
>
> > This is about robustness and language wrangling. It's a big code-
> > base, and this is the price of our managing technical debt for
> > permanent robustness improvements. (The numbers I ran from Gustavo's
> > earlier patches were that about 10% of the places adjusted were
> > identified as legitimate bugs being fixed. This final series may be
> > lower, but there are still bugs being found from it -- we need to
> > finish this and shut the door on it for good.)
>
> I got my six patches by analyzing the lwn.net report of the fixes that
> was cited which had 21 of which 50% didn't actually change the emitted
> code, and 25% didn't have a user visible effect.
>
> But the broader point I'm making is just because the compiler people
> come up with a shiny new warning doesn't necessarily mean the problem
That's not what this is though; you're attacking a strawman. I'd
encourage you to bring that up when that actually occurs, unlike this
case since it's actively hindering getting -Wimplicit-fallthrough
enabled for Clang. This is not a shiny new warning; it's already on
for GCC and has existed in both compilers for multiple releases.
And I'll also note that warnings are warnings and not errors because
they cannot be proven to be bugs in 100% of cases, but they have led
to bugs in the past. They require a human to review their intent and
remove ambiguities. If 97% of cases would end in a break ("Expert C
Programming: Deep C Secrets" - Peter van der Linden), then it starts
to look to me like a language defect; certainly an incorrectly chosen
default. But the compiler can't know those 3% were intentional,
unless you're explicit for those exceptional cases.
> it's detecting is one that causes us actual problems in the code base.
> I'd really be happier if we had a theory about what classes of CVE or
> bug we could eliminate before we embrace the next new warning.
We don't generally file CVEs and waiting for them to occur might be
too reactive, but I agree that pointing to some additional
documentation in commit messages about how a warning could lead to a
bug would make it clearer to reviewers why being able to enable it
treewide, even if there's no bug in their particular subsystem, is in
the general interest of the commons.
On Mon, Nov 23, 2020 at 7:58 AM James Bottomley
wrote:
>
> We're also complaining about the inability to recruit maintainers:
>
> https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/
>
> And burn out:
>
> http://antirez.com/news/129
>
> The whole crux of your argument seems to be maintainers' time isn't
> important so we should accept all trivial patches ... I'm pushing back
> on that assumption in two places, firstly the valulessness of the time
> and secondly that all trivial patches are valuable.
It's critical to the longevity of any open source project that there
are not single points of failure. If someone is not expendable or
replaceable (or claims to be) then that's a risk to the project and a
bottleneck. Not having a replacement in training or some form of
redundancy is short sighted.
If trivial patches are adding too much to your workload, consider
training a co-maintainer or asking for help from one of your reviewers
whom you trust. I don't doubt it's hard to find maintainers, but
existing maintainers should go out of their way to entrust
co-maintainers especially when they find their workload becomes too
high. And review
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, Nov 22, 2020 at 8:17 AM Kees Cook wrote:
>
> On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > If none of the 140 patches here fix a real bug, and there is no change
> > to machine code then it sounds to me like a W=2 kind of a warning.
>
> FWIW, this series has found at least one bug so far:
> https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/
So looks like the bulk of these are:
switch (x) {
case 0:
++x;
default:
break;
}
I have a patch that fixes those up for clang:
https://reviews.llvm.org/D91895
There's 3 other cases that don't quite match between GCC and Clang I
observe in the kernel:
switch (x) {
case 0:
++x;
default:
goto y;
}
y:;
switch (x) {
case 0:
++x;
default:
return;
}
switch (x) {
case 0:
++x;
default:
;
}
Based on your link, and Nathan's comment on my patch, maybe Clang
should continue to warn for the above (at least the `default: return;`
case) and GCC should change? While the last case looks harmless,
there were only 1 or 2 across the tree in my limited configuration
testing; I really think we should just add `break`s for those.
--
Thanks,
~Nick Desaulniers
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC] treewide: cleanup unreachable breaks
On Sat, Oct 17, 2020 at 10:43 PM Greg KH wrote: > > On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > > From: Tom Rix > > > > This is a upcoming change to clean up a new warning treewide. > > I am wondering if the change could be one mega patch (see below) or > > normal patch per file about 100 patches or somewhere half way by collecting > > early acks. > > Please break it up into one-patch-per-subsystem, like normal, and get it > merged that way. > > Sending us a patch, without even a diffstat to review, isn't going to > get you very far... Tom, If you're able to automate this cleanup, I suggest checking in a script that can be run on a directory. Then for each subsystem you can say in your commit "I ran scripts/fix_whatever.py on this subdir." Then others can help you drive the tree wide cleanup. Then we can enable -Wunreachable-code-break either by default, or W=2 right now might be a good idea. Ah, George (gbiv@, cc'ed), did an analysis recently of `-Wunreachable-code-loop-increment`, `-Wunreachable-code-break`, and `-Wunreachable-code-return` for Android userspace. From the review: ``` Spoilers: of these, it seems useful to turn on -Wunreachable-code-loop-increment and -Wunreachable-code-return by default for Android ... While these conventions about always having break arguably became obsolete when we enabled -Wfallthrough, my sample turned up zero potential bugs caught by this warning, and we'd need to put a lot of effort into getting a clean tree. So this warning doesn't seem to be worth it. ``` Looks like there's an order of magnitude of `-Wunreachable-code-break` than the other two. We probably should add all 3 to W=2 builds (wrapped in cc-option). I've filed https://github.com/ClangBuiltLinux/linux/issues/1180 to follow up on. -- Thanks, ~Nick Desaulniers ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 06/19] asm/rwonce: Don't pull into 'asm-generic/rwonce.h'
On Fri, Jul 10, 2020 at 9:52 AM Will Deacon wrote: > > Now that 'smp_read_barrier_depends()' has gone the way of the Norwegian > Blue, drop the inclusion of in 'asm-generic/rwonce.h'. > > This requires fixups to some architecture vdso headers which were > previously relying on 'asm/barrier.h' coming in via 'linux/compiler.h'. > > Signed-off-by: Will Deacon > --- > arch/arm/include/asm/vdso/gettimeofday.h | 1 + > arch/arm64/include/asm/vdso/compat_gettimeofday.h | 1 + > arch/arm64/include/asm/vdso/gettimeofday.h| 1 + > arch/riscv/include/asm/vdso/gettimeofday.h| 1 + > include/asm-generic/rwonce.h | 2 -- > include/linux/nospec.h| 2 ++ > 6 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/include/asm/vdso/gettimeofday.h > b/arch/arm/include/asm/vdso/gettimeofday.h > index 36dc18553ed8..1b207cf07697 100644 > --- a/arch/arm/include/asm/vdso/gettimeofday.h > +++ b/arch/arm/include/asm/vdso/gettimeofday.h > @@ -7,6 +7,7 @@ > > #ifndef __ASSEMBLY__ > > +#include > #include > #include > #include > diff --git a/arch/arm64/include/asm/vdso/compat_gettimeofday.h > b/arch/arm64/include/asm/vdso/compat_gettimeofday.h > index b6907ae78e53..bcf764a4 100644 > --- a/arch/arm64/include/asm/vdso/compat_gettimeofday.h > +++ b/arch/arm64/include/asm/vdso/compat_gettimeofday.h > @@ -7,6 +7,7 @@ > > #ifndef __ASSEMBLY__ > > +#include > #include > #include > > diff --git a/arch/arm64/include/asm/vdso/gettimeofday.h > b/arch/arm64/include/asm/vdso/gettimeofday.h > index afba6ba332f8..127fa63893e2 100644 > --- a/arch/arm64/include/asm/vdso/gettimeofday.h > +++ b/arch/arm64/include/asm/vdso/gettimeofday.h > @@ -7,6 +7,7 @@ > > #ifndef __ASSEMBLY__ > > +#include > #include > > #define VDSO_HAS_CLOCK_GETRES 1 > diff --git a/arch/riscv/include/asm/vdso/gettimeofday.h > b/arch/riscv/include/asm/vdso/gettimeofday.h > index c8e818688ec1..3099362d9f26 100644 > --- a/arch/riscv/include/asm/vdso/gettimeofday.h > +++ b/arch/riscv/include/asm/vdso/gettimeofday.h > @@ -4,6 +4,7 @@ > > #ifndef __ASSEMBLY__ > > +#include > #include > #include > #include > diff --git a/include/asm-generic/rwonce.h b/include/asm-generic/rwonce.h > index cc810f1f18ca..cd0302746fb4 100644 > --- a/include/asm-generic/rwonce.h > +++ b/include/asm-generic/rwonce.h > @@ -26,8 +26,6 @@ > #include > #include > > -#include > - > /* > * Use __READ_ONCE() instead of READ_ONCE() if you do not require any > * atomicity. Note that this may result in tears! > diff --git a/include/linux/nospec.h b/include/linux/nospec.h > index 0c5ef54fd416..c1e79f72cd89 100644 > --- a/include/linux/nospec.h > +++ b/include/linux/nospec.h > @@ -5,6 +5,8 @@ > > #ifndef _LINUX_NOSPEC_H > #define _LINUX_NOSPEC_H > + > +#include The other hunks LGTM, but this one is a little more curious to me. Can you walk me through this addition? > #include > > struct task_struct; > -- > 2.27.0.383.g050319c2ae-goog > -- Thanks, ~Nick Desaulniers ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 18/18] arm64: lto: Strengthen READ_ONCE() to acquire when CLANG_LTO=y
I'm trying to put together a Micro Conference for Linux Plumbers conference focused on "make LLVM slightly less shitty." Do you all plan on attending the conference? Would it be worthwhile to hold a session focused on discussing this (LTO and memory models) be worthwhile? On Tue, Jul 7, 2020 at 3:51 PM Paul E. McKenney wrote: > > On Tue, Jul 07, 2020 at 11:29:15AM +0100, Dave Martin wrote: > > On Mon, Jul 06, 2020 at 10:36:28AM -0700, Paul E. McKenney wrote: > > > On Mon, Jul 06, 2020 at 06:05:57PM +0100, Dave Martin wrote: > > [ . . . ] > > > > > The underlying problem here seems to be that the necessary ordering > > > > rule is not part of what passes for the C memory model prior to C11. > > > > If we want to control the data flow, don't we have to wrap the entire > > > > dereference in a macro? > > > > > > Yes, exactly. Because we are relying on things that are not guaranteed > > > by the C memory model, we need to pay attention to the implementations. > > > As I have said elsewhere, the price of control dependencies is eternal > > > vigilance. > > > > > > And this also applies, to a lesser extent, to address and data > > > dependencies, which are also not well supported by the C standard. > > > > > > There is one important case in which the C memory model -does- support > > > control dependencies, and that is when the dependent write is a normal > > > C-language write that is not involved in a data race. In that case, > > > if the compiler broke the control dependency, it might have introduced > > > a data race, which it is forbidden to do. However, this rule can also > > > be broken when the compiler knows too much, as it might be able to prove > > > that breaking the dependency won't introduce a data race. In that case, > > > according to the standard, it is free to break the dependency. > > > > Which only matters because the C abstract machine may not match reality. > > > > LTO has no bearing on the abstract machine though. > > > > If specific compiler options etc. can be added to inhibit the > > problematic optimisations, that would be ideal. I guess that can't > > happen overnight though. > > Sadly, I must agree. > > > > > > > > We likely won't realise if/when this goes wrong, other than > > > > > > > impossible to > > > > > > > debug, subtle breakage that crops up seemingly randomly. Ideally, > > > > > > > we'd be > > > > > > > able to detect this sort of thing happening at build time, and > > > > > > > perhaps > > > > > > > even prevent it with compiler options or annotations, but none of > > > > > > > that is > > > > > > > close to being available and I'm keen to progress the LTO patches > > > > > > > in the > > > > > > > meantime because they are a requirement for CFI. > > > > > > > > > > > > My concern was not so much why LTO makes things dangerous, as why > > > > > > !LTO > > > > > > makes things safe... > > > > > > > > > > Because ignorant compilers are safe compilers! ;-) > > > > > > > > AFAICT ignorance is no gurantee of ordering in general -- the compiler > > > > is free to speculatively invent knowledge any place that the language > > > > spec allows it to. !LTO doesn't stop this happening. > > > > > > Agreed, according to the standard, the compiler has great freedom. > > > > > > We have two choices: (1) Restrict ourselves to live within the confines of > > > the standard or (2) Pay continued close attention to the implementation. > > > We have made different choices at different times, but for many ordering > > > situations we have gone with door #2. > > > > > > Me, I have been working to get the standard to better support our > > > use case. This is at best slow going. But don't take my word for it, > > > ask Will. > > > > I can believe it. They want to enable optimisations rather than prevent > > them... > > Right in one! ;-) > > > > > Hopefully some of the knowledge I invented in my reply is valid... > > > > > > It is. It is just that there are multiple valid strategies, and the > > > Linux kernel is currently taking a mixed-strategy approach. > > > > Ack. The hope that there is a correct way to fix everything dies > > hard ;) > > Either that, or one slowly degrades ones definition of "correct". :-/ > > > Life was cosier before I started trying to reason about language specs. > > Same here! > > Thanx, Paul -- Thanks, ~Nick Desaulniers ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] x86/cpu/vmware: use the full form of inl in VMWARE_PORT
On Mon, Oct 7, 2019 at 12:21 PM 'Sami Tolvanen' via Clang Built Linux
wrote:
>
> LLVM's assembler doesn't accept the short form inl (%%dx) instruction,
> but instead insists on the output register to be explicitly specified:
>
> :1:7: error: invalid operand for instruction
> inl (%dx)
> ^
> LLVM ERROR: Error parsing inline asm
>
> Use the full form of the instruction to fix the build.
>
> Signed-off-by: Sami Tolvanen
Thanks Sami, this looks like it addresses:
Link: https://github.com/ClangBuiltLinux/linux/issues/734
Looks like GAS' testsuite has some cases where the second operand is
indeed implicitly %eax if unspecified. (This should still be fixed in
Clang).
Just to triple check that they're equivalent:
$ cat inl.s
inl (%dx)
inl (%dx), %eax
$ as inl.s
$ objdump -d a.out
a.out: file format elf64-x86-64
Disassembly of section .text:
<.text>:
0: edin (%dx),%eax
1: edin (%dx),%eax
Reviewed-by: Nick Desaulniers
> ---
> arch/x86/kernel/cpu/vmware.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
> index 9735139cfdf8..46d732696c1c 100644
> --- a/arch/x86/kernel/cpu/vmware.c
> +++ b/arch/x86/kernel/cpu/vmware.c
> @@ -49,7 +49,7 @@
> #define VMWARE_CMD_VCPU_RESERVED 31
>
> #define VMWARE_PORT(cmd, eax, ebx, ecx, edx) \
> - __asm__("inl (%%dx)" : \
> + __asm__("inl (%%dx), %%eax" : \
> "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) :\
> "a"(VMWARE_HYPERVISOR_MAGIC), \
> "c"(VMWARE_CMD_##cmd), \
--
Thanks,
~Nick Desaulniers
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
