| Message ID | 20230405204423.2113418-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/svm: Provide EXITINFO decodes for MOV CR/DR intercepts | expand |
On 05.04.2023 22:44, Andrew Cooper wrote: > This removes raw number manipulation, and makes the logic easier to follow. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> One remark though: > --- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h > +++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h > @@ -450,6 +450,11 @@ struct vmcb_struct { > > uint64_t nrip; > } io; > + struct { > + uint64_t gpr:4; > + uint64_t :59; > + bool mov_insn:1; /* MOV, as opposed to LMSW, CLTS, etc */ > + } mov; The field being named just "mov" makes it apparently applicable to DRn moves, too (and the title supports this), yet the top bit doesn't have this meaning there. So perhaps say "MOV-CR" (or alike) in the comment? Jan
On 06/04/2023 10:31 am, Jan Beulich wrote: > On 05.04.2023 22:44, Andrew Cooper wrote: >> This removes raw number manipulation, and makes the logic easier to follow. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. > > One remark though: > >> --- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h >> +++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h >> @@ -450,6 +450,11 @@ struct vmcb_struct { >> >> uint64_t nrip; >> } io; >> + struct { >> + uint64_t gpr:4; >> + uint64_t :59; >> + bool mov_insn:1; /* MOV, as opposed to LMSW, CLTS, etc */ >> + } mov; > The field being named just "mov" makes it apparently applicable to DRn > moves, too (and the title supports this), yet the top bit doesn't have > this meaning there. So perhaps say "MOV-CR" (or alike) in the comment? Hmm - I'd not spotted that distinction. Xen never decodes the exitinfo for a DR access - we just resync dr state, drop the intercept and reenter the guest. Therefore I think it would be better to rename "mov" to "mov_cr" so you can't use the mov_insn field in a context that plausibly looks like a dr access. Thoughts? ~Andrew
On 06.04.2023 11:52, Andrew Cooper wrote: > On 06/04/2023 10:31 am, Jan Beulich wrote: >> On 05.04.2023 22:44, Andrew Cooper wrote: >>> --- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h >>> +++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h >>> @@ -450,6 +450,11 @@ struct vmcb_struct { >>> >>> uint64_t nrip; >>> } io; >>> + struct { >>> + uint64_t gpr:4; >>> + uint64_t :59; >>> + bool mov_insn:1; /* MOV, as opposed to LMSW, CLTS, etc */ >>> + } mov; >> The field being named just "mov" makes it apparently applicable to DRn >> moves, too (and the title supports this), yet the top bit doesn't have >> this meaning there. So perhaps say "MOV-CR" (or alike) in the comment? > > Hmm - I'd not spotted that distinction. > > Xen never decodes the exitinfo for a DR access - we just resync dr > state, drop the intercept and reenter the guest. > > Therefore I think it would be better to rename "mov" to "mov_cr" so you > can't use the mov_insn field in a context that plausibly looks like a dr > access. > > Thoughts? That was my other thought; it merely seemed to me that updating the comment would allow future (if ever) use with MOV-DR. So yes, if you prefer going that route: Fine with me. Jan
On 06/04/2023 10:59 am, Jan Beulich wrote: > On 06.04.2023 11:52, Andrew Cooper wrote: >> On 06/04/2023 10:31 am, Jan Beulich wrote: >>> On 05.04.2023 22:44, Andrew Cooper wrote: >>>> --- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h >>>> +++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h >>>> @@ -450,6 +450,11 @@ struct vmcb_struct { >>>> >>>> uint64_t nrip; >>>> } io; >>>> + struct { >>>> + uint64_t gpr:4; >>>> + uint64_t :59; >>>> + bool mov_insn:1; /* MOV, as opposed to LMSW, CLTS, etc */ >>>> + } mov; >>> The field being named just "mov" makes it apparently applicable to DRn >>> moves, too (and the title supports this), yet the top bit doesn't have >>> this meaning there. So perhaps say "MOV-CR" (or alike) in the comment? >> Hmm - I'd not spotted that distinction. >> >> Xen never decodes the exitinfo for a DR access - we just resync dr >> state, drop the intercept and reenter the guest. >> >> Therefore I think it would be better to rename "mov" to "mov_cr" so you >> can't use the mov_insn field in a context that plausibly looks like a dr >> access. >> >> Thoughts? > That was my other thought; it merely seemed to me that updating the comment > would allow future (if ever) use with MOV-DR. So yes, if you prefer going > that route: Fine with me. Will do. I don't foresee us ever needing to inspect the dr decode information. ~Andrew
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index 8d8b250101ce..90c2f89c1b0d 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1740,7 +1740,7 @@ static void svm_vmexit_do_cr_access( cr = vmcb->exitcode - VMEXIT_CR0_READ; dir = (cr > 15); cr &= 0xf; - gp = vmcb->exitinfo1 & 0xf; + gp = vmcb->ei.mov.gpr; rc = dir ? hvm_mov_to_cr(cr, gp) : hvm_mov_from_cr(cr, gp); @@ -2961,7 +2961,7 @@ void svm_vmexit_handler(void) case VMEXIT_CR0_READ ... VMEXIT_CR15_READ: case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE: - if ( cpu_has_svm_decode && (vmcb->exitinfo1 & (1ULL << 63)) ) + if ( cpu_has_svm_decode && vmcb->ei.mov.mov_insn ) svm_vmexit_do_cr_access(vmcb, regs); else if ( !hvm_emulate_one_insn(x86_insn_is_cr_access, "CR access") ) hvm_inject_hw_exception(X86_EXC_GP, 0); diff --git a/xen/arch/x86/include/asm/hvm/svm/vmcb.h b/xen/arch/x86/include/asm/hvm/svm/vmcb.h index b809e85507aa..77e3bd9aa048 100644 --- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h +++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h @@ -450,6 +450,11 @@ struct vmcb_struct { uint64_t nrip; } io; + struct { + uint64_t gpr:4; + uint64_t :59; + bool mov_insn:1; /* MOV, as opposed to LMSW, CLTS, etc */ + } mov; struct { uint16_t sel; uint64_t :48;
This removes raw number manipulation, and makes the logic easier to follow. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> --- xen/arch/x86/hvm/svm/svm.c | 4 ++-- xen/arch/x86/include/asm/hvm/svm/vmcb.h | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-)