| Message ID | 56C704F6.5000604@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2016-02-19 13:05, Paolo Bonzini wrote: > > > On 19/02/2016 12:11, Aurelien Jarno wrote: > > On 2015-08-29 17:49, Valdis Kletnieks wrote: > >> Compiler warning: > >> > >> CC [M] arch/x86/kvm/emulate.o > >> arch/x86/kvm/emulate.c: In function "__do_insn_fetch_bytes": > >> arch/x86/kvm/emulate.c:814:9: warning: "linear" may be used uninitialized in this function [-Wmaybe-uninitialized] > >> > >> GCC is smart enough to realize that the inlined __linearize may return before > >> setting the value of linear, but not smart enough to realize the same > >> X86EMU_CONTINUE blocks actual use of the value. However, the value of > >> 'linear' can only be set to one value, so hoisting the one line of code > >> upwards makes GCC happy with the code. > >> > >> Reported-by: Aruna Hewapathirane <aruna.hewapathirane@gmail.com> > >> Tested-by: Aruna Hewapathirane <aruna.hewapathirane@gmail.com> > >> Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu> > >> > >> --- a/arch/x86/kvm/emulate.c.dist 2015-08-11 14:10:05.366061993 -0400 > >> +++ b/arch/x86/kvm/emulate.c 2015-08-29 13:43:13.014163958 -0400 > >> @@ -650,6 +650,7 @@ static __always_inline int __linearize(s > >> u16 sel; > >> > >> la = seg_base(ctxt, addr.seg) + addr.ea; > >> + *linear = la; > >> *max_size = 0; > >> switch (mode) { > >> case X86EMUL_MODE_PROT64: > >> @@ -693,7 +694,6 @@ static __always_inline int __linearize(s > >> } > >> if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0)) > >> return emulate_gp(ctxt, 0); > >> - *linear = la; > >> return X86EMUL_CONTINUE; > >> bad: > >> if (addr.seg == VCPU_SREG_SS) > >> > > > > Unfortunately this patch broke GNU/Hurd when running under KVM. It fails > > to boot almost immediately. I haven't debug it more, but it looks like > > *linear should not always be written. This can easily be reproduced by > > trying to boot Debian Installer from this ISO: > > > > http://ftp.debian-ports.org/debian-cd/hurd-i386/debian-hurd-2015/debian-hurd-2015-i386-CD-1.iso > > The bug is that la can be changed by the "la &= (u32)-1" line. > > So the fix could be like: > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index 1505587d06e9..b9b09fec173b 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -650,10 +650,10 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt, > u16 sel; > > la = seg_base(ctxt, addr.seg) + addr.ea; > - *linear = la; > *max_size = 0; > switch (mode) { > case X86EMUL_MODE_PROT64: > + *linear = la; > if (is_noncanonical_address(la)) > goto bad; > > @@ -662,6 +662,7 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt, > goto bad; > break; > default: > + *linear = la = (u32)la; > usable = ctxt->ops->get_segment(ctxt, &sel, &desc, NULL, > addr.seg); > if (!usable) > @@ -689,7 +690,6 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt, > if (size > *max_size) > goto bad; > } > - la &= (u32)-1; > break; > } > if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0)) > > > Can you test it? Sorry about the other mail, I missed this one. I have just tested you patch above, and I confirm it works fine. Thanks. Tested-by: Aurelien Jarno <aurelien@aurel32.net>
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index 1505587d06e9..b9b09fec173b 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -650,10 +650,10 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt, u16 sel; la = seg_base(ctxt, addr.seg) + addr.ea; - *linear = la; *max_size = 0; switch (mode) { case X86EMUL_MODE_PROT64: + *linear = la; if (is_noncanonical_address(la)) goto bad; @@ -662,6 +662,7 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt, goto bad; break; default: + *linear = la = (u32)la; usable = ctxt->ops->get_segment(ctxt, &sel, &desc, NULL, addr.seg); if (!usable) @@ -689,7 +690,6 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt, if (size > *max_size) goto bad; } - la &= (u32)-1; break; } if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0))