| Message ID | 20191203171030.11680-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/debug: Plumb pending_dbg through the monitor and devicemodel interfaces | expand |
> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h > index 959083d8c4..76676ff4c0 100644 > --- a/xen/include/public/vm_event.h > +++ b/xen/include/public/vm_event.h > @@ -281,6 +281,7 @@ struct vm_event_debug { > uint32_t insn_length; > uint8_t type; /* HVMOP_TRAP_* */ > uint8_t _pad[3]; > + uint64_t pending_dbg; This is just a nitpick but I would prefer if we had the _pad field as the last element in the struct and keep all 64-bit members up in the front. Thanks, Tamas
On 03/12/2019 18:05, Tamas K Lengyel wrote: >> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h >> index 959083d8c4..76676ff4c0 100644 >> --- a/xen/include/public/vm_event.h >> +++ b/xen/include/public/vm_event.h >> @@ -281,6 +281,7 @@ struct vm_event_debug { >> uint32_t insn_length; >> uint8_t type; /* HVMOP_TRAP_* */ >> uint8_t _pad[3]; >> + uint64_t pending_dbg; > This is just a nitpick but I would prefer if we had the _pad field as > the last element in the struct and keep all 64-bit members up in the > front. Well the reason I did it like this is that this version will continue to function with older introspection code. The extra field is within a union and no other data moves. By repositioning to the start, it will almost certainly break older introspection code even though it compiled correctly. Your choice. ~Andrew P.S. What is the point of tail-padding a struct inside a union?
On Tue, Dec 3, 2019 at 1:05 PM Tamas K Lengyel <tamas@tklengyel.com> wrote: > > > diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h > > index 959083d8c4..76676ff4c0 100644 > > --- a/xen/include/public/vm_event.h > > +++ b/xen/include/public/vm_event.h > > @@ -281,6 +281,7 @@ struct vm_event_debug { > > uint32_t insn_length; > > uint8_t type; /* HVMOP_TRAP_* */ > > uint8_t _pad[3]; > > + uint64_t pending_dbg; > > This is just a nitpick but I would prefer if we had the _pad field as > the last element in the struct and keep all 64-bit members up in the > front. Also, since pending_dbg uses unsigned int in Xen, do we need uint64_t for it here? Seems to me a uint32_t would suffice. Tamas
On Tue, Dec 3, 2019 at 1:09 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 03/12/2019 18:05, Tamas K Lengyel wrote: > >> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h > >> index 959083d8c4..76676ff4c0 100644 > >> --- a/xen/include/public/vm_event.h > >> +++ b/xen/include/public/vm_event.h > >> @@ -281,6 +281,7 @@ struct vm_event_debug { > >> uint32_t insn_length; > >> uint8_t type; /* HVMOP_TRAP_* */ > >> uint8_t _pad[3]; > >> + uint64_t pending_dbg; > > This is just a nitpick but I would prefer if we had the _pad field as > > the last element in the struct and keep all 64-bit members up in the > > front. > > Well the reason I did it like this is that this version will continue to > function with older introspection code. The extra field is within a > union and no other data moves. > > By repositioning to the start, it will almost certainly break older > introspection code even though it compiled correctly. > > Your choice. We are already bumping the interface version for the next release so old introspection code by design will stop working. We make no ABI stability guarantees between interface versions so this is a non-issue. > > ~Andrew > > P.S. What is the point of tail-padding a struct inside a union? To always make sure all structures used by the interface are 64-bit aligned without having to keep in mind which one is used in a union and which one isn't or having to remember their relative position in the overall structure. So it just reduces the cognitive load. Tamas
On 03/12/2019 18:09, Tamas K Lengyel wrote: > On Tue, Dec 3, 2019 at 1:05 PM Tamas K Lengyel <tamas@tklengyel.com> wrote: >>> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h >>> index 959083d8c4..76676ff4c0 100644 >>> --- a/xen/include/public/vm_event.h >>> +++ b/xen/include/public/vm_event.h >>> @@ -281,6 +281,7 @@ struct vm_event_debug { >>> uint32_t insn_length; >>> uint8_t type; /* HVMOP_TRAP_* */ >>> uint8_t _pad[3]; >>> + uint64_t pending_dbg; >> This is just a nitpick but I would prefer if we had the _pad field as >> the last element in the struct and keep all 64-bit members up in the >> front. > Also, since pending_dbg uses unsigned int in Xen, do we need uint64_t > for it here? Seems to me a uint32_t would suffice. Its %dr6 (but not quite, due to complexity with exception priorities, interrupt shadows, and backwards compatibility of the RTM bit with inverted polarity). All other registers have 64 bit fields in the interface. The only interesting bits in it fall within the first 32 which is why it is handled in a shorter way within Xen. Like %cr0, I don't expect anything interesting to appear in the upper 32 bits. ~Andrew
On Tue, Dec 3, 2019 at 1:18 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 03/12/2019 18:09, Tamas K Lengyel wrote: > > On Tue, Dec 3, 2019 at 1:05 PM Tamas K Lengyel <tamas@tklengyel.com> wrote: > >>> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h > >>> index 959083d8c4..76676ff4c0 100644 > >>> --- a/xen/include/public/vm_event.h > >>> +++ b/xen/include/public/vm_event.h > >>> @@ -281,6 +281,7 @@ struct vm_event_debug { > >>> uint32_t insn_length; > >>> uint8_t type; /* HVMOP_TRAP_* */ > >>> uint8_t _pad[3]; > >>> + uint64_t pending_dbg; > >> This is just a nitpick but I would prefer if we had the _pad field as > >> the last element in the struct and keep all 64-bit members up in the > >> front. > > Also, since pending_dbg uses unsigned int in Xen, do we need uint64_t > > for it here? Seems to me a uint32_t would suffice. > > Its %dr6 (but not quite, due to complexity with exception priorities, > interrupt shadows, and backwards compatibility of the RTM bit with > inverted polarity). All other registers have 64 bit fields in the > interface. > > The only interesting bits in it fall within the first 32 which is why it > is handled in a shorter way within Xen. Like %cr0, I don't expect > anything interesting to appear in the upper 32 bits. > Perhaps it would be better to call it dr6 in the interface then to make it more clear that this is a register value? Tamas
On 03/12/2019 18:16, Tamas K Lengyel wrote: > On Tue, Dec 3, 2019 at 1:09 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> On 03/12/2019 18:05, Tamas K Lengyel wrote: >>>> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h >>>> index 959083d8c4..76676ff4c0 100644 >>>> --- a/xen/include/public/vm_event.h >>>> +++ b/xen/include/public/vm_event.h >>>> @@ -281,6 +281,7 @@ struct vm_event_debug { >>>> uint32_t insn_length; >>>> uint8_t type; /* HVMOP_TRAP_* */ >>>> uint8_t _pad[3]; >>>> + uint64_t pending_dbg; >>> This is just a nitpick but I would prefer if we had the _pad field as >>> the last element in the struct and keep all 64-bit members up in the >>> front. >> Well the reason I did it like this is that this version will continue to >> function with older introspection code. The extra field is within a >> union and no other data moves. >> >> By repositioning to the start, it will almost certainly break older >> introspection code even though it compiled correctly. >> >> Your choice. > We are already bumping the interface version for the next release so > old introspection code by design will stop working. We make no ABI > stability guarantees between interface versions so this is a > non-issue. Ok fine. Updated locally, but I won't send a new version of the patch just for this delta. diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h index 76676ff4c0..8c24a58964 100644 --- a/xen/include/public/vm_event.h +++ b/xen/include/public/vm_event.h @@ -278,10 +278,10 @@ struct vm_event_singlestep { struct vm_event_debug { uint64_t gfn; + uint64_t pending_dbg; uint32_t insn_length; uint8_t type; /* HVMOP_TRAP_* */ uint8_t _pad[3]; - uint64_t pending_dbg; }; struct vm_event_mov_to_msr { However, this does raise the question of why insn_length is uint32_t. It has a value which is at most 15. ~Andrew
On Tue, Dec 3, 2019 at 1:22 PM Tamas K Lengyel <tamas@tklengyel.com> wrote: > > On Tue, Dec 3, 2019 at 1:18 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > > > On 03/12/2019 18:09, Tamas K Lengyel wrote: > > > On Tue, Dec 3, 2019 at 1:05 PM Tamas K Lengyel <tamas@tklengyel.com> wrote: > > >>> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h > > >>> index 959083d8c4..76676ff4c0 100644 > > >>> --- a/xen/include/public/vm_event.h > > >>> +++ b/xen/include/public/vm_event.h > > >>> @@ -281,6 +281,7 @@ struct vm_event_debug { > > >>> uint32_t insn_length; > > >>> uint8_t type; /* HVMOP_TRAP_* */ > > >>> uint8_t _pad[3]; > > >>> + uint64_t pending_dbg; > > >> This is just a nitpick but I would prefer if we had the _pad field as > > >> the last element in the struct and keep all 64-bit members up in the > > >> front. > > > Also, since pending_dbg uses unsigned int in Xen, do we need uint64_t > > > for it here? Seems to me a uint32_t would suffice. > > > > Its %dr6 (but not quite, due to complexity with exception priorities, > > interrupt shadows, and backwards compatibility of the RTM bit with > > inverted polarity). All other registers have 64 bit fields in the > > interface. > > > > The only interesting bits in it fall within the first 32 which is why it > > is handled in a shorter way within Xen. Like %cr0, I don't expect > > anything interesting to appear in the upper 32 bits. > > > > Perhaps it would be better to call it dr6 in the interface then to > make it more clear that this is a register value? > Which then begs the question, why not just use dr6 that's already present in the vm_event_regs_x86 struct? Tamas
On Tue, Dec 3, 2019 at 1:23 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 03/12/2019 18:16, Tamas K Lengyel wrote: > > On Tue, Dec 3, 2019 at 1:09 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >> On 03/12/2019 18:05, Tamas K Lengyel wrote: > >>>> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h > >>>> index 959083d8c4..76676ff4c0 100644 > >>>> --- a/xen/include/public/vm_event.h > >>>> +++ b/xen/include/public/vm_event.h > >>>> @@ -281,6 +281,7 @@ struct vm_event_debug { > >>>> uint32_t insn_length; > >>>> uint8_t type; /* HVMOP_TRAP_* */ > >>>> uint8_t _pad[3]; > >>>> + uint64_t pending_dbg; > >>> This is just a nitpick but I would prefer if we had the _pad field as > >>> the last element in the struct and keep all 64-bit members up in the > >>> front. > >> Well the reason I did it like this is that this version will continue to > >> function with older introspection code. The extra field is within a > >> union and no other data moves. > >> > >> By repositioning to the start, it will almost certainly break older > >> introspection code even though it compiled correctly. > >> > >> Your choice. > > We are already bumping the interface version for the next release so > > old introspection code by design will stop working. We make no ABI > > stability guarantees between interface versions so this is a > > non-issue. > > Ok fine. Updated locally, but I won't send a new version of the patch > just for this delta. > > diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h > index 76676ff4c0..8c24a58964 100644 > --- a/xen/include/public/vm_event.h > +++ b/xen/include/public/vm_event.h > @@ -278,10 +278,10 @@ struct vm_event_singlestep { > > struct vm_event_debug { > uint64_t gfn; > + uint64_t pending_dbg; > uint32_t insn_length; > uint8_t type; /* HVMOP_TRAP_* */ > uint8_t _pad[3]; > - uint64_t pending_dbg; > }; > > struct vm_event_mov_to_msr { > > > However, this does raise the question of why insn_length is uint32_t. > It has a value which is at most 15. > We could shorten it (feel free to do that as part of this patch or another if you are inclined to do so). Overall I don't think it would make much of a difference since we aren't saving any space on the ring page with that because its used in a union with larger structs. Tamas
On 03/12/2019 18:24, Tamas K Lengyel wrote: > On Tue, Dec 3, 2019 at 1:22 PM Tamas K Lengyel <tamas@tklengyel.com> wrote: >> On Tue, Dec 3, 2019 at 1:18 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> On 03/12/2019 18:09, Tamas K Lengyel wrote: >>>> On Tue, Dec 3, 2019 at 1:05 PM Tamas K Lengyel <tamas@tklengyel.com> wrote: >>>>>> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h >>>>>> index 959083d8c4..76676ff4c0 100644 >>>>>> --- a/xen/include/public/vm_event.h >>>>>> +++ b/xen/include/public/vm_event.h >>>>>> @@ -281,6 +281,7 @@ struct vm_event_debug { >>>>>> uint32_t insn_length; >>>>>> uint8_t type; /* HVMOP_TRAP_* */ >>>>>> uint8_t _pad[3]; >>>>>> + uint64_t pending_dbg; >>>>> This is just a nitpick but I would prefer if we had the _pad field as >>>>> the last element in the struct and keep all 64-bit members up in the >>>>> front. >>>> Also, since pending_dbg uses unsigned int in Xen, do we need uint64_t >>>> for it here? Seems to me a uint32_t would suffice. >>> Its %dr6 (but not quite, due to complexity with exception priorities, >>> interrupt shadows, and backwards compatibility of the RTM bit with >>> inverted polarity). All other registers have 64 bit fields in the >>> interface. >>> >>> The only interesting bits in it fall within the first 32 which is why it >>> is handled in a shorter way within Xen. Like %cr0, I don't expect >>> anything interesting to appear in the upper 32 bits. >>> >> Perhaps it would be better to call it dr6 in the interface then to >> make it more clear that this is a register value? >> > Which then begs the question, why not just use dr6 that's already > present in the vm_event_regs_x86 struct? Because it (specifically) isn't exactly %dr6. The ABI it follows is strictly like the VT-x's pending_dbg VMCS field. All bits have positive polarity, and are specific to the debug exception in question. %dr6 accumulates some debug bits or-wise (and until the guest #DB handler decides to clear them), some debug bits overwrite-wise, and some bits with inverted polarity. Providing %dr6 alone, either before or after merging pending_dbg, is insufficient to disambiguate the debug exception. pending_dbg is strictly "the new exception(s) to add into the %dr6 mix". ~Andrew
On Tue, Dec 3, 2019 at 1:31 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 03/12/2019 18:24, Tamas K Lengyel wrote: > > On Tue, Dec 3, 2019 at 1:22 PM Tamas K Lengyel <tamas@tklengyel.com> wrote: > >> On Tue, Dec 3, 2019 at 1:18 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >>> On 03/12/2019 18:09, Tamas K Lengyel wrote: > >>>> On Tue, Dec 3, 2019 at 1:05 PM Tamas K Lengyel <tamas@tklengyel.com> wrote: > >>>>>> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h > >>>>>> index 959083d8c4..76676ff4c0 100644 > >>>>>> --- a/xen/include/public/vm_event.h > >>>>>> +++ b/xen/include/public/vm_event.h > >>>>>> @@ -281,6 +281,7 @@ struct vm_event_debug { > >>>>>> uint32_t insn_length; > >>>>>> uint8_t type; /* HVMOP_TRAP_* */ > >>>>>> uint8_t _pad[3]; > >>>>>> + uint64_t pending_dbg; > >>>>> This is just a nitpick but I would prefer if we had the _pad field as > >>>>> the last element in the struct and keep all 64-bit members up in the > >>>>> front. > >>>> Also, since pending_dbg uses unsigned int in Xen, do we need uint64_t > >>>> for it here? Seems to me a uint32_t would suffice. > >>> Its %dr6 (but not quite, due to complexity with exception priorities, > >>> interrupt shadows, and backwards compatibility of the RTM bit with > >>> inverted polarity). All other registers have 64 bit fields in the > >>> interface. > >>> > >>> The only interesting bits in it fall within the first 32 which is why it > >>> is handled in a shorter way within Xen. Like %cr0, I don't expect > >>> anything interesting to appear in the upper 32 bits. > >>> > >> Perhaps it would be better to call it dr6 in the interface then to > >> make it more clear that this is a register value? > >> > > Which then begs the question, why not just use dr6 that's already > > present in the vm_event_regs_x86 struct? > > Because it (specifically) isn't exactly %dr6. The ABI it follows is > strictly like the VT-x's pending_dbg VMCS field. > > All bits have positive polarity, and are specific to the debug exception > in question. > > %dr6 accumulates some debug bits or-wise (and until the guest #DB > handler decides to clear them), some debug bits overwrite-wise, and some > bits with inverted polarity. > > Providing %dr6 alone, either before or after merging pending_dbg, is > insufficient to disambiguate the debug exception. > > pending_dbg is strictly "the new exception(s) to add into the %dr6 mix". > OK, thanks for the explanation. SGTM. Tamas
On Tue, Dec 3, 2019 at 1:23 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 03/12/2019 18:16, Tamas K Lengyel wrote: > > On Tue, Dec 3, 2019 at 1:09 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >> On 03/12/2019 18:05, Tamas K Lengyel wrote: > >>>> diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h > >>>> index 959083d8c4..76676ff4c0 100644 > >>>> --- a/xen/include/public/vm_event.h > >>>> +++ b/xen/include/public/vm_event.h > >>>> @@ -281,6 +281,7 @@ struct vm_event_debug { > >>>> uint32_t insn_length; > >>>> uint8_t type; /* HVMOP_TRAP_* */ > >>>> uint8_t _pad[3]; > >>>> + uint64_t pending_dbg; > >>> This is just a nitpick but I would prefer if we had the _pad field as > >>> the last element in the struct and keep all 64-bit members up in the > >>> front. > >> Well the reason I did it like this is that this version will continue to > >> function with older introspection code. The extra field is within a > >> union and no other data moves. > >> > >> By repositioning to the start, it will almost certainly break older > >> introspection code even though it compiled correctly. > >> > >> Your choice. > > We are already bumping the interface version for the next release so > > old introspection code by design will stop working. We make no ABI > > stability guarantees between interface versions so this is a > > non-issue. > > Ok fine. Updated locally, but I won't send a new version of the patch > just for this delta. > > diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h > index 76676ff4c0..8c24a58964 100644 > --- a/xen/include/public/vm_event.h > +++ b/xen/include/public/vm_event.h > @@ -278,10 +278,10 @@ struct vm_event_singlestep { > > struct vm_event_debug { > uint64_t gfn; > + uint64_t pending_dbg; > uint32_t insn_length; > uint8_t type; /* HVMOP_TRAP_* */ > uint8_t _pad[3]; > - uint64_t pending_dbg; > }; With the above adjustment: Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
On 03.12.2019 18:10, Andrew Cooper wrote: > --- a/xen/include/public/hvm/dm_op.h > +++ b/xen/include/public/hvm/dm_op.h > @@ -324,7 +324,7 @@ struct xen_dm_op_inject_event { > /* IN - error code (or ~0 to skip) */ > uint32_t error_code; > uint32_t pad1; > - /* IN - CR2 for page faults */ > + /* IN - type-specific extra data (%cr2 for #PF, pending_dbg for #DB) */ > uint64_aligned_t cr2; > }; How about uint32_t error_code; uint32_t pad1; /* IN - type-specific extra data (%cr2 for #PF, pending_dbg for #DB) */ #if defined(__XEN__) || defined(__XEN_TOOLS__) uint64_aligned_t extra; #else uint64_aligned_t cr2; #endif }; or something along these lines (e.g. could also be an unnamed union guarded by a __GNUC__ check, or a __XEN_INTERFACE_VERSION__ conditional), to have a less confusing name in Xen and the tools? Either way hypervisor bits Acked-by: Jan Beulich <jbeulich@suse.com> Jan
On Wed, 2019-12-04 at 10:04 +0100, Jan Beulich wrote: > On 03.12.2019 18:10, Andrew Cooper wrote: > > --- a/xen/include/public/hvm/dm_op.h > > +++ b/xen/include/public/hvm/dm_op.h > > @@ -324,7 +324,7 @@ struct xen_dm_op_inject_event { > > /* IN - error code (or ~0 to skip) */ > > uint32_t error_code; > > uint32_t pad1; > > - /* IN - CR2 for page faults */ > > + /* IN - type-specific extra data (%cr2 for #PF, pending_dbg > > for #DB) */ > > uint64_aligned_t cr2; > > }; > > How about > > uint32_t error_code; > uint32_t pad1; > /* IN - type-specific extra data (%cr2 for #PF, pending_dbg for > #DB) */ > #if defined(__XEN__) || defined(__XEN_TOOLS__) > uint64_aligned_t extra; > #else > uint64_aligned_t cr2; > #endif > }; > > or something along these lines (e.g. could also be an unnamed > union guarded by a __GNUC__ check, or a __XEN_INTERFACE_VERSION__ > conditional), to have a less confusing name in Xen and the tools? > Either way hypervisor bits > Honestly, this looks a little bit scary. However, it might be a good opportunity to discuss upgrading to C11, which supports anonymous unions and offers a cleaner (and safer) way to handle this type of problems. Best regards, Petre
On Tue, 2019-12-03 at 17:10 +0000, Andrew Cooper wrote: > Like %cr2 for pagefaults, %dr6 contains ancillary information for > debug > exceptions, and needs similar handling. > > For xendevicemodel_inject_event(), no ABI change is needed (although > an API > one would be ideal). Switch from 'cr2' to 'extra' in variable names > which > don't constitute an API change, and update the documentation to > match. > > For the monitor interface, vm_event_debug needs extending with a > pending_dbg > field. Extend hvm_monitor_debug() and for now, always pass in 0 - > this will > be fixed eventually, when other hypervisor bugfixes are complete. > > While modifying hvm_monitor_debug(), take the opportunity to correct > trap type > and instruction length from unsigned long to unsigned int, as they > are both > tiny values. > > Finally, adjust xen-access.c to the new expectations. Introspection > tools > intercepting debug exceptions should mirror the new pending_dbg field > into > xendevicemodel_inject_event() for %dr6 to be processed correctly for > the > guest. > > Signed-off-by: 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: Razvan Cojocaru <rcojocaru@bitdefender.com> > CC: Tamas K Lengyel <tamas@tklengyel.com> > CC: Alexandru Isaila <aisaila@bitdefender.com> > CC: Petre Pircalabu <ppircalabu@bitdefender.com> > CC: Ian Jackson <Ian.Jackson@citrix.com> > > I'm expecting to commit this alongside "x86/svm: Correct vm_event API > for > descriptor accesses" which covers the bump of the VM_EVENT interface > version. > --- > tools/libs/devicemodel/core.c | 4 ++-- > tools/libs/devicemodel/include/xendevicemodel.h | 4 ++-- > tools/tests/xen-access/xen-access.c | 7 ++++--- > xen/arch/x86/hvm/monitor.c | 4 +++- > xen/arch/x86/hvm/svm/svm.c | 4 ++-- > xen/arch/x86/hvm/vmx/vmx.c | 6 +++--- > xen/include/asm-x86/hvm/monitor.h | 3 ++- > xen/include/public/hvm/dm_op.h | 2 +- > xen/include/public/vm_event.h | 1 + > 9 files changed, 20 insertions(+), 15 deletions(-) Reviewed-by: Petre Pircalabu <ppircalabu@bitdefender.com>
diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c index f76e3d305e..db501d9e80 100644 --- a/tools/libs/devicemodel/core.c +++ b/tools/libs/devicemodel/core.c @@ -536,7 +536,7 @@ int xendevicemodel_set_mem_type( int xendevicemodel_inject_event( xendevicemodel_handle *dmod, domid_t domid, int vcpu, uint8_t vector, - uint8_t type, uint32_t error_code, uint8_t insn_len, uint64_t cr2) + uint8_t type, uint32_t error_code, uint8_t insn_len, uint64_t extra) { struct xen_dm_op op; struct xen_dm_op_inject_event *data; @@ -551,7 +551,7 @@ int xendevicemodel_inject_event( data->type = type; data->error_code = error_code; data->insn_len = insn_len; - data->cr2 = cr2; + data->cr2 = extra; return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op)); } diff --git a/tools/libs/devicemodel/include/xendevicemodel.h b/tools/libs/devicemodel/include/xendevicemodel.h index 08cb0d4374..e877f5c8a6 100644 --- a/tools/libs/devicemodel/include/xendevicemodel.h +++ b/tools/libs/devicemodel/include/xendevicemodel.h @@ -309,12 +309,12 @@ int xendevicemodel_set_mem_type( * @parm type the event type (see the definition of enum x86_event_type) * @parm error_code the error code or ~0 to skip * @parm insn_len the instruction length - * @parm cr2 the value of CR2 for page faults + * @parm extra type-specific extra data (%cr2 for #PF, pending_dbg for #DB) * @return 0 on success, -1 on failure. */ int xendevicemodel_inject_event( xendevicemodel_handle *dmod, domid_t domid, int vcpu, uint8_t vector, - uint8_t type, uint32_t error_code, uint8_t insn_len, uint64_t cr2); + uint8_t type, uint32_t error_code, uint8_t insn_len, uint64_t extra); /** * Shuts the domain down. diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c index 6aaee16d67..044a3a3c57 100644 --- a/tools/tests/xen-access/xen-access.c +++ b/tools/tests/xen-access/xen-access.c @@ -826,18 +826,19 @@ int main(int argc, char *argv[]) break; case VM_EVENT_REASON_DEBUG_EXCEPTION: - printf("Debug exception: rip=%016"PRIx64", vcpu %d. Type: %u. Length: %u\n", + printf("Debug exception: rip=%016"PRIx64", vcpu %d. Type: %u. Length: %u. Pending dbg %08"PRIx64"\n", req.data.regs.x86.rip, req.vcpu_id, req.u.debug_exception.type, - req.u.debug_exception.insn_length); + req.u.debug_exception.insn_length, + req.u.debug_exception.pending_dbg); /* Reinject */ rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id, X86_TRAP_DEBUG, req.u.debug_exception.type, -1, req.u.debug_exception.insn_length, - req.data.regs.x86.cr2); + req.u.debug_exception.pending_dbg); if (rc < 0) { ERROR("Error %d injecting breakpoint\n", rc); diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c index 7fb1e2c04e..e7fb9f4254 100644 --- a/xen/arch/x86/hvm/monitor.c +++ b/xen/arch/x86/hvm/monitor.c @@ -136,7 +136,8 @@ static inline unsigned long gfn_of_rip(unsigned long rip) } int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type, - unsigned long trap_type, unsigned long insn_length) + unsigned int trap_type, unsigned int insn_length, + unsigned int pending_dbg) { /* * rc < 0 error in monitor/vm_event, crash @@ -175,6 +176,7 @@ int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type, req.u.debug_exception.gfn = gfn_of_rip(rip); req.u.debug_exception.type = trap_type; req.u.debug_exception.insn_length = insn_length; + req.u.debug_exception.pending_dbg = pending_dbg; sync = !!ad->monitor.debug_exception_sync; break; diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index 0fb1908c18..72b1dcbf54 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -2649,7 +2649,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) rc = hvm_monitor_debug(regs->rip, HVM_MONITOR_DEBUG_EXCEPTION, - trap_type, inst_len); + trap_type, inst_len, 0); if ( rc < 0 ) goto unexpected_exit_type; if ( !rc ) @@ -2680,7 +2680,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs) rc = hvm_monitor_debug(regs->rip, HVM_MONITOR_SOFTWARE_BREAKPOINT, X86_EVENTTYPE_SW_EXCEPTION, - inst_len); + inst_len, 0); if ( rc < 0 ) goto unexpected_exit_type; if ( !rc ) diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 7450cbe40d..39efd91991 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -3834,7 +3834,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) rc = hvm_monitor_debug(regs->rip, HVM_MONITOR_DEBUG_EXCEPTION, - trap_type, insn_len); + trap_type, insn_len, 0); if ( rc < 0 ) goto exit_and_crash; @@ -3855,7 +3855,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) rc = hvm_monitor_debug(regs->rip, HVM_MONITOR_SOFTWARE_BREAKPOINT, X86_EVENTTYPE_SW_EXCEPTION, - insn_len); + insn_len, 0); if ( rc < 0 ) goto exit_and_crash; @@ -4157,7 +4157,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs) { hvm_monitor_debug(regs->rip, HVM_MONITOR_SINGLESTEP_BREAKPOINT, - 0, 0); + 0, 0, 0); if ( v->domain->debugger_attached ) domain_pause_for_debugger(); diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h index 325b44674d..66de24cb75 100644 --- a/xen/include/asm-x86/hvm/monitor.h +++ b/xen/include/asm-x86/hvm/monitor.h @@ -42,7 +42,8 @@ void hvm_monitor_descriptor_access(uint64_t exit_info, uint64_t vmx_exit_qualification, uint8_t descriptor, bool is_write); int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type, - unsigned long trap_type, unsigned long insn_length); + unsigned int trap_type, unsigned int insn_length, + unsigned int pending_dbg); int hvm_monitor_cpuid(unsigned long insn_length, unsigned int leaf, unsigned int subleaf); void hvm_monitor_interrupt(unsigned int vector, unsigned int type, diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h index d3b554d019..fd00e9d761 100644 --- a/xen/include/public/hvm/dm_op.h +++ b/xen/include/public/hvm/dm_op.h @@ -324,7 +324,7 @@ struct xen_dm_op_inject_event { /* IN - error code (or ~0 to skip) */ uint32_t error_code; uint32_t pad1; - /* IN - CR2 for page faults */ + /* IN - type-specific extra data (%cr2 for #PF, pending_dbg for #DB) */ uint64_aligned_t cr2; }; diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h index 959083d8c4..76676ff4c0 100644 --- a/xen/include/public/vm_event.h +++ b/xen/include/public/vm_event.h @@ -281,6 +281,7 @@ struct vm_event_debug { uint32_t insn_length; uint8_t type; /* HVMOP_TRAP_* */ uint8_t _pad[3]; + uint64_t pending_dbg; }; struct vm_event_mov_to_msr {
Like %cr2 for pagefaults, %dr6 contains ancillary information for debug exceptions, and needs similar handling. For xendevicemodel_inject_event(), no ABI change is needed (although an API one would be ideal). Switch from 'cr2' to 'extra' in variable names which don't constitute an API change, and update the documentation to match. For the monitor interface, vm_event_debug needs extending with a pending_dbg field. Extend hvm_monitor_debug() and for now, always pass in 0 - this will be fixed eventually, when other hypervisor bugfixes are complete. While modifying hvm_monitor_debug(), take the opportunity to correct trap type and instruction length from unsigned long to unsigned int, as they are both tiny values. Finally, adjust xen-access.c to the new expectations. Introspection tools intercepting debug exceptions should mirror the new pending_dbg field into xendevicemodel_inject_event() for %dr6 to be processed correctly for the guest. Signed-off-by: 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: Razvan Cojocaru <rcojocaru@bitdefender.com> CC: Tamas K Lengyel <tamas@tklengyel.com> CC: Alexandru Isaila <aisaila@bitdefender.com> CC: Petre Pircalabu <ppircalabu@bitdefender.com> CC: Ian Jackson <Ian.Jackson@citrix.com> I'm expecting to commit this alongside "x86/svm: Correct vm_event API for descriptor accesses" which covers the bump of the VM_EVENT interface version. --- tools/libs/devicemodel/core.c | 4 ++-- tools/libs/devicemodel/include/xendevicemodel.h | 4 ++-- tools/tests/xen-access/xen-access.c | 7 ++++--- xen/arch/x86/hvm/monitor.c | 4 +++- xen/arch/x86/hvm/svm/svm.c | 4 ++-- xen/arch/x86/hvm/vmx/vmx.c | 6 +++--- xen/include/asm-x86/hvm/monitor.h | 3 ++- xen/include/public/hvm/dm_op.h | 2 +- xen/include/public/vm_event.h | 1 + 9 files changed, 20 insertions(+), 15 deletions(-)