| Message ID | 58247671020000780011DA5A@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 10, 2016 at 05:30:25AM -0700, Jan Beulich wrote: > With us incremementally adding proper CPUID checks to x86_emulate() > (see commit de05bd965a ["x86emul: correct {,F}CMOV and F{,U}COMI{,P} > emulation"]) it is no longer appropriate to invoke the function with > that hook being NULL, as long as respective instructions may get used > in that case. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > I've noticed the problem first with a not yet submitted patch checking > cx8 (PV 32-bit kernels occasionally use cmpxchg8b for page table > accesses), and I think we don't strictly need the change here for 4.8, > but then again it may be better to handle this properly right away. I'm fine with having this if we have some free cycles. If it is reviewed today, feel free to commit it. Otherwise I will try to sneak it in after it is reviewed. Wei.
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 10 November 2016 12:30 > To: xen-devel <xen-devel@lists.xenproject.org> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant > <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Tim (Xen.org) > <tim@xen.org> > Subject: [PATCH] x86: always supply .cpuid() handler to x86_emulate() > > With us incremementally adding proper CPUID checks to x86_emulate() > (see commit de05bd965a ["x86emul: correct {,F}CMOV and F{,U}COMI{,P} > emulation"]) it is no longer appropriate to invoke the function with > that hook being NULL, as long as respective instructions may get used > in that case. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> This all looks sensible to me and I'm kind of surprised we got away without it before. Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > --- > I've noticed the problem first with a not yet submitted patch checking > cx8 (PV 32-bit kernels occasionally use cmpxchg8b for page table > accesses), and I think we don't strictly need the change here for 4.8, > but then again it may be better to handle this properly right away. > > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -1542,7 +1542,7 @@ static int hvmemul_wbinvd( > return X86EMUL_OKAY; > } > > -static int hvmemul_cpuid( > +int hvmemul_cpuid( > unsigned int *eax, > unsigned int *ebx, > unsigned int *ecx, > @@ -1892,11 +1892,13 @@ int hvm_emulate_one_mmio(unsigned long m > .read = x86emul_unhandleable_rw, > .insn_fetch = hvmemul_insn_fetch, > .write = mmcfg_intercept_write, > + .cpuid = hvmemul_cpuid, > }; > static const struct x86_emulate_ops hvm_ro_emulate_ops_mmio = { > .read = x86emul_unhandleable_rw, > .insn_fetch = hvmemul_insn_fetch, > - .write = mmio_ro_emulated_write > + .write = mmio_ro_emulated_write, > + .cpuid = hvmemul_cpuid, > }; > struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla }; > struct hvm_emulate_ctxt ctxt; > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -5327,6 +5327,7 @@ static const struct x86_emulate_ops ptwr > .insn_fetch = ptwr_emulated_read, > .write = ptwr_emulated_write, > .cmpxchg = ptwr_emulated_cmpxchg, > + .cpuid = pv_emul_cpuid, > }; > > /* Write page fault handler: check if guest is trying to modify a PTE. */ > @@ -5414,6 +5415,7 @@ static const struct x86_emulate_ops mmio > .read = x86emul_unhandleable_rw, > .insn_fetch = ptwr_emulated_read, > .write = mmio_ro_emulated_write, > + .cpuid = pv_emul_cpuid, > }; > > int mmcfg_intercept_write( > @@ -5451,6 +5453,7 @@ static const struct x86_emulate_ops mmcf > .read = x86emul_unhandleable_rw, > .insn_fetch = ptwr_emulated_read, > .write = mmcfg_intercept_write, > + .cpuid = pv_emul_cpuid, > }; > > /* Check if guest is trying to modify a r/o MMIO page. */ > --- a/xen/arch/x86/mm/shadow/common.c > +++ b/xen/arch/x86/mm/shadow/common.c > @@ -306,6 +306,7 @@ static const struct x86_emulate_ops hvm_ > .insn_fetch = hvm_emulate_insn_fetch, > .write = hvm_emulate_write, > .cmpxchg = hvm_emulate_cmpxchg, > + .cpuid = hvmemul_cpuid, > }; > > static int > @@ -374,6 +375,7 @@ static const struct x86_emulate_ops pv_s > .insn_fetch = pv_emulate_read, > .write = pv_emulate_write, > .cmpxchg = pv_emulate_cmpxchg, > + .cpuid = pv_emul_cpuid, > }; > > const struct x86_emulate_ops *shadow_init_emulation( > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -2755,6 +2755,24 @@ static int priv_op_write_msr(unsigned in > return X86EMUL_UNHANDLEABLE; > } > > +int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx, > + unsigned int *edx, struct x86_emulate_ctxt *ctxt) > +{ > + struct cpu_user_regs regs = *ctxt->regs; > + > + regs._eax = *eax; > + regs._ecx = *ecx; > + > + pv_cpuid(®s); > + > + *eax = regs._eax; > + *ebx = regs._ebx; > + *ecx = regs._ecx; > + *edx = regs._edx; > + > + return X86EMUL_OKAY; > +} > + > /* Instruction fetch with error handling. */ > #define insn_fetch(type, base, eip, limit) \ > ({ unsigned long _rc, _ptr = (base) + (eip); \ > --- a/xen/include/asm-x86/hvm/emulate.h > +++ b/xen/include/asm-x86/hvm/emulate.h > @@ -60,6 +60,12 @@ void hvm_emulate_init( > unsigned int insn_bytes); > void hvm_emulate_writeback( > struct hvm_emulate_ctxt *hvmemul_ctxt); > +int hvmemul_cpuid( > + unsigned int *eax, > + unsigned int *ebx, > + unsigned int *ecx, > + unsigned int *edx, > + struct x86_emulate_ctxt *ctxt); > struct segment_register *hvmemul_get_seg_reg( > enum x86_segment seg, > struct hvm_emulate_ctxt *hvmemul_ctxt); > --- a/xen/include/asm-x86/mm.h > +++ b/xen/include/asm-x86/mm.h > @@ -504,6 +504,8 @@ extern int mmcfg_intercept_write(enum x8 > void *p_data, > unsigned int bytes, > struct x86_emulate_ctxt *ctxt); > +int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx, > + unsigned int *edx, struct x86_emulate_ctxt *ctxt); > > int ptwr_do_page_fault(struct vcpu *, unsigned long, > struct cpu_user_regs *); >
On 10/11/16 12:30, Jan Beulich wrote: > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -2755,6 +2755,24 @@ static int priv_op_write_msr(unsigned in > return X86EMUL_UNHANDLEABLE; > } > > +int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx, > + unsigned int *edx, struct x86_emulate_ctxt *ctxt) > +{ > + struct cpu_user_regs regs = *ctxt->regs; You need a CPUID faulting check here, matching the hvm side of things, or you will leave a latent bug which gets exposed when switching to using full x86_emulate() for PV guests. Would it be wise to add a fail_if(!ops->cpuid) to x86_emulate() to catch other misuses? ~Andrew > + > + regs._eax = *eax; > + regs._ecx = *ecx; > + > + pv_cpuid(®s); > + > + *eax = regs._eax; > + *ebx = regs._ebx; > + *ecx = regs._ecx; > + *edx = regs._edx; > + > + return X86EMUL_OKAY; > +} > + > /* Instruction fetch with error handling. */ > #define insn_fetch(type, base, eip, limit) \ > ({ unsigned long _rc, _ptr = (base) + (eip); \
>>> On 11.11.16 at 15:16, <andrew.cooper3@citrix.com> wrote: > On 10/11/16 12:30, Jan Beulich wrote: >> --- a/xen/arch/x86/traps.c >> +++ b/xen/arch/x86/traps.c >> @@ -2755,6 +2755,24 @@ static int priv_op_write_msr(unsigned in >> return X86EMUL_UNHANDLEABLE; >> } >> >> +int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx, >> + unsigned int *edx, struct x86_emulate_ctxt *ctxt) >> +{ >> + struct cpu_user_regs regs = *ctxt->regs; > > You need a CPUID faulting check here, matching the hvm side of things, > or you will leave a latent bug which gets exposed when switching to > using full x86_emulate() for PV guests. That addition is part of the patch actually switching to x86_emulate(). I don't think it belongs here, as for now we don't mean to emulate CPUID insns. > Would it be wise to add a fail_if(!ops->cpuid) to x86_emulate() to catch > other misuses? I don't think so - there may be cases where it's indeed unneeded (and I foresee such arising quickly once the ->validate() hooks is in place, addition of which is now too part of aforementioned patch). Jan
On 11/11/16 14:58, Jan Beulich wrote: >>>> On 11.11.16 at 15:16, <andrew.cooper3@citrix.com> wrote: >> On 10/11/16 12:30, Jan Beulich wrote: >>> --- a/xen/arch/x86/traps.c >>> +++ b/xen/arch/x86/traps.c >>> @@ -2755,6 +2755,24 @@ static int priv_op_write_msr(unsigned in >>> return X86EMUL_UNHANDLEABLE; >>> } >>> >>> +int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx, >>> + unsigned int *edx, struct x86_emulate_ctxt *ctxt) >>> +{ >>> + struct cpu_user_regs regs = *ctxt->regs; >> You need a CPUID faulting check here, matching the hvm side of things, >> or you will leave a latent bug which gets exposed when switching to >> using full x86_emulate() for PV guests. > That addition is part of the patch actually switching to x86_emulate(). > I don't think it belongs here, as for now we don't mean to emulate > CPUID insns. Fine, so long as it doesn't get forgotten. > >> Would it be wise to add a fail_if(!ops->cpuid) to x86_emulate() to catch >> other misuses? > I don't think so - there may be cases where it's indeed unneeded > (and I foresee such arising quickly once the ->validate() hooks is > in place, addition of which is now too part of aforementioned patch). Ok. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> On 11.11.16 at 14:54, <wei.liu2@citrix.com> wrote: > On Thu, Nov 10, 2016 at 05:30:25AM -0700, Jan Beulich wrote: >> With us incremementally adding proper CPUID checks to x86_emulate() >> (see commit de05bd965a ["x86emul: correct {,F}CMOV and F{,U}COMI{,P} >> emulation"]) it is no longer appropriate to invoke the function with >> that hook being NULL, as long as respective instructions may get used >> in that case. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> I've noticed the problem first with a not yet submitted patch checking >> cx8 (PV 32-bit kernels occasionally use cmpxchg8b for page table >> accesses), and I think we don't strictly need the change here for 4.8, >> but then again it may be better to handle this properly right away. > > I'm fine with having this if we have some free cycles. If it is reviewed > today, feel free to commit it. Otherwise I will try to sneak it in after > it is reviewed. In the hope that you don't mind, I've translated this to R-a-b. Jan
On Fri, Nov 11, 2016 at 09:21:00AM -0700, Jan Beulich wrote: > >>> On 11.11.16 at 14:54, <wei.liu2@citrix.com> wrote: > > On Thu, Nov 10, 2016 at 05:30:25AM -0700, Jan Beulich wrote: > >> With us incremementally adding proper CPUID checks to x86_emulate() > >> (see commit de05bd965a ["x86emul: correct {,F}CMOV and F{,U}COMI{,P} > >> emulation"]) it is no longer appropriate to invoke the function with > >> that hook being NULL, as long as respective instructions may get used > >> in that case. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> --- > >> I've noticed the problem first with a not yet submitted patch checking > >> cx8 (PV 32-bit kernels occasionally use cmpxchg8b for page table > >> accesses), and I think we don't strictly need the change here for 4.8, > >> but then again it may be better to handle this properly right away. > > > > I'm fine with having this if we have some free cycles. If it is reviewed > > today, feel free to commit it. Otherwise I will try to sneak it in after > > it is reviewed. > > In the hope that you don't mind, I've translated this to R-a-b. That's absolutely fine. > > Jan >
--- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -1542,7 +1542,7 @@ static int hvmemul_wbinvd( return X86EMUL_OKAY; } -static int hvmemul_cpuid( +int hvmemul_cpuid( unsigned int *eax, unsigned int *ebx, unsigned int *ecx, @@ -1892,11 +1892,13 @@ int hvm_emulate_one_mmio(unsigned long m .read = x86emul_unhandleable_rw, .insn_fetch = hvmemul_insn_fetch, .write = mmcfg_intercept_write, + .cpuid = hvmemul_cpuid, }; static const struct x86_emulate_ops hvm_ro_emulate_ops_mmio = { .read = x86emul_unhandleable_rw, .insn_fetch = hvmemul_insn_fetch, - .write = mmio_ro_emulated_write + .write = mmio_ro_emulated_write, + .cpuid = hvmemul_cpuid, }; struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla }; struct hvm_emulate_ctxt ctxt; --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -5327,6 +5327,7 @@ static const struct x86_emulate_ops ptwr .insn_fetch = ptwr_emulated_read, .write = ptwr_emulated_write, .cmpxchg = ptwr_emulated_cmpxchg, + .cpuid = pv_emul_cpuid, }; /* Write page fault handler: check if guest is trying to modify a PTE. */ @@ -5414,6 +5415,7 @@ static const struct x86_emulate_ops mmio .read = x86emul_unhandleable_rw, .insn_fetch = ptwr_emulated_read, .write = mmio_ro_emulated_write, + .cpuid = pv_emul_cpuid, }; int mmcfg_intercept_write( @@ -5451,6 +5453,7 @@ static const struct x86_emulate_ops mmcf .read = x86emul_unhandleable_rw, .insn_fetch = ptwr_emulated_read, .write = mmcfg_intercept_write, + .cpuid = pv_emul_cpuid, }; /* Check if guest is trying to modify a r/o MMIO page. */ --- a/xen/arch/x86/mm/shadow/common.c +++ b/xen/arch/x86/mm/shadow/common.c @@ -306,6 +306,7 @@ static const struct x86_emulate_ops hvm_ .insn_fetch = hvm_emulate_insn_fetch, .write = hvm_emulate_write, .cmpxchg = hvm_emulate_cmpxchg, + .cpuid = hvmemul_cpuid, }; static int @@ -374,6 +375,7 @@ static const struct x86_emulate_ops pv_s .insn_fetch = pv_emulate_read, .write = pv_emulate_write, .cmpxchg = pv_emulate_cmpxchg, + .cpuid = pv_emul_cpuid, }; const struct x86_emulate_ops *shadow_init_emulation( --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -2755,6 +2755,24 @@ static int priv_op_write_msr(unsigned in return X86EMUL_UNHANDLEABLE; } +int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx, + unsigned int *edx, struct x86_emulate_ctxt *ctxt) +{ + struct cpu_user_regs regs = *ctxt->regs; + + regs._eax = *eax; + regs._ecx = *ecx; + + pv_cpuid(®s); + + *eax = regs._eax; + *ebx = regs._ebx; + *ecx = regs._ecx; + *edx = regs._edx; + + return X86EMUL_OKAY; +} + /* Instruction fetch with error handling. */ #define insn_fetch(type, base, eip, limit) \ ({ unsigned long _rc, _ptr = (base) + (eip); \ --- a/xen/include/asm-x86/hvm/emulate.h +++ b/xen/include/asm-x86/hvm/emulate.h @@ -60,6 +60,12 @@ void hvm_emulate_init( unsigned int insn_bytes); void hvm_emulate_writeback( struct hvm_emulate_ctxt *hvmemul_ctxt); +int hvmemul_cpuid( + unsigned int *eax, + unsigned int *ebx, + unsigned int *ecx, + unsigned int *edx, + struct x86_emulate_ctxt *ctxt); struct segment_register *hvmemul_get_seg_reg( enum x86_segment seg, struct hvm_emulate_ctxt *hvmemul_ctxt); --- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -504,6 +504,8 @@ extern int mmcfg_intercept_write(enum x8 void *p_data, unsigned int bytes, struct x86_emulate_ctxt *ctxt); +int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx, + unsigned int *edx, struct x86_emulate_ctxt *ctxt); int ptwr_do_page_fault(struct vcpu *, unsigned long, struct cpu_user_regs *);
With us incremementally adding proper CPUID checks to x86_emulate() (see commit de05bd965a ["x86emul: correct {,F}CMOV and F{,U}COMI{,P} emulation"]) it is no longer appropriate to invoke the function with that hook being NULL, as long as respective instructions may get used in that case. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I've noticed the problem first with a not yet submitted patch checking cx8 (PV 32-bit kernels occasionally use cmpxchg8b for page table accesses), and I think we don't strictly need the change here for 4.8, but then again it may be better to handle this properly right away. x86: always supply .cpuid() handler to x86_emulate() With us incremementally adding proper CPUID checks to x86_emulate() (see commit de05bd965a ["x86emul: correct {,F}CMOV and F{,U}COMI{,P} emulation"]) it is no longer appropriate to invoke the function with that hook being NULL, as long as respective instructions may get used in that case. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I've noticed the problem first with a not yet submitted patch checking cx8 (PV 32-bit kernels occasionally use cmpxchg8b for page table accesses), and I think we don't strictly need the change here for 4.8, but then again it may be better to handle this properly right away. --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -1542,7 +1542,7 @@ static int hvmemul_wbinvd( return X86EMUL_OKAY; } -static int hvmemul_cpuid( +int hvmemul_cpuid( unsigned int *eax, unsigned int *ebx, unsigned int *ecx, @@ -1892,11 +1892,13 @@ int hvm_emulate_one_mmio(unsigned long m .read = x86emul_unhandleable_rw, .insn_fetch = hvmemul_insn_fetch, .write = mmcfg_intercept_write, + .cpuid = hvmemul_cpuid, }; static const struct x86_emulate_ops hvm_ro_emulate_ops_mmio = { .read = x86emul_unhandleable_rw, .insn_fetch = hvmemul_insn_fetch, - .write = mmio_ro_emulated_write + .write = mmio_ro_emulated_write, + .cpuid = hvmemul_cpuid, }; struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla }; struct hvm_emulate_ctxt ctxt; --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -5327,6 +5327,7 @@ static const struct x86_emulate_ops ptwr .insn_fetch = ptwr_emulated_read, .write = ptwr_emulated_write, .cmpxchg = ptwr_emulated_cmpxchg, + .cpuid = pv_emul_cpuid, }; /* Write page fault handler: check if guest is trying to modify a PTE. */ @@ -5414,6 +5415,7 @@ static const struct x86_emulate_ops mmio .read = x86emul_unhandleable_rw, .insn_fetch = ptwr_emulated_read, .write = mmio_ro_emulated_write, + .cpuid = pv_emul_cpuid, }; int mmcfg_intercept_write( @@ -5451,6 +5453,7 @@ static const struct x86_emulate_ops mmcf .read = x86emul_unhandleable_rw, .insn_fetch = ptwr_emulated_read, .write = mmcfg_intercept_write, + .cpuid = pv_emul_cpuid, }; /* Check if guest is trying to modify a r/o MMIO page. */ --- a/xen/arch/x86/mm/shadow/common.c +++ b/xen/arch/x86/mm/shadow/common.c @@ -306,6 +306,7 @@ static const struct x86_emulate_ops hvm_ .insn_fetch = hvm_emulate_insn_fetch, .write = hvm_emulate_write, .cmpxchg = hvm_emulate_cmpxchg, + .cpuid = hvmemul_cpuid, }; static int @@ -374,6 +375,7 @@ static const struct x86_emulate_ops pv_s .insn_fetch = pv_emulate_read, .write = pv_emulate_write, .cmpxchg = pv_emulate_cmpxchg, + .cpuid = pv_emul_cpuid, }; const struct x86_emulate_ops *shadow_init_emulation( --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -2755,6 +2755,24 @@ static int priv_op_write_msr(unsigned in return X86EMUL_UNHANDLEABLE; } +int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx, + unsigned int *edx, struct x86_emulate_ctxt *ctxt) +{ + struct cpu_user_regs regs = *ctxt->regs; + + regs._eax = *eax; + regs._ecx = *ecx; + + pv_cpuid(®s); + + *eax = regs._eax; + *ebx = regs._ebx; + *ecx = regs._ecx; + *edx = regs._edx; + + return X86EMUL_OKAY; +} + /* Instruction fetch with error handling. */ #define insn_fetch(type, base, eip, limit) \ ({ unsigned long _rc, _ptr = (base) + (eip); \ --- a/xen/include/asm-x86/hvm/emulate.h +++ b/xen/include/asm-x86/hvm/emulate.h @@ -60,6 +60,12 @@ void hvm_emulate_init( unsigned int insn_bytes); void hvm_emulate_writeback( struct hvm_emulate_ctxt *hvmemul_ctxt); +int hvmemul_cpuid( + unsigned int *eax, + unsigned int *ebx, + unsigned int *ecx, + unsigned int *edx, + struct x86_emulate_ctxt *ctxt); struct segment_register *hvmemul_get_seg_reg( enum x86_segment seg, struct hvm_emulate_ctxt *hvmemul_ctxt); --- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -504,6 +504,8 @@ extern int mmcfg_intercept_write(enum x8 void *p_data, unsigned int bytes, struct x86_emulate_ctxt *ctxt); +int pv_emul_cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx, + unsigned int *edx, struct x86_emulate_ctxt *ctxt); int ptwr_do_page_fault(struct vcpu *, unsigned long, struct cpu_user_regs *);