[PATCH] VMX: fix vmx_{find,del}_msr() build

[PATCH] VMX: fix vmx_{find,del}_msr() build

[Jan Beulich]

For a reason that I can't explain, it is only the shim build that fails
for me with an older gcc due to the compiler not recognizing that
apparently uninitialized variables aren't really uninitialized. Pull out
the assignments used by two of the three case blocks and make them
initializers of the variables, as I think I had suggested during review.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1305,7 +1305,8 @@ struct vmx_msr_entry *vmx_find_msr(const
 {
     const struct arch_vmx_struct *vmx = &v->arch.hvm_vmx;
     struct vmx_msr_entry *start = NULL, *ent, *end;
-    unsigned int substart, subend, total;
+    unsigned int substart = 0, subend = vmx->msr_save_count;
+    unsigned int total = vmx->msr_load_count;
 
     ASSERT(v == current || !vcpu_runnable(v));
 
@@ -1313,23 +1314,18 @@ struct vmx_msr_entry *vmx_find_msr(const
     {
     case VMX_MSR_HOST:
         start    = vmx->host_msr_area;
-        substart = 0;
         subend   = vmx->host_msr_count;
         total    = subend;
         break;
 
     case VMX_MSR_GUEST:
         start    = vmx->msr_area;
-        substart = 0;
-        subend   = vmx->msr_save_count;
-        total    = vmx->msr_load_count;
         break;
 
     case VMX_MSR_GUEST_LOADONLY:
         start    = vmx->msr_area;
-        substart = vmx->msr_save_count;
-        subend   = vmx->msr_load_count;
-        total    = subend;
+        substart = subend;
+        subend   = total;
         break;
 
     default:
@@ -1461,7 +1457,8 @@ int vmx_del_msr(struct vcpu *v, uint32_t
 {
     struct arch_vmx_struct *vmx = &v->arch.hvm_vmx;
     struct vmx_msr_entry *start = NULL, *ent, *end;
-    unsigned int substart, subend, total;
+    unsigned int substart = 0, subend = vmx->msr_save_count;
+    unsigned int total = vmx->msr_load_count;
 
     ASSERT(v == current || !vcpu_runnable(v));
 
@@ -1469,23 +1466,18 @@ int vmx_del_msr(struct vcpu *v, uint32_t
     {
     case VMX_MSR_HOST:
         start    = vmx->host_msr_area;
-        substart = 0;
         subend   = vmx->host_msr_count;
         total    = subend;
         break;
 
     case VMX_MSR_GUEST:
         start    = vmx->msr_area;
-        substart = 0;
-        subend   = vmx->msr_save_count;
-        total    = vmx->msr_load_count;
         break;
 
     case VMX_MSR_GUEST_LOADONLY:
         start    = vmx->msr_area;
-        substart = vmx->msr_save_count;
-        subend   = vmx->msr_load_count;
-        total    = subend;
+        substart = subend;
+        subend   = total;
         break;
 
     default:





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] VMX: fix vmx_{find,del}_msr() build

[Andrew Cooper]

On 16/07/18 17:46, Jan Beulich wrote:
> For a reason that I can't explain, it is only the shim build that fails
> for me with an older gcc due to the compiler not recognizing that
> apparently uninitialized variables aren't really uninitialized.

The only thing that comes to mind is some differences in CFLAGS et al. 
There is nothing in kconfig which would plausibly impact that code.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] VMX: fix vmx_{find,del}_msr() build

[Jan Beulich]

>>> On 16.07.18 at 18:56, <andrew.cooper3@citrix.com> wrote:
> On 16/07/18 17:46, Jan Beulich wrote:
>> For a reason that I can't explain, it is only the shim build that fails
>> for me with an older gcc due to the compiler not recognizing that
>> apparently uninitialized variables aren't really uninitialized.
> 
> The only thing that comes to mind is some differences in CFLAGS et al. 
> There is nothing in kconfig which would plausibly impact that code.

Oh, right - looks like the shim build inherits the tool stack's CFLAGS,
which specify -O0 in debug builds. Otoh I'm surprised the hypervisor
builds at all with -O0, so I guess I'll have to look at that in some
more detail, not the least because xen/Rules.mk uses += to insert
its own -O<n>. In any event, comparing the two object files clearly
suggests a difference in optimization level.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] VMX: fix vmx_{find,del}_msr() build

[Andrew Cooper]

On 17/07/2018 07:57, Jan Beulich wrote:
>>>> On 16.07.18 at 18:56, <andrew.cooper3@citrix.com> wrote:
>> On 16/07/18 17:46, Jan Beulich wrote:
>>> For a reason that I can't explain, it is only the shim build that fails
>>> for me with an older gcc due to the compiler not recognizing that
>>> apparently uninitialized variables aren't really uninitialized.
>> The only thing that comes to mind is some differences in CFLAGS et al. 
>> There is nothing in kconfig which would plausibly impact that code.
> Oh, right - looks like the shim build inherits the tool stack's CFLAGS,
> which specify -O0 in debug builds. Otoh I'm surprised the hypervisor
> builds at all with -O0, so I guess I'll have to look at that in some
> more detail, not the least because xen/Rules.mk uses += to insert
> its own -O<n>. In any event, comparing the two object files clearly
> suggests a difference in optimization level.

Hmm - interesting.  I had intended not to inherit the tools dubious
CFLAGS setting (nothing should use O0, not even for debugging), but how
that I think back, that probably fell through the cracks in the
associated chaos.

Nevertheless, all of our code should build at any optimisation level.  I
know from other attempts that Xen also doesn't build at O3, and we could
do with a way to select the optimisation level, independently of debug
settings.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] VMX: fix vmx_{find,del}_msr() build

[Jan Beulich]

>>> On 16.07.18 at 18:56, <andrew.cooper3@citrix.com> wrote:
> On 16/07/18 17:46, Jan Beulich wrote:
>> For a reason that I can't explain, it is only the shim build that fails
>> for me with an older gcc due to the compiler not recognizing that
>> apparently uninitialized variables aren't really uninitialized.
> 
> The only thing that comes to mind is some differences in CFLAGS et al. 
> There is nothing in kconfig which would plausibly impact that code.

Indeed - I was under the wrong impression that one of my configs
on that box had CONFIG_DEBUG=n, but that was wrong. Switching
the supposed config back into that mode makes the issue surface in
a non-shim build as well. The difference is -O1 vs -O2; the latter
allows the compiler to kind of see "deeper" through expressions. So
I think the patch should be considered as is, perhaps with the extra
question whether to make vmx_add_msr() match the two changed
functions, even if the problem doesn't surface there (presumably
due to the "return" in the initial switch statement).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] VMX: fix vmx_{find,del}_msr() build

[Wei Liu]

On Mon, Jul 16, 2018 at 10:46:11AM -0600, Jan Beulich wrote:
> For a reason that I can't explain, it is only the shim build that fails
> for me with an older gcc due to the compiler not recognizing that
> apparently uninitialized variables aren't really uninitialized. Pull out
> the assignments used by two of the three case blocks and make them
> initializers of the variables, as I think I had suggested during review.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Code:

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

But please update the commit message to say it is caused by compiler
optimisation level.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] VMX: fix vmx_{find,del}_msr() build

[Jan Beulich]

>>> On 18.07.18 at 11:36, <wei.liu2@citrix.com> wrote:
> On Mon, Jul 16, 2018 at 10:46:11AM -0600, Jan Beulich wrote:
>> For a reason that I can't explain, it is only the shim build that fails
>> for me with an older gcc due to the compiler not recognizing that
>> apparently uninitialized variables aren't really uninitialized. Pull out
>> the assignments used by two of the three case blocks and make them
>> initializers of the variables, as I think I had suggested during review.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Code:
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> 
> But please update the commit message to say it is caused by compiler
> optimisation level.

Sure, now that it's clear what the reason is. It now reads:

Older gcc at -O2 (and perhaps higher) does not recognize that apparently
uninitialized variables aren't really uninitialized. Pull out the
assignments used by two of the three case blocks and make them
initializers of the variables, as I think I had suggested during review.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] VMX: fix vmx_{find,del}_msr() build

