| Message ID | c668f355-0a31-0ffb-a7c2-4fee46705a3e@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes to debugging facilities | expand |
On 24.08.2023 17:25, Jinoh Kang wrote: > Prepare for an upcoming patch that overloads the 'cr2' field for #DB. Seeing the subsequent change and the fact that earlier on Andrew didn't need such an adjustment, I'm afraid I can't see the need for this change, and the one sentence above also doesn't answer the "Why?", but only the "What?" Jan > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -503,9 +503,14 @@ void hvm_migrate_pirqs(struct vcpu *v) > > static bool hvm_get_pending_event(struct vcpu *v, struct x86_event *info) > { > - info->cr2 = v->arch.hvm.guest_cr[2]; > + if ( !alternative_call(hvm_funcs.get_pending_event, v, info) ) > + return false; > + > + if ( info->type == X86_EVENTTYPE_HW_EXCEPTION && > + info->vector == X86_EXC_PF ) > + info->cr2 = v->arch.hvm.guest_cr[2]; > > - return alternative_call(hvm_funcs.get_pending_event, v, info); > + return true; > } > > void hvm_do_resume(struct vcpu *v)
On 30/08/2023 2:41 pm, Jan Beulich wrote: > On 24.08.2023 17:25, Jinoh Kang wrote: >> Prepare for an upcoming patch that overloads the 'cr2' field for #DB. > Seeing the subsequent change and the fact that earlier on Andrew didn't > need such an adjustment, I'm afraid I can't see the need for this change, > and the one sentence above also doesn't answer the "Why?", but only the > "What?" I agree WRT the commit message. I don't see why, but I also didn't spot this specific bug so I can't rule out a bug in my original series. That said, my original series was RFC because of the Monitor breakage and didn't get much testing. > > Jan > >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -503,9 +503,14 @@ void hvm_migrate_pirqs(struct vcpu *v) >> >> static bool hvm_get_pending_event(struct vcpu *v, struct x86_event *info) >> { >> - info->cr2 = v->arch.hvm.guest_cr[2]; >> + if ( !alternative_call(hvm_funcs.get_pending_event, v, info) ) >> + return false; >> + >> + if ( info->type == X86_EVENTTYPE_HW_EXCEPTION && >> + info->vector == X86_EXC_PF ) >> + info->cr2 = v->arch.hvm.guest_cr[2]; For the change itself, this needs pushing down into the vmx/svm hooks, because guest_cr[2] has different liveness between Intel and AMD, and this callpath needs to work for both scheduled-in and scheduled-out guests. On AMD, I think you need to pull it straight out of the VMCB, rather than relying on this being correct for a scheduled-in guest. ~Andrew
On 8/30/23 22:41, Jan Beulich wrote: > On 24.08.2023 17:25, Jinoh Kang wrote: >> Prepare for an upcoming patch that overloads the 'cr2' field for #DB. > > Seeing the subsequent change and the fact that earlier on Andrew didn't > need such an adjustment, I'm afraid I can't see the need for this change, > and the one sentence above also doesn't answer the "Why?", but only the > "What?" This is part of the hvm_monitor_interrupt() fix (patch 4), which would otherwise get CR2 value (instead of PENDING_DBG) even for #DB. I might have been overzealous, though, since there is no known (broken) use for VM_EVENT_REASON_INTERRUPT in the first place. > > Jan > >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -503,9 +503,14 @@ void hvm_migrate_pirqs(struct vcpu *v) >> >> static bool hvm_get_pending_event(struct vcpu *v, struct x86_event *info) >> { >> - info->cr2 = v->arch.hvm.guest_cr[2]; >> + if ( !alternative_call(hvm_funcs.get_pending_event, v, info) ) >> + return false; >> + >> + if ( info->type == X86_EVENTTYPE_HW_EXCEPTION && >> + info->vector == X86_EXC_PF ) >> + info->cr2 = v->arch.hvm.guest_cr[2]; >> >> - return alternative_call(hvm_funcs.get_pending_event, v, info); >> + return true; >> } >> >> void hvm_do_resume(struct vcpu *v) >
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 66ead0b878..c726947ccb 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -503,9 +503,14 @@ void hvm_migrate_pirqs(struct vcpu *v) static bool hvm_get_pending_event(struct vcpu *v, struct x86_event *info) { - info->cr2 = v->arch.hvm.guest_cr[2]; + if ( !alternative_call(hvm_funcs.get_pending_event, v, info) ) + return false; + + if ( info->type == X86_EVENTTYPE_HW_EXCEPTION && + info->vector == X86_EXC_PF ) + info->cr2 = v->arch.hvm.guest_cr[2]; - return alternative_call(hvm_funcs.get_pending_event, v, info); + return true; } void hvm_do_resume(struct vcpu *v)
Prepare for an upcoming patch that overloads the 'cr2' field for #DB. Signed-off-by: Jinoh Kang <jinoh.kang.kr@gmail.com> --- CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Tamas K Lengyel <tamas@tklengyel.com> CC: Alexandru Isaila <aisaila@bitdefender.com> CC: Petre Pircalabu <ppircalabu@bitdefender.com> --- xen/arch/x86/hvm/hvm.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)