[Wei Liu]

On Wed, Jul 18, 2018 at 03:39:20AM -0600, Jan Beulich wrote:
> >>> On 18.07.18 at 11:36, <wei.liu2@citrix.com> wrote:
> > On Mon, Jul 16, 2018 at 10:46:11AM -0600, Jan Beulich wrote:
> >> For a reason that I can't explain, it is only the shim build that fails
> >> for me with an older gcc due to the compiler not recognizing that
> >> apparently uninitialized variables aren't really uninitialized. Pull out
> >> the assignments used by two of the three case blocks and make them
> >> initializers of the variables, as I think I had suggested during review.
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Code:
> > 
> > Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > But please update the commit message to say it is caused by compiler
> > optimisation level.
> 
> Sure, now that it's clear what the reason is. It now reads:
> 
> Older gcc at -O2 (and perhaps higher) does not recognize that apparently
> uninitialized variables aren't really uninitialized. Pull out the
> assignments used by two of the three case blocks and make them
> initializers of the variables, as I think I had suggested during review.

LGTM.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] VMX: fix vmx_{find,del}_msr() build

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, July 18, 2018 5:39 PM
> 
> >>> On 18.07.18 at 11:36, <wei.liu2@citrix.com> wrote:
> > On Mon, Jul 16, 2018 at 10:46:11AM -0600, Jan Beulich wrote:
> >> For a reason that I can't explain, it is only the shim build that fails
> >> for me with an older gcc due to the compiler not recognizing that
> >> apparently uninitialized variables aren't really uninitialized. Pull out
> >> the assignments used by two of the three case blocks and make them
> >> initializers of the variables, as I think I had suggested during review.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > Code:
> >
> > Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> >
> > But please update the commit message to say it is caused by compiler
> > optimisation level.
> 
> Sure, now that it's clear what the reason is. It now reads:
> 
> Older gcc at -O2 (and perhaps higher) does not recognize that apparently
> uninitialized variables aren't really uninitialized. Pull out the
> assignments used by two of the three case blocks and make them
> initializers of the variables, as I think I had suggested during review.
> 

Acked-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] VMX: fix vmx_{find,del}_msr() build

[Wei Liu]

On Tue, Jul 17, 2018 at 09:39:03AM +0100, Andrew Cooper wrote:
> On 17/07/2018 07:57, Jan Beulich wrote:
> >>>> On 16.07.18 at 18:56, <andrew.cooper3@citrix.com> wrote:
> >> On 16/07/18 17:46, Jan Beulich wrote:
> >>> For a reason that I can't explain, it is only the shim build that fails
> >>> for me with an older gcc due to the compiler not recognizing that
> >>> apparently uninitialized variables aren't really uninitialized.
> >> The only thing that comes to mind is some differences in CFLAGS et al. 
> >> There is nothing in kconfig which would plausibly impact that code.
> > Oh, right - looks like the shim build inherits the tool stack's CFLAGS,
> > which specify -O0 in debug builds. Otoh I'm surprised the hypervisor
> > builds at all with -O0, so I guess I'll have to look at that in some
> > more detail, not the least because xen/Rules.mk uses += to insert
> > its own -O<n>. In any event, comparing the two object files clearly
> > suggests a difference in optimization level.
> 
> Hmm - interesting.  I had intended not to inherit the tools dubious
> CFLAGS setting (nothing should use O0, not even for debugging), but how
> that I think back, that probably fell through the cracks in the
> associated chaos.
> 
> Nevertheless, all of our code should build at any optimisation level.  I
> know from other attempts that Xen also doesn't build at O3, and we could
> do with a way to select the optimisation level, independently of debug
> settings.

I agree with what you said but unfortunately reworking the build system
is rather low on my priority list.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